Change how we handle config locking #149
Conversation
This PR moves the state of config locking out of the driver and into the device to more closely follow the Juniper model of locking. Additionally, we're making a better attempt to ensure close() is called for each object no matter what (it's not called by the parent unless the object is called in a with block) to ensure that we're dropping the lock on the device on exit.
>>> from napalm_eos import EOSDriver
>>> i = EOSDriver('veos1', 'admin', 'admin')
>>> i.open()
>>> i.device.run_commands(['show configuration sessions'])
[{u'maxSavedSessions': 1, u'maxOpenSessions': 5, u'sessions': {}}]
>>> i.load_merge_candidate(config='ntp server 192.0.2.1')
>>> i.device.run_commands(['show configuration sessions'])
[{u'maxSavedSessions': 1, u'maxOpenSessions': 5, u'sessions': {u'napalm_949610': {u'instances': {}, u'state': u'pending', u'description': u''}}}]
>>> i.load_merge_candidate(config='ntp server 192.0.2.2')
>>> i.commit_config()
>>> i.device.run_commands(['configure session blocker'])
[{}]
>>> i.device.run_commands(['show configuration sessions'])
[{u'maxSavedSessions': 1, u'maxOpenSessions': 5, u'sessions': {u'blocker': {u'instances': {}, u'state': u'pending', u'description': u''}}}]
>>> i.load_merge_candidate(config='ntp server 192.0.2.3')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "napalm_eos/eos.py", line 176, in load_merge_candidate
self._load_config(filename, config, False)
File "napalm_eos/eos.py", line 139, in _load_config
self._lock()
File "napalm_eos/eos.py", line 134, in _lock
raise SessionLockedException('Session is already in use')
napalm_base.exceptions.SessionLockedException: Session is already in use
>>> i.device.run_commands(['configure session blocker', 'abort'])
[{}, {}]
>>> i.device.run_commands(['show configuration sessions'])
[{}]
>>> i.load_merge_candidate(config='ntp server 192.0.2.3')
>>> i.device.run_commands(['show configuration sessions'])
[{u'maxSavedSessions': 1, u'maxOpenSessions': 5, u'sessions': {u'napalm_40161': {u'instances': {}, u'state': u'pending', u'description': u''}}}]
>>> exit()
# ssh admin@veos1
Password:
veos1#show configuration sessions | json
{
"maxSavedSessions": 1,
"maxOpenSessions": 5,
"sessions": {}
}
veos1# |
|
Old exit behavior: >>> from napalm_eos import EOSDriver
>>> i = EOSDriver('veos1', 'admin', 'admin')
>>> i.open()
>>> i.load_merge_candidate(config='ntp server 192.0.2.3')
>>> i.device.run_commands(['show configuration sessions'])
[{u'maxSavedSessions': 1, u'maxOpenSessions': 5, u'sessions': {u'napalm_228704': {u'instances': {}, u'state': u'pending', u'description': u''}}}]
>>> exit()
#ssh admin@veos1
Password:
Last login: Thu Mar 9 21:38:14 2017 from 10.198.0.35
veos1#show configuration sessions | json
{
"maxSavedSessions": 1,
"maxOpenSessions": 5,
"sessions": {
"napalm_228704": {
"description": "",
"state": "pending",
"instances": {}
}
}
} |
|
Interesting findings @bewing. Thanks for spending time on this! I will have a closer look next week, when I have scheduled a pretty good amount of time working on many tasks I have assigned. However, I'm not sure about the |
| @@ -85,6 +85,9 @@ def __init__(self, hostname, username, password, timeout=60, optional_args=None) | |||
|
|
|||
| self.enablepwd = optional_args.get('enable_password', '') | |||
|
|
|||
| def __del__(self): | |||
There was a problem hiding this comment.
Can you please expand on this? Why is __del__ needed?
There was a problem hiding this comment.
See #149 (comment)
It's possible (through exceptions, poorly written scripts, etc) to exit the python process without freeing the session on the switch. Since the session is treated as a lock by NAPALM, and by default only 5 configuration sessions can be saved at a time, we should try really really hard to ensure that we're discarding configuration sessions when exiting. This doesn't happen on the junos driver, as JunOS discards the configuration lock when the SSH connection closes.
It looks like napalm-base tries to accomplish this with entry and exit, but those blocks are only called inside a with block.
There was a problem hiding this comment.
Yes, I confirm it's like that:
>>> i.load_merge_candidate(config='ntp server 1.2.3.6')
>>> import sys
>>> sys.exit(0)still keeps the config locked:
{
"maxSavedSessions": 1,
"maxOpenSessions": 5,
"sessions": {
"napalm_394464": {
"description": "",
"state": "pending",
"instances": {}
}
}
}
There was a problem hiding this comment.
I commented on another PR because I had no clue where this was coming from (reading PRs in the wrong order :() but this should be in napalm-base to make sure we have a consistent behavior across drivers.
|
Firstly, I confirm I was able to reproduce the bug (not that I didn't believe you): >>> i.load_merge_candidate(filename='dummy')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "napalm_eos/eos.py", line 183, in load_merge_candidate
self._load_config(filename, config, False)
File "napalm_eos/eos.py", line 150, in _load_config
with open(filename, 'r') as f:
IOError: [Errno 2] No such file or directory: 'dummy'
>>> i.discard_config()
>>> i.load_merge_candidate(config='ntp server 1.2.3.4')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "napalm_eos/eos.py", line 183, in load_merge_candidate
self._load_config(filename, config, False)
File "napalm_eos/eos.py", line 140, in _load_config
self._lock()
File "napalm_eos/eos.py", line 131, in _lock
raise SessionLockedException('Session is already in use by napalm')
napalm_base.exceptions.SessionLockedException: Session is already in use by napalm
>>>Which says that it was not able to lock the session, although the device didn't have it locked: {
"maxSavedSessions": 1,
"maxOpenSessions": 5,
"sessions": {}
} |
|
With your changes, it works fine in that case: >>> i.load_merge_candidate(filename='dummy')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "napalm_eos/eos.py", line 176, in load_merge_candidate
self._load_config(filename, config, False)
File "napalm_eos/eos.py", line 145, in _load_config
with open(filename, 'r') as f:
IOError: [Errno 2] No such file or directory: 'dummy'
>>> i.discard_config()
>>> i.load_merge_candidate(config='ntp server 1.2.3.4')And the session is acquired correctly: {
"maxSavedSessions": 1,
"maxOpenSessions": 5,
"sessions": {
"napalm_798351": {
"description": "",
"state": "pending",
"instances": {}
}
}
}Now, just let me make some more tests to make sure all these changes are fine. |
|
Thanks @bewing |
Closes #148.
This PR moves the state of config locking out of the driver and into the device
to more closely follow the Juniper model of locking. Additionally, we're making
a better attempt to ensure close() is called for each object no matter what
(it's not called by the parent unless the object is called in a with block) to
ensure that we're dropping the lock on the device on exit.