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.

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

Merged
merged 1 commit into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 35 additions & 15 deletions cmd/crictl/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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.

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

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]

},
&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{
Expand All @@ -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 {
Expand Down Expand Up @@ -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"),
},
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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"),
}
Expand Down Expand Up @@ -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
}
}
Expand Down
28 changes: 25 additions & 3 deletions cmd/crictl/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"strconv"
"strings"
"syscall"
"time"

"github.com/docker/go-units"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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.

request := &pb.PullImageRequest{
Image: &pb.ImageSpec{
Image: image,
Expand All @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
golang.org/x/sys v0.21.0
golang.org/x/term v0.21.0
golang.org/x/text v0.16.0
google.golang.org/grpc v1.64.0
gopkg.in/yaml.v3 v3.0.1
k8s.io/api v0.30.1
k8s.io/apimachinery v0.30.1
Expand Down Expand Up @@ -85,7 +86,6 @@ require (
golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20240520151616-dc85e6b867a5 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240515191416-fc5f0ca64291 // indirect
google.golang.org/grpc v1.64.0 // indirect
google.golang.org/protobuf v1.34.1 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
Expand Down
12 changes: 9 additions & 3 deletions pkg/framework/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,8 @@ func ListImage(c internalapi.ImageManagerService, filter *runtimeapi.ImageFilter
return images
}

// PullPublicImage pulls the public image named imageName.
func PullPublicImage(c internalapi.ImageManagerService, imageName string, podConfig *runtimeapi.PodSandboxConfig) string {

// PrepareImageName builds a pullable image name for the provided one.
func PrepareImageName(imageName string) string {
ref, err := reference.ParseNamed(imageName)
if err == nil {
// Modify the image if it's a fully qualified image name
Expand All @@ -352,6 +351,13 @@ func PullPublicImage(c internalapi.ImageManagerService, imageName string, podCon
Failf("Unable to parse imageName: %v", err)
}

return imageName
}

// PullPublicImage pulls the public image named imageName.
func PullPublicImage(c internalapi.ImageManagerService, imageName string, podConfig *runtimeapi.PodSandboxConfig) string {
imageName = PrepareImageName(imageName)

By("Pull image : " + imageName)
imageSpec := &runtimeapi.ImageSpec{
Image: imageName,
Expand Down
18 changes: 18 additions & 0 deletions pkg/validate/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import (
"fmt"
"runtime"
"sort"
"time"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
internalapi "k8s.io/cri-api/pkg/apis"
runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
"sigs.k8s.io/cri-tools/pkg/framework"
Expand All @@ -45,6 +48,21 @@ var _ = framework.KubeDescribe("Image Manager", func() {
})
})

It("public image should timeout if requested [Conformance]", func() {
imageName := framework.PrepareImageName(testImageWithTag)

ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond)
defer cancel()
res, err := c.PullImage(ctx, &runtimeapi.ImageSpec{Image: imageName}, nil, nil)

Expect(res).To(BeEmpty())
Expect(err).To(HaveOccurred())

statusErr, ok := status.FromError(err)
Expect(ok).To(BeTrue())
Expect(statusErr.Code()).To(Equal(codes.DeadlineExceeded))
})

It("public image without tag should be pulled and removed [Conformance]", func() {
testPullPublicImage(c, testImageWithoutTag, testImagePodSandbox, func(s *runtimeapi.Image) {
Expect(s.RepoTags).To(Equal([]string{testImageWithoutTag + ":latest"}))
Expand Down
Loading