[VOQ] Distributed VOQ LAG HLD #697
Conversation
Updating my fork to latest master
call flows
| * Every LAG needs to be created in SAI of all of the asics in the system. This is irrespective of which asic the member ports of a LAG "belong" to. | ||
| * The active member port list for each LAG should be the same in the SAI instance of every asic. Any update to the member port list of any LAG must be propagated to all of the SAI instances. | ||
| * The chnages made to SAI for VOQ support allow the member port list for a LAG to be specified as a list of system ports | ||
| * SAI allows the application layer to specify a "LAG_ID" (SAI_LAG_ATTR_SYSTEM_PORT_AGGREGATE_ID) as part of LAG creation. For a given LAG - the same value must be used on all the SAI instances. |
There was a problem hiding this comment.
Mention dynamic lag member add/remove at runtime
doc/voq/lag_hld.md
Outdated
| ``` | ||
|
|
||
| Changes in orchagent/portsorch include: | ||
| * Assign unique system_lag_id across system by using switch_id. The encoding scheme is described as above. |
There was a problem hiding this comment.
System lag id has a range restriction (1 to max lags supported). Having switch id encoded in the lag id will make the id bigger. Consider make this id be managed by configuration to make it unique. Revisit after checking with hardware restrictions. Also consider generating globally unique lag id if configuration is difficult
doc/voq/lag_hld.md
Outdated
| ``` | ||
| Switch<local switch_id>-<local PortChannel name> | ||
| ``` |
There was a problem hiding this comment.
Make the PortChannel name globally unique across the chassis in the local asic configuration itself. We do not need this encoding while syncing to CHASSIS_APP_DB
doc/voq/lag_hld.md
Outdated
| ``` | ||
|
|
||
| Changes in orchagent/portsorch include: | ||
| * Assign unique system_lag_id across system by using switch_id. The encoding scheme is described as above. |
There was a problem hiding this comment.
Preventing misconfiguration: When the lag id is configured, make a validation system at the configuration to make sure that it is unique across system. Proposal: take the chassis app db system lag info and populate the local state db and check with this local state db lag id info during configuration.
Alerting when there is error during run time: If the configuration passed due to timing, there should be alerting when lag id collision is detected during run time at later point of time
There was a problem hiding this comment.
Instead of configuring system_lag_id, acquire it from global redis (with atomic acition insured) so that all asics will get unique available id. By this we can avoid availability validation and race conditions.
Updated images for review comments (to include system lag id config)
Updated for review comments. Added configuration of system_lag_id, duplicate system_lag_id validation, call flow update for system_lag_id configuration
Updated for ToC link fix and some minor edits
Updated for using system_lag_id allocated from chassis_app_db.
Images changed to use system_lag_id allocated from chassis_app_db
Updated for allocation/de-allocation of globally unique system_lag_id
Update of release of system_lag_id to central database
Updated for clarification for releasing system_lag_id
Fixed some typos and other minor edits
Updated for system lag name scheme, system lag id boundary initialization by supervisor card and some minor edits
Added section 9 test considerations and changed system lag naming scheme to use asic_name instead of instance name.
|
|
||
| Changes in orchagent/portsorch include: | ||
| * Port structure enhancement to store the system lag info such as system lag name (alias), system lag id and switch_id. | ||
| * Getting a chassis wide unique system_lag_id from centralized database using lua script that can do atomic allocation/deallocation of system_lag_id globally |
There was a problem hiding this comment.
Minor: This reads as if the lua script is running inside orchagent/portsorch.
| key = SYSTEM_LAG_TABLE|system lag name ; System LAG name | ||
| ; field = value | ||
| system_lag_id = 1*10DIGIT ; LAG id. | ||
| switch_id = 1*4DIGIT ; Switch id |
There was a problem hiding this comment.
Curious where switch_id will be used?
Updated for fixing file name that holds the boundaries of system lag.
What I did Defined class for lag id allocator and added lua script for allocating/freeing lag id in atomic fashion Why I did it For portchannels in VOQ based chassis systems we need unique lag id across the system. The lag id (aka system port aggreggator id) is allocated during portchannel creation. The changes are for a class for lag id allocation in atomic fashion. The LAG ID is allocated from central chassis app db. A lua script loaded in the redis at the time of lag id allocator instantiation ensures allocating unique lag id when multiple clients requests for lag id simultaneously. Ref: VOQ LAG HLD PR: sonic-net/SONiC#697
| ``` | ||
| <Host name>|<Asic name>|<port channel name in its local asic instance> | ||
| ``` | ||
| The _Host name_ and _Asic name_ are available in CONFIG_DB device meta data attributes **hostnmae** and **asic_name** respectively. |
There was a problem hiding this comment.
asic_name is only available on multi-asic devices. On single-asic devices the asic can be identified by the hostname only.
Do you plan to make asic_name available on single-asic devices as well? Or can a lag name simply omit for single-asic devices, i.e. Slot1|PortChannel001?
There was a problem hiding this comment.
As per current system lag implementation, if the switch type is "voq", "asic_name" is mandatory in DEVICE_METADATA['localhost'] and it should have a valid non-zero sized value. So even for the single asic voq linecards, "asic_name" must be provided with a valid value.
There was a problem hiding this comment.
I see that it became mandatory when #1605 merged (https://github.com/Azure/sonic-swss/pull/1605/files#diff-3cba33a216c0ea2b942dc03b2638fa27a40a8b6eb81abf9f83f13e1ee7831ac4R231-R237), i.e.:
if (!cfgDeviceMetaDataTable.hget("localhost", "asic_name", value))
{
// asic_name is not configured.
SWSS_LOG_ERROR("Asic name is not configured");
return false;
}However I'm a bit surprised as this basically prevents system ports to be configured without asic_name. This doesn't seem necessary.
What is the reason to make asic_name mandatory even on single-asic devices?
There was a problem hiding this comment.
I agree. "asic_name" for single asic seems unnecessary.
However, there is a reason why we need asic_name as mandatory in the system lag name. We use "|" as separator between the hostname, asic_name and port name. In the Distributed VOQ LAG HLD design discussion meeting it was told that in the future all tables will have "|" as the table separator. So for the system lag member table the key will look something like
"Linecard1|Asic0|PortChannel001|Linecard1|Asic0|Ethernet1". This is <derived system lag name>|<system port name>. In the lag member processing, to extract the port channel alias and member system port name alias from the key, it was suggested to use the fixed format of system lag and system port names. So we do not want two different formats.
Please note that for now, the separator is ":" and porchannel alias and system port member alias extraction is based on this. But in the future this may not be the case.
There was a problem hiding this comment.
Ok. Then I'll make sure that asic_name is set for VoQ regardless of of the number of asics in sonic-buildimage PR5997.
There was a problem hiding this comment.
I assume this is for single-chip linecards that are in a chassis with other chips. I don't think it's unreasonable to expect table entries are qualified with 'slotname|chipname' always. It's a little clunky for single-chip linecards, but not terrible and if it helps simplify code, we can do that.
For single-chip fixed systems that are not a in chassis, we shouldn't set the switch_type to 'voq' anyway.
What I did Defined class for lag id allocator and added lua script for allocating/freeing lag id in atomic fashion Why I did it For portchannels in VOQ based chassis systems we need unique lag id across the system. The lag id (aka system port aggreggator id) is allocated during portchannel creation. The changes are for a class for lag id allocation in atomic fashion. The LAG ID is allocated from central chassis app db. A lua script loaded in the redis at the time of lag id allocator instantiation ensures allocating unique lag id when multiple clients requests for lag id simultaneously. Ref: VOQ LAG HLD PR: sonic-net/SONiC#697
No description provided.