Skip to content
159 changes: 159 additions & 0 deletions utilities_common/multi_asic.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import subprocess
import json
import click
import functools
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

alphabetical order?

Copy link
Copy Markdown
Contributor Author

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

from sonic_device_util import get_num_npus
from sonic_device_util import get_namespaces
from sonic_device_util import is_multi_npu
from sonic_device_util import is_port_internal
from sonic_device_util import is_port_channel_internal
from sonic_device_util import is_bgp_session_internal
from sonic_device_util import get_all_namespaces

import swsssdk
from swsssdk import ConfigDBConnector
import argparse
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Copy Markdown
Contributor

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.

https://www.python.org/dev/peps/pep-0008/#imports

Copy link
Copy Markdown
Contributor Author

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.


DEFAULT_NAMESPACE = ''
DISPLAY_ALL = 'all'
DISPLAY_EXTERNAL = 'frontend'
PORT_CHANNEL_OBJ = 'PORT_CHANNEL'
PORT_OBJ = 'PORT'
BGP_NEIGH_OBJ = 'BGP_NEIGH'
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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


class MultiAsic(object):
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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.


def __init__(self, display_option=DISPLAY_ALL, namespace_option=None):
self.namespace_option = namespace_option
self.display_option = display_option
swsssdk.SonicDBConfig.load_sonic_global_db_config()
self.current_namespace = None

def connect_dbs_for_ns(self,namespace=DEFAULT_NAMESPACE):
Copy link
Copy Markdown
Contributor

@jleveque jleveque Jul 20, 2020

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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

'''
The function connects to the DBs for a given namespace and
returns the handle
If no namespace is provide, it will connect to the db in the
default namespace.
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
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

'''

db = swsssdk.SonicV2Connector(use_unix_socket_path=True, namespace=namespace)
db.connect(db.APPL_DB)
db.connect(db.CONFIG_DB)
db.connect(db.STATE_DB)
db.connect(db.COUNTERS_DB)
db.connect(db.ASIC_DB)
return db

def connect_config_db_for_ns(self, namespace=DEFAULT_NAMESPACE):
'''
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"...provided.."

Copy link
Copy Markdown
Contributor Author

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

default namespace.
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
Copy link
Copy Markdown
Contributor

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 :)"

Copy link
Copy Markdown
Contributor Author

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

'''
config_db = ConfigDBConnector(use_unix_socket_path=True, namespace=namespace)
config_db.connect()
return config_db

def is_object_internal(self, object_type, cli_object):
'''
The function check if a CLI object is internal and returns true or false.
For single asic, this function is not applicable
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

'''
if object_type == PORT_OBJ:
return is_port_internal(cli_object)
elif object_type == PORT_CHANNEL_OBJ:
return is_port_channel_internal(cli_object)
elif object_type == BGP_NEIGH_OBJ:
return is_bgp_session_internal(cli_object)

def skip_display(self, object_type, cli_object):
'''
The function determines if the passed cli_object has to be displayed or not.
returns true if the display_option is external and the cli object is internal.
returns false, if the cli option is all or if it the platform is single ASIC.

'''
if not is_multi_npu():
Copy link
Copy Markdown
Contributor

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()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed

return False
if self.display_option == DISPLAY_ALL:
return False
return self.is_object_internal(object_type, cli_object)

def get_ns_list_based_on_options(self):
ns_list = []
if not is_multi_npu():
return [DEFAULT_NAMESPACE]
else:
namespaces = get_all_namespaces()
if self.namespace_option is None:
if self.display_option == DISPLAY_ALL:
ns_list = namespaces['front_ns'] + namespaces['back_ns']
else:
ns_list = namespaces['front_ns']
else:
ns_list = [self.namespace_option]
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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

return ns_list

def multi_asic_ns_choices():
Copy link
Copy Markdown
Contributor

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.

if not is_multi_npu() :
return []
choices = get_namespaces()
return choices

def multi_asic_display_choices():
if not is_multi_npu():
return [DISPLAY_ALL]
else:
return [DISPLAY_ALL, DISPLAY_EXTERNAL]

def multi_asic_display_default_option():
if not is_multi_npu():
return DISPLAY_ALL
else:
return DISPLAY_EXTERNAL



_multi_asic_click_options = [
click.option('--display', '-d', 'display', default=multi_asic_display_default_option(), show_default=True, type=click.Choice(multi_asic_display_choices()), help='Show internal interfaces'),
click.option('--namespace', '-n', 'namespace', default=None, type=click.Choice(multi_asic_ns_choices()), show_default=True, help='Namespace name or all'),
#click.option('--namespace', '-n', 'namespace', callback=multi_asic_ns_callback, show_default=True, help='Namespace name or all'),
]

def multi_asic_click_options(func):
for option in reversed(_multi_asic_click_options):
func = option(func)
return func

def run_on_all_asics(func):
'''
This decorator is used on the CLI functions which needs to be
run on all the namespaces in the multi ASIC platform
The decorator loops through all the required namespaces,
for every iteration, it connects to all the DBs and provides an handle
to the wrapped function.

'''
@functools.wraps(func)
def wrapped_run_on_all_asics(self, *args, **kwargs):
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

@smaheshm smaheshm Jul 31, 2020

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)

ns_list = self.multi_asic.get_ns_list_based_on_options()
for ns in ns_list:
self.multi_asic.current_namespace = ns
self.db = self.multi_asic.connect_dbs_for_ns(ns)
self.config_db = self.multi_asic.connect_config_db_for_ns(ns)
func(self, *args, **kwargs)
return wrapped_run_on_all_asics

def multi_asic_args(parser=None):
if parser is None:
parser = argparse.ArgumentParser(formatter_class=argparse.RawTextHelpFormatter)

parser.add_argument('-d','--display', default=DISPLAY_EXTERNAL, help='Display all interfaces or only external interfaces')
parser.add_argument('-n','--namespace', default=None, help='Display interfaces for specific namespace')
return parser