Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added config commands for CBF #1799

Merged

Conversation

Cosmin-Jinga-MS
Copy link
Contributor

[config]
###What I did:
Added reload and clear commands for CBF.
###How I did it:
Added the commands in a similar manner to the existing QOS support.
###How to verify it:
Run the commands on a CBF capable platform.
###File changes: config/main.py

Signed-off-by: Cosmin Jinga <v-cjinga@microsoft.com>

What I did:
 Added reload and clear commands for CBF.
How I did it:
 Added the commands in a similar manner to the existing QOS support.
How to verify it:
 Run the commands on a CBF capable platform.
Files changes: config/main.py

Signed-off-by: Cosmin Jinga <v-cjinga@microsoft.com>
@Cosmin-Jinga-MS
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@smaheshm smaheshm self-requested a review September 30, 2021 20:40
@smaheshm
Copy link
Contributor

smaheshm commented Sep 30, 2021

@Cosmin-Jinga-MS Can you also add unit test for your code. You can use the following as an example:
https://github.com/Azure/sonic-utilities/blob/280dd290acfa4bdbb09a92e908cce6d40fa7e79e/tests/config_test.py#L132

In case you are not familiar with creating a test environment, here's the link:
https://github.com/Azure/sonic-utilities/#setting-up-a-buildtest-environment

@Cosmin-Jinga-MS
Copy link
Contributor Author

Requested UT added.
@smaheshm Can you please have another look?

@@ -0,0 +1,12 @@
{
Copy link
Contributor

@smaheshm smaheshm Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no variables here to use a template. In any case the file here should be the one that will be used when generating the config using "sonic_cfggen" command. How will these entries be created in production switch. Is there going to be a new template or are you going to use the existing qos templates.

eg:
https://github.com/Azure/sonic-buildimage/blob/master/files/build_templates/qos_config.j2
https://github.com/Azure/sonic-buildimage/blob/master/files/build_templates/buffers_config.j2

There should be a PR for those changes if not already there. Copy the same template (that's used in prod switches) here for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is a separate PR for the default CBF template config. It's been updated as well with the new config now found in the UTs too.
sonic-net/sonic-buildimage#8689

@@ -0,0 +1,8 @@
build_version: 'master.487-a98cf221'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be we can reuse the exsiting one.. CBF, QoS are close related anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reused the qos one

print("TEARDOWN")
os.environ['UTILITIES_UNIT_TESTING'] = "0"


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this feature will be used in WAN there's a high chance it will be multi ASIC platform. Please add test case for multi ASIC as well. You already added the code to generate, just need to add test cases. It can be the same pattern as QoS tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the multi asic UT

Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left comments.

@Cosmin-Jinga-MS
Copy link
Contributor Author

Pushed new commit addressing the latest review comments.

@@ -608,6 +608,27 @@ def _change_hostname(hostname):
clicommon.run_command(r'sed -i "/\s{}$/d" /etc/hosts'.format(current_hostname), display_cmd=True)
clicommon.run_command('echo "127.0.0.1 {}" >> /etc/hosts'.format(hostname), display_cmd=True)

def _clear_cbf():
CBF_TABLE_NAMES = [
'DSCP_TO_FC_MAP',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elsewhere it's DSCP_TO_TC_MAP. Please update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other updates were incorrect. CBF has to have its own DSCP mapping, I've reverted the names to DSCP_TO_FC_MAP in the other occurrences too.

Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants