Skip to content
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

Jump to bottom

Add --pull-timeout flag to crictl create, run and pull commands #1448

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Add --pull-timeout flag to crictl create, run and pull commands #1448

merged 1 commit into from
Jun 12, 2024

Conversation

saschagrunert
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

This allows to set a pull context timeout for the supported commands. Runtimes may or may not use the timeout from the RPC for image pulls, but crictl should generally support that feature.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Added `--pull-timeout` flag to `crictl` `create`, `run` and `pull` commands.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2024
@haircommander
Copy link
Contributor

LGTM

@SergeyKanzhelev
Copy link
Member

Runtimes may or may not use the timeout from the RPC for image pulls

Should we make it a part of critest? Not sure how we will test the behavior, but at least runtime should stop processing and exit when requested.

SergeyKanzhelev
SergeyKanzhelev reviewed Jun 10, 2024
View reviewed changes
cmd/crictl/image.go
@@ -633,7 +641,7 @@ func normalizeRepoDigest(repoDigests []string) (string, string) {

// PullImageWithSandbox sends a PullImageRequest to the server, and parses
// the returned PullImageResponse.
func PullImageWithSandbox(client internalapi.ImageManagerService, image string, auth *pb.AuthConfig, sandbox *pb.PodSandboxConfig, ann map[string]string) (*pb.PullImageResponse, error) {
func PullImageWithSandbox(client internalapi.ImageManagerService, image string, auth *pb.AuthConfig, sandbox *pb.PodSandboxConfig, ann map[string]string, timeout time.Duration) (*pb.PullImageResponse, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what should be a behavior on Ctrl+C. Should we have a context cancelled as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting idea, I don't think we consider Ctrl+C in any RPC for now (means the RPC will run through but we will not catch the result). We can probably follow-up on that and add it to all commands.

SergeyKanzhelev
SergeyKanzhelev reviewed Jun 10, 2024
View reviewed changes
cmd/crictl/container.go
&cli.DurationFlag{
Name: "cancel-timeout",
Aliases: []string{"T"},
Usage: "Seconds to wait for a container create request to complete before cancelling the request",
Copy link
Member

Choose a reason for hiding this comment

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

should it say - "disabled if 0" as well?

Copy link
Member

Choose a reason for hiding this comment

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

or anything about the default

Copy link
Member Author

@saschagrunert saschagrunert Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The help mentions the default value:

--pull-timeout value, --pt value        Maximum time to be used for pulling the image, disabled if set to 0s (default: 0s) [$CRICTL_PULL_TIMEOUT]

SergeyKanzhelev
SergeyKanzhelev reviewed Jun 10, 2024
View reviewed changes
cmd/crictl/container.go
@@ -111,6 +114,17 @@ var createPullFlags = []cli.Flag{
Value: "",
Usage: "Use `USERNAME` for accessing the registry. The password will be requested on the command line",
},
&cli.DurationFlag{
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need two of those or just one parameter timeout will be enough? Are those two having different semantics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically we now have three per subcommand:

  • the --timeout / -t flag on the root level (default 2s)
  • a command specific timeout
    • crictl create: a --cancel-timeout / -T flag which overrides the root flag (default 0s, but inherits the 2s from the root flag)
    • crictl run: a --timeout / -t flag which overrides the root flag (default 0s, but inherits the 2s from the root flag)
  • the --pull-timeout / -pt flag, specific for the image pull

Merging the pull timeout with the existing ones would probably break most users since a pull in 2s is unlikely per default. So adding another timeout seemed to be reasonable, because it will default to 0s as the previous behavior (no pull timeout) and can be adjusted as being set.

@saschagrunert
Add --pull-timeout flag to crictl create, run and pull comm…
780d044 
…ands

This allows to set a pull context timeout for the supported commands.
Runtimes may or may not use the timeout from the RPC for image pulls,
but `crictl` should generally support that feature.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert
Copy link
Member Author

Runtimes may or may not use the timeout from the RPC for image pulls

Should we make it a part of critest? Not sure how we will test the behavior, but at least runtime should stop processing and exit when requested.

Yes, I added a critest case for that.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2024
SergeyKanzhelev
SergeyKanzhelev approved these changes Jun 12, 2024
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert, SergeyKanzhelev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 314287e into kubernetes-sigs:master Jun 12, 2024
23 checks passed
@saschagrunert saschagrunert deleted the pull-timeout branch June 13, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@haircommander haircommander Awaiting requested review from haircommander

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@SergeyKanzhelev SergeyKanzhelev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@saschagrunert @haircommander @SergeyKanzhelev @k8s-ci-robot