[config] Add support for accepting port alias in lieu of port name by paavaanan · Pull Request #260 · sonic-net/sonic-utilities

paavaanan · 2018-05-30T12:51:01Z

- What I did
Implemented portalias feature for two basic commands "config interface shutdown/startup"

-Introduced new CLI to configure the interface mode:
-config interface_mode "alias"
-config interface_mode "defaut"

User can stick to any mode. Mode selection is unique for each sessions. Default mode stick to sonic ethernet interface style and Alias mode stick to individual switch vendor interface style. User selection sticks to the current logged session. If multiple login session do exists it maintains the user selection seperately.

- How I did it

Introduced convert_interface_name() function to toggle interfaces between alias/default mode.
Included a termporary file for each individual session to maintain the mode selection.
interface_mode.sh script is included in /etc/profile.d to create a unique file for each session at the time of login.

- How to verify it

config interface_mode alias
config interface shutdown fortyGigE1/1/7
config interface_mode default
config interface startup Ethernet6

--> interface_mode.sh is raised in sonic-net/sonic-buildimage#1751

jleveque · 2018-06-05T17:57:55Z

config/main.py

+
+def set_interface_mode(mode):
+    tmp = "/tmp/"
+    output = subprocess.check_output(['tty']).strip('\n')


After our discussion earlier, I understand that the interface_status.sh script in PR sonic-net/sonic-buildimage#1751 will remove stale entries upon a new tty connection. However, I am still concerned that this approach is unnecessarily complicated.

Why not simply set an environment variable per user (e.g., export SONIC_CLI_ALIAS_MODE="true")? Using this approach, we could also cause the setting to persist between sessions by writing this to the user's .profile file. If the variable is either "false" or not defined, we assume input will be SONiC port names (the default behavior). If the variable is "true" we assume the input will be port aliases.

Agreed. Initially we approched the issue in the above context. Since python is the child process of parent bash (as like for any sessios) we can inherit the status of enviroment variable say SONIC_CLI_ALIAS_MODE to python but we can't revert back the state to TRUE/FALSE back to bash environment variable. . The change do exists until the script terminates. SONIC_CLI_ALIAS_MODE value cannot be exported to parent bash.

Ah, yes ... you can't modify the environment of the parent shell. That's a problem. If we wanted to proceed with this approach, I guess we would need to prompt the user to log out and log back in again for the change to take effect; I think that's quite an inconvenience.

@lguohan: Do you have any alternative suggestions? Otherwise, I believe the tty-based approach may be our best bet.

jleveque · 2018-06-05T18:04:54Z

config/main.py

    if proc.returncode != 0 and not ignore_error:
        sys.exit(proc.returncode)

+def convert_interface_name(interface_name):


This function will blindly convert a port name to a port alias and vice-versa. This could pose issues if a device shares the same string for both a port name and a port alias, as it will always find a match on line 49 and return that port's alias.

I think it is better to have two functions with distinct functionality: interface_name_to_alias(interface_name) and interface_alias_to_name(interface_alias).

Introduced interface_alias_to_name() to convert the alias to default mode. In the current code change, there is no any need for interface_name_to_alias() conversion. While implementing the feature for other subset of commands if needed will implement on need basis.

jleveque · 2018-06-13T01:26:19Z

config/main.py

    if proc.returncode != 0 and not ignore_error:
        sys.exit(proc.returncode)

+def interface_alias_to_name(interface_name):


The parameter should be called interface_alias. Otherwise it's very confusing.

paavaanan · 2018-07-03T13:30:54Z

• Modified the design to user based approach. Harnessed the power of /etc/sudoers and /etc/environment file to retain the environment variable value even in sudo sessions. So, when the user request the change from default mode to alias mode, the action is completed in two phases.
• config/main.py script fetch the current value of IFMODE and modify as user requested alias mode and write in bashrc.
• When the user logout and login the session bashrc update the environment variable.
• This works for both root, and non-root sessions.

paavaanan · 2018-07-09T07:17:03Z

In this latest commit, addressed review comments.
Maintained uniformity in naming convention.
Environement variable IFMODE alias/default switch code is refined.

port_alias_config.log

jleveque · 2018-07-10T22:30:06Z

config/main.py

+    f = open(bashrc,'w')
+    f.write(newdata)
+    f.close()
+    print "Please logout and login again!"


Suggest change to "Please log out and log back in for changes take effect."

jleveque · 2018-07-10T23:19:03Z

config/main.py

+            if interface_name == port_dict[port_name]['alias']:
+                return port_name
+        print "Invalid interface {}".format(interface_name)
+        sys.exit(0)


As this is a utility function, I'm not sure it should cause the program to exit. Maybe return None and have the caller exit if it does?

comment addressed below

jleveque · 2018-07-10T23:19:19Z

config/main.py

+            if interface_name == port_name:
+                return port_dict[port_name]['alias']
+        print "Invalid interface {}".format(interface_name)
+        sys.exit(0)


As this is a utility function, I'm not sure it should cause the program to exit. Maybe return None and have the caller exit if it does?

shutdown command as an example:

def shutdown(interface_name, verbose): """Shut down interface""" print "lias" if get_interface_mode() == "alias": interface_name = interface_alias(interface_name)

If we fail to return sys.exit(0) in function interface_alias() it propogates to next line and execute "ip link set <interface_nam> down. Where interface_name is invalid.

This is unnecessary and tends to double opertaion. To avoid this sys.exit(0) introduced. and also it alerts the user about invalid interface.

If you are skeptical to the exit in utiltiy function we can shift the exit in parent shutdown().

command = "ip link set {} down".format(interface_name) run_command(command, display_cmd=verbose)

Yes. I think the caller functions should check for None, print an error and exit.

Will addres the changes

jleveque · 2018-07-10T23:21:53Z

config/main.py

    if proc.returncode != 0 and not ignore_error:
        sys.exit(proc.returncode)

+def interface_alias(interface_name):


This was not the change I requested. I requested you change the parameter name to interface_alias, not the function name. It should be def interface_alias_to_name(interface_alias):

Thanks for the clarification

jleveque · 2018-07-10T23:24:20Z

config/main.py

+
+    port_dict = json.loads(p.stdout.read())
+
+    if interface_name is not None:


What happens in the case where interface_name is None?

The interface_name validation is done by pthon click package at the initial phase of the function it throws missing argument error.

admin@sonic:~$ sudo config interface shutdown None Invalid interface None

admin@sonic:~$ sudo config interface shutdown Usage: config interface shutdown [OPTIONS] <interface_name> Error: Missing argument "interface_name".

I wasn't asking what happens if a user provides the string "None" as the interface name.

My question is regarding the fact that you have an if statement (if interface_name is not None:) which returns a value but no corresponding else statement. Thus, if interface_name is of type None (not the string "None"), this function will not return anything. I think this was overlooked, so I was asking for an explanation to see if this is the behavior you expect.

I wasn't asking what happens if a user provides the string "None" as the interface name.

My question is regarding the fact that you have an if statement (if interface_name is not None:) which returns a value, but there is no corresponding else statement. Thus, if interface_name is of type None (not the string "None"), this function will not return anything. I think this was overlooked, so I was asking for an explanation to see if this is the behavior you expect.

Joe I got your point. When the interface_name variable type is None, even though else is not defined inside the corresponding function the return type of interface_alias_to_name() will be None. All parent functions are equipped to handle this case. So, Invalid Interface message will be raised to the user. Even if we add an else the behavior will be the same.

"""Shut down interface""" if get_interface_mode() == "alias": interface_name = interface_alias_to_name(interface_name) if interface_name is None: print "Invalid interface {}".format(interface_name)<-------- raise click.Abort() )

admin@sonic:~$ sudo config interface shutdown hundredGigE1/1/120 Invalid interface hundredGigE1/1/120 Aborted! admin@sonic:~$

I still believe that every path should explicitly return a value. Otherwise the behavior of the function isn't easily determined by the reader. I think you can easily fix this as follows:

if interface_alias is not None: for port_name in natsorted(port_dict.keys()): if interface_alias == port_dict[port_name]['alias']: return port_name print "Invalid interface {}".format(interface_alias) return None

jleveque · 2018-07-11T00:21:27Z

config/main.py

+    print "Please logout and login again!"
+
+def get_interface_mode():
+    mode = os.getenv('IFMODE')


What if the environment variable is not set? You should probably return "default" in this case so that this function consistently returns a value.

Will address this.

jleveque · 2018-07-11T00:27:26Z

config/main.py

+
+@cli.group()
+def interface_mode():
+    """Interface mode change tasks"""


Suggest """Modify interface naming mode for interacting with SONiC CLI"""

jleveque · 2018-07-11T00:28:14Z

config/main.py

+
+@interface_mode.command('default')
+def interface_mode_default():
+    """Set interface mode to DEFAULT"""


Suggest """Set CLI interface naming mode to DEFAULT (SONiC port name)"""

jleveque · 2018-07-11T00:29:03Z

config/main.py

+
+@interface_mode.command('alias')
+def interface_mode_alias():
+    """Set interface mode to ALIAS"""


Suggest """Set CLI interface naming mode to ALIAS (Vendor port alias)"""

jleveque · 2018-07-11T00:31:53Z

config/main.py

+#
+
+@cli.group()
+def interface_mode():


Suggest change to interface_naming_mode()

jleveque · 2018-07-16T18:40:05Z

config/main.py

-    if not sudo_user:
-        bashrc = "/{}/.bashrc".format(user)
+    user = os.getenv('SUDO_USER')
+    bashrc_ifacemode_line = "SONIC_CLI_IFACE_MODE={}".format(mode)


I think this line is best moved down just above line 87, right before it is used. That will also keep all of the user name determination code together.

jleveque · 2018-07-16T18:42:09Z

config/main.py

-        newdata = re.sub(r"IFMODE=\w+",set_mode,filedata)
-
+        newdata = re.sub(r"SONIC_CLI_IFACE_MODE=\w+", bashrc_ifacemode_line, filedata)
+    


Delete whitespace.

jleveque · 2018-07-16T18:46:19Z

config/main.py


    if get_interface_mode() == "alias": 
-        interface_name = interface_alias(interface_name)
+        default_name = interface_alias_to_name(interface_name)


The introduction of the variable default_name is unnecessary. Simply assign directly to interface_name and compare it to None. You can then also eliminate the else clause:

interface_name = interface_alias_to_name(interface_name) if interface_name is None: print "Invalid interface {}".format(interface_name) raise click.Abort()

jleveque · 2018-07-16T18:47:27Z

config/main.py

-        interface_name = interface_alias(interface_name)
+        default_name = interface_alias_to_name(interface_name)
+        if default_name is None:
+            print "Invalid interface {}".format(interface_name)


interface_name is not defined. This will crash. However, instead of replacing it with default_name, please refactor the code as I suggested in my comment on line 411.

How do this crash? interface_name is the incoming argument for add_vlan_member() So, argument do have a value when invoked.

def add_vlan_member(ctx, vid, interface_name, untagged):

You're correct. Technically, Click should prevent interface_name from being None. I think I got a bit lost due the introduction of default_interface. Ignore this comment. #Closed

jleveque · 2018-07-16T18:50:02Z

config/main.py

            alias_name = interface_name_to_alias(interface_name)
+            if alias_name is None:
+                print "Invalid interface {}".format(interface_name)
+                sys.exit(0)


Please replace sys.exit(0) raise click.Abort(), which will cause the program to exit with code 1.

jleveque · 2018-07-16T18:51:13Z

config/main.py


    if get_interface_mode() == "alias": 
-        interface_name = interface_alias(interface_name)
+        default_name = interface_alias_to_name(interface_name)


Please refactor this code in the same manner as I suggested in my comment on line 411.

jleveque · 2018-07-16T18:52:38Z

config/main.py

    """Shut down interface"""
    if get_interface_mode() == "alias": 
-        interface_name = interface_alias(interface_name)
+        default_name = interface_alias_to_name(interface_name)


Please refactor this code in the same manner as I suggested in my comment on line 411.

jleveque · 2018-07-16T18:52:43Z

config/main.py

    """Start up interface"""
    if get_interface_mode() == "alias": 
-        interface_name = interface_alias(interface_name)
+        default_name = interface_alias_to_name(interface_name)


Please refactor this code in the same manner as I suggested in my comment on line 411.

jleveque · 2018-07-16T18:52:47Z

config/main.py

    """Set interface speed"""
    if get_interface_mode() == "alias": 
-        interface_name = interface_alias(interface_name)
+        default_name = interface_alias_to_name(interface_name)


Please refactor this code in the same manner as I suggested in my comment on line 411.

jleveque · 2018-07-16T18:59:07Z

portalias/PORTALIAS.md

@@ -0,0 +1,75 @@
+#Portalias


A design doc of this sort doesn't belong in this repo alongside the code; if anything it should be placed in the GitHub wiki. Please delete this file.

paavaanan · 2018-08-07T06:46:20Z

@jleveque addressed all review comments and raised 2 commits with all changes.
Please, have a look on this.

jleveque · 2018-08-07T21:26:27Z

config/main.py

        bashrc = "/home/{}/.bashrc".format(user)
    else:
-        sys.exit(0)
+        raise click.Abort
