Skip to content

Add a timeout argument to the junos_commit module #97

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 5, 2016
Merged

Add a timeout argument to the junos_commit module #97

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions library/junos_commit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ options:
- TCP port number to use when connecting to the device
required: false
default: 830
timeout:
description:
- Extend the NETCONF RPC timeout beyond the default value of
30 seconds. Set this value to accommodate commits that
might take longer than the default timeout interval.
required: false
default: "0"
logfile:
description:
- Path on the local server where the progress status is logged
Expand Down Expand Up @@ -110,6 +117,7 @@ def main():
user=dict(required=False, default=os.getenv('USER')),
passwd=dict(required=False, default=None),
port=dict(required=False, default=830),
timeout=dict(required=False, default=0),
logfile=dict(required=False, default=None),
comment=dict(required=False, default=None),
confirm=dict(required=False, default=None)
Expand Down Expand Up @@ -142,6 +150,10 @@ def main():
# --- UNREACHABLE ---

try:
timeout = int(args['timeout'])
if timeout > 0:
dev.timeout = timeout

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stacywsmith Can we change the logic where we can simply have 30 sec default value @ timeout=dict(required=False, default=0),
hence the code would change to

        timeout = int(args['timeout']) 
        dev.timeout = timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vnitinv Hi Nitin the code in my pull request was copied from the implementation of the timeout argument that's currently used for junos_install_config. I believe you may have misinterpreted the behavior of the code.

If the user does not pass the timeout argument, then line 153 will set timeout = 0, line 154 is not true, and dev.timeout will not be set.

If the user passes a timeout value, then line 153 sets timeout to that value (rounded to nearest int). If the value the user passed is greater than 0, line 154 is true and dev.timeout is set to the user supplied value.

Therefore, if a user passes any numeric value greater than 0, the RPC timeout will be set to the user supplied value.

Here's an example from the debugger.

'''
(Epdb) print args
{'comment': None, 'host': 'core1.site1', 'user': 'user', 'timeout': 10, 'confirm': None, 'passwd': 'user123', 'logfile': None, 'port': 830}
(Epdb) n

/home/user/.ansible/tmp/ansible-tmp-1449948726.22-132107123925044/junos_commit(156)main()
-> timeout = int(args['timeout'])
(Epdb) n
/home/user/.ansible/tmp/ansible-tmp-1449948726.22-132107123925044/junos_commit(157)main()
-> if timeout > 0:
(Epdb) timeout
10
(Epdb) n
/home/user/.ansible/tmp/ansible-tmp-1449948726.22-132107123925044/junos_commit(158)main()
-> dev.timeout = timeout
(Epdb) n
/home/user/.ansible/tmp/ansible-tmp-1449948726.22-132107123925044/junos_commit(160)main()
-> cu = Config(dev)
(Epdb) dev.timeout
10
(Epdb)
'''

You're suggested change is similar in behavior, but would allow the user to set the timeout to a negative value. This causes PyEZ to raise a jnpr.junos.exception.LockError exception which in turn causes the Ansible task/module to fail. Worse yet, it leaves the configuration databases locked causing all further attempts to lock the database to fail.

I believe my existing pull request (copied from the junos_install_config implementation) is preferrable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stacywsmith
Can you check line https://github.com/Juniper/ansible-junos-stdlib/blob/master/library/junos_install_os#L234
to predefine timeout data type. with this code would change to
timeout = intargs['timeout']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting changing line 120 from:
''' timeout=dict(required=False, default=0), '''

to:
''' timeout=dict(required=False, type='int', default=0), '''

If so, I'm fine with that change. However, we should also change this line to match:
https://github.com/Juniper/ansible-junos-stdlib/blob/master/library/junos_install_config#L399

Even with this change, lines 153-155 need to be:
'''
timeout = args['timeout']
if timeout > 0:
dev.timeout = timeout
'''

I'm not sure if your suggesting this, but I don't support changing line 120 to:
''' timeout=dict(required=False, type='int', default=30), '''

I believe the default RPC timeout should continue to be set in only one location (in PyEZ), rather than in both PyEZ (and within multiple locations in the Ansible for Junos modules).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Stacy,

I just meant it to be

timeout=dict(required=False, type='int', default=0)

cu = Config(dev)

logging.info("taking lock")
Expand Down