-
Notifications
You must be signed in to change notification settings - Fork 454
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
Add --pull-timeout
flag to crictl
create
, run
and pull
commands
#1448
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,9 @@ type pullOptions struct { | |
// Username to use for accessing the registry | ||
// password will be requested on the command line | ||
username string | ||
|
||
// timeout is the maximum time used for the image pull | ||
timeout time.Duration | ||
} | ||
|
||
var createPullFlags = []cli.Flag{ | ||
|
@@ -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{ | ||
Name: "cancel-timeout", | ||
Aliases: []string{"T"}, | ||
Usage: "Seconds to wait for a container create request to complete before cancelling the request", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should it say - "disabled if 0" as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or anything about the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The help mentions the default value:
|
||
}, | ||
&cli.DurationFlag{ | ||
Name: "pull-timeout", | ||
Aliases: []string{"pt"}, | ||
Usage: "Maximum time to be used for pulling the image, disabled if set to 0s", | ||
EnvVars: []string{"CRICTL_PULL_TIMEOUT"}, | ||
}, | ||
} | ||
|
||
var runPullFlags = []cli.Flag{ | ||
|
@@ -137,17 +151,29 @@ var runPullFlags = []cli.Flag{ | |
Value: "", | ||
Usage: "Use `USERNAME` for accessing the registry. password will be requested", | ||
}, | ||
&cli.StringFlag{ | ||
Name: "runtime", | ||
Aliases: []string{"r"}, | ||
Usage: "Runtime handler to use. Available options are defined by the container runtime.", | ||
}, | ||
&cli.DurationFlag{ | ||
Name: "timeout", | ||
Aliases: []string{"t"}, | ||
Usage: "Seconds to wait for a container create request before cancelling the request", | ||
}, | ||
&cli.DurationFlag{ | ||
Name: "pull-timeout", | ||
Aliases: []string{"pt"}, | ||
Usage: "Maximum time to be used for pulling the image, disabled if set to 0s", | ||
EnvVars: []string{"CRICTL_PULL_TIMEOUT"}, | ||
}, | ||
} | ||
|
||
var createContainerCommand = &cli.Command{ | ||
Name: "create", | ||
Usage: "Create a new container", | ||
ArgsUsage: "POD container-config.[json|yaml] pod-config.[json|yaml]", | ||
Flags: append(createPullFlags, &cli.DurationFlag{ | ||
Name: "cancel-timeout", | ||
Aliases: []string{"T"}, | ||
Usage: "Seconds to wait for a container create request to complete before cancelling the request", | ||
}), | ||
Flags: createPullFlags, | ||
|
||
Action: func(c *cli.Context) (err error) { | ||
if c.Args().Len() != 3 { | ||
|
@@ -177,6 +203,7 @@ var createContainerCommand = &cli.Command{ | |
creds: c.String("creds"), | ||
auth: c.String("auth"), | ||
username: c.String("username"), | ||
timeout: c.Duration("pull-timeout"), | ||
}, | ||
timeout: c.Duration("cancel-timeout"), | ||
}, | ||
|
@@ -581,15 +608,7 @@ var runContainerCommand = &cli.Command{ | |
Name: "run", | ||
Usage: "Run a new container inside a sandbox", | ||
ArgsUsage: "container-config.[json|yaml] pod-config.[json|yaml]", | ||
Flags: append(runPullFlags, &cli.StringFlag{ | ||
Name: "runtime", | ||
Aliases: []string{"r"}, | ||
Usage: "Runtime handler to use. Available options are defined by the container runtime.", | ||
}, &cli.DurationFlag{ | ||
Name: "timeout", | ||
Aliases: []string{"t"}, | ||
Usage: "Seconds to wait for a container create request before cancelling the request", | ||
}), | ||
Flags: runPullFlags, | ||
|
||
Action: func(c *cli.Context) (err error) { | ||
if c.Args().Len() != 2 { | ||
|
@@ -617,6 +636,7 @@ var runContainerCommand = &cli.Command{ | |
creds: c.String("creds"), | ||
auth: c.String("auth"), | ||
username: c.String("username"), | ||
timeout: c.Duration("pull-timeout"), | ||
}, | ||
timeout: c.Duration("timeout"), | ||
} | ||
|
@@ -747,7 +767,7 @@ func CreateContainer( | |
|
||
// Try to pull the image before container creation | ||
ann := config.GetImage().GetAnnotations() | ||
if _, err := PullImageWithSandbox(iClient, image, auth, podConfig, ann); err != nil { | ||
if _, err := PullImageWithSandbox(iClient, image, auth, podConfig, ann, opts.pullOptions.timeout); err != nil { | ||
return "", err | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ import ( | |
"strconv" | ||
"strings" | ||
"syscall" | ||
"time" | ||
|
||
"github.com/docker/go-units" | ||
"github.com/sirupsen/logrus" | ||
|
@@ -83,6 +84,12 @@ var pullImageCommand = &cli.Command{ | |
Aliases: []string{"a"}, | ||
Usage: "Annotation to be set on the pulled image", | ||
}, | ||
&cli.DurationFlag{ | ||
Name: "pull-timeout", | ||
Aliases: []string{"pt"}, | ||
Usage: "Maximum time to be used for pulling the image, disabled if set to 0s", | ||
EnvVars: []string{"CRICTL_PULL_TIMEOUT"}, | ||
}, | ||
}, | ||
ArgsUsage: "NAME[:TAG|@DIGEST]", | ||
Action: func(c *cli.Context) error { | ||
|
@@ -119,7 +126,8 @@ var pullImageCommand = &cli.Command{ | |
return err | ||
} | ||
} | ||
r, err := PullImageWithSandbox(imageClient, imageName, auth, sandbox, ann) | ||
timeout := c.Duration("timeout") | ||
r, err := PullImageWithSandbox(imageClient, imageName, auth, sandbox, ann, timeout) | ||
if err != nil { | ||
return fmt.Errorf("pulling image: %w", err) | ||
} | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder what should be a behavior on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
request := &pb.PullImageRequest{ | ||
Image: &pb.ImageSpec{ | ||
Image: image, | ||
|
@@ -647,7 +655,21 @@ func PullImageWithSandbox(client internalapi.ImageManagerService, image string, | |
request.SandboxConfig = sandbox | ||
} | ||
logrus.Debugf("PullImageRequest: %v", request) | ||
res, err := client.PullImage(context.TODO(), request.Image, request.Auth, request.SandboxConfig) | ||
|
||
if timeout < 0 { | ||
return nil, errors.New("timeout should be bigger than 0") | ||
} | ||
|
||
ctx, cancel := context.WithCancel(context.Background()) | ||
defer cancel() | ||
|
||
if timeout > 0 { | ||
logrus.Debugf("Using context with timeout of %s", timeout) | ||
ctx, cancel = context.WithTimeout(ctx, timeout) | ||
defer cancel() | ||
} | ||
|
||
res, err := client.PullImage(ctx, request.Image, request.Auth, request.SandboxConfig) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
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.
I wonder if we need two of those or just one parameter
timeout
will be enough? Are those two having different semantics?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.
Technically we now have three per subcommand:
--timeout
/-t
flag on the root level (default 2s)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)--pull-timeout
/-pt
flag, specific for the image pullMerging 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.