-
Notifications
You must be signed in to change notification settings - Fork 656
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
[config qos] QoS and Buffer config genration for multi ASIC platforms #978
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments
@arlakshm @neethajohn Let me know if I should add any other reviewer. |
@arlakshm Ok with the changes? |
format(ns), | ||
fg='yellow' | ||
) | ||
raise click.Abort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have sonic_device_util.get_all_namespaces() that provide the dictionary of front and back namespace with string as namespace value. Then no need to convert from id to string. We can see if this API can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need the ASIC ID. That API doesn't give the ASIC ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we have directory name as "asic0" ? i feel that is better and correlate to namespace directory created by linux.
We can change port_config.ini to also be in similar folder
@judyjoseph @SuvarnaMeenakshi what do you think here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary directory, it can be anything. I still need the ASIC ID to read, and matches that format.
/usr/share/sonic/device/0
/usr/share/sonic/device/1
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASIC IDs and ASIC namespaces are different. The configuration is for each ASIC ID rather than namespace. I think it's OK to specify ASIC ID for configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple approach would be to get the num of asics present in the platform and do a for range loop. We are using the asic_index as asic_id's internally.
If we derive the asic_id from namespace name, could cause problems if we create a namespace for a different purpose in the switch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using 'get_namespaces()' API, this should return the valid namespaces for ASICs. It's better to use common APIs so there's one place that calculates the ASIC ID and namespace. Using a range loop can be fragile if the ASIC ID allocation scheme changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary directory, it can be anything. I still need the ASIC ID to read, and matches that format.
/usr/share/sonic/device/0
/usr/share/sonic/device/1
...
@smaheshm why can't asic_id_suffix be namespace name directly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the directories use 'asic ID'.
/usr/share/sonic/device/0
/usr/share/sonic/device/1
...
retest this please |
retest this please |
1 similar comment
retest this please |
retest this please |
retest this please |
retest this please |
2 similar comments
retest this please |
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@smaheshm Create PR for 201911 |
- What I did
To support buffer and QoS config generation for multi ASIC platform. In case of multi ASIC platforms each ASIC has its own buffer and qos configuration template which is used to populate corresponding ASIC's config DB.
Following commands were modified for this purpose:
In multi ASIC platforms the command runs on all namespaces. In case of single ASIC platform the command runs in global namespace.
- How I did it
Modified config generation script to generate configs for all ASICs in case of multi-ASIC platform.
- How to verify it
verified configs generated on both multi-ASIC and single ASIC platforms. Verified traffic with different priorities using queue counters.
- Previous command output (if the output of a command-line utility has changed)
- New command output (if the output of a command-line utility has changed)