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

Moving get_routing_stack() to a centralized location to avoid code dups #1714

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

rodnymolina
Copy link
Contributor

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.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…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.
@rodnymolina
Copy link
Contributor Author

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'.

@jleveque jleveque requested a review from taoyl-ms May 16, 2018 18:02
@@ -53,3 +53,19 @@ def get_system_mac():
mac = mac[:-2] + aligned_last_byte
return mac

def get_system_routing_stack():
Copy link
Collaborator

@nikos-github nikos-github May 17, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taoyl-ms @jleveque, can you guys please propose a file-name and a location where to dump this function? I have a few PRs pending on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a file in the current folder will do. How about a name like "sonic_component.py"?

@rodnymolina
Copy link
Contributor Author

@taoyl-ms, could you please review? Thanks.

@lguohan
Copy link
Collaborator

lguohan commented Jun 23, 2018

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ps | grep [](start = 27, length = 9)

pgrep

@lguohan lguohan merged commit b7eeba8 into sonic-net:master Aug 7, 2018
volodymyrsamotiy added a commit to volodymyrsamotiy/sonic-buildimage that referenced this pull request May 28, 2021
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 <[email protected]>
liat-grozovik pushed a commit that referenced this pull request May 31, 2021
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 <[email protected]>
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
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 <[email protected]>
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
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
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.

6 participants