Add --pull-progress-timeout / pull_progress_timeout option

saschagrunert · saschagrunert · commit b0c05db0bbd2 · 2025-02-24T15:39:06.000+01:00
Add the option to be able to fine-tune the pull progress timeout or even disable it. Fixes #8764 and #8760 Follow-up on: #7887 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
diff --git a/completions/bash/crio b/completions/bash/crio
@@ -102,6 +102,7 @@ h
 --profile-cpu
 --profile-mem
 --profile-port
+--pull-progress-timeout
 --rdt-config-file
 --read-only
 --registries-conf
diff --git a/completions/fish/crio.fish b/completions/fish/crio.fish
@@ -138,6 +138,7 @@ complete -c crio -n '__fish_crio_no_subcommand' -f -l profile -d 'Enable pprof r
 complete -c crio -n '__fish_crio_no_subcommand' -f -l profile-cpu -r -d 'Write a pprof CPU profile to the provided path.'
 complete -c crio -n '__fish_crio_no_subcommand' -f -l profile-mem -r -d 'Write a pprof memory profile to the provided path.'
 complete -c crio -n '__fish_crio_no_subcommand' -f -l profile-port -r -d 'Port for the pprof profiler.'
+complete -c crio -n '__fish_crio_no_subcommand' -f -l pull-progress-timeout -r -d 'The timeout for an image pull to make progress until the pull operation gets canceled. This value will be also used for calculating the pull progress interval to --pull-progress-timeout / 10. Can be set to 0 to disable the timeout as well as the progress output.'
 complete -c crio -n '__fish_crio_no_subcommand' -f -l rdt-config-file -r -d 'Path to the RDT configuration file for configuring the resctrl pseudo-filesystem.'
 complete -c crio -n '__fish_crio_no_subcommand' -f -l read-only -d 'Setup all unprivileged containers to run as read-only. Automatically mounts the containers\' tmpfs on \'/run\', \'/tmp\' and \'/var/tmp\'.'
 complete -c crio -n '__fish_crio_no_subcommand' -f -l registry -r -d 'Registry to be prepended when pulling unqualified images. Can be specified multiple times.'
diff --git a/completions/zsh/_crio b/completions/zsh/_crio
@@ -109,6 +109,7 @@ it later with **--config**. Global options will modify the output.'
         '--profile-cpu'
         '--profile-mem'
         '--profile-port'
+        '--pull-progress-timeout'
         '--rdt-config-file'
         '--read-only'
         '--registries-conf'
diff --git a/docs/crio.8.md b/docs/crio.8.md
@@ -100,6 +100,7 @@ crio
 [--profile-mem]=[value]
 [--profile-port]=[value]
 [--profile]
+[--pull-progress-timeout]=[value]
 [--rdt-config-file]=[value]
 [--read-only]
 [--registry]=[value]
@@ -374,6 +375,8 @@ crio [GLOBAL OPTIONS] command [COMMAND OPTIONS] [ARGUMENTS...]
 
 **--profile-port**="": Port for the pprof profiler. (default: 6060)
 
+**--pull-progress-timeout**="": The timeout for an image pull to make progress until the pull operation gets canceled. This value will be also used for calculating the pull progress interval to --pull-progress-timeout / 10. Can be set to 0 to disable the timeout as well as the progress output. (default: 10s)
+
 **--rdt-config-file**="": Path to the RDT configuration file for configuring the resctrl pseudo-filesystem.
 
 **--read-only**: Setup all unprivileged containers to run as read-only. Automatically mounts the containers' tmpfs on '/run', '/tmp' and '/var/tmp'.
diff --git a/docs/crio.conf.5.md b/docs/crio.conf.5.md
@@ -466,6 +466,9 @@ CRI-O reads its configured registries defaults from the system wide containers-r
 **separate_pull_cgroup**=""
   [EXPERIMENTAL] If its value is set, then images are pulled into the specified cgroup.  If its value is set to "pod", then the pod's cgroup is used.  It is currently supported only with the systemd cgroup manager.
 
+**pull_progress_timeout**="10s"
+The timeout for an image pull to make progress until the pull operation gets canceled. This value will be also used for calculating the pull progress interval to pull_progress_timeout / 10. Can be set to 0 to disable the timeout as well as the progress output.
+
 ## CRIO.NETWORK TABLE
 The `crio.network` table containers settings pertaining to the management of CNI plugins.
 
diff --git a/internal/criocli/criocli.go b/internal/criocli/criocli.go
@@ -402,6 +402,9 @@ func mergeConfig(config *libconfig.Config, ctx *cli.Context) error {
 	if ctx.IsSet("big-files-temporary-dir") {
 		config.BigFilesTemporaryDir = ctx.String("big-files-temporary-dir")
 	}
+	if ctx.IsSet("pull-progress-timeout") {
+		config.PullProgressTimeout = ctx.Duration("pull-progress-timeout")
+	}
 	if ctx.IsSet("separate-pull-cgroup") {
 		config.SeparatePullCgroup = ctx.String("separate-pull-cgroup")
 	}
@@ -926,6 +929,12 @@ func getCrioFlags(defConf *libconfig.Config) []cli.Flag {
 			EnvVars: []string{"CONTAINER_BIG_FILES_TEMPORARY_DIR"},
 			Value:   defConf.BigFilesTemporaryDir,
 		},
+		&cli.DurationFlag{
+			Name:    "pull-progress-timeout",
+			Usage:   "The timeout for an image pull to make progress until the pull operation gets canceled. This value will be also used for calculating the pull progress interval to --pull-progress-timeout / 10. Can be set to 0 to disable the timeout as well as the progress output.",
+			EnvVars: []string{"CONTAINER_PULL_PROGRESS_TIMEOUT"},
+			Value:   defConf.PullProgressTimeout,
+		},
 		&cli.BoolFlag{
 			Name:    "read-only",
 			Usage:   "Setup all unprivileged containers to run as read-only. Automatically mounts the containers' tmpfs on '/run', '/tmp' and '/var/tmp'.",
diff --git a/internal/storage/image.go b/internal/storage/image.go
@@ -131,7 +131,7 @@ type ImageServer interface {
 	// for further analysis. Call Close() on the resulting image.
 	PrepareImage(systemContext *types.SystemContext, imageName RegistryImageReference) (types.ImageCloser, error)
 	// PullImage imports an image from the specified location.
-	PullImage(imageName RegistryImageReference, options *ImageCopyOptions) (types.ImageReference, error)
+	PullImage(ctx context.Context, imageName RegistryImageReference, options *ImageCopyOptions) (types.ImageReference, error)
 
 	// DeleteImage deletes a storage image (impacting all its tags)
 	DeleteImage(systemContext *types.SystemContext, id StorageImageID) error
@@ -554,11 +554,11 @@ func formatPullImageOutputItemGoroutine(dest io.Writer, items <-chan pullImageOu
 	}
 }
 
-func (svc *imageService) pullImageParent(imageName RegistryImageReference, parentCgroup string, options *ImageCopyOptions) (types.ImageReference, error) {
+func (svc *imageService) pullImageParent(ctx context.Context, imageName RegistryImageReference, parentCgroup string, options *ImageCopyOptions) (types.ImageReference, error) {
 	progress := options.Progress
 	// the first argument imageName is not used by the re-execed command but it is useful for debugging as it
 	// shows in the ps output.
-	cmd := reexec.CommandContext(svc.ctx, "crio-pull-image", imageName.StringForOutOfProcessConsumptionOnly())
+	cmd := reexec.CommandContext(ctx, "crio-pull-image", imageName.StringForOutOfProcessConsumptionOnly())
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
 		return nil, fmt.Errorf("error getting stdout pipe for image copy process: %w", err)
@@ -650,11 +650,11 @@ func (svc *imageService) pullImageParent(imageName RegistryImageReference, paren
 	return destRef, nil
 }
 
-func (svc *imageService) PullImage(imageName RegistryImageReference, options *ImageCopyOptions) (types.ImageReference, error) {
+func (svc *imageService) PullImage(ctx context.Context, imageName RegistryImageReference, options *ImageCopyOptions) (types.ImageReference, error) {
 	if options.CgroupPull.UseNewCgroup {
-		return svc.pullImageParent(imageName, options.CgroupPull.ParentCgroup, options)
+		return svc.pullImageParent(ctx, imageName, options.CgroupPull.ParentCgroup, options)
 	} else {
-		return pullImageImplementation(svc.ctx, svc.lookup, svc.store, imageName, options)
+		return pullImageImplementation(ctx, svc.lookup, svc.store, imageName, options)
 	}
 }
 
diff --git a/internal/storage/image_test.go b/internal/storage/image_test.go
@@ -574,7 +574,7 @@ var _ = t.Describe("Image", func() {
 			Expect(err).To(BeNil())
 
 			// When
-			res, err := sut.PullImage(imageRef, &storage.ImageCopyOptions{
+			res, err := sut.PullImage(context.Background(), imageRef, &storage.ImageCopyOptions{
 				SourceCtx: &types.SystemContext{SignaturePolicyPath: "/not-existing"},
 			})
 
@@ -589,7 +589,7 @@ var _ = t.Describe("Image", func() {
 			Expect(err).To(BeNil())
 
 			// When
-			res, err := sut.PullImage(imageRef, &storage.ImageCopyOptions{
+			res, err := sut.PullImage(context.Background(), imageRef, &storage.ImageCopyOptions{
 				SourceCtx: &types.SystemContext{SignaturePolicyPath: "../../test/policy.json"},
 			})
 
@@ -604,14 +604,32 @@ var _ = t.Describe("Image", func() {
 			Expect(err).To(BeNil())
 
 			// When
-			res, err := sut.PullImage(imageRef, &storage.ImageCopyOptions{
+			res, err := sut.PullImage(context.Background(), imageRef, &storage.ImageCopyOptions{
 				SourceCtx: &types.SystemContext{SignaturePolicyPath: "../../test/policy.json"},
 			})
 
 			// Then
 			Expect(err).NotTo(BeNil())
 			Expect(res).To(BeNil())
 		})
+
+		It("should fail on cancelled context", func() {
+			// Given
+			imageRef, err := references.ParseRegistryImageReferenceFromOutOfProcessData("localhost/busybox:latest")
+			Expect(err).ToNot(HaveOccurred())
+
+			// When
+			ctx, cancel := context.WithCancel(context.Background())
+			cancel()
+			res, err := sut.PullImage(ctx, imageRef, &storage.ImageCopyOptions{
+				SourceCtx: &types.SystemContext{SignaturePolicyPath: "../../test/policy.json"},
+			})
+
+			// Then
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("context canceled"))
+			Expect(res).To(BeNil())
+		})
 	})
 
 	t.Describe("CompileRegexpsForPinnedImages", func() {
diff --git a/internal/storage/runtime.go b/internal/storage/runtime.go
@@ -315,7 +315,7 @@ func (r *runtimeService) CreatePodSandbox(systemContext *types.SystemContext, po
 		if imageAuthFile != "" {
 			sourceCtx.AuthFilePath = imageAuthFile
 		}
-		ref, err = r.storageImageServer.PullImage(pauseImage, &ImageCopyOptions{
+		ref, err = r.storageImageServer.PullImage(context.Background(), pauseImage, &ImageCopyOptions{
 			SourceCtx:      &sourceCtx,
 			DestinationCtx: systemContext,
 		})
diff --git a/internal/storage/runtime_test.go b/internal/storage/runtime_test.go
@@ -765,7 +765,7 @@ var _ = t.Describe("Runtime", func() {
 				imageServerMock.EXPECT().GetStore().Return(storeMock),
 				mockResolveReference(storeMock, storageTransportMock,
 					"docker.io/library/pauseimagename:latest", "", ""),
-				imageServerMock.EXPECT().PullImage(pauseImageRef, expectedCopyOptions).Return(pulledRef, nil),
+				imageServerMock.EXPECT().PullImage(gomock.Any(), pauseImageRef, expectedCopyOptions).Return(pulledRef, nil),
 				mockResolveReference(storeMock, storageTransportMock,
 					"docker.io/library/pauseimagename:latest", "", imageID.IDStringForOutOfProcessConsumptionOnly()),
 				imageServerMock.EXPECT().GetStore().Return(storeMock),
diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"time"
 
 	"github.com/BurntSushi/toml"
 	"github.com/containers/common/pkg/hooks"
@@ -533,6 +534,11 @@ type ImageConfig struct {
 	Registries []string `toml:"registries"`
 	// Temporary directory for big files
 	BigFilesTemporaryDir string `toml:"big_files_temporary_dir"`
+	// PullProgressTimeout is the timeout for an image pull to make progress
+	// until the pull operation gets canceled. This value will be also used for
+	// calculating the pull progress interval to pullProgressTimeout / 10.
+	// Can be set to 0 to disable the timeout as well as the progress output.
+	PullProgressTimeout time.Duration `toml:"pull_progress_timeout"`
 }
 
 // NetworkConfig represents the "crio.network" TOML config table
@@ -892,11 +898,12 @@ func DefaultConfig() (*Config, error) {
 			DisableHostPortMapping:      false,
 		},
 		ImageConfig: ImageConfig{
-			DefaultTransport:   "docker://",
-			PauseImage:         DefaultPauseImage,
-			PauseCommand:       "/pause",
-			ImageVolumes:       ImageVolumesMkdir,
-			SignaturePolicyDir: "/etc/crio/policies",
+			DefaultTransport:    "docker://",
+			PauseImage:          DefaultPauseImage,
+			PauseCommand:        "/pause",
+			ImageVolumes:        ImageVolumesMkdir,
+			SignaturePolicyDir:  "/etc/crio/policies",
+			PullProgressTimeout: 10 * time.Second,
 		},
 		NetworkConfig: NetworkConfig{
 			NetworkDir: cniConfigDir,
diff --git a/pkg/config/template.go b/pkg/config/template.go
@@ -535,6 +535,11 @@ func initCrioTemplateConfig(c *Config) ([]*templateConfigValue, error) {
 			group:          crioImageConfig,
 			isDefaultValue: simpleEqual(dc.BigFilesTemporaryDir, c.BigFilesTemporaryDir),
 		},
+		{
+			templateString: templateStringCrioImagePullProgressTimeout,
+			group:          crioImageConfig,
+			isDefaultValue: simpleEqual(dc.PullProgressTimeout, c.PullProgressTimeout),
+		},
 		{
 			templateString: templateStringCrioNetworkCniDefaultNetwork,
 			group:          crioNetworkConfig,
@@ -1470,6 +1475,13 @@ const templateStringCrioImageBigFilesTemporaryDir = `# Temporary directory to us
 
 `
 
+const templateStringCrioImagePullProgressTimeout = `# The timeout for an image pull to make progress until the pull operation
+# gets canceled. This value will be also used for calculating the pull progress interval to pull_progress_timeout / 10.
+# Can be set to 0 to disable the timeout as well as the progress output.
+{{ $.Comment }}pull_progress_timeout = "{{ .PullProgressTimeout }}"
+
+`
+
 const templateStringCrioNetwork = `# The crio.network table containers settings pertaining to the management of
 # CNI plugins.
 [crio.network]
diff --git a/server/image_pull.go b/server/image_pull.go
@@ -244,13 +244,16 @@ func (s *Server) pullImageCandidate(ctx context.Context, sourceCtx *imageTypes.S
 	// Collect pull progress metrics
 	progress := make(chan imageTypes.ProgressProperties)
 	defer close(progress) // nolint:gocritic
-	go metricsFromProgressGoroutine(ctx, progress, remoteCandidateName, tmpImg)
 
-	_, err = s.StorageImageServer().PullImage(remoteCandidateName, &storage.ImageCopyOptions{
+	// Cancel the pull if no progress is made
+	pullCtx, cancel := context.WithCancel(ctx)
+	go consumeImagePullProgress(ctx, cancel, s.Config().PullProgressTimeout, progress, remoteCandidateName, tmpImg)
+
+	_, err = s.StorageImageServer().PullImage(pullCtx, remoteCandidateName, &storage.ImageCopyOptions{
 		SourceCtx:        sourceCtx,
 		DestinationCtx:   s.config.SystemContext,
 		OciDecryptConfig: decryptConfig,
-		ProgressInterval: time.Second,
+		ProgressInterval: s.Config().PullProgressTimeout / 10,
 		Progress:         progress,
 		CgroupPull: storage.CgroupPullConfiguration{
 			UseNewCgroup: s.config.SeparatePullCgroup != "",
@@ -265,9 +268,23 @@ func (s *Server) pullImageCandidate(ctx context.Context, sourceCtx *imageTypes.S
 	return nil
 }
 
-// metricsFromProgressGoroutine consumes progress and turns it into metrics updates.
-func metricsFromProgressGoroutine(ctx context.Context, progress <-chan imageTypes.ProgressProperties, remoteCandidateName storage.RegistryImageReference, remoteImage imageTypes.Image) {
+// consumeImagePullProgress consumes progress and turns it into metrics updates.
+// It also checks if progress is being made within a constant timeout.
+// If the timeout is reached because no progress updates have been made, then
+// the cancel function will be called.
+func consumeImagePullProgress(ctx context.Context, cancel context.CancelFunc, pullProgressTimeout time.Duration, progress <-chan imageTypes.ProgressProperties, remoteCandidateName storage.RegistryImageReference, remoteImage imageTypes.Image) {
+	timer := time.AfterFunc(pullProgressTimeout, func() {
+		if pullProgressTimeout != 0 {
+			log.Warnf(ctx, "Timed out on waiting up to %s for image pull progress updates", pullProgressTimeout)
+			cancel()
+		}
+	})
+	timer.Stop()       // don't start the timer immediately
+	defer timer.Stop() // ensure that the timer is stopped when we exit the progress loop
+
 	for p := range progress {
+		timer.Reset(pullProgressTimeout)
+
 		if p.Event == imageTypes.ProgressEventSkipped {
 			// Skipped digests metrics
 			tryRecordSkippedMetric(ctx, remoteCandidateName, p.Artifact.Digest)
diff --git a/server/image_pull_test.go b/server/image_pull_test.go
@@ -43,7 +43,7 @@ var _ = t.Describe("ImagePull", func() {
 					Return(&storage.ImageResult{ID: otherImageID}, nil),
 				imageCloserMock.EXPECT().ConfigInfo().
 					Return(imageTypes.BlobInfo{Digest: digest.Digest("")}),
-				imageServerMock.EXPECT().PullImage(imageCandidate, gomock.Any()).
+				imageServerMock.EXPECT().PullImage(gomock.Any(), imageCandidate, gomock.Any()).
 					Return(nil, nil),
 				imageCloserMock.EXPECT().Close().Return(nil),
 				imageServerMock.EXPECT().ImageStatusByName(
@@ -119,7 +119,7 @@ var _ = t.Describe("ImagePull", func() {
 					Return(&storage.ImageResult{ID: otherImageID}, nil),
 				imageCloserMock.EXPECT().ConfigInfo().
 					Return(imageTypes.BlobInfo{Digest: digest.Digest("")}),
-				imageServerMock.EXPECT().PullImage(imageCandidate, gomock.Any()).
+				imageServerMock.EXPECT().PullImage(gomock.Any(), imageCandidate, gomock.Any()).
 					Return(nil, nil),
 				imageCloserMock.EXPECT().Close().Return(nil),
 				imageServerMock.EXPECT().ImageStatusByName(
@@ -172,7 +172,7 @@ var _ = t.Describe("ImagePull", func() {
 					Return(&storage.ImageResult{ID: otherImageID}, nil),
 				imageCloserMock.EXPECT().ConfigInfo().
 					Return(imageTypes.BlobInfo{Digest: digest.Digest("")}),
-				imageServerMock.EXPECT().PullImage(imageCandidate, gomock.Any()).
+				imageServerMock.EXPECT().PullImage(gomock.Any(), imageCandidate, gomock.Any()).
 					Return(nil, t.TestError),
 				imageCloserMock.EXPECT().Close().Return(nil),
 			)
diff --git a/test/image.bats b/test/image.bats
@@ -486,3 +486,16 @@ EOF
 
 	run crictl run "$TESTDIR"/memory.json "$TESTDATA"/sandbox_config.json
 }
+
+@test "pull progress timeout should trigger when being set too low" {
+	CONTAINER_PULL_PROGRESS_TIMEOUT=1ms start_crio
+
+	run ! crictl pull "$IMAGE_LIST_TAG"
+	[[ "$output" == *"context canceled"* ]]
+}
+
+@test "pull progress timeout should not timeout when set to 0" {
+	CONTAINER_PULL_PROGRESS_TIMEOUT=0 start_crio
+
+	crictl pull "$IMAGE_LIST_TAG"
+}
diff --git a/test/mocks/criostorage/criostorage.go b/test/mocks/criostorage/criostorage.go

Original file line number	Diff line number	Diff line change
`@@ -315,7 +315,7 @@ func (r runtimeService) CreatePodSandbox(systemContext types.SystemContext, po`
`315`	`315`	`if imageAuthFile != "" {`
`316`	`316`	`sourceCtx.AuthFilePath = imageAuthFile`
`317`	`317`	`}`
`318`		`- ref, err = r.storageImageServer.PullImage(pauseImage, &ImageCopyOptions{`
	`318`	`+ ref, err = r.storageImageServer.PullImage(context.Background(), pauseImage, &ImageCopyOptions{`
`319`	`319`	`SourceCtx: &sourceCtx,`
`320`	`320`	`DestinationCtx: systemContext,`
`321`	`321`	`})`