Moving get_routing_stack() to a centralized location to avoid code dups #1714
Conversation
…plication Various areas of sonic-utilities are now demanding this functionality so i'm simply moving this routing to a centralized location. After spending some time debating about the ideal location for this function, we thought about sonic-config-engine/sonic_platform.py as the closest match. Functional tests' output will be provided as part of the equivalent PR to be submitted shortly within sonic-utilities repo.
|
Changes in this PR are connected to those ones in PR/254.. The 'get_routing_stack()' function is being brought from 'sonic-utilities/show/main.py'. |
| @@ -53,3 +53,19 @@ def get_system_mac(): | |||
| mac = mac[:-2] + aligned_last_byte | |||
| return mac | |||
|
|
|||
| def get_system_routing_stack(): | |||
There was a problem hiding this comment.
This is a file that is meant to retrieve platform specific related information from config files. Doesn't seem the python function you are introducing belongs here and it will be best placed in utilities. Ultimately sonic MS team is to bless this.
There was a problem hiding this comment.
It may not be the perfect fit, but it's probably the best centralized location we currently have. It's better than sonic-utilities, as that repo houses command-line utilities.
In the future, I'd like to see sonic_platform.py, minigraph.py, openconfig_acl.py and portconfig.py moved out of sonic-config-engine and built into a new package, separate from sonic-cfggen. They are modules which are imported by sonic-cfggen, but can also be imported into other scripts. At that point, we could create a separate module for this function because it's technically not platform-specific. Until then, I think this is fine.
Maybe a comment should be added stating that the function is not platform-specific and may be better moved to a new module in the future?
There was a problem hiding this comment.
Right, this is what i meant in my original PR comment -- there's no ideal location for this function, but it definitely has to be outside sonic-utilities as various submodules are/will-be in need of this feature. I'll add a comment as Joe suggested.
And as previously discussed (Joe), we should definitely move all this generic functionality outside sonic-buildimage superproject -- a new repo with no configuration-specific semantics should be hosting all this system/generic functionality.
There was a problem hiding this comment.
Agree with Joe's comment that ideally all those library functions should be moved into a separate module in the future. I also understand that by now here might be the best place that hosts generic-purpose functions. However, as this function really differs from others in sonic_platform.py - all existing functions there are to read hardware-specific information that remain constant for a specific device, while this added function is about container information, which might be affected by operation.
How about creating a new python file hosting all docker-related lib functions? I'm sure we'll need more such facilities when we come back to revisit the container upgrade scenario.
There was a problem hiding this comment.
I think a file in the current folder will do. How about a name like "sonic_component.py"?
|
@taoyl-ms, could you please review? Thanks. |
|
retest this please |
| # suitable location is identified as part of upcoming refactoring efforts. | ||
| # | ||
| def get_system_routing_stack(): | ||
| command = "sudo docker ps | grep bgp | awk '{print$2}' | cut -d'-' -f3 | cut -d':' -f1" |
There was a problem hiding this comment.
ps | grep [](start = 27, length = 9)
pgrep
dad1287 [tunneldecaporch] Set default MTU for the overlay loopback interface (sonic-net#1756) 1bc94d1 [orchagent] Fix typo in PortsOrch::initPortSupportedSpeeds (sonic-net#1755) a44e651 [nhg]: Add support for weight in nexthop group member. (sonic-net#1752) 5c625b2 [Bulk mode] Limit the size of bulker (sonic-net#1744) d1cd0fd Fix error msg due to not supported "SAI_SWITCH_ATTR_MAX_NUMBER_OF_TEMP_SENSORS" attributes (sonic-net#1745) 278770d [sub intf] Fix kernel side processing to enslave sub interface to non-default vrf (sonic-net#1521) 031f536 support flush FDB entries per port and per vlan (sonic-net#1064) 3629d70 [sonic-swss] Add port auto negotiation support to swss (sonic-net#1714) 7c6ebb1 [fix] Use the same storm detection condition for queue occupancy non-zero case as the zero case (sonic-net#1111) fb06c32 [fabricportsorch] Add fabric support (sonic-net#1459) Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
dad1287 [tunneldecaporch] Set default MTU for the overlay loopback interface (#1756) 1bc94d1 [orchagent] Fix typo in PortsOrch::initPortSupportedSpeeds (#1755) a44e651 [nhg]: Add support for weight in nexthop group member. (#1752) 5c625b2 [Bulk mode] Limit the size of bulker (#1744) d1cd0fd Fix error msg due to not supported "SAI_SWITCH_ATTR_MAX_NUMBER_OF_TEMP_SENSORS" attributes (#1745) 278770d [sub intf] Fix kernel side processing to enslave sub interface to non-default vrf (#1521) 031f536 support flush FDB entries per port and per vlan (#1064) 3629d70 [sonic-swss] Add port auto negotiation support to swss (#1714) 7c6ebb1 [fix] Use the same storm detection condition for queue occupancy non-zero case as the zero case (#1111) fb06c32 [fabricportsorch] Add fabric support (#1459) Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
dad1287 [tunneldecaporch] Set default MTU for the overlay loopback interface (sonic-net#1756) 1bc94d1 [orchagent] Fix typo in PortsOrch::initPortSupportedSpeeds (sonic-net#1755) a44e651 [nhg]: Add support for weight in nexthop group member. (sonic-net#1752) 5c625b2 [Bulk mode] Limit the size of bulker (sonic-net#1744) d1cd0fd Fix error msg due to not supported "SAI_SWITCH_ATTR_MAX_NUMBER_OF_TEMP_SENSORS" attributes (sonic-net#1745) 278770d [sub intf] Fix kernel side processing to enslave sub interface to non-default vrf (sonic-net#1521) 031f536 support flush FDB entries per port and per vlan (sonic-net#1064) 3629d70 [sonic-swss] Add port auto negotiation support to swss (sonic-net#1714) 7c6ebb1 [fix] Use the same storm detection condition for queue occupancy non-zero case as the zero case (sonic-net#1111) fb06c32 [fabricportsorch] Add fabric support (sonic-net#1459) Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
1. Added port auto negotiation attributes handle 2. Get supported speeds for each port and save them to CONFIG_DB 3. Added new test cases in VS test to verify the change
Moving get_routing_stack() to a centralized location to avoid code duplication.
Various areas of sonic-utilities are now demanding this functionality so i'm simply moving this routing to a centralized location. After spending some time debating about the ideal location for this function, we thought about sonic-config-engine/sonic_platform.py as the closest match.
Functional tests' output will be provided as part of the equivalent PR to be submitted shortly within sonic-utilities repo.