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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6d6704
Moving get_routing_stack() to a centralized location to avoid code du…
May 16, 2018
357c7ff
Adding comment to function.
May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/sonic-config-engine/sonic_platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def get_system_mac():
proc = subprocess.Popen("ip link show eth0 | grep ether | awk '{print $2}'", shell=True, stdout=subprocess.PIPE)
(mac, err) = proc.communicate()
mac = mac.strip()

# Align last byte of MAC if necessary
version_info = get_sonic_version_info()
if version_info and (version_info['asic_type'] == 'mellanox' or version_info['asic_type'] == 'centec'):
Expand All @@ -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 •
edited
Loading

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"?

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


try:
proc = subprocess.Popen(command,
stdout=subprocess.PIPE,
shell=True,
stderr=subprocess.STDOUT)
stdout = proc.communicate()[0]
proc.wait()
result = stdout.rstrip('\n')

except OSError, e:
raise OSError("Cannot detect routing-stack")

return result