search

napalm-automation-community / napalm-aos

NAPALM driver for Alcatel Lucent Enterprise AOS
Apache License 2.0
11 stars 13 forks source link

Merging corrupt commands doesn't yield exception #11

Closed cs-1 closed 6 years ago

cs-1 commented 6 years ago

If you merge a configuration with invalid commands, no exception is thrown. I used the command bla bla bla in a candidate configuration which seems to merge finde. But if you apply the configuration manually on the switch you get the following error:

> configuration apply candidate.cfg
Errors: 1
Log file name: candidate.cfg.4.err

napalm-aos should detect errors and yield an exception because otherwise there's no feedback whether or not applying the configuration really worked.

EDIT: I think that the following regex should also be changed: https://github.com/napalm-automation-community/napalm-aos/blob/1fdb92ee0ee35b272c9a4c0999ca1f2873f10ac3/napalm_aos/utils/AlcatelOS.py#L141 and https://github.com/napalm-automation-community/napalm-aos/blob/1fdb92ee0ee35b272c9a4c0999ca1f2873f10ac3/napalm_aos/utils/AlcatelOS.py#L191 They should not be

regex = re.compile('ERROR:')

they should be

regex = re.compile('(?:ERROR|Errors?):')

because especially the configuration apply ... command yields an error as "Errors:". Still, changing these lines doesn't solve the problem. I can't see an ad-hoc solution for the problem.

EDIT 2: Sorry, completely forgot: I'm using Python 3 and the current napalm-ansible development branch https://github.com/napalm-automation/napalm-ansible as well as napalm and Ansible installed via pip.

napalmaos commented 6 years ago

This issue have been fixed, please help to test again. Thanks.

cs-1 commented 6 years ago

The fix works. However, it'd be very useful to have the output from *.err.* in the error message so that you know exactly where to look for the problem.

napalmaos commented 6 years ago

Please help to test again. Thanks.

cs-1 commented 6 years ago

I'd like to do that, however, we're currently working with the patches in pull request https://github.com/napalm-automation-community/napalm-aos/pull/20 and we really do need them for ansible + ansible-napalm because otherwise we can't have parallel processes using napalm-aos due to issue https://github.com/napalm-automation-community/napalm-aos/issues/13. Could you have a look at the pull request first? If the changes can actually be incorporated, we can start testing again. I could try incorporating them in the fork that the pull request is for.

cs-1 commented 6 years ago

I've updated my fork with the commits so it should be easier to incorporate pull request https://github.com/napalm-automation-community/napalm-aos/pull/20.

napalmaos commented 6 years ago

Please close an issue if it was resolve. Thanks

vmuthukrishnan commented 6 years ago

Hi, Kindly close the issue if it is resolved, otherwise let us know if there any issues still present.