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

adding optional arguments for load_replace_candidate #508

Merged
merged 12 commits into from Nov 21, 2017

Conversation

@bharath-ravindranath
Copy link
Contributor

commented Nov 10, 2017

We need option for autoComplete, timeStamp and expandAliases

I ran into an issue where using load_replace_candidate failed because of the autoComplete is False

I had two commands back to back:
router bgp 1
router multicast

Since I had autoComplete False as default, the router multicast was considered as 'router-id multicast'.
This is only because 'router' is valid under bgp configuration.

Submitting a change here to add optional arguments

@coveralls

This comment has been minimized.

Copy link

commented Nov 10, 2017

Coverage Status

Coverage increased (+0.004%) to 77.819% when pulling 7ea7889 on bharath-ravindranath:EOSDriver into 8a356dd on napalm-automation:develop.

@dbarrosop

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

This is interesting and by interesting I mean what the fuck Arista! They didn't used to allow short commands and now not only they did add an option for that but they enable it by default leading to unexpected situation like the one you describe.

Looking at the PR where they implemented this feature (https://github.com/arista-eosplus/pyeapi/pull/123/files#diff-9f2621c7ea2151019c29cdd006e08f1fR398) I think this change is going to break code running against older EOS images.

So, basically, we will have to figure out which exact EOS version started supporting this and verify we are running it before passing this option.

@dbarrosop

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

Could you also let me know in which version you are seeing this behavior so I can try to replicate? I am still in shock this default changed so I'd like to see it by myself and maybe even complain to them :P

@bharath-ravindranath

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

You can complain directly to me. I am one of “them” :) :P
I will check the version details and let me know.

Just my thought: We are constantly improving the eAPI, and may not be up to some “other” vendor standards. So probably will have more changes in future

@dbarrosop

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

You can complain directly to me. I am one of “them” :) :P

I won't blame it on you directly, I promise ;)

Just my thought: We are constantly improving the eAPI, and may not be up to some “other” vendor standards. So probably will have more changes in future

That's good to hear, there a few improvements I could propose ;) However, even though improvements are good changing the behavior of things is not. Specially if that behavior couldn't be specified before.

I will check the version details

Yes, please, we need to gate this behavior on that version and I also want to test this personally to bring it up with my SE as this is not an OK change IMHO. I don't want my network to go down a few days after an upgrade because my automation hit a similar issue to the one you described.

Thanks for reporting this and welcome by the way :)

@bewing

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

For some clarity: This is related to a case I have open with Arista.

autoComplete is off by default in eAPI, and always has been:

In [6]: with get_network_driver('eos')('switch', 'bewing', p) as d:
   ...:     print(d.get_facts()['os_version'])
   ...:     print(d.device.run_commands(["show ip rout 0.0.0.0"]))
   ...:
4.16.9M-3799680.4169M
NAPALM didn't catch this exception. Please, fill a bugfix on https://github.com/napalm-automation/napalm/issues
Don't forget to include this traceback.
---------------------------------------------------------------------------
CommandError                              Traceback (most recent call last)
<ipython-input-6-9f68c4859e6b> in <module>()
      1 with get_network_driver('eos')('schcm00a', 'bewing', p) as d:
      2     print(d.get_facts()['os_version'])
----> 3     print(d.device.run_commands(["show ip rout 0.0.0.0"]))
      4

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/client.py in run_commands(self, commands, encoding, send_enable, **kwargs)
    728                 commands.insert(0, 'enable')
    729
--> 730         response = self._connection.execute(commands, encoding, **kwargs)
    731
    732         # pop enable command from the response only if we sent enable

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/eapilib.py in execute(self, commands, encoding, **kwargs)
    493             self.error = None
    494             request = self.request(commands, encoding=encoding, **kwargs)
--> 495             response = self.send(request)
    496             return response
    497

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/eapilib.py in send(self, data)
    412                     _LOGGER.error(auto_msg)
    413                     msg = msg + '. ' + auto_msg
--> 414                 raise CommandError(code, msg, command_error=err, output=out)
    415
    416             return decoded

CommandError: Error [1002]: CLI command 2 of 2 'show ip rout 0.0.0.0' failed: invalid command [incomplete token (at token 2: 'rout')]

expandAliases is on by default in eAPI (I think), and always has been, and expands built-in aliases as well (sh -> show). Arista please correct me here if I am wrong.

Update: sh-> show might be a special case. Review of JsonCli.py shows that expandAliases is also set to false by default.

In [9]: with get_network_driver('eos')('switch', 'bewing', p) as d:
   ...:     print(d.get_facts()['os_version'])
   ...:     print(d.device.run_commands(["sh ip route 0.0.0.0"]))
   ...:
4.16.9M-3799680.4169M
[{'vrfs': {'default': {'routes': {'0.0.0.0/0': {'kernelProgrammed': True, 'directlyConnected': False, 'preference': 1, 'routeAction': 'forward', 'vias': [{'interface': 'Management1', 'nexthopAddr': '10.203.3.254'}], 'metric': 0, 'hardwareProgrammed': True, 'routeType': 'static'}}, 'allRoutesProgrammedKernel': True, 'routingDisabled': True, 'allRoutesProgrammedHardware': True, 'defaultRouteState': 'reachable'}}}]

I ran into an issue where we cannot load a replacement configuration due to how the parser interprets tokens in various stanzas ("router multicast" stanza immediately follows "router bgp" stanza, and the parser tries to eat token "router" as "router-id" in the bgp stanza context instead of auto-completing to the global context of router):

In [14]: with get_network_driver('eos')('switch', 'bewing', p) as d:
    ...:     config = d.get_config()["running"]
    ...:     d.load_replace_candidate(config=config)
    ...:     d.discard_config()
    ...:
---------------------------------------------------------------------------
CommandError                              Traceback (most recent call last)
~/PycharmProjects/napalm/napalm/eos/eos.py in _load_config(self, filename, config, replace)
    163         try:
--> 164             self.device.run_commands(commands, autoComplete=self.autoComplete)
    165         except pyeapi.eapilib.CommandError as e:

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/client.py in run_commands(self, commands, encoding, send_enable, **kwargs)
    729
--> 730         response = self._connection.execute(commands, encoding, **kwargs)
    731

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/eapilib.py in execute(self, commands, encoding, **kwargs)
    494             request = self.request(commands, encoding=encoding, **kwargs)
--> 495             response = self.send(request)
    496             return response

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/eapilib.py in send(self, data)
    413                     msg = msg + '. ' + auto_msg
--> 414                 raise CommandError(code, msg, command_error=err, output=out)
    415

CommandError: Error [1002]: CLI command 1467 of 1650 'router multicast' failed: invalid command [incomplete token (at token 0: 'router')]

During handling of the above exception, another exception occurred:

ReplaceConfigException                    Traceback (most recent call last)
<ipython-input-14-153a7a21ef42> in <module>()
      1 with get_network_driver('eos')('schpl01', 'bewing', p) as d:
      2     config = d.get_config()["running"]
----> 3     d.load_replace_candidate(config=config)
      4     d.discard_config()
      5

~/PycharmProjects/napalm/napalm/eos/eos.py in load_replace_candidate(self, filename, config)
    172     def load_replace_candidate(self, filename=None, config=None):
    173         """Implementation of NAPALM method load_replace_candidate."""
--> 174         self._load_config(filename, config, True)
    175
    176     def load_merge_candidate(self, filename=None, config=None):

~/PycharmProjects/napalm/napalm/eos/eos.py in _load_config(self, filename, config, replace)
    166             self.discard_config()
    167             if replace:
--> 168                 raise ReplaceConfigException(e.message)
    169             else:
    170                 raise MergeConfigException(e.message)

ReplaceConfigException: Error [1002]: CLI command 1467 of 1650 'router multicast' failed: invalid command [incomplete token (at token 0: 'router')]

This patch allows us to set autoComplete to True to enable the workaround from the development team for this specific problem, only when loading configurations. While it does resolve this specific error, I have to go back to TAC because now it horks on my banner:

In [15]: with get_network_driver('eos')('switch', 'bewing', p, optional_args={"autoComplete": True}) as d:
    ...:     config = d.get_config()["running"]
    ...:     d.load_replace_candidate(config=config)
    ...:     d.discard_config()
    ...:
---------------------------------------------------------------------------
CommandError                              Traceback (most recent call last)
~/PycharmProjects/napalm/napalm/eos/eos.py in _load_config(self, filename, config, replace)
    163         try:
--> 164             self.device.run_commands(commands, autoComplete=self.autoComplete)
    165         except pyeapi.eapilib.CommandError as e:

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/client.py in run_commands(self, commands, encoding, send_enable, **kwargs)
    729
--> 730         response = self._connection.execute(commands, encoding, **kwargs)
    731

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/eapilib.py in execute(self, commands, encoding, **kwargs)
    494             request = self.request(commands, encoding=encoding, **kwargs)
--> 495             response = self.send(request)
    496             return response

~/.virtualenvs/napalm/lib/python3.6/site-packages/pyeapi/eapilib.py in send(self, data)
    413                     msg = msg + '. ' + auto_msg
--> 414                 raise CommandError(code, msg, command_error=err, output=out)
    415

CommandError: Error [1002]: CLI command 1629 of 1650 '************************************************************************' failed: invalid command [Invalid input (at token 0: '************************************************************************')]

During handling of the above exception, another exception occurred:

ReplaceConfigException                    Traceback (most recent call last)
<ipython-input-15-7b4485f54afb> in <module>()
      1 with get_network_driver('eos')('schpl01', 'bewing', p, optional_args={"autoComplete": True}) as d:
      2     config = d.get_config()["running"]
----> 3     d.load_replace_candidate(config=config)
      4     d.discard_config()
      5

~/PycharmProjects/napalm/napalm/eos/eos.py in load_replace_candidate(self, filename, config)
    172     def load_replace_candidate(self, filename=None, config=None):
    173         """Implementation of NAPALM method load_replace_candidate."""
--> 174         self._load_config(filename, config, True)
    175
    176     def load_merge_candidate(self, filename=None, config=None):

~/PycharmProjects/napalm/napalm/eos/eos.py in _load_config(self, filename, config, replace)
    166             self.discard_config()
    167             if replace:
--> 168                 raise ReplaceConfigException(e.message)
    169             else:
    170                 raise MergeConfigException(e.message)

ReplaceConfigException: Error [1002]: CLI command 1629 of 1650 '************************************************************************' failed: invalid command [Invalid input (at token 0: '************************************************************************')]
@mirceaulinic mirceaulinic added this to the DISCUSSION milestone Nov 12, 2017
@bharath-ravindranath

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

I just confirmed that the default behavior has not changed. i.e. the napalm library do not have to worry about the backward compatibility:

Example: Perviously the Arista pyeapi did not have auto complete. So commands like "sh ver" did not work. Which is still the default behavior

import pyeapi
conn = pyeapi.connect(host='IP',username='user', password='pw')
conn.execute(['show version'])
{'jsonrpc': '2.0', 'id': 'xyzzy', 'result': [{'modelName': 'DCS-7150S-52-CL-F', 'internalVersion': '4.19.2.1F-2GB-6619411.41921F', 'systemMacAddress': 'xyzzy', 'serialNumber': 'xyzzy', 'memTotal': 3963676, 'bootupTimestamp': 1510366564.69, 'memFree': 2333536, 'version': '4.19.2.1F-2GB', 'architecture': 'i386', 'isIntlVersion': False, 'internalBuildId': 'xyzzy', 'hardwareRevision': '00.00'}]}
conn.execute(['sh ver'])
Traceback (most recent call last):
File "", line 1, in
File "/usr/local/lib/python3.6/site-packages/pyeapi/eapilib.py", line 495, in execute
response = self.send(request)
File "/usr/local/lib/python3.6/site-packages/pyeapi/eapilib.py", line 414, in send
raise CommandError(code, msg, command_error=err, output=out)
pyeapi.eapilib.CommandError: Error [1002]: CLI command 1 of 1 'sh ver' failed: invalid command [incomplete token (at token 1: 'ver')]

But Arista has added the option autoComplete to the optional arguments list from version v0.8.0
http://pyeapi.readthedocs.io/en/master/release-notes-0.8.0.html?highlight=autocomplete

conn.execute(['sh ver'], autoComplete=True)
{'jsonrpc': '2.0', 'id': 'xyzzy', 'result': [{'modelName': 'DCS-7150S-52-CL-F', 'internalVersion': '4.19.2.1F-2GB-6619411.41921F', 'systemMacAddress': 'xyzzy', 'serialNumber': 'xyzzy', 'memTotal': 3963676, 'bootupTimestamp': 1510366564.69, 'memFree': 2333536, 'version': '4.19.2.1F-2GB', 'architecture': 'i386', 'isIntlVersion': False, 'internalBuildId': 'xyzzy', 'hardwareRevision': '00.00'}]}

@bharath-ravindranath

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

The EOS versions that support this feature: 4.17.1.1F and greater:
arista-eosplus/pyeapi#119

As far as I can tell a few 4.16.7 and greater also support these. Function to test the EOS version for support of autoComplete option is also added comment#4 :
arista-eosplus/pyeapi#123

@coveralls

This comment has been minimized.

Copy link

commented Nov 13, 2017

Coverage Status

Coverage increased (+0.004%) to 77.903% when pulling e919984 on bharath-ravindranath:EOSDriver into b957e2d on napalm-automation:develop.

@dbarrosop

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

autoComplete is off by default in eAPI, and always has been:

Ok, thanks for the clarification. I completely misunderstood the original problem and I am happy to see no default/behavior changed :)

@bharath-ravindranath AFAICT from the changes in this code pyeapi doesn't test if the feature is available, it just fails if you try to use it and it's not supported. So what I'd suggest is to default the optional arg to None and pass it to pyeapi when it is not None. Also, would you mind renaming the optional arg to eos_autoComplete and documenting it here?

@ktbyers

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2017

@bharath-ravindranath Can you explain the exact behavior of autoComplete and exandAlias?

This statement seems strange to me on the face of it?

Since I had autoComplete False as default, the router multicast was considered 
as 'router-id multicast'. This is only because 'router' is valid under bgp configuration.
@bharath-ravindranath

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

@ktbyers I might have confused everyone (probably because I myself was confused)

  • autoComplete: false:

Simple short hand fail:
Example:

sh ver
**Error**

A short handed command in any configuration mode will fail
Example:

router bgp 1
router 192.168.1.1
**Error**

Here we have the command router-id in BGP config mode. So the EOS considers we are using this command. But since the autoComplete is off, it will throw error

But if there is no such command in the current configuration mode, we try to match the previous configuration mode

Example:

interface Ethernet 1
router bgp 1
**Valid**

Here router command is not valid under interface configuration. So the EOS tries the global configuration mode (Please note that it should match exactly in the global config mode)

  • autoComplete: true

Simple short hand works in any mode
Example:

router bgp 1
router 192.168.1.1
**Valid**

If the command is not valid in current configuration mode we check for previous config mode(like when autoComplete is false)

interface Ethernet 1
router bgp 1
**Valid**

Now if the command is incomplete, the EOS tries to match the next parameter
Example

router bgp 1
router 192.168.1.1
router multicast
**Valid**

Here the router 192.168.1.1 command matches router-id configuration. And the router multicast will match the global configuration (although this can be matched for router-id, the second argument is not a router id format)

  • expandAliases

We can create aliases in EOS. for example let's say I create alias to check non-zero interface counters

alias shnz show interfaces counters | nz

Now on the EOS you can just say shnz to run the above command. To use the shnz as a command from pyeapi, we need to give the expandAliases option

Hope this explains it.

@coveralls

This comment has been minimized.

Copy link

commented Nov 14, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.878% when pulling 8f79704 on bharath-ravindranath:EOSDriver into b957e2d on napalm-automation:develop.

@coveralls

This comment has been minimized.

Copy link

commented Nov 14, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.878% when pulling 11e8a5a on bharath-ravindranath:EOSDriver into b5fc88f on napalm-automation:develop.

@@ -159,7 +161,10 @@ def _load_config(self, filename=None, config=None, replace=True):
commands.append(line)

try:
self.device.run_commands(commands)
if self.eos_autoComplete:

This comment has been minimized.

Copy link
@dbarrosop

dbarrosop Nov 17, 2017

Member

This should probably be if self.eos_autoComplete is not None in case False has to be enforced in the future.

@coveralls

This comment has been minimized.

Copy link

commented Nov 17, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.953% when pulling bcf98fb on bharath-ravindranath:EOSDriver into 4b2fcbf on napalm-automation:develop.

@coveralls

This comment has been minimized.

Copy link

commented Nov 17, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.953% when pulling bcf98fb on bharath-ravindranath:EOSDriver into 4b2fcbf on napalm-automation:develop.

@dbarrosop

This comment has been minimized.

Copy link
Member

commented Nov 19, 2017

@bewing could you test and let us know if this is what you needed?

@bewing

This comment has been minimized.

Copy link
Member

commented Nov 20, 2017

/mnt/c/Users/bewing/PycharmProjects/napalm/napalm/eos/eos.py in _load_config(self, filename, config, replace)
    189         try:
    190             if self.eos_autoComplete is not None:
--> 191                 self.devcie.run_commands(commands, autoComplete=self.eos_autoComplete)
    192             else:
    193                 self.device.run_commands(commands)

AttributeError: 'EOSDriver' object has no attribute 'devcie'
@bewing

This comment has been minimized.

Copy link
Member

commented Nov 20, 2017

With the typo corrected, this does do what I need it to do:

# git checkout issue-519
# git checkout -b pr508_520
# git pull https://github.com/bharath-ravindranath/napalm.git EOSDriver
# ipython
with get_network_driver("eos")(s, u, p, optional_args={"eos_autoComplete": True}) as d:
    d.load_replace_config(config=d.get_config()["running"])
    print(d.compare_config())
    d.discard_config()

""
@coveralls

This comment has been minimized.

Copy link

commented Nov 20, 2017

Coverage Status

Coverage increased (+0.1%) to 78.109% when pulling 86b09e0 on bharath-ravindranath:EOSDriver into 4b2fcbf on napalm-automation:develop.

dbarrosop added 2 commits Nov 21, 2017
@coveralls

This comment has been minimized.

Copy link

commented Nov 21, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.109% when pulling 5bce3ce on bharath-ravindranath:EOSDriver into 7359a7b on napalm-automation:develop.

@dbarrosop dbarrosop merged commit 3ef2a1a into napalm-automation:develop Nov 21, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.109%
Details
dbarrosop added a commit that referenced this pull request Nov 24, 2017
* Canonical interfaces fix dictionary update behavior.

* Fixing argument reference.

* Fixing NXOS file write issue Python3

* Added mocking method for nxos

* add tests for and fix canonical name

* add optional arg, and re-order optional args

* create extend driver docs

* clean up docs, add to index

* update doc strings

* fix pylama

* fix rev update dict

* add section headers

* spelling

* more consitent test, fix st

* Fix nxos_ssh mac table processing bug

* adding optional arguments for load_replace_candidate  (#508)

* adding optional arguments for load_replace_candidate and load_merge_candidate

* adding optional argument 'autoComplete'

* adding optional argument 'autoComplete' changing failed tests

* making required changes as discussed

* changing the if condition

* correcting the typo

* Handle multiple spaces in device_id from show lldp neighbors

* Add test to get_lldp_neighbors with hostname with spaces

* Loop through loaded config to convert HEREDOC commands.  Fixes #519 (#520)

(eos) loop through loaded config to convert HEREDOC commands.  Fixes #519

* Add code to build documentation for napalm-ansible (#535)

* Add code to build documentation for napalm-ansible

* pep8

* Change placement in documentation tree

* Change formatting

* Fix formatting for return values

* Sort parameters alphabetically

* Text changes

* Add script to clone napalm-ansible repo

* remove partial install (#554)

* Update documentation to corrrect driver ref (#559)

* [docs] Add "-c /dev/null" arguments to py.test command (#561) (#562)

* Add "-c /dev/null" arguments to py.test command (#561)

Before the NAPALM tests were picked up when running from RTD.

* fix requirements filepaths

* LLDP code with space in the device id (#549)

* LLDP code with space in the device id

* LLDP fixed width for device_id field
cspeidel added a commit to cspeidel/napalm that referenced this pull request Nov 11, 2018
* Canonical interfaces fix dictionary update behavior.

* Fixing argument reference.

* Fixing NXOS file write issue Python3

* Added mocking method for nxos

* add tests for and fix canonical name

* add optional arg, and re-order optional args

* create extend driver docs

* clean up docs, add to index

* update doc strings

* fix pylama

* fix rev update dict

* add section headers

* spelling

* more consitent test, fix st

* Fix nxos_ssh mac table processing bug

* adding optional arguments for load_replace_candidate  (napalm-automation#508)

* adding optional arguments for load_replace_candidate and load_merge_candidate

* adding optional argument 'autoComplete'

* adding optional argument 'autoComplete' changing failed tests

* making required changes as discussed

* changing the if condition

* correcting the typo

* Handle multiple spaces in device_id from show lldp neighbors

* Add test to get_lldp_neighbors with hostname with spaces

* Loop through loaded config to convert HEREDOC commands.  Fixes napalm-automation#519 (napalm-automation#520)

(eos) loop through loaded config to convert HEREDOC commands.  Fixes napalm-automation#519

* Add code to build documentation for napalm-ansible (napalm-automation#535)

* Add code to build documentation for napalm-ansible

* pep8

* Change placement in documentation tree

* Change formatting

* Fix formatting for return values

* Sort parameters alphabetically

* Text changes

* Add script to clone napalm-ansible repo

* remove partial install (napalm-automation#554)

* Update documentation to corrrect driver ref (napalm-automation#559)

* [docs] Add "-c /dev/null" arguments to py.test command (napalm-automation#561) (napalm-automation#562)

* Add "-c /dev/null" arguments to py.test command (napalm-automation#561)

Before the NAPALM tests were picked up when running from RTD.

* fix requirements filepaths

* LLDP code with space in the device id (napalm-automation#549)

* LLDP code with space in the device id

* LLDP fixed width for device_id field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.