Allow custom timeout when fetching container tags #235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshfrench
commented
Jun 29, 2023
| @@ -131,6 +131,12 @@ func init() { | |||
| klog.Exitf("Failed to bind show-errored-containers flag: %v", err) | |||
| } | |||
|
|
|||
| findCmd.Flags().Uint16P("timeout", "t", 10, "When finding container images, the time in seconds before canceling the operation.") | |||
There was a problem hiding this comment.
uint8 would cap at less than 5 minutes, which feels within the realm of possibility for a poorly-behaved repo. On the other hand, I can't imagine anyone needing more than 9 hours :)
| @@ -15,7 +15,9 @@ Flags: | |||
| --containers Show old container image versions. There will be no helm output unless the --helm flag is set as well | |||
| --helm Show old helm releases. You can combine this flag with `--containers` to have both output in a single run. | |||
| -h, --help help for find | |||
| --show-non-semver When finding container images, show all containers even if they don't follow semver. | |||
| --show-errored-containers When finding container images, show errors encountered when scanning. | |||
There was a problem hiding this comment.
Just adding --show-errored-containers to the overview while I'm at it.
sudermanjr
approved these changes
Jun 29, 2023
rbren
approved these changes
Jul 5, 2023
|
Thank you! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #234
Checklist
Description
What's the goal of this PR?
Allows user to override the default timeout to overcome slow/large queries when fetching container tags.
What changes did you make?
Added a new flag (
--timeout/-t); use the value incontext.WithTimeoutinstead of hard-coding it.What alternative solution should we consider, if any?
Initially I'd proposed setting a limit on the number of flags fetched, but that depends on getting the tags in reverse order. According to the OCI Distribution Spec, "tags MUST be in lexical order (i.e. case-insensitive alphanumeric order)" so that's a non-starter 😅
For my use case, setting the timeout to 20 seconds is enough to grab all ~9800 tags from calico/node so one possibility would be to simply bump the default timeout in addition to (or instead of) this PR.
Alternatively, bumping the number of tags fetched in each batch or letting the user specify the batch size might also address the issue. A timeout feels more intuitive to me, though.