-
Notifications
You must be signed in to change notification settings - Fork 44
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
Enhancement/943/add wait zos operator and query #976
Enhancement/943/add wait zos operator and query #976
Conversation
Working on zos_operator_action_query Added initial changelog fragment
Added wait time and wait values to zoaq
tweaked description of new feature in both affected functions.
…on for wait_arg Added mention of this to documentation of interface
After discussion, I've updated the code to check for zoau 1.2.5 or later, and will re-request reviews when the ansible tests have passed. |
…-wait-zos-operator-and-query' into Enhancement/943/Add-wait-zos-operator-and-query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean and direct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a couple of comments
changelogs/fragments/943-enhance-Add-wait-zos-operator-and-query.yml
Outdated
Show resolved
Hide resolved
I have not reviewed the code yet but would like to understand the value of So the opercmd docs are here for reference , my question is how more more value do really get from having I feel like if someone says wait 60, you might just want to always honor that, i see the value in returning early but only if we can be sure that returning early does not truncate anything, in that case we could always set the value to true and avoid exposing it. |
@ddimatos This has been a back-and-forth. Customers don't want a big delay all the time, and in some cases they don't want to miss data. In the zoau slack channel, p1696597067029359 is a new entry, through support, asking about this. Ironically, this one is complaining that 1.2.4's 1-second delay is too long. The return, as I understand if from Anthony, is once something is received and stopped, so wait=false means a 60 second delay could return in 2-3 seconds if output was received. Just a thought here... maybe we should have a get-together with zoau team so we're all on the same page with what's being asked for, since I've heard a few contradictory issues. |
@richp405 thanks for that, while I agree no one wants to wait longer then needed especially in a user interactive situation, eg using the the ZOAU cmd line interface. With an Ansible experience where users are more interested in success over performance, they probably don't mind the extra wait time (wait=true - meaning we default to this and don't expose it) else what they face is failing tasks that might return to soon where responses are chunks. Ansible and ZOAU have two very difference audiences, I want to be sure we are always addressing the Ansible audience when exposing ZOAU options. |
Based on the discussion above, I think the new wait option needs to be integrated silently. Don't expose a new option in the module. Updated #943 to drop the option requirement. Just driving -T in the future alone, however, may cause early returns and truncation issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @richp405 thanks for the updates and speedy turn around, as I was reading the source I found what I think might be a bug in both modules, more so please don't rush to make changes just yet, I have some concerns about adding wait_time_s to zos_operation_action_query
and trying to understand where the original request came from and more so why.
Renamed vague variable name to "use_wait_arg" Reflected changes and 1.2.5 dependancy in the changelog fragment
This was addressed in code, with other change (version) being addressed in #1018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richp405 - this looks good and thank you for the the added changes per the review comments, your great to work with, thanks so much Rich.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments around the zoau version checker are being addressed and the rest of this looks good to me.
@ddimatos One last question: should we add version detection here, to decide if default delay is passed as 5 (old style) or 500 (for 1.3 and later?). |
@richp405 I did think about this and while I did not look their python source, my guess is the timeout is using shell interface option In 1.2.5 in appears to wait up to the time but reads as if it will return early UNLESS -w is specified.
In 1.2.4.1 Python accepts the shell interface default , no change on the shell In 1.2.4 it appears to wait the full time.
Short answer, i don't think so, but correct me if I don't understand your question. |
SUMMARY
Enhancement to zos_operator, adding the 'wait' function back in with a new definition
Enhancement to zos_operator_action_query, adding both wait and wait_time_s
Addresses #943
ISSUE TYPE
COMPONENT NAME
zos_operator
zos_operator_action_query
ADDITIONAL INFORMATION
The timing system in zoau has changed to add new options:
wait_time_s is the maximum number of seconds to wait for a response
If wait is True, zoau will always wait the maximum amount of time
If wait is False, zoau will return after data is received
If wait is not included, it defaults to False.
name: Execute operator command to show jobs, always waiting 5 seconds for response
zos_operator:
cmd: 'd a,all'
wait_time_s: 5
wait: true
name: Execute operator command to show jobs, waiting up to 7 seconds for response
zos_operator:
cmd: 'd a,all'
wait_time_s: 7