Add 'alias mode' support for show commands by paavaanan · Pull Request #298 · sonic-net/sonic-utilities

paavaanan · 2018-08-11T15:05:54Z

- What I did

Added portalias support for following commands. To enable alias output in CLI execute "sudo config interface_naming_mode alias" command and use the below commands.

show ip route
show ipv6 route
show interface transceiver eeprom
show arp
show lldp neighbors
show vlan brief
show vlan config

- How I did it

Implemented a seperate run_command_in_alias_mode function to change the results of linux commands from default interface name to vendor specific name. This function read each line of the output and replace the equivalent vendor names. To speed up the result for mutli-line output ( show ip route/ipv6 route/arp) we cache the alias_names from port_config.ini at init. This results in quick substitution.

- How to verify it
Execute the commands as described above

jleveque · 2018-08-15T00:35:49Z

show/main.py

    def __init__(self):
        self.path = os.getcwd()
        self.aliases = {}
+        global port_dict


Prefer not to introduce new global variables if possible. Since you're initializing this in the Config class, why not make it a member of the class? Or better yet, create a new class for name-to-alias translation, and make this a member and interface_name_to_alias() a method of that class.

Addressed in the new commit

show/main.py

jleveque · 2018-08-15T00:39:12Z

show/main.py

+        for port_name in port_dict.keys():
+            if interface_name == port_name:
+                return port_dict[port_name]['alias']
+        print "Invalid interface {}".format(interface_name)


Use click.echo() instead of print for consistency.

Addressed in the new commit

jleveque · 2018-08-15T00:39:18Z

show/main.py

+                if interface_name != "":
+                    output = output.replace(interface_name,
+                                    interface_name_to_alias(interface_name))
+                print output.rstrip('\n')


Use click.echo() instead of print for consistency.

Addressed in the new commit

jleveque · 2018-08-15T00:42:44Z

show/main.py

+
+
+def run_command_in_alias_mode(command, linux_command=False):
+    """Replace command output results from default


Clarify docstring. Suggest: "Run command and replace all instances of SONiC interface names in output with vendor-specific interface aliases."

Addressed in the new commit

jleveque · 2018-08-15T00:48:17Z

show/main.py

+             break
+        if output:
+            if linux_command:
+                result = re.findall('Ethernet\w+', output)


This assumes all SONiC interfaces names begin with "Ethernet". This is not necessarily true. It is also possible that vendor aliases begin with "Ethernet", so you will try to convert those, as well. This should instead try to find instances of all SONiC interface names configured for the current device.

Addressed in the new commit

show/main.py

jleveque · 2018-08-15T00:53:38Z

show/main.py

                r.append(data[k]['vlanid'])
-                r.append(m)
+                if get_interface_mode() == "alias":
+                    alias =  interface_name_to_alias(m)


Delete extra space after =

Addressed in the new commit

paavaanan · 2018-08-22T13:16:43Z

In this new pull request implemented portalias feature for sonic-specific commands mentioned below:

show pfc counters
show queue counters
show interface status
show interface description
show interface transceiver lpmode
show interface transceiver presence
show interface transceiver eeprom
show lldp table
show mac

As discussed implemented an Alias class for most of the portalias related functions. Most the above commands are in tabulated form. In function run_command_in_alias_mode else part handles above commands. Above command reusults consist of 3 parts. Header, Underline, and tabulate data. These are splitted into different categories and handled accordingly.

Header------------> Adjusted spacing with the help of vendor_name_length variable
Underline---------> Introduced index variable to pick the column to be adjusted.  
Data--------------> String replace default name to alias name.

jleveque · 2018-08-22T17:56:17Z

show/main.py

            pass


+class Alias(object):


Please give class a more descriptive name, something along the lines of "InterfaceAliasConverter"

Addressed in the new commit...

jleveque · 2018-08-22T17:57:11Z

show/main.py



+class Alias(object):
+    """Object to Alias conversion"""


What is "Object"? Description should be more along the lines of "Class which handles conversion between interface name and alias"

Addressed in the new commit

jleveque · 2018-08-22T17:58:04Z

show/main.py

+    """Object to Alias conversion"""
+
+    def __init__(self):
+        self.vendor_name_length = 0


Rename: vendor_name_max_length

Addressed in the new commit

jleveque · 2018-08-22T18:00:39Z

show/main.py

+                if interface_name == port_name:
+                    return self.port_dict[port_name]['alias']
+        click.echo("Invalid interface {}".format(interface_name))
+


Remove blank line 67. Consider adding blank line between 65 and 66.

jleveque · 2018-08-22T18:00:58Z

show/main.py

+                if interface_alias == self.port_dict[port_name]['alias']:
+                    return port_name
+        click.echo("Invalid interface {}".format(interface_alias))
+


Remove blank line 78. Consider adding blank line between 76 and 77.

jleveque · 2018-08-22T18:06:50Z

show/main.py

+                output = output.lstrip()
+                index = 0
+
+                if command_type == 1:


command_type values as simple integers are not clearly defined (what does "1" mean? etc.). This makes them confusing. Also, if we have one "command_type" for each command, this will become unwieldy. It would be better if you could key off the command string itself, and not rely on an integer which represents the command.

This is good one. Initially I thougt of using enums, but due to lack of package I ruled out the option. As suggested I used the command strings directly in the function and included in the latest commit.

jleveque · 2018-08-29T19:46:02Z

show/main.py

+
+
+# Global Alias object
+_alias = InterfaceAliasConverter()


Change variable name to be more descriptive and reflect new class name.

Addressed in the new commit..

jleveque · 2018-08-29T19:49:11Z

show/main.py

+               self.vendor_name_max_length = len(
+                    self.port_dict[port_name]['alias'])
+
+    def interface_name_to_alias(self, interface_name):


Now that the class name has changed to "InterfaceAliasConverter", I think you can remove "interface" from the method name (e.g., name_to_alias(self, interface_name):)

jleveque · 2018-08-29T19:49:26Z

show/main.py

+        click.echo("Invalid interface {}".format(interface_name))
+        raise click.Abort()
+
+    def interface_alias_to_name(self, interface_alias):


Now that the class name has changed to "InterfaceAliasConverter", I think you can remove "interface" from the method name (e.g., alias_to_name(self, interface_alias):)

Addressed in the new commit..

jleveque · 2018-08-29T19:51:42Z

show/main.py

+                    elif output.startswith("Port Rx"):
+                        output = output.replace("Port Rx", "Port Rx".rjust(
+                                                _alias.vendor_name_max_length))
+                elif (command == "sudo sfputil show eeprom") or(


Can you simplify this to elif command.startswith("sudo sfputil show"):?

Addressed in the new commit..

jleveque · 2018-08-29T20:00:41Z

show/main.py

+        if output == '' and process.poll() is not None:
+            break
+        if output:
+            if linux_command:


Is there a way to make Linux commands the default (i.e., "else") case here, such that if the command doesn't match any of the if/elif cases, it is treated as a Linux command? This would eliminate the need for the linux_command parameter.

I like the idea. But, without the linux_command it triggers lot of confusion in replacing interfaces from default to alias. Currently we are using different substitution methods for linux commands and sonic utilities. Without this differentiation we are forced to club the logic. This is an overhead to take care all possibilities. Would you like to do this approach?

Could you explain what you mean by "we are forced to club the logic"? My thought is that if the command doesn't match any of the specific if command == ... clauses, it would fall through into a "default" case, and this case would handle all Linux commands.

It would allow us to remove a lot of duplicate code such as this:

if get_interface_mode() == "alias": run_command_in_alias_mode(cmd, linux_command=True) else: run_command(cmd, display_cmd=verbose)

And instead, be able to check the alias mode inside run_command(), and if alias mode is enabled, then run_command() would call run_command_in_alias_mode().

Joe, the linux/sonic commands use 2 different logics in replacing the interface names. I tested the else style of approach what you suggested. I moved the sonic_utitilies replacement logic inside a generic function "print_output_in_alias_mode" and included in most of the "if" statements.

if command == "portstat": """Show interface counters""" if output.startswith("IFACE"): output = output.replace("IFACE", "IFACE".rjust( _interface.vendor_name_max_length)) print_output_in_alias_mode(output, index)<---- elif command == "pfcstat": """Show pfc counters""" if output.startswith("Port Tx"): output = output.replace("Port Tx", "Port Tx".rjust( _interface.vendor_name_max_length)) elif output.startswith("Port Rx"): output = output.replace("Port Rx", "Port Rx".rjust( _interface.vendor_name_max_length)) print_output_in_alias_mode(output, index)<----

Since, sonic_utitilies replacement logic is safely binded inside the if statements now it is free to introduce else for linux_utitlites commands. This approach works fine.

I agree with checking alias mode inside the run_command. Will raise pull request post testing.

jleveque · 2018-09-06T17:52:57Z

show/main.py

    if display_cmd:
        click.echo(click.style("Command: ", fg='cyan') + click.style(command, fg='green'))

+    if get_interface_mode() == "alias" and not command.startswith("intfutil"):


Suggest to add a comment explaining that we don't translate the output of intfutil because it already displays both the name and alias for each interface.

Addressed as above

show/main.py

jleveque · 2018-09-06T18:05:05Z

show/main.py

+        word[index] = underline
+        output = '  ' .join(word)
+
+    # Replace default interface name to vendor-name


# Replace SONiC interface name with vendor alias

jleveque · 2018-09-09T18:45:16Z

show/main.py

+    """Class which handles conversion between interface name and alias"""
+
+    def __init__(self):
+        self.vendor_name_max_length = 0


Please rename to "alias_max_length"

Addressed in new commit!

jleveque · 2018-09-10T18:04:11Z

show/main.py

+                    self.port_dict[port_name]['alias'])
+
+    def name_to_alias(self, interface_name):
+        """Return alias interface name if default


Clarify comment: "Return vendor interface alias if SONiC interface name is given as argument"

jleveque · 2018-09-10T18:04:40Z

show/main.py

+        raise click.Abort()
+
+    def alias_to_name(self, interface_alias):
+        """Return default interface name if alias name is given as argument


Clarify comment: "Return SONiC interface name if vendor port alias given as argument"

jleveque · 2018-09-10T20:33:27Z

show/main.py

    body = []

    if interfacename is not None:
+


Remove blank line

show/main.py

jleveque · 2018-09-14T18:41:03Z

Is there a reason you didn't add alias-to-name conversion for the "show interfaces transceiver ..." commands? If not, please add it.

Also, please fix the merge conflict (it's only whitespace-related).

Could you also please add a new command, show interfaces naming-mode which displays either "default" or "alias" depending on what is currently set?

paavaanan · 2018-09-15T11:07:01Z

transciever support is not added due to the lack of xvcrd implemetation in Dell boxes. Since it is added recently, I can able to test with the new change. Issue addresed in new commit.

Addressed space conflicts and added "show interface naming_mode" command support in new commit..

jleveque · 2018-09-17T17:54:37Z

show/main.py

+                                iface_alias_converter.alias_max_length))
+                print_output_in_alias_mode(output, index)
+
+            elif command == "sudo sfputil show eeprom":


This should be command.startswith("sudo sfputil show eeprom") because the command can take flags and an optional interface argument.

You also need to add code to translate the optional interface name from vendor alias to SONiC name in the "show interfaces transceiver ..." commands on lines 506-548 like you did in the other commands (see lines 560-562 for reference).

Addressed and updated in new commit.. 👍

qiluo-msft · 2018-10-18T22:23:56Z

This will break 'show interface status' in vs, please check.

root@346360b6760c:/# show interface status
Traceback (most recent call last):
  File "/usr/local/bin/sonic-cfggen", line 256, in <module>
    main()
  File "/usr/local/bin/sonic-cfggen", line 240, in main
    print(json.dumps(FormatConverter.to_serialized(data[args.var_json]), indent=4, cls=minigraph_encoder))
KeyError: 'PORT'
Traceback (most recent call last):
  File "/usr/bin/show", line 9, in <module>
    load_entry_point('sonic-utilities==1.2', 'console_scripts', 'show')()
  File "/usr/local/lib/python2.7/dist-packages/pkg_resources/__init__.py", line 484, in load_entry_point
    return get_distribution(dist).load_entry_point(group, name)
  File "/usr/local/lib/python2.7/dist-packages/pkg_resources/__init__.py", line 2714, in load_entry_point
    return ep.load()
  File "/usr/local/lib/python2.7/dist-packages/pkg_resources/__init__.py", line 2332, in load
    return self.resolve()
  File "/usr/local/lib/python2.7/dist-packages/pkg_resources/__init__.py", line 2338, in resolve
    module = __import__(self.module_name, fromlist=['__name__'], level=0)
  File "/usr/lib/python2.7/dist-packages/show/main.py", line 193, in <module>
    iface_alias_converter = InterfaceAliasConverter()
  File "/usr/lib/python2.7/dist-packages/show/main.py", line 54, in __init__
    self.port_dict = json.loads(p.stdout.read())
  File "/usr/lib/python2.7/json/__init__.py", line 338, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 366, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 384, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

jleveque · 2018-10-19T00:05:03Z

@qiluo-msft: The root cause was due to the lack of a "PORT" table in ConfigDB on the virtual switch. The "PORT" table will be populated by this PR: sonic-net/sonic-buildimage#2169

nikos-github · 2018-12-05T18:56:50Z

@paavaanan @jleveque Your commit is breaking 'show interface status', 'show platform summary' and other cmds. Alias is an optional field. See #403

show commands with alias support for linux utilities

ab66306

jleveque suggested changes Aug 15, 2018

View reviewed changes

show commands with alias support for sonic utilities

e33af6f

jleveque suggested changes Aug 22, 2018

View reviewed changes

added alias class support for vlan commands

cd42d7b

stcheng added the Enhancement label Aug 27, 2018

Addressed review comments for show commands

c6d0c46

jleveque suggested changes Aug 29, 2018

View reviewed changes

paavaanan added 3 commits August 30, 2018 13:01

show command changes with additional alias commands

3161fc2

removal of unwated code

a38bcdb

removal of linux_cmd variable and retaining run_cmd()

867047c

jleveque suggested changes Sep 6, 2018

View reviewed changes

paavaanan added 2 commits September 7, 2018 08:05

show command class instance name change

04f747e

renaming convert_interface to iface_alias_converter

03521a5

jleveque reviewed Sep 9, 2018

View reviewed changes

renamed variable to alias_max_length

dee1c31

jleveque suggested changes Sep 10, 2018

View reviewed changes

Rephrased comments

8a33b82

jleveque suggested changes Sep 11, 2018

View reviewed changes

show/main.py Show resolved Hide resolved

added naming_mode command support and transeiver

2080fe8

jleveque suggested changes Sep 17, 2018

View reviewed changes

show interface transceiver eeprom/lmode/presence support alias mode

b733b55

jleveque approved these changes Sep 20, 2018

View reviewed changes

jleveque changed the title ~~show commands with alias support for linux utilities~~ Add 'alias mode' support for show commands Sep 27, 2018

jleveque merged commit b8a62d7 into sonic-net:master Sep 27, 2018

nikos-github mentioned this pull request Dec 5, 2018

show interface status, show platform summary and other cmds are broken #403

Closed

davidpil2002 mentioned this pull request Nov 28, 2022

Secure Boot HLD sonic-net/SONiC#1028

Merged



		def run_command_in_alias_mode(command, linux_command=False):
		"""Replace command output results from default

Conversation

paavaanan commented Aug 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paavaanan commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paavaanan Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleveque Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

paavaanan Aug 30, 2018 •

edited

Loading

jleveque Sep 4, 2018 •

edited

Loading