[sosreport] Allow user-controllable plugin timeouts #1499
Conversation
Allows users to specify a timeout for each plugin using the '-k plugin.timeout=value' syntax by adding the 'timeout' option to every plugin. Additionally, adds the --plugin-timeout option to set a timeout for _all_ plugins. If --plugin-timeout and a specific -k timeout option is provided, the -k timeout option will be applied for those specific plugins, with --plugin-timeout being applied to all others. In either case, specifying a timeout of 0 seconds results in no timeout being applied. In the event that an invalid timeout is set, the timeout will be set to the default value for the plugin. Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>
TurboTurtle
force-pushed
the
per-plugin-timeouts
branch
from
fd73430
to
3748493
Compare
| if _timeout is not None and _timeout > -1: | ||
| return _timeout | ||
| return self.plugin_timeout | ||
|
|
There was a problem hiding this comment.
I thought the elif are wrong but I am unable to find particular counter-example.
Can't something be simplified if we allow just numerical values for the options? Like ./sosreport --log-size=a1 fails with sosreport: error: argument --log-size: invalid int value: 'a1'?
There was a problem hiding this comment.
While it'd certainly be helpful to restrict the type of the options, I don't think that'd make this block any simpler. The issue comes in with how both argparse and the futures libraries handle int zero (differently), so we have to do some shuffling here to compensate for that.
I'm certainly open for suggestions on how to condense it, as I'm not particularly fond of it myself right now either - I just couldn't come up with another solution.
There was a problem hiding this comment.
Havent realized that complexity behind, what can easily justify that code, then.
There was a problem hiding this comment.
Yeah, that kinda sucks but as @TurboTurtle explained it's a consequence of how the Python bits deal with certain special values. All that said, it's really not too bad - we have far more icky blocks in the rest of the codebase, and the logic is quite easy to follow if you understand the intent of the precedence.
|
+1, maybe the restriction of argument values to numbers only would be beneficial? |
|
Thanks for working on this - very nice feature. |
Let's keep that discussion going in #1526 |
Allows users to specify a timeout for each plugin using the '-k
plugin.timeout=value' syntax by adding the 'timeout' option to every
plugin.
Additionally, adds the --plugin-timeout option to set a timeout for
all plugins. If --plugin-timeout and a specific -k timeout option is
provided, the -k timeout option will be applied for those specific
plugins, with --plugin-timeout being applied to all others.
In either case, specifying a timeout of 0 seconds results in no timeout
being applied. In the event that an invalid timeout is set, the timeout
will be set to the default value for the plugin.
Signed-off-by: Jake Hunsaker jhunsake@redhat.com
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines