EOS syntax change updates

[bewing]

Arista has released FN 0039 (available to Arista customers with support
contract) detailing changes to command syntax that impacts several show
commands used by NAPALM beginning in EOS Release 4.23.0. NAPALM will
re-try specific show commands if a CommandError is raised with the newer
syntax.

[bewing]

Items for discussion:

• Is this the right approach?
  • Should we change the driver to check version on open, and choose command sets appropriately?
  • Should we shim run_commands, and catch CommandErrors for deprecated commands, and feed it a set of regexeps for old-> new replacements?
• How do we test this?
  • Update EOS Mock driver to raise CommandError if specific text is present in mock files?

[coveralls]

Coverage remained the same at 0.0% when pulling e28ac71 on fn-0039 into 9731790 on develop.

[coveralls]

Coverage decreased (-0.2%) to 81.288% when pulling 81090d9 on fn-0039 into 2d0f77e on develop.

[mirceaulinic]

That'd be perfectly fine by me to go as-is, although I fear that catching an exception whenever (even when it might be a genuine error), might mask actual issues. What if have an internal method, say _run_commands that has a hash with the possible commands to be remapped when the EOS version is >= 4.23? Something like (very rudimentary example):

def _run_commands(self, cmds):
    convert = {
      # old command : new command
      'show user-account': 'show user accounts'
    }
    commands = []
    for cmd in cmds:
        if cmd in convert:
            commands.append(conver[cmd])
        else:
             commands.append(cmd)
    return self.device.run_commands(commands)

Do you think that'd work?

[ktbyers]

Yes, I agree with @mirceaulinic here that it would probably be better if we had this centeralized in the code i.e. some sort of dictionary and a version check.

[bewing]

Based on earlier discussions, I believe we wanted to try to avoid blindly changing user input / what is being sent to switches. Perhaps something like

def _run_commands(self, cmds):
    convert = {
      # old command : new command
      'show user-account': 'show user accounts'
    }
    commands = []
    try:
        return self.device.run_commands(cmds)
    except pyeapi.eapilib.CommandError as e:
        # Check e for bad commands and convert individually
        ...
        return self.device.run_commands(cmds)

[ktbyers]

Yes, good point. I apologize if I contradicted something I said earlier also...

I was assuming we would only be doing this for hard-coded commands that were used in napalm getters and not for config commands nor for cli (i.e. arbitrary user commands).

So for config commands--I think the issue should be punted onto the end-user (i.e. they need to get their commands correct for their OS-version). Same for cli commands (i.e. what you specify is what you get).

So that leaves what do we do for getters where we have one set of hard-coded commands that won't work in certain contexts.

[bewing]

The other unfortunate part here is that the new commands don't work in older EOS versions -- so we can't blindly use the new commands unless we put a version gate on the EOS driver, and state that older versions aren't supported. Is that something that we should consider? I don't think so, as the first version that supports the new syntax was released less than a year ago.

[tcaiazza]

What about adding something to the open function to determine the version number like this below and storing that version number

# let's try to run a very simple command
self.device.run_commands(["show clock"], encoding="text")
version = self.device.run_commands(["show version"])[0]
major, minor, _ = version.get("version").split(".")
self.verion_number = int(major) + int(minor) * .01

Then the new _run_commands function could be something like this

def _run_commands(self, cmds):
    convert = {
      # old command : new command
      'show user-account': 'show user accounts'
    }
    commands = []
    # Convert commands to 4.23+ syntax
    if self.version_number >= 4.23
        cmds = [i if i not in convert.keys() else convert[i] for i in cmds]
    return self.device.run_commands(cmds)

[mirceaulinic]

Sorry, I dropped the ball here.

I believe we wanted to try to avoid blindly changing user input / what is being sent to switches.

My comment was probably misleading - let me try to clarify a little: I meant _run_commands to be used exclusively by NAPALM internally (of course, without restricting access if the user wants to use it), so the methods would use it out of the box; arbitrary CLI commands via the cli method shouldn't be affected. Does this make more sense?

Additionally, I think I like @tcaiazza's suggestion. We could even consider an optional argument if we want to provide a little more flexibility to toggle this feature.

[tcaiazza]

Having just learned about distutils.version.LooseVersion, I would change my suggestion to this

# let's try to run a very simple command
self.device.run_commands(["show clock"], encoding="text")
version = self.device.run_commands(["show version"])[0]['version']
self.version = distutils.version.LooseVersion(version)

and

def _run_commands(self, cmds):
    convert = {
      # old command : new command
      'show user-account': 'show user accounts'
    }
    commands = []
    # Convert commands to 4.23+ syntax
    if self.version_number >= distutils.version.LooseVersion("4.23")
        cmds = [i if i not in convert.keys() else convert[i] for i in cmds]
    return self.device.run_commands(cmds)

[mirceaulinic]

Hey @bewing - have you had a chance to think about the suggestions above? I think your implementation is probably fine either way, just would like to hear your thoughts on a broader use case. Cheers!

[bewing]

Sorry for the immense delay on this PR. Based on feedback, and internal work at $DAY_JOB, I've force-pushed a different changeset done by a co-worker of mine that implements a pyeapi.Node() shim to handle command translation -- take a look and provide some feedback.

(if we hate it, the old code is still in branch fn-0039_old)

[mirceaulinic]

I think this looks good overall. Just left a first-pass questions and nits. :]

[mirceaulinic]

Okay, I guess that looks good to me. Will leave it open till next week in case anyone would like to add or suggest anything, then should get this into 3.0.0.

[rbcollins123]

@mirceaulinic @bewing So since load_replace_candidate() relies upon run_commands(), we just had a situation where our rendered configs were being arbitrarily changed back to old syntax now using napalm 3.0.0. EOS 4.21.4F-4.22.X support old and new formats so we shouldn't arbitrarily swap designed configurations. It makes sense for things internal to napalm, but if someone is loading a config from Jinja2 template, napalm shouldn't affect the CLI, it should just load it into the config session as-is. This change will impact people upstream using napalm-ansible also if they're on any of those many interim releases depending upon where they are in their configuration migration. Converting candidate configs magically in the background shouldn't really be the job of napalm when loading configuration files.

Quick additional edit for clarity... If left in its current state, it will revert a devices config to old CLI commands for anyone that has already made the move to the new commands after v4.21.4F. If you change the behavior to send new commands for anyone using v4.21.4F or higher (instead of the current 4.23 cutoff), then it will convert people to new commands that may not have made the move yet that are still on 4.21.4F-4.22.X.

[mirceaulinic]

Thanks for reporting @rbcollins123.

just had a situation where our rendered configs were being arbitrarily changed back to old syntax now using napalm 3.0.0

Wondering if we should revisit this discussion: #1093 (comment) (i.e., whether we should offer the flip side, new -> old)?

I'm thinking maybe we'll want to add a new optional argument to toggle if the user wants the conversion in the config or not. Thoughts?

[rbcollins123]

I'm thinking maybe we'll want to add a new optional argument to toggle if the user wants the conversion in the config or not. Thoughts?

I haven't crawled the code yet, but my initial knee-jerk thought when I saw this behavior was to only use the conversion wrapper in an internal _run_commands method and change all internal uses to call that and then have the "public" api method keep the old behavior with no auto-conversion; that way the responsibility for passing valid commands in a call to run_commands() falls to the user who would know what they're automating against.

But an arg to control the behavior would work as well. To avoid changing the upstream API consumers, just default the arg to not auto-convert and then have internal methods use the arg where it makes sense.

[mirceaulinic]

Yep, I understood what you were saying from the first comment @rbcollins123. What I'm suggesting is an optional argument that lets this decision down to the user (i.e., if they want _run_commands to transform the config, or not -- which would default to what you were suggesting: won't transform the config, unless explicitly requested). Does this make more sense like that?

[rbcollins123]

Yep, makes sense to me

[mirceaulinic]

@rbcollins123 would you be able to test my #1212 patch and confirm that'd fix this issue? (@yeled you may be interested in this as well). Feedback welcome from anyone else watching this thread. Thanks!

[rbcollins123]

@rbcollins123 would you be able to test my #1212 patch and confirm that'd fix this issue? (@yeled you may be interested in this as well). Feedback welcome from anyone else watching this thread. Thanks!

FYI I tested and verified that the behavior is now same in v2.4.0, 2.5.0 and 3.0.1 when doing a full config-replace via napalm-ansible.