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

Common functions for show CLI support on multi ASIC #999

Merged
merged 10 commits into from
Aug 14, 2020
9 changes: 9 additions & 0 deletions utilities_common/constants.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#All the constant used in sonic-utilities

DEFAULT_NAMESPACE = ''
DISPLAY_ALL = 'all'
DISPLAY_EXTERNAL = 'frontend'
BGP_NEIGH_OBJ = 'BGP_NEIGH'
PORT_CHANNEL_OBJ = 'PORT_CHANNEL'
PORT_OBJ = 'PORT'

138 changes: 138 additions & 0 deletions utilities_common/multi_asic.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
import argparse
import functools

import click
from sonic_py_common import multi_asic
from utilities_common import constants


class MultiAsic(object):
Copy link
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
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=constants.DISPLAY_ALL,
namespace_option=None):
self.namespace_option = namespace_option
self.display_option = display_option
self.current_namespace = None
self.is_multi_asic = multi_asic.is_multi_asic()

def is_object_internal(self, object_type, cli_object):
'''
The function checks if a CLI object is internal and returns true or false.
Internal objects are port or portchannel which are connected to other
ports or portchannels within a multi ASIC device.

For single asic, this function is not applicable
'''
if object_type == constants.PORT_OBJ:
return multi_asic.is_port_internal(cli_object)
elif object_type == constants.PORT_CHANNEL_OBJ:
return multi_asic.is_port_channel_internal(cli_object)
elif object_type == constants.BGP_NEIGH_OBJ:
return multi_asic.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 self.is_multi_asic:
return False
if self.display_option == constants.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 self.is_multi_asic:
return [constants.DEFAULT_NAMESPACE]
else:
namespaces = multi_asic.get_all_namespaces()
if self.namespace_option is None:
if self.display_option == constants.DISPLAY_ALL:
ns_list = namespaces['front_ns'] + namespaces['back_ns']
else:
ns_list = namespaces['front_ns']
else:
if self.namespace_option not in namespaces['front_ns'] and \
self.namespace_option not in namespaces['back_ns']:
raise ValueError(
'Unknown Namespace {}'.format(self.namespace_option))
ns_list = [self.namespace_option]
return ns_list


def multi_asic_ns_choices():
Copy link
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 multi_asic.is_multi_asic():
return [constants.DEFAULT_NAMESPACE]
choices = multi_asic.get_namespace_list()
return choices


def multi_asic_display_choices():
if not multi_asic.is_multi_asic():
return [constants.DISPLAY_ALL]
else:
return [constants.DISPLAY_ALL, constants.DISPLAY_EXTERNAL]


def multi_asic_display_default_option():
if not multi_asic.is_multi_asic():
return constants.DISPLAY_ALL
else:
return constants.DISPLAY_EXTERNAL


_multi_asic_click_options = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I"m not sure how this works, is it possible to not display these options on single ASIC platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe Click 7.0 added a hidden keyword. However, Click 6.x (which is what we have in 201911) does not have support for this, which would make backporting a bit tedious. I believe @judyjoseph was working with it recently.

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'),
]


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


def run_on_multi_asic(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
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
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
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 = multi_asic.connect_to_all_dbs_for_ns(ns)
self.config_db = 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=constants.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