-
Notifications
You must be signed in to change notification settings - Fork 666
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
Common functions for show CLI support on multi ASIC #999
Conversation
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
This pull request introduces 3 alerts when merging 768ab07 into 82dfe50 - view on LGTM.com new alerts:
|
utilities_common/multi_asic.py
Outdated
swsssdk.SonicDBConfig.load_sonic_global_db_config() | ||
self.current_namespace = None | ||
|
||
def connect_dbs_for_ns(self,namespace=DEFAULT_NAMESPACE): |
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 recently opened a PR to add a "sonic-py-common" library in sonic-buildimage to house common Python functions (sonic-net/sonic-buildimage#5003). It will replace sonic_device_util. I think some of these functions like this one (which are not CLI-specific) can eventually be moved there, in a multi_asic.py
file. That way they can be shared by more than just sonic-utilities. This file should hold multi-ASIC functions that are specific to the CLI.
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.
@jleveque I agree. I will move the common functions 'sonic-py-common' once the PRs are merged.
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.
Great! I have created an issue here to track it: #1000
Retest this please |
utilities_common/multi_asic.py
Outdated
import subprocess | ||
import json | ||
import click | ||
import functools |
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.
alphabetical order?
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.
Fixed in the latest commit
utilities_common/multi_asic.py
Outdated
import swsssdk | ||
from swsssdk import ConfigDBConnector | ||
import argparse |
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.
same 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.
Also, follow PEP8 import ordering convention:
Imports should be grouped in the following order:
Standard library imports.
Related third party imports.
Local application/library specific imports.
You should put a blank line between each group of imports.
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.
Fixed in the latest commit.
utilities_common/multi_asic.py
Outdated
DEFAULT_NAMESPACE = '' | ||
DISPLAY_ALL = 'all' | ||
DISPLAY_EXTERNAL = 'frontend' | ||
PORT_CHANNEL_OBJ = 'PORT_CHANNEL' | ||
PORT_OBJ = 'PORT' | ||
BGP_NEIGH_OBJ = 'BGP_NEIGH' |
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.
wondering if some of these constants can potentially be used in other files. If that's the case lets create utilities_common/consts.py. It's cleaner and more readable if all constants are in one place.
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.
add a new file constants.py with all the constants in it
utilities_common/multi_asic.py
Outdated
In case of multi ASIC, the default namespace in the database instance running the on the host | ||
In case of single ASIC, the namespace has to DEFAULT_NAMESPACE |
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.
"is the database instance"
"has to be .."
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.
Fixed in the latest commit
utilities_common/multi_asic.py
Outdated
''' | ||
The function connects to the config DB for a given namespace and | ||
returns the handle | ||
If no namespace is provide, it will connect to the db in the |
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.
"...provided.."
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.
Fixed in the latest commit
utilities_common/multi_asic.py
Outdated
In case of multi ASIC, the default namespace in the database instance running the on the host | ||
In case of single ASIC, the namespace has to DEFAULT_NAMESPACE |
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.
same as above.. "copy - pasta :)"
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.
Fixed in the latest commit
utilities_common/multi_asic.py
Outdated
The function check if a CLI object is internal and returns true or false. | ||
For single asic, this function is not applicable |
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.
can you also add the definition of "internal".
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.
added in the latest commit
utilities_common/multi_asic.py
Outdated
returns false, if the cli option is all or if it the platform is single ASIC. | ||
|
||
''' | ||
if not is_multi_npu(): |
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.
perhaps this can be added in the constuctor.
self.is_multi_npu = is_multi_npu()
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.
Changed
utilities_common/multi_asic.py
Outdated
else: | ||
ns_list = [self.namespace_option] |
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 should validate namespace here. With this class we don't expect CLIs to get namespace list and validate the namespace.
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.
added validation in the latest commit
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.
Minor changes.
PORT_OBJ = 'PORT' | ||
BGP_NEIGH_OBJ = 'BGP_NEIGH' | ||
|
||
class MultiAsic(object): |
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 sounds like the class is for 'MultiAsic' only where as, If I understand correctly, all CLIs will use it on both single ASIC and multi ASIC platforms. Wondering if we should rename to something like 'SwitchAsic'. Either way is fine, lets hear others opinion.
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 agree the name is a bit misleading, but I'm not sure if SwitchAsic
is better. I feel like it needs a more generic name, but I can't think of anything at the moment. Maybe as more functionality is added to it we'll have a better idea of what to call it. I'm OK with saving a renaming for a future PR.
|
||
''' | ||
@functools.wraps(func) | ||
def wrapped_run_on_all_asics(self, *args, **kwargs): |
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 accessing class instance inside the decorator. Either decorator should be inside the CLI class, or we should come up with another way to run command in multiple namespaces.
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.
The decorator is for a function in the CLI class.
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.
Would prefer not changing the object state outside its domain. How about adding an API inside class, and call it from here.
def connect_dbs(ns):
self.multi_asic.current_namespace = ns
self.db = connect_to_all_dbs_for_ns(ns)
self.config_db = connect_config_db_for_ns(ns)
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
This pull request introduces 4 alerts when merging 15dbd2f into aa1b072 - view on LGTM.com new alerts:
|
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
retest this please |
3 similar comments
retest this please |
retest this please |
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.
looks good.
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
return ns_list | ||
|
||
|
||
def multi_asic_ns_choices(): |
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 API may not be required as multi_asic.get_namespace_list() already gives the namespacelist correctly. But, it is ok for now - could remove later as well.
Common changes will be used to support SONiC CLIs for multi ASIC - New MultiAsic class to support not displaying of internal object - Common CLI options which needed for multi ASIC platforms - a new decorator to execute a function on all namespaces Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
Common functions for show CLI support on multi ASIC (sonic-net#999)
Common functions for show CLI support on multi ASIC
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan arlakshm@microsoft.com
Depends on this PR
sonic-net/sonic-buildimage#4973
- What I did
Add a set of common function will be used to support SONiC CLIs for multi ASIC
- How I did it
- How to verify it
- Previous command output (if the output of a command-line utility has changed)
NA
- New command output (if the output of a command-line utility has changed)
NA