Add CLI flag for aggregation timeout #686
Conversation
|
// @ceridwen |
Codecov Report
@@ Coverage Diff @@
## master #686 +/- ##
=========================================
+ Coverage 42.44% 42.55% +0.1%
=========================================
Files 62 62
Lines 3409 3419 +10
=========================================
+ Hits 1447 1455 +8
- Misses 1864 1866 +2
Partials 98 98
Continue to review full report at Codecov.
|
|
So now there are 2 flags which determine "wait". There's |
|
This could be a future design PR, but is there a way to define the timeout per plugin? For instance, the e2e tests may take a long time, but maybe my systemd_logs only takes 2mins? This flag could be the default of all and then later maybe let plugin developers further define? |
| func AddTimeoutFlag(flag *int, flags *pflag.FlagSet) { | ||
| flags.IntVar( | ||
| flag, timeoutFlag, config.DefaultAggregationServerTimeoutSeconds, | ||
| "How long (in seconds) Sonobuoy will wait for plugins to complete before exiting. 0 indicates no timeout.", |
There was a problem hiding this comment.
Might be cool to make this a duration type and let users enter 1m or 2h`, etc. Otherwise, you've got to do the math to figure out the number of seconds.
There was a problem hiding this comment.
nit: Maybe print the default to the help so users know what it is if not defined?
There was a problem hiding this comment.
cobra adds the default; this was copied from my console just now:
... before exiting. 0 indicates no timeout. (default 10800)
So there are two flags but they really do indicate different things and could be used in various ways. The The The reason I don't want to sync the two is that a pretty reasonable use case is the following:
|
So I agree something like that may eventually be implemented. There are definitely impacts of this overall timeout and per-plugin timeout that play a role in #627 My POC implementation of plugin ordering also showed me that it could be very easy to adjust some of these details by adding them into the plugin manifest metadata. But yes, I agree it is for another day since this is currently coded for just one big timeout and this was just to make it easier to set. |
|
Yeah that holds up, but I can still
But should it? If you had CI running and this happened you'd get a false positive wouldn't you? I'm just trying to challenge the overall design to make sure we're solving the problem and not just throwing flags at it. Is there a scenario where you'd want the client to wait 5 mins and the server wait 2 hours? |
True, though you may want to do something like this even in CI. Before the
As the example above says, I don't think forcing the server to error and stop running because we hit a CLI timeout is always the right thing to do. It prevents a user (including CI systems) from investigating if they want to. Just because
Absolutely 👍 , IMO that is the discussion that we had when deciding to put this ticket into the milestone though. I appreciate the design questions though and am happy to defend the choice to make the new flag.
Yes but I also think your comment about CTRL+C plays a role here too; personally, I run checks all the time where this is the [somewhat] the case. If I am iterating on some plugin (in my case, e2e test) and I think it will just take 5m or less and I run it as such. If it runs and I find out it actually took 7m, I really didn't want the server to err at 5m. But the CLI TL;DR; I think that in most situations your sentiment is right, you'd just want the server to pass/fail on its own and then the CLI wait for that terminal status. But I can think of situations where you want to ensure the CLI stops and gives you time to investigate before cleaning anything up. I think some of these things are also related to when/how/if plugin resources are cleaned up. In the end though, this is already a customizable value, just one that multiple people edit regularly and want it to be more simple. If we decide to prioritize per-plugin timeouts, cleanup logic, etc then I think we should bring up these ideas again though because I do acknowledge there are surely some improvements that could be made. |
One of the most common config settings that may change between different environments and test cases is the amount of time Sonobuoy will wait for results. Currently, this value is configurable but it is in a nested config structure and not terribly well documented at this time. Personally, in the past, I had projects where we had to start using a sonobuoy config-based flow instead of flags due solely to this field. This change adds the flag to make it more easy to set. Fixes #482 Signed-off-by: John Schnake <[email protected]>
What this PR does / why we need it:
One of the most common config settings that may change between
different environments and test cases is the amount of time
Sonobuoy will wait for results. Currently, this value is configurable
but it is in a nested config structure and not terribly well
documented at this time.
Personally, in the past, I had projects where we had to start
using a sonobuoy config-based flow instead of flags due solely
to this field.
This change adds the flag to make it more easy to set.
Signed-off-by: John Schnake [email protected]
Which issue(s) this PR fixes
Fixes #482
Release note: