From 6d655e9f557099e35be7b33067905c2564c53419 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= Date: Thu, 2 Jan 2025 07:01:55 +0900 Subject: [PATCH 1/7] Cherry-pick changes from containers/image project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cherry-pick the following changes: - containers/image/pull#2636 Signed-off-by: Krzysztof Wilczyński --- go.mod | 2 +- go.sum | 4 +-- .../image/v5/storage/storage_dest.go | 34 +++++++++++-------- vendor/modules.txt | 2 +- 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/go.mod b/go.mod index 859f8ca6033..969bf9dc19e 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,7 @@ require ( github.com/containers/common v0.57.5-0.20241001151712-f77c20871a20 github.com/containers/conmon v2.0.20+incompatible github.com/containers/conmon-rs v0.6.2-0.20230920142715-f5a362044a57 - github.com/containers/image/v5 v5.29.3-0.20240528085521-f8d6234cf12d + github.com/containers/image/v5 v5.29.3-0.20250101212958-64440ba01d29 github.com/containers/kubensmnt v1.2.0 github.com/containers/ocicrypt v1.1.10 github.com/containers/podman/v4 v4.9.3 diff --git a/go.sum b/go.sum index 5308f72c3e2..b48653a0e57 100644 --- a/go.sum +++ b/go.sum @@ -737,8 +737,8 @@ github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6J github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I= github.com/containers/conmon-rs v0.6.2-0.20230920142715-f5a362044a57 h1:EHiHMe0cOPvBzi20g/f+NzhjhPFTxpyAkpRE4h1ZAzw= github.com/containers/conmon-rs v0.6.2-0.20230920142715-f5a362044a57/go.mod h1:rMhWT6H3demwJCFuT+Bc9b+T9oypfZ5SnPzOrt6QNkg= -github.com/containers/image/v5 v5.29.3-0.20240528085521-f8d6234cf12d h1:A6OsN1qfe4EolTeYMpg3Nkzh0sZKWt1c0sOgLX/vFwA= -github.com/containers/image/v5 v5.29.3-0.20240528085521-f8d6234cf12d/go.mod h1:kQ7qcDsps424ZAz24thD+x7+dJw1vgur3A9tTDsj97E= +github.com/containers/image/v5 v5.29.3-0.20250101212958-64440ba01d29 h1:+SZuqXj5p7xjrY133Gqr3KnrpztXdZrX863lPavb8k8= +github.com/containers/image/v5 v5.29.3-0.20250101212958-64440ba01d29/go.mod h1:kQ7qcDsps424ZAz24thD+x7+dJw1vgur3A9tTDsj97E= github.com/containers/kubensmnt v1.2.0 h1:BDtkaOFQ5fN7FnB9kC6peMW50KkwI1KI8E9ROBFeQIg= github.com/containers/kubensmnt v1.2.0/go.mod h1:1/HG09N/a1+WSD3zkurzeWtqlKRSfUUnlIF/08zloqk= github.com/containers/libtrust v0.0.0-20230121012942-c1716e8a8d01 h1:Qzk5C6cYglewc+UyGf6lc8Mj2UaPTHy/iF2De0/77CA= diff --git a/vendor/github.com/containers/image/v5/storage/storage_dest.go b/vendor/github.com/containers/image/v5/storage/storage_dest.go index 6b59be1fd97..3f2a5ff10b4 100644 --- a/vendor/github.com/containers/image/v5/storage/storage_dest.go +++ b/vendor/github.com/containers/image/v5/storage/storage_dest.go @@ -21,7 +21,6 @@ import ( "github.com/containers/image/v5/internal/imagedestination/stubs" "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/internal/putblobdigest" - "github.com/containers/image/v5/internal/set" "github.com/containers/image/v5/internal/signature" "github.com/containers/image/v5/internal/tmpdir" "github.com/containers/image/v5/manifest" @@ -84,6 +83,9 @@ type storageImageDestination struct { indexToAddedLayerInfo map[int]addedLayerInfo // Mapping from layer (by index) to blob to add to the image blobAdditionalLayer map[digest.Digest]storage.AdditionalLayer // Mapping from layer blobsums to their corresponding additional layer diffOutputs map[digest.Digest]*graphdriver.DriverWithDifferOutput // Mapping from digest to differ output + + // Config + configDigest digest.Digest // "" if N/A or not known yet. } // addedLayerInfo records data about a layer to use in this image. @@ -170,7 +172,17 @@ func (s *storageImageDestination) PutBlobWithOptions(ctx context.Context, stream return info, err } - if options.IsConfig || options.LayerIndex == nil { + if options.IsConfig { + s.lock.Lock() + defer s.lock.Unlock() + if s.configDigest != "" { + return private.UploadedBlob{}, fmt.Errorf("after config %q, refusing to record another config %q", + s.configDigest.String(), info.Digest.String()) + } + s.configDigest = info.Digest + return info, nil + } + if options.LayerIndex == nil { return info, nil } @@ -776,22 +788,14 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t options.CreationDate = *inspect.Created } - // Set up to save the non-layer blobs as data items. Since we only share layers, they should all be in files, so - // we just need to screen out the ones that are actually layers to get the list of non-layers. - dataBlobs := set.New[digest.Digest]() - for blob := range s.filenames { - dataBlobs.Add(blob) - } - for _, layerBlob := range layerBlobs { - dataBlobs.Delete(layerBlob.Digest) - } - for _, blob := range dataBlobs.Values() { - v, err := os.ReadFile(s.filenames[blob]) + // Set up to save the config as a data item. Since we only share layers, the config should be in a file. + if s.configDigest != "" { + v, err := os.ReadFile(s.filenames[s.configDigest]) if err != nil { - return fmt.Errorf("copying non-layer blob %q to image: %w", blob, err) + return fmt.Errorf("copying config blob %q to image: %w", s.configDigest, err) } options.BigData = append(options.BigData, storage.ImageBigDataOption{ - Key: blob.String(), + Key: s.configDigest.String(), Data: v, Digest: digest.Canonical.FromBytes(v), }) diff --git a/vendor/modules.txt b/vendor/modules.txt index 708b213ecb0..1194f07370a 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -261,7 +261,7 @@ github.com/containers/conmon/runner/config ## explicit; go 1.20 github.com/containers/conmon-rs/internal/proto github.com/containers/conmon-rs/pkg/client -# github.com/containers/image/v5 v5.29.3-0.20240528085521-f8d6234cf12d +# github.com/containers/image/v5 v5.29.3-0.20250101212958-64440ba01d29 ## explicit; go 1.19 github.com/containers/image/v5/copy github.com/containers/image/v5/directory From ee48ae2db78005b9422792c25d4d70ce85370cac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= Date: Mon, 9 Dec 2024 22:09:12 +0900 Subject: [PATCH 2/7] Cherry-pick changes from containers/storage project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cherry-pick the following changes: - containers/storage/pull#2156 - containers/storage/pull#2162 - containers/storage/pull#2185 This cherry-pick brings the following Pull Request as a dependency: - containers/storage/pull#1833 Signed-off-by: Krzysztof Wilczyński --- go.mod | 2 +- go.sum | 4 ++-- .../containers/storage/pkg/chrootarchive/archive.go | 2 +- vendor/modules.txt | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index fccd07128a6..d13a5bd821a 100644 --- a/go.mod +++ b/go.mod @@ -27,7 +27,7 @@ require ( github.com/containers/kubensmnt v1.2.0 github.com/containers/ocicrypt v1.1.10 github.com/containers/podman/v4 v4.9.3 - github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958 + github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3 github.com/coreos/go-systemd/v22 v22.5.1-0.20231103132048-7d375ecc2b09 github.com/cpuguy83/go-md2man v1.0.10 github.com/creack/pty v1.1.21 diff --git a/go.sum b/go.sum index 23a70955ad2..09954802d93 100644 --- a/go.sum +++ b/go.sum @@ -747,8 +747,8 @@ github.com/containers/ocicrypt v1.1.10 h1:r7UR6o8+lyhkEywetubUUgcKFjOWOaWz8cEBrC github.com/containers/ocicrypt v1.1.10/go.mod h1:YfzSSr06PTHQwSTUKqDSjish9BeW1E4HUmreluQcMd8= github.com/containers/podman/v4 v4.9.3 h1:3tEnvIqijxBYtILRdHcbn0UNHAyUiQ1Y5hcvkYmutZA= github.com/containers/podman/v4 v4.9.3/go.mod h1:J2qLop+mWjAOxh0QQyYPdnPA3jI6ay2eU0OKakgMniQ= -github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958 h1:jcI4WOwt+Q4t2tGCVk03i/wbmIDfLTHkt+iKbpMZpuA= -github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958/go.mod h1:W+4hYMT3FHhVMqf+J//T3+uDRfqKh5IPx+6m4xb8EhY= +github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3 h1:gxrr8HDgDQI8kXGi08/PeQhQpT/SiirchFkolq454qU= +github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3/go.mod h1:1vrhsjgb/mfuIOGk1TIX6890/LGWhC8Kqzps3Bh0CRA= github.com/coreos/go-systemd/v22 v22.5.1-0.20231103132048-7d375ecc2b09 h1:OoRAFlvDGCUqDLampLQjk0yeeSGdF9zzst/3G9IkBbc= github.com/coreos/go-systemd/v22 v22.5.1-0.20231103132048-7d375ecc2b09/go.mod h1:m2r/smMKsKwgMSAoFKHaa68ImdCSNuKE1MxvQ64xuCQ= github.com/cpuguy83/go-md2man v1.0.10 h1:BSKMNlYxDvnunlTymqtgONjNnaRV1sTpcovwwjF22jk= diff --git a/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go b/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go index 5287cf380f5..f221a22835c 100644 --- a/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go +++ b/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go @@ -46,7 +46,7 @@ func Untar(tarArchive io.Reader, dest string, options *archive.TarOptions) error // This should be used to prevent a potential attacker from manipulating `dest` // such that it would provide access to files outside of `dest` through things // like symlinks. Normally `ResolveSymlinksInScope` would handle this, however -// sanitizing symlinks in this manner is inherently racey: +// sanitizing symlinks in this manner is inherrently racey: // ref: CVE-2018-15664 func UntarWithRoot(tarArchive io.Reader, dest string, options *archive.TarOptions, root string) error { return untarHandler(tarArchive, dest, options, true, root) diff --git a/vendor/modules.txt b/vendor/modules.txt index dea3a5bc842..0149fae2cad 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -351,7 +351,7 @@ github.com/containers/ocicrypt/utils/keyprovider # github.com/containers/podman/v4 v4.9.3 ## explicit; go 1.18 github.com/containers/podman/v4/pkg/checkpoint/crutils -# github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958 +# github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3 ## explicit; go 1.19 github.com/containers/storage github.com/containers/storage/drivers From 1250a7e409a651f2766ce76bf601071e38dd0ad1 Mon Sep 17 00:00:00 2001 From: Pannaga Rao Bhoja Ramamanohara Date: Thu, 6 Feb 2025 12:21:10 -0500 Subject: [PATCH 3/7] Revert "Cherry-pick changes from containers/storage project" This reverts commit ee48ae2db78005b9422792c25d4d70ce85370cac. The required changes from container/storage are already merged in the previous commit. Signed-off-by: Pannaga Rao Bhoja Ramamanohara --- go.mod | 2 +- go.sum | 4 ++-- .../containers/storage/pkg/chrootarchive/archive.go | 2 +- vendor/modules.txt | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index d13a5bd821a..fccd07128a6 100644 --- a/go.mod +++ b/go.mod @@ -27,7 +27,7 @@ require ( github.com/containers/kubensmnt v1.2.0 github.com/containers/ocicrypt v1.1.10 github.com/containers/podman/v4 v4.9.3 - github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3 + github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958 github.com/coreos/go-systemd/v22 v22.5.1-0.20231103132048-7d375ecc2b09 github.com/cpuguy83/go-md2man v1.0.10 github.com/creack/pty v1.1.21 diff --git a/go.sum b/go.sum index 09954802d93..23a70955ad2 100644 --- a/go.sum +++ b/go.sum @@ -747,8 +747,8 @@ github.com/containers/ocicrypt v1.1.10 h1:r7UR6o8+lyhkEywetubUUgcKFjOWOaWz8cEBrC github.com/containers/ocicrypt v1.1.10/go.mod h1:YfzSSr06PTHQwSTUKqDSjish9BeW1E4HUmreluQcMd8= github.com/containers/podman/v4 v4.9.3 h1:3tEnvIqijxBYtILRdHcbn0UNHAyUiQ1Y5hcvkYmutZA= github.com/containers/podman/v4 v4.9.3/go.mod h1:J2qLop+mWjAOxh0QQyYPdnPA3jI6ay2eU0OKakgMniQ= -github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3 h1:gxrr8HDgDQI8kXGi08/PeQhQpT/SiirchFkolq454qU= -github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3/go.mod h1:1vrhsjgb/mfuIOGk1TIX6890/LGWhC8Kqzps3Bh0CRA= +github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958 h1:jcI4WOwt+Q4t2tGCVk03i/wbmIDfLTHkt+iKbpMZpuA= +github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958/go.mod h1:W+4hYMT3FHhVMqf+J//T3+uDRfqKh5IPx+6m4xb8EhY= github.com/coreos/go-systemd/v22 v22.5.1-0.20231103132048-7d375ecc2b09 h1:OoRAFlvDGCUqDLampLQjk0yeeSGdF9zzst/3G9IkBbc= github.com/coreos/go-systemd/v22 v22.5.1-0.20231103132048-7d375ecc2b09/go.mod h1:m2r/smMKsKwgMSAoFKHaa68ImdCSNuKE1MxvQ64xuCQ= github.com/cpuguy83/go-md2man v1.0.10 h1:BSKMNlYxDvnunlTymqtgONjNnaRV1sTpcovwwjF22jk= diff --git a/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go b/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go index f221a22835c..5287cf380f5 100644 --- a/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go +++ b/vendor/github.com/containers/storage/pkg/chrootarchive/archive.go @@ -46,7 +46,7 @@ func Untar(tarArchive io.Reader, dest string, options *archive.TarOptions) error // This should be used to prevent a potential attacker from manipulating `dest` // such that it would provide access to files outside of `dest` through things // like symlinks. Normally `ResolveSymlinksInScope` would handle this, however -// sanitizing symlinks in this manner is inherrently racey: +// sanitizing symlinks in this manner is inherently racey: // ref: CVE-2018-15664 func UntarWithRoot(tarArchive io.Reader, dest string, options *archive.TarOptions, root string) error { return untarHandler(tarArchive, dest, options, true, root) diff --git a/vendor/modules.txt b/vendor/modules.txt index 0149fae2cad..dea3a5bc842 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -351,7 +351,7 @@ github.com/containers/ocicrypt/utils/keyprovider # github.com/containers/podman/v4 v4.9.3 ## explicit; go 1.18 github.com/containers/podman/v4/pkg/checkpoint/crutils -# github.com/containers/storage v1.51.1-0.20241210064209-75435e49ced3 +# github.com/containers/storage v1.51.1-0.20250123153027-e1bc50e6f958 ## explicit; go 1.19 github.com/containers/storage github.com/containers/storage/drivers From 3dab91d5609e41ff0b0881e8a33e3f243582a9c1 Mon Sep 17 00:00:00 2001 From: Ayato Tokubi Date: Thu, 19 Dec 2024 16:43:33 +0000 Subject: [PATCH 4/7] Avoid using UpdateContainerStatus for ReopenContainerLog and add logs tests Signed-off-by: Ayato Tokubi (cherry picked from commit 05296551a060b9c96f0ca9001a0abb37780a5dc3) Signed-off-by: Ayato Tokubi --- internal/oci/container.go | 3 ++ internal/oci/oci.go | 10 +++++++ internal/oci/runtime_oci.go | 4 +++ internal/oci/runtime_pod.go | 4 +++ internal/oci/runtime_vm.go | 4 +++ server/container_reopen_log.go | 17 +++++------ test/logs.bats | 53 ++++++++++++++++++++++++++++++++++ test/mocks/oci/oci.go | 14 +++++++++ 8 files changed, 100 insertions(+), 9 deletions(-) create mode 100644 test/logs.bats diff --git a/internal/oci/container.go b/internal/oci/container.go index dd60ed3046c..beca05fc566 100644 --- a/internal/oci/container.go +++ b/internal/oci/container.go @@ -421,6 +421,7 @@ func (c *Container) State() *ContainerState { } // StateNoLock returns the state of a container without using a lock. +// It has been known to cause segfaults in the past so it really should be used sparingly. func (c *Container) StateNoLock() *ContainerState { return c.state } @@ -492,6 +493,8 @@ func (c *Container) exitFilePath() string { // Living checks if a container's init PID exists and it's running, without calling // a given runtime directly to check the state, which is expensive. +// This can't be used for runtimeVM because it uses the VM's pid as a container pid, +// and the VM doesn't necessarily stop when the container stops. func (c *Container) Living() error { _, _, err := c.pid() if err != nil { diff --git a/internal/oci/oci.go b/internal/oci/oci.go index 7b887647fdd..5e68bac1f73 100644 --- a/internal/oci/oci.go +++ b/internal/oci/oci.go @@ -79,6 +79,7 @@ type RuntimeImpl interface { ReopenContainerLog(context.Context, *Container) error CheckpointContainer(context.Context, *Container, *rspec.Spec, bool) error RestoreContainer(context.Context, *Container, string, string) error + IsContainerAlive(*Container) bool } // New creates a new Runtime with options provided @@ -493,3 +494,12 @@ func (r *Runtime) RestoreContainer(ctx context.Context, c *Container, cgroupPare return impl.RestoreContainer(ctx, c, cgroupParent, mountLabel) } + +func (r *Runtime) IsContainerAlive(c *Container) (bool, error) { + impl, err := r.RuntimeImpl(c) + if err != nil { + return false, err + } + + return impl.IsContainerAlive(c), nil +} diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index 44d3c49d82b..33c4edc3baa 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -1569,3 +1569,7 @@ func (r *runtimeOCI) checkpointRestoreSupported(runtimePath string) error { } return nil } + +func (r *runtimeOCI) IsContainerAlive(c *Container) bool { + return c.Living() == nil +} diff --git a/internal/oci/runtime_pod.go b/internal/oci/runtime_pod.go index 14ec7131b24..b446a7afae6 100644 --- a/internal/oci/runtime_pod.go +++ b/internal/oci/runtime_pod.go @@ -296,3 +296,7 @@ func (r *runtimePod) ReopenContainerLog(ctx context.Context, c *Container) error ID: c.ID(), }) } + +func (r *runtimePod) IsContainerAlive(c *Container) bool { + return c.Living() == nil +} diff --git a/internal/oci/runtime_vm.go b/internal/oci/runtime_vm.go index dc749c0aba2..1dd1b4ace9c 100644 --- a/internal/oci/runtime_vm.go +++ b/internal/oci/runtime_vm.go @@ -1274,3 +1274,7 @@ func EncodeKataVirtualVolumeToBase64(ctx context.Context, volume *katavolume.Kat option := base64.StdEncoding.EncodeToString(validKataVirtualVolumeJSON) return option, nil } + +func (r *runtimeVM) IsContainerAlive(c *Container) bool { + return r.kill(c.ID(), "", 0, false) == nil +} diff --git a/server/container_reopen_log.go b/server/container_reopen_log.go index 93091ba7a71..ff2b15b591d 100644 --- a/server/container_reopen_log.go +++ b/server/container_reopen_log.go @@ -1,16 +1,16 @@ package server import ( + "context" "errors" "fmt" - "github.com/cri-o/cri-o/internal/log" - "github.com/cri-o/cri-o/internal/oci" - "golang.org/x/net/context" types "k8s.io/cri-api/pkg/apis/runtime/v1" + + "github.com/cri-o/cri-o/internal/log" ) -// ReopenContainerLog reopens the containers log file +// ReopenContainerLog reopens the containers log file. func (s *Server) ReopenContainerLog(ctx context.Context, req *types.ReopenContainerLogRequest) (*types.ReopenContainerLogResponse, error) { ctx, span := log.StartSpan(ctx) defer span.End() @@ -19,13 +19,12 @@ func (s *Server) ReopenContainerLog(ctx context.Context, req *types.ReopenContai return nil, fmt.Errorf("could not find container %s: %w", req.ContainerId, err) } - if err := s.ContainerServer.Runtime().UpdateContainerStatus(ctx, c); err != nil { + isRunning, err := s.ContainerServer.Runtime().IsContainerAlive(c) + if err != nil { return nil, err } - - cState := c.State() - if !(cState.Status == oci.ContainerStateRunning || cState.Status == oci.ContainerStateCreated) { - return nil, errors.New("container is not created or running") + if !isRunning { + return nil, errors.New("container is not running") } if err := s.ContainerServer.Runtime().ReopenContainerLog(ctx, c); err != nil { diff --git a/test/logs.bats b/test/logs.bats new file mode 100644 index 00000000000..d703f7247e4 --- /dev/null +++ b/test/logs.bats @@ -0,0 +1,53 @@ +#!/usr/bin/env bats +# vim:set ft=bash : + +load helpers + +function setup() { + setup_test +} + +function teardown() { + cleanup_test +} + +@test "ReopenContainerLog should succeed when the container is running" { + start_crio + + jq '.metadata.name = "sleep" + | .command = ["/bin/sh", "-c", "sleep 600"]' \ + "$TESTDATA"/container_config.json > "$TESTDIR"/trap.json + + ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json) + crictl logs -r "$ctr_id" +} + +@test "ReopenContainerLog should fail when the container is stopped" { + start_crio + + jq '.metadata.name = "sleep" + | .command = ["/bin/sh", "-c", "sleep 600"]' \ + "$TESTDATA"/container_config.json > "$TESTDIR"/trap.json + + ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json) + crictl stop "$ctr_id" + run ! crictl logs -r "$ctr_id" +} + +@test "ReopenContainerLog should not be blocked during deletion" { + start_crio + + jq '.metadata.name = "trap" + | .command = ["/bin/sh", "-c", "trap \"sleep 600\" TERM && sleep 600"]' \ + "$TESTDATA"/container_config.json > "$TESTDIR"/trap.json + + ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json) + # Especially when using kata, it sometimes takes a few seconds to actually run container + sleep 5 + + crictl stop -t 10 "$ctr_id" & + wait_for_log "Request: &StopContainerRequest" + crictl logs -r "$ctr_id" + output=$(crictl inspect "$ctr_id" | jq -r ".status.state") + [[ "$output" == "CONTAINER_RUNNING" ]] +} diff --git a/test/mocks/oci/oci.go b/test/mocks/oci/oci.go index 460151366fe..90ab4c8bf14 100644 --- a/test/mocks/oci/oci.go +++ b/test/mocks/oci/oci.go @@ -141,6 +141,20 @@ func (mr *MockRuntimeImplMockRecorder) ExecSyncContainer(arg0, arg1, arg2, arg3 return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExecSyncContainer", reflect.TypeOf((*MockRuntimeImpl)(nil).ExecSyncContainer), arg0, arg1, arg2, arg3) } +// IsContainerAlive mocks base method. +func (m *MockRuntimeImpl) IsContainerAlive(arg0 *oci.Container) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsContainerAlive", arg0) + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsContainerAlive indicates an expected call of IsContainerAlive. +func (mr *MockRuntimeImplMockRecorder) IsContainerAlive(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsContainerAlive", reflect.TypeOf((*MockRuntimeImpl)(nil).IsContainerAlive), arg0) +} + // PauseContainer mocks base method. func (m *MockRuntimeImpl) PauseContainer(arg0 context.Context, arg1 *oci.Container) error { m.ctrl.T.Helper() From 2d83590d104db38e1432cd03f6e2709749c86b84 Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Mon, 18 Nov 2024 12:52:25 +0100 Subject: [PATCH 5/7] Add `--pull-progress-timeout` / `pull_progress_timeout` option Add the option to be able to fine-tune the pull progress timeout or even disable it. Fixes #8764 and #8760 Follow-up on: https://github.com/cri-o/cri-o/pull/7887 Signed-off-by: Sascha Grunert --- completions/bash/crio | 1 + completions/fish/crio.fish | 1 + completions/zsh/_crio | 1 + docs/crio.8.md | 3 +++ docs/crio.conf.5.md | 3 +++ internal/criocli/criocli.go | 9 +++++++++ pkg/config/config.go | 16 +++++++++++----- pkg/config/template.go | 12 ++++++++++++ server/image_pull.go | 17 +++++++---------- test/image.bats | 13 +++++++++++++ 10 files changed, 61 insertions(+), 15 deletions(-) diff --git a/completions/bash/crio b/completions/bash/crio index fe37275a5fb..be6e11298b5 100755 --- a/completions/bash/crio +++ b/completions/bash/crio @@ -106,6 +106,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 index c6772c37e8d..a7097cead34 100644 --- a/completions/fish/crio.fish +++ b/completions/fish/crio.fish @@ -141,6 +141,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 index 623a8b5f67f..d094df5324f 100644 --- a/completions/zsh/_crio +++ b/completions/zsh/_crio @@ -131,6 +131,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 index 8bade388851..fa0d02b6563 100644 --- a/docs/crio.8.md +++ b/docs/crio.8.md @@ -103,6 +103,7 @@ crio [--profile-mem]=[value] [--profile-port]=[value] [--profile] +[--pull-progress-timeout]=[value] [--rdt-config-file]=[value] [--read-only] [--registry]=[value] @@ -383,6 +384,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 index 0e6fa4a7948..451131c6113 100644 --- a/docs/crio.conf.5.md +++ b/docs/crio.conf.5.md @@ -479,6 +479,9 @@ CRI-O reads its configured registries defaults from the system wide containers-r **auto_reload_registries**=false If true, CRI-O will automatically reload the mirror registry when there is an update to the 'registries.conf.d' directory. Default value is set to 'false'. +**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 index 4b317491276..6619e9bd475 100644 --- a/internal/criocli/criocli.go +++ b/internal/criocli/criocli.go @@ -404,6 +404,9 @@ func mergeConfig(config *libconfig.Config, ctx *cli.Context) error { if ctx.IsSet("auto-reload-registries") { config.AutoReloadRegistries = ctx.Bool("auto-reload-registries") } + 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") } @@ -937,6 +940,12 @@ func getCrioFlags(defConf *libconfig.Config) []cli.Flag { EnvVars: []string{"AUTO_RELOAD_REGISTRIES"}, Value: defConf.AutoReloadRegistries, }, + &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/pkg/config/config.go b/pkg/config/config.go index c1a5bf36a69..e0abdc63747 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -555,6 +555,11 @@ type ImageConfig struct { // reload the mirror registry when there is an update to the // 'registries.conf.d' directory. AutoReloadRegistries bool `toml:"auto_reload_registries"` + // 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 @@ -923,11 +928,12 @@ func DefaultConfig() (*Config, error) { EnableCriuSupport: true, }, 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 index 3fd0b4a28ac..c595eadade9 100644 --- a/pkg/config/template.go +++ b/pkg/config/template.go @@ -540,6 +540,11 @@ func initCrioTemplateConfig(c *Config) ([]*templateConfigValue, error) { group: crioImageConfig, isDefaultValue: simpleEqual(dc.AutoReloadRegistries, c.AutoReloadRegistries), }, + { + templateString: templateStringCrioImagePullProgressTimeout, + group: crioImageConfig, + isDefaultValue: simpleEqual(dc.PullProgressTimeout, c.PullProgressTimeout), + }, { templateString: templateStringCrioNetworkCniDefaultNetwork, group: crioNetworkConfig, @@ -1493,6 +1498,13 @@ const templateStringCrioImageAutoReloadRegistries = `# If true, CRI-O will autom ` +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 index 90c81cd0de9..89a7d24e5d2 100644 --- a/server/image_pull.go +++ b/server/image_pull.go @@ -244,14 +244,14 @@ func (s *Server) pullImageCandidate(ctx context.Context, sourceCtx *imageTypes.S defer close(progress) // nolint:gocritic // Cancel the pull if no progress is made - pullCtx, cancel := context.WithCancel(context.Background()) - go consumeImagePullProgress(ctx, cancel, progress, remoteCandidateName) + pullCtx, cancel := context.WithCancel(ctx) + go consumeImagePullProgress(ctx, cancel, s.Config().PullProgressTimeout, progress, remoteCandidateName) _, 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 != "", @@ -270,19 +270,16 @@ func (s *Server) pullImageCandidate(ctx context.Context, sourceCtx *imageTypes.S // 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, progress <-chan imageTypes.ProgressProperties, remoteCandidateName storage.RegistryImageReference) { - // The progress interval is 1s, but we give it a bit more time just in case - // that the connection revives. - const timeout = 10 * time.Second - timer := time.AfterFunc(timeout, func() { - log.Warnf(ctx, "Timed out on waiting up to %s for image pull progress updates", timeout) +func consumeImagePullProgress(ctx context.Context, cancel context.CancelFunc, pullProgressTimeout time.Duration, progress <-chan imageTypes.ProgressProperties, remoteCandidateName storage.RegistryImageReference) { + timer := time.AfterFunc(pullProgressTimeout, func() { + 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(timeout) + timer.Reset(pullProgressTimeout) if p.Event == imageTypes.ProgressEventSkipped { // Skipped digests metrics diff --git a/test/image.bats b/test/image.bats index 5426792f391..097c7ce224b 100644 --- a/test/image.bats +++ b/test/image.bats @@ -518,3 +518,16 @@ EOF wait_for_log 'Runtime handler \\"mem\\" container minimum memory set to 12582912 bytes' 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" +} From c95e9328b3dfc9c45e19e0c4f1e05e559d548d01 Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Wed, 12 Feb 2025 14:26:58 +0100 Subject: [PATCH 6/7] Fix context cancellation when image pull progress timeout is `0` We should never cancel the context when the pull timeout is `0`, means we now add an additional check to prevent this corner case. Deflakes the integration tests and also fixes possible issues around a disabled pull progress timeout. Signed-off-by: Sascha Grunert --- server/image_pull.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/image_pull.go b/server/image_pull.go index 89a7d24e5d2..c7b5985710b 100644 --- a/server/image_pull.go +++ b/server/image_pull.go @@ -272,8 +272,10 @@ func (s *Server) pullImageCandidate(ctx context.Context, sourceCtx *imageTypes.S // the cancel function will be called. func consumeImagePullProgress(ctx context.Context, cancel context.CancelFunc, pullProgressTimeout time.Duration, progress <-chan imageTypes.ProgressProperties, remoteCandidateName storage.RegistryImageReference) { timer := time.AfterFunc(pullProgressTimeout, func() { - log.Warnf(ctx, "Timed out on waiting up to %s for image pull progress updates", pullProgressTimeout) - cancel() + 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 From 499723d55c67d688fb795eaa410951fa9c385818 Mon Sep 17 00:00:00 2001 From: Kubernetes Release Robot Date: Sat, 1 Mar 2025 00:26:57 +0000 Subject: [PATCH 7/7] version: bump to 1.30.11 Signed-off-by: Kubernetes Release Robot --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index 29e541ef5ef..78c360c5421 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -21,7 +21,7 @@ import ( ) // Version is the version of the build. -const Version = "1.30.10" +const Version = "1.30.11" // Variables injected during build-time var (