diff --git a/contrib/fuzz/cri_server_fuzzer.go b/contrib/fuzz/cri_server_fuzzer.go index 7c2538bee..eed97bb35 100644 --- a/contrib/fuzz/cri_server_fuzzer.go +++ b/contrib/fuzz/cri_server_fuzzer.go @@ -44,7 +44,7 @@ func FuzzCRIServer(data []byte) int { config := criconfig.Config{} - imageService, err := images.NewService(config, map[string]string{}, client) + imageService, err := images.NewService(config.ImageConfig, map[string]string{}, map[string]images.RuntimePlatform{}, client) if err != nil { panic(err) } diff --git a/integration/image_pull_timeout_test.go b/integration/image_pull_timeout_test.go index 2ecc171db..d78e12474 100644 --- a/integration/image_pull_timeout_test.go +++ b/integration/image_pull_timeout_test.go @@ -478,17 +478,17 @@ func initLocalCRIPlugin(client *containerd.Client, tmpDir string, registryCfg cr cfg := criconfig.Config{ PluginConfig: criconfig.PluginConfig{ - ContainerdConfig: criconfig.ContainerdConfig{ - Snapshotter: containerd.DefaultSnapshotter, + ImageConfig: criconfig.ImageConfig{ + Snapshotter: containerd.DefaultSnapshotter, + Registry: registryCfg, + ImagePullProgressTimeout: defaultImagePullProgressTimeout.String(), + StatsCollectPeriod: 10, }, - Registry: registryCfg, - ImagePullProgressTimeout: defaultImagePullProgressTimeout.String(), - StatsCollectPeriod: 10, }, ContainerdRootDir: containerdRootDir, RootDir: filepath.Join(criWorkDir, "root"), StateDir: filepath.Join(criWorkDir, "state"), } - return images.NewService(cfg, map[string]string{}, client) + return images.NewService(cfg.ImageConfig, map[string]string{}, map[string]images.RuntimePlatform{}, client) } diff --git a/pkg/cri/config/config.go b/pkg/cri/config/config.go index f849eca77..75a72829a 100644 --- a/pkg/cri/config/config.go +++ b/pkg/cri/config/config.go @@ -116,8 +116,6 @@ type Runtime struct { // ContainerdConfig contains toml config related to containerd type ContainerdConfig struct { - // Snapshotter is the snapshotter used by containerd. - Snapshotter string `toml:"snapshotter" json:"snapshotter"` // DefaultRuntimeName is the default runtime name to use from the runtimes table. DefaultRuntimeName string `toml:"default_runtime_name" json:"defaultRuntimeName"` @@ -125,16 +123,6 @@ type ContainerdConfig struct { // configurations, to the matching configurations. Runtimes map[string]Runtime `toml:"runtimes" json:"runtimes"` - // DisableSnapshotAnnotations disables to pass additional annotations (image - // related information) to snapshotters. These annotations are required by - // stargz snapshotter (https://github.com/containerd/stargz-snapshotter). - DisableSnapshotAnnotations bool `toml:"disable_snapshot_annotations" json:"disableSnapshotAnnotations"` - - // DiscardUnpackedLayers is a boolean flag to specify whether to allow GC to - // remove layers from the content store after successfully unpacking these - // layers to the snapshotter. - DiscardUnpackedLayers bool `toml:"discard_unpacked_layers" json:"discardUnpackedLayers"` - // IgnoreBlockIONotEnabledErrors is a boolean flag to ignore // blockio related errors when blockio support has not been // enabled. @@ -249,17 +237,57 @@ type ImageDecryption struct { KeyModel string `toml:"key_model" json:"keyModel"` } +type ImageConfig struct { + // Snapshotter is the snapshotter used by containerd. + Snapshotter string `toml:"snapshotter" json:"snapshotter"` + + // DisableSnapshotAnnotations disables to pass additional annotations (image + // related information) to snapshotters. These annotations are required by + // stargz snapshotter (https://github.com/containerd/stargz-snapshotter). + DisableSnapshotAnnotations bool `toml:"disable_snapshot_annotations" json:"disableSnapshotAnnotations"` + + // DiscardUnpackedLayers is a boolean flag to specify whether to allow GC to + // remove layers from the content store after successfully unpacking these + // layers to the snapshotter. + DiscardUnpackedLayers bool `toml:"discard_unpacked_layers" json:"discardUnpackedLayers"` + + // Registry contains config related to the registry + Registry Registry `toml:"registry" json:"registry"` + + // ImageDecryption contains config related to handling decryption of encrypted container images + ImageDecryption `toml:"image_decryption" json:"imageDecryption"` + + // MaxConcurrentDownloads restricts the number of concurrent downloads for each image. + // TODO: Migrate to transfer service + MaxConcurrentDownloads int `toml:"max_concurrent_downloads" json:"maxConcurrentDownloads"` + + // ImagePullProgressTimeout is the maximum duration that there is no + // image data read from image registry in the open connection. It will + // be reset whatever a new byte has been read. If timeout, the image + // pulling will be cancelled. A zero value means there is no timeout. + // + // The string is in the golang duration format, see: + // https://golang.org/pkg/time/#ParseDuration + ImagePullProgressTimeout string `toml:"image_pull_progress_timeout" json:"imagePullProgressTimeout"` + + // ImagePullWithSyncFs is an experimental setting. It's to force sync + // filesystem during unpacking to ensure that data integrity. + // TODO: Migrate to transfer service + ImagePullWithSyncFs bool `toml:"image_pull_with_sync_fs" json:"imagePullWithSyncFs"` + + // StatsCollectPeriod is the period (in seconds) of snapshots stats collection. + StatsCollectPeriod int `toml:"stats_collect_period" json:"statsCollectPeriod"` +} + // PluginConfig contains toml config related to CRI plugin, // it is a subset of Config. type PluginConfig struct { + // ImageConfig is the image service configuration + ImageConfig // ContainerdConfig contains config related to containerd ContainerdConfig `toml:"containerd" json:"containerd"` // CniConfig contains config related to cni CniConfig `toml:"cni" json:"cni"` - // Registry contains config related to the registry - Registry Registry `toml:"registry" json:"registry"` - // ImageDecryption contains config related to handling decryption of encrypted container images - ImageDecryption `toml:"image_decryption" json:"imageDecryption"` // DisableTCPService disables serving CRI on the TCP server. DisableTCPService bool `toml:"disable_tcp_service" json:"disableTCPService"` // StreamServerAddress is the ip address streaming server is listening on. @@ -278,8 +306,6 @@ type PluginConfig struct { SelinuxCategoryRange int `toml:"selinux_category_range" json:"selinuxCategoryRange"` // SandboxImage is the image used by sandbox container. SandboxImage string `toml:"sandbox_image" json:"sandboxImage"` - // StatsCollectPeriod is the period (in seconds) of snapshots stats collection. - StatsCollectPeriod int `toml:"stats_collect_period" json:"statsCollectPeriod"` // EnableTLSStreaming indicates to enable the TLS streaming support. EnableTLSStreaming bool `toml:"enable_tls_streaming" json:"enableTLSStreaming"` // X509KeyPairStreaming is a x509 key pair used for TLS streaming @@ -298,8 +324,6 @@ type PluginConfig struct { // current OOMScoreADj. // This is useful when the containerd does not have permission to decrease OOMScoreAdj. RestrictOOMScoreAdj bool `toml:"restrict_oom_score_adj" json:"restrictOOMScoreAdj"` - // MaxConcurrentDownloads restricts the number of concurrent downloads for each image. - MaxConcurrentDownloads int `toml:"max_concurrent_downloads" json:"maxConcurrentDownloads"` // DisableProcMount disables Kubernetes ProcMount support. This MUST be set to `true` // when using containerd with Kubernetes <=1.11. DisableProcMount bool `toml:"disable_proc_mount" json:"disableProcMount"` @@ -345,14 +369,7 @@ type PluginConfig struct { // For more details about CDI configuration please refer to // https://github.com/container-orchestrated-devices/container-device-interface#containerd-configuration CDISpecDirs []string `toml:"cdi_spec_dirs" json:"cdiSpecDirs"` - // ImagePullProgressTimeout is the maximum duration that there is no - // image data read from image registry in the open connection. It will - // be reset whatever a new byte has been read. If timeout, the image - // pulling will be cancelled. A zero value means there is no timeout. - // - // The string is in the golang duration format, see: - // https://golang.org/pkg/time/#ParseDuration - ImagePullProgressTimeout string `toml:"image_pull_progress_timeout" json:"imagePullProgressTimeout"` + // DrainExecSyncIOTimeout is the maximum duration to wait for ExecSync // API' IO EOF event after exec init process exits. A zero value means // there is no timeout. @@ -362,9 +379,6 @@ type PluginConfig struct { // // For example, the value can be '5h', '2h30m', '10s'. DrainExecSyncIOTimeout string `toml:"drain_exec_sync_io_timeout" json:"drainExecSyncIOTimeout"` - // ImagePullWithSyncFs is an experimental setting. It's to force sync - // filesystem during unpacking to ensure that data integrity. - ImagePullWithSyncFs bool `toml:"image_pull_with_sync_fs" json:"imagePullWithSyncFs"` } // X509KeyPairStreaming contains the x509 configuration for streaming diff --git a/pkg/cri/config/config_test.go b/pkg/cri/config/config_test.go index 362e49161..d4233a4b6 100644 --- a/pkg/cri/config/config_test.go +++ b/pkg/cri/config/config_test.go @@ -54,9 +54,11 @@ func TestValidateConfig(t *testing.T) { RuntimeDefault: {}, }, }, - Registry: Registry{ - Auths: map[string]AuthConfig{ - "https://gcr.io": {Username: "test"}, + ImageConfig: ImageConfig{ + Registry: Registry{ + Auths: map[string]AuthConfig{ + "https://gcr.io": {Username: "test"}, + }, }, }, }, @@ -69,16 +71,18 @@ func TestValidateConfig(t *testing.T) { }, }, }, - Registry: Registry{ - Configs: map[string]RegistryConfig{ - "gcr.io": { - Auth: &AuthConfig{ - Username: "test", + ImageConfig: ImageConfig{ + Registry: Registry{ + Configs: map[string]RegistryConfig{ + "gcr.io": { + Auth: &AuthConfig{ + Username: "test", + }, }, }, - }, - Auths: map[string]AuthConfig{ - "https://gcr.io": {Username: "test"}, + Auths: map[string]AuthConfig{ + "https://gcr.io": {Username: "test"}, + }, }, }, }, @@ -108,10 +112,12 @@ func TestValidateConfig(t *testing.T) { }, }, }, - Registry: Registry{ - ConfigPath: "/etc/containerd/conf.d", - Mirrors: map[string]Mirror{ - "something.io": {}, + ImageConfig: ImageConfig{ + Registry: Registry{ + ConfigPath: "/etc/containerd/conf.d", + Mirrors: map[string]Mirror{ + "something.io": {}, + }, }, }, }, @@ -125,9 +131,11 @@ func TestValidateConfig(t *testing.T) { RuntimeDefault: {}, }, }, - Registry: Registry{ - Mirrors: map[string]Mirror{ - "example.com": {}, + ImageConfig: ImageConfig{ + Registry: Registry{ + Mirrors: map[string]Mirror{ + "example.com": {}, + }, }, }, }, @@ -140,9 +148,11 @@ func TestValidateConfig(t *testing.T) { }, }, }, - Registry: Registry{ - Mirrors: map[string]Mirror{ - "example.com": {}, + ImageConfig: ImageConfig{ + Registry: Registry{ + Mirrors: map[string]Mirror{ + "example.com": {}, + }, }, }, }, @@ -156,11 +166,13 @@ func TestValidateConfig(t *testing.T) { RuntimeDefault: {}, }, }, - Registry: Registry{ - Configs: map[string]RegistryConfig{ - "gcr.io": { - Auth: &AuthConfig{ - Username: "test", + ImageConfig: ImageConfig{ + Registry: Registry{ + Configs: map[string]RegistryConfig{ + "gcr.io": { + Auth: &AuthConfig{ + Username: "test", + }, }, }, }, @@ -175,11 +187,13 @@ func TestValidateConfig(t *testing.T) { }, }, }, - Registry: Registry{ - Configs: map[string]RegistryConfig{ - "gcr.io": { - Auth: &AuthConfig{ - Username: "test", + ImageConfig: ImageConfig{ + Registry: Registry{ + Configs: map[string]RegistryConfig{ + "gcr.io": { + Auth: &AuthConfig{ + Username: "test", + }, }, }, }, diff --git a/pkg/cri/config/config_unix.go b/pkg/cri/config/config_unix.go index 05e90e914..7092f044d 100644 --- a/pkg/cri/config/config_unix.go +++ b/pkg/cri/config/config_unix.go @@ -62,8 +62,18 @@ func DefaultConfig() PluginConfig { NetworkPluginSetupSerially: false, NetworkPluginConfTemplate: "", }, + ImageConfig: ImageConfig{ + Snapshotter: containerd.DefaultSnapshotter, + DisableSnapshotAnnotations: true, + MaxConcurrentDownloads: 3, + ImageDecryption: ImageDecryption{ + KeyModel: KeyModelNode, + }, + ImagePullProgressTimeout: defaultImagePullProgressTimeoutDuration.String(), + ImagePullWithSyncFs: false, + StatsCollectPeriod: 10, + }, ContainerdConfig: ContainerdConfig{ - Snapshotter: containerd.DefaultSnapshotter, DefaultRuntimeName: "runc", Runtimes: map[string]Runtime{ "runc": { @@ -72,7 +82,6 @@ func DefaultConfig() PluginConfig { Sandboxer: string(ModePodSandbox), }, }, - DisableSnapshotAnnotations: true, }, DisableTCPService: true, StreamServerAddress: "127.0.0.1", @@ -86,22 +95,15 @@ func DefaultConfig() PluginConfig { TLSCertFile: "", }, SandboxImage: "registry.k8s.io/pause:3.9", - StatsCollectPeriod: 10, MaxContainerLogLineSize: 16 * 1024, - MaxConcurrentDownloads: 3, DisableProcMount: false, TolerateMissingHugetlbController: true, DisableHugetlbController: true, IgnoreImageDefinedVolumes: false, - ImageDecryption: ImageDecryption{ - KeyModel: KeyModelNode, - }, - EnableCDI: false, - CDISpecDirs: []string{"/etc/cdi", "/var/run/cdi"}, - ImagePullProgressTimeout: defaultImagePullProgressTimeoutDuration.String(), - DrainExecSyncIOTimeout: "0s", - ImagePullWithSyncFs: false, - EnableUnprivilegedPorts: true, - EnableUnprivilegedICMP: true, + EnableCDI: false, + CDISpecDirs: []string{"/etc/cdi", "/var/run/cdi"}, + DrainExecSyncIOTimeout: "0s", + EnableUnprivilegedPorts: true, + EnableUnprivilegedICMP: true, } } diff --git a/pkg/cri/config/config_windows.go b/pkg/cri/config/config_windows.go index f4f23ea7f..f009c9a14 100644 --- a/pkg/cri/config/config_windows.go +++ b/pkg/cri/config/config_windows.go @@ -34,8 +34,16 @@ func DefaultConfig() PluginConfig { NetworkPluginSetupSerially: false, NetworkPluginConfTemplate: "", }, + ImageConfig: ImageConfig{ + Snapshotter: containerd.DefaultSnapshotter, + StatsCollectPeriod: 10, + MaxConcurrentDownloads: 3, + ImageDecryption: ImageDecryption{ + KeyModel: KeyModelNode, + }, + ImagePullProgressTimeout: defaultImagePullProgressTimeoutDuration.String(), + }, ContainerdConfig: ContainerdConfig{ - Snapshotter: containerd.DefaultSnapshotter, DefaultRuntimeName: "runhcs-wcow-process", Runtimes: map[string]Runtime{ "runhcs-wcow-process": { @@ -74,16 +82,10 @@ func DefaultConfig() PluginConfig { TLSCertFile: "", }, SandboxImage: "registry.k8s.io/pause:3.9", - StatsCollectPeriod: 10, MaxContainerLogLineSize: 16 * 1024, - MaxConcurrentDownloads: 3, IgnoreImageDefinedVolumes: false, // TODO(windows): Add platform specific config, so that most common defaults can be shared. - ImageDecryption: ImageDecryption{ - KeyModel: KeyModelNode, - }, - ImagePullProgressTimeout: defaultImagePullProgressTimeoutDuration.String(), - DrainExecSyncIOTimeout: "0s", + DrainExecSyncIOTimeout: "0s", } } diff --git a/pkg/cri/cri.go b/pkg/cri/cri.go index 13e3b7ad3..2399ac1e2 100644 --- a/pkg/cri/cri.go +++ b/pkg/cri/cri.go @@ -94,8 +94,8 @@ func initCRIService(ic *plugin.InitContext) (interface{}, error) { } // TODO: Update this logic to use runtime snapshotter - if client.SnapshotService(c.ContainerdConfig.Snapshotter) == nil { - return nil, fmt.Errorf("failed to find snapshotter %q", c.ContainerdConfig.Snapshotter) + if client.SnapshotService(c.ImageConfig.Snapshotter) == nil { + return nil, fmt.Errorf("failed to find snapshotter %q", c.ImageConfig.Snapshotter) } // TODO(dmcgowan): Get the full list directly from configured plugins diff --git a/pkg/cri/server/container_create.go b/pkg/cri/server/container_create.go index ebc71e8c5..da14a275c 100644 --- a/pkg/cri/server/container_create.go +++ b/pkg/cri/server/container_create.go @@ -208,7 +208,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta log.G(ctx).Debugf("Container %q spec: %#+v", id, spew.NewFormatter(spec)) // Grab any platform specific snapshotter opts. - sOpts, err := snapshotterOpts(c.config.ContainerdConfig.Snapshotter, config) + sOpts, err := snapshotterOpts(config) if err != nil { return nil, err } diff --git a/pkg/cri/server/container_create_linux.go b/pkg/cri/server/container_create_linux.go index 86f3c2a92..99b86c2ab 100644 --- a/pkg/cri/server/container_create_linux.go +++ b/pkg/cri/server/container_create_linux.go @@ -264,7 +264,7 @@ func appArmorProfileExists(profile string) (bool, error) { } // snapshotterOpts returns any Linux specific snapshotter options for the rootfs snapshot -func snapshotterOpts(snapshotterName string, config *runtime.ContainerConfig) ([]snapshots.Opt, error) { +func snapshotterOpts(config *runtime.ContainerConfig) ([]snapshots.Opt, error) { nsOpts := config.GetLinux().GetSecurityContext().GetNamespaceOptions() return snapshotterRemapOpts(nsOpts) } diff --git a/pkg/cri/server/container_create_other.go b/pkg/cri/server/container_create_other.go index 775ad3823..6154a9bed 100644 --- a/pkg/cri/server/container_create_other.go +++ b/pkg/cri/server/container_create_other.go @@ -31,6 +31,6 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon } // snapshotterOpts returns snapshotter options for the rootfs snapshot -func snapshotterOpts(snapshotterName string, config *runtime.ContainerConfig) ([]snapshots.Opt, error) { +func snapshotterOpts(config *runtime.ContainerConfig) ([]snapshots.Opt, error) { return []snapshots.Opt{}, nil } diff --git a/pkg/cri/server/container_create_windows.go b/pkg/cri/server/container_create_windows.go index d4f866c24..09d5ede63 100644 --- a/pkg/cri/server/container_create_windows.go +++ b/pkg/cri/server/container_create_windows.go @@ -32,18 +32,16 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon } // snapshotterOpts returns any Windows specific snapshotter options for the r/w layer -func snapshotterOpts(snapshotterName string, config *runtime.ContainerConfig) ([]snapshots.Opt, error) { +func snapshotterOpts(config *runtime.ContainerConfig) ([]snapshots.Opt, error) { var opts []snapshots.Opt - switch snapshotterName { - case "windows", "cimfs": - rootfsSize := config.GetWindows().GetResources().GetRootfsSizeInBytes() - if rootfsSize != 0 { - labels := map[string]string{ - "containerd.io/snapshot/windows/rootfs.sizebytes": strconv.FormatInt(rootfsSize, 10), - } - opts = append(opts, snapshots.WithLabels(labels)) + // TODO: Only set for windows and cimfs snapshotter + rootfsSize := config.GetWindows().GetResources().GetRootfsSizeInBytes() + if rootfsSize != 0 { + labels := map[string]string{ + "containerd.io/snapshot/windows/rootfs.sizebytes": strconv.FormatInt(rootfsSize, 10), } + opts = append(opts, snapshots.WithLabels(labels)) } return opts, nil diff --git a/pkg/cri/server/container_status_test.go b/pkg/cri/server/container_status_test.go index 769922324..d20d1f12a 100644 --- a/pkg/cri/server/container_status_test.go +++ b/pkg/cri/server/container_status_test.go @@ -275,6 +275,8 @@ func (s *fakeImageService) RuntimeSnapshotter(ctx context.Context, ociRuntime cr func (s *fakeImageService) UpdateImage(ctx context.Context, r string) error { return nil } +func (s *fakeImageService) CheckImages(ctx context.Context) error { return nil } + func (s *fakeImageService) GetImage(id string) (imagestore.Image, error) { return s.imageStore.Get(id) } func (s *fakeImageService) GetSnapshot(key, snapshotter string) (snapshotstore.Snapshot, error) { diff --git a/pkg/cri/server/events.go b/pkg/cri/server/events.go index 0542a3e88..65d183ff9 100644 --- a/pkg/cri/server/events.go +++ b/pkg/cri/server/events.go @@ -362,6 +362,7 @@ func (em *eventMonitor) handleEvent(any interface{}) error { if err != nil { return fmt.Errorf("failed to update container status for TaskOOM event: %w", err) } + // TODO: ImageService should handle these events directly case *eventtypes.ImageCreate: log.L.Infof("ImageCreate event %+v", e) return em.c.UpdateImage(ctx, e.Name) diff --git a/pkg/cri/server/images/check.go b/pkg/cri/server/images/check.go new file mode 100644 index 000000000..8616e0d52 --- /dev/null +++ b/pkg/cri/server/images/check.go @@ -0,0 +1,77 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package images + +import ( + "context" + "fmt" + "sync" + + "github.com/containerd/containerd/v2/images" + "github.com/containerd/containerd/v2/platforms" + "github.com/containerd/log" +) + +// LoadImages checks all existing images to ensure they are ready to +// be used for CRI. It may try to recover images which are not ready +// but will only log errors, not return any. +func (c *CRIImageService) CheckImages(ctx context.Context) error { + // TODO: Move way from `client.ListImages` to directly using image store + cImages, err := c.client.ListImages(ctx) + if err != nil { + return fmt.Errorf("unable to list images: %w", err) + } + + // TODO: Support all snapshotter + snapshotter := c.config.Snapshotter + var wg sync.WaitGroup + for _, i := range cImages { + wg.Add(1) + i := i + go func() { + defer wg.Done() + // TODO: Check platform/snapshot combination. Snapshot check should come first + ok, _, _, _, err := images.Check(ctx, i.ContentStore(), i.Target(), platforms.Default()) + if err != nil { + log.G(ctx).WithError(err).Errorf("Failed to check image content readiness for %q", i.Name()) + return + } + if !ok { + log.G(ctx).Warnf("The image content readiness for %q is not ok", i.Name()) + return + } + // Checking existence of top-level snapshot for each image being recovered. + // TODO: This logic should be done elsewhere and owned by the image service + unpacked, err := i.IsUnpacked(ctx, snapshotter) + if err != nil { + log.G(ctx).WithError(err).Warnf("Failed to check whether image is unpacked for image %s", i.Name()) + return + } + if !unpacked { + log.G(ctx).Warnf("The image %s is not unpacked.", i.Name()) + // TODO(random-liu): Consider whether we should try unpack here. + } + if err := c.UpdateImage(ctx, i.Name()); err != nil { + log.G(ctx).WithError(err).Warnf("Failed to update reference for image %q", i.Name()) + return + } + log.G(ctx).Debugf("Loaded image %q", i.Name()) + }() + } + wg.Wait() + return nil +} diff --git a/pkg/cri/server/images/image_pull.go b/pkg/cri/server/images/image_pull.go index de3b41dca..abe7d5aeb 100644 --- a/pkg/cri/server/images/image_pull.go +++ b/pkg/cri/server/images/image_pull.go @@ -194,12 +194,12 @@ func (c *CRIImageService) PullImage(ctx context.Context, name string, credential // Temporarily removed for v2 upgrade //pullOpts = append(pullOpts, c.encryptedImagesPullOpts()...) - if !c.config.ContainerdConfig.DisableSnapshotAnnotations { + if !c.config.DisableSnapshotAnnotations { pullOpts = append(pullOpts, containerd.WithImageHandlerWrapper(snpkg.AppendInfoHandlerWrapper(ref))) } - if c.config.ContainerdConfig.DiscardUnpackedLayers { + if c.config.DiscardUnpackedLayers { // Allows GC to clean layers up from the content store after unpacking pullOpts = append(pullOpts, containerd.WithChildLabelMap(containerdimages.ChildGCLabelsFilterLayers)) @@ -333,7 +333,8 @@ func (c *CRIImageService) createImageReference(ctx context.Context, name string, // getLabels get image labels to be added on CRI image func (c *CRIImageService) getLabels(ctx context.Context, name string) map[string]string { labels := map[string]string{crilabels.ImageLabelKey: crilabels.ImageLabelValue} - configSandboxImage := c.config.SandboxImage + // TODO: Separate config here to generalize pinned image list + configSandboxImage := "" //c.config.SandboxImage // parse sandbox image sandboxNamedRef, err := distribution.ParseDockerRef(configSandboxImage) if err != nil { @@ -756,7 +757,7 @@ func (rt *pullRequestReporterRoundTripper) RoundTrip(req *http.Request) (*http.R // See https://github.com/containerd/containerd/issues/6657 func (c *CRIImageService) snapshotterFromPodSandboxConfig(ctx context.Context, imageRef string, s *runtime.PodSandboxConfig) (string, error) { - snapshotter := c.config.ContainerdConfig.Snapshotter + snapshotter := c.config.Snapshotter if s == nil || s.Annotations == nil { return snapshotter, nil } @@ -766,13 +767,10 @@ func (c *CRIImageService) snapshotterFromPodSandboxConfig(ctx context.Context, i return snapshotter, nil } - // TODO: Find other way to retrieve sandbox runtime, this must belong to the Runtime part of the CRI. - ociRuntime, err := c.config.GetSandboxRuntime(s, runtimeHandler) - if err != nil { - return "", fmt.Errorf("experimental: failed to get sandbox runtime for %s: %w", runtimeHandler, err) + if p, ok := c.runtimePlatforms[runtimeHandler]; ok && p.Snapshotter != snapshotter { + snapshotter = p.Snapshotter + log.G(ctx).Infof("experimental: PullImage %q for runtime %s, using snapshotter %s", imageRef, runtimeHandler, snapshotter) } - snapshotter = c.RuntimeSnapshotter(ctx, ociRuntime) - log.G(ctx).Infof("experimental: PullImage %q for runtime %s, using snapshotter %s", imageRef, runtimeHandler, snapshotter) return snapshotter, nil } diff --git a/pkg/cri/server/images/image_pull_test.go b/pkg/cri/server/images/image_pull_test.go index c07f3672f..981103a5b 100644 --- a/pkg/cri/server/images/image_pull_test.go +++ b/pkg/cri/server/images/image_pull_test.go @@ -425,11 +425,13 @@ func TestSnapshotterFromPodSandboxConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { cri, _ := newTestCRIService() - cri.config.ContainerdConfig.Snapshotter = defaultSnashotter - cri.config.ContainerdConfig.Runtimes = make(map[string]criconfig.Runtime) - cri.config.ContainerdConfig.Runtimes["exiting-runtime"] = criconfig.Runtime{ - Snapshotter: runtimeSnapshotter, - } + cri.config.Snapshotter = defaultSnashotter + /* + cri.config.ContainerdConfig.Runtimes = make(map[string]criconfig.Runtime) + cri.config.ContainerdConfig.Runtimes["exiting-runtime"] = criconfig.Runtime{ + Snapshotter: runtimeSnapshotter, + } + */ snapshotter, err := cri.snapshotterFromPodSandboxConfig(context.Background(), "test-image", tt.podSandboxConfig) assert.Equal(t, tt.expectSnapshotter, snapshotter) if tt.expectErr { @@ -531,7 +533,8 @@ func TestImageGetLabels(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - criService.config.SandboxImage = tt.configSandboxImage + // Change this config name + //criService.config.SandboxImage = tt.configSandboxImage labels := criService.getLabels(context.Background(), tt.pullImageName) assert.Equal(t, tt.expectedLabel, labels) diff --git a/pkg/cri/server/images/service.go b/pkg/cri/server/images/service.go index bafb91f68..3a7976ca5 100644 --- a/pkg/cri/server/images/service.go +++ b/pkg/cri/server/images/service.go @@ -28,7 +28,6 @@ import ( "github.com/containerd/containerd/v2/pkg/cri/server/base" imagestore "github.com/containerd/containerd/v2/pkg/cri/store/image" snapshotstore "github.com/containerd/containerd/v2/pkg/cri/store/snapshot" - ctrdutil "github.com/containerd/containerd/v2/pkg/cri/util" "github.com/containerd/containerd/v2/pkg/kmutex" "github.com/containerd/containerd/v2/platforms" "github.com/containerd/containerd/v2/plugins" @@ -71,22 +70,28 @@ func init() { return nil, fmt.Errorf("unable to init client for cri image service: %w", err) } + snapshotterOverrides := map[string]RuntimePlatform{} imageFSPaths := map[string]string{} // TODO: Figure out a way to break this plugin's dependency on a shared runtime config - for _, ociRuntime := range c.ContainerdConfig.Runtimes { + for runtimeName, ociRuntime := range c.ContainerdConfig.Runtimes { // Can not use `c.RuntimeSnapshotter() yet, so hard-coding here.` snapshotter := ociRuntime.Snapshotter if snapshotter != "" { + snapshotterOverrides[runtimeName] = RuntimePlatform{ + Snapshotter: snapshotter, + // TODO: This must be provided by runtime + Platform: platforms.DefaultSpec(), + } imageFSPaths[snapshotter] = filepath.Join(c.ContainerdRootDir, plugins.SnapshotPlugin.String()+"."+snapshotter) log.L.Infof("Get image filesystem path %q for snapshotter %q", imageFSPaths[snapshotter], snapshotter) } } - snapshotter := c.ContainerdConfig.Snapshotter + snapshotter := c.ImageConfig.Snapshotter imageFSPaths[snapshotter] = filepath.Join(c.ContainerdRootDir, plugins.SnapshotPlugin.String()+"."+snapshotter) log.L.Infof("Get image filesystem path %q for snapshotter %q", imageFSPaths[snapshotter], snapshotter) // TODO: Pull out image specific configs here! - service, err := NewService(c, imageFSPaths, client) + service, err := NewService(c.ImageConfig, imageFSPaths, snapshotterOverrides, client) if err != nil { return nil, fmt.Errorf("failed to create image service: %w", err) } @@ -96,6 +101,11 @@ func init() { }) } +type RuntimePlatform struct { + Snapshotter string + Platform platforms.Platform +} + type CRIImageService struct { // config contains all configurations. // TODO: Migrate configs from cri type once moved to its own plugin @@ -110,12 +120,14 @@ type CRIImageService struct { // - image decryption (moved to transfer service) // - default runtime // - stats collection interval (only used to startup snapshot sync) - config criconfig.Config + config criconfig.ImageConfig // client is an instance of the containerd client // TODO: Remove this in favor of using plugins directly client *containerd.Client // imageFSPaths contains path to image filesystem for snapshotters. imageFSPaths map[string]string + // runtimePlatforms are the platforms configured for a runtime. + runtimePlatforms map[string]RuntimePlatform // imageStore stores all resources associated with images. imageStore *imagestore.Store // snapshotStore stores information of all snapshots. @@ -140,31 +152,29 @@ type GRPCCRIImageService struct { // - Image Service (from metadata) // - Content store (from metadata) // 3. Separate image cache and snapshot cache to first class plugins, make the snapshot cache much more efficient and intelligent -func NewService(config criconfig.Config, imageFSPaths map[string]string, client *containerd.Client) (*CRIImageService, error) { +func NewService(config criconfig.ImageConfig, imageFSPaths map[string]string, runtimePlatforms map[string]RuntimePlatform, client *containerd.Client) (*CRIImageService, error) { svc := CRIImageService{ config: config, client: client, imageStore: imagestore.NewStore(client.ImageService(), client.ContentStore(), platforms.Default()), imageFSPaths: imageFSPaths, + runtimePlatforms: runtimePlatforms, snapshotStore: snapshotstore.NewStore(), unpackDuplicationSuppressor: kmutex.New(), } snapshotters := map[string]snapshot.Snapshotter{} - ctx := ctrdutil.NamespacedContext() - // Add runtime specific snapshotters - for _, runtime := range config.ContainerdConfig.Runtimes { - snapshotterName := svc.RuntimeSnapshotter(ctx, runtime) - if snapshotter := svc.client.SnapshotService(snapshotterName); snapshotter != nil { - snapshotters[snapshotterName] = snapshotter + for _, rp := range runtimePlatforms { + if snapshotter := svc.client.SnapshotService(rp.Snapshotter); snapshotter != nil { + snapshotters[rp.Snapshotter] = snapshotter } else { - return nil, fmt.Errorf("failed to find snapshotter %q", snapshotterName) + return nil, fmt.Errorf("failed to find snapshotter %q", rp.Snapshotter) } } // Add default snapshotter - snapshotterName := svc.config.ContainerdConfig.Snapshotter + snapshotterName := svc.config.Snapshotter if snapshotter := svc.client.SnapshotService(snapshotterName); snapshotter != nil { snapshotters[snapshotterName] = snapshotter } else { @@ -220,9 +230,10 @@ func (c *CRIImageService) LocalResolve(refOrID string) (imagestore.Image, error) // RuntimeSnapshotter overrides the default snapshotter if Snapshotter is set for this runtime. // See https://github.com/containerd/containerd/issues/6657 +// TODO: Pass in name and get back runtime platform func (c *CRIImageService) RuntimeSnapshotter(ctx context.Context, ociRuntime criconfig.Runtime) string { if ociRuntime.Snapshotter == "" { - return c.config.ContainerdConfig.Snapshotter + return c.config.Snapshotter } log.G(ctx).Debugf("Set snapshotter for runtime %s to %s", ociRuntime.Type, ociRuntime.Snapshotter) diff --git a/pkg/cri/server/images/service_test.go b/pkg/cri/server/images/service_test.go index c5b48c946..130cff3aa 100644 --- a/pkg/cri/server/images/service_test.go +++ b/pkg/cri/server/images/service_test.go @@ -41,7 +41,7 @@ const ( // newTestCRIService creates a fake criService for test. func newTestCRIService() (*CRIImageService, *GRPCCRIImageService) { service := &CRIImageService{ - config: testConfig, + config: testConfig.ImageConfig, imageFSPaths: map[string]string{"overlayfs": testImageFSPath}, imageStore: imagestore.NewStore(nil, nil, platforms.Default()), snapshotStore: snapshotstore.NewStore(), @@ -125,9 +125,7 @@ func TestRuntimeSnapshotter(t *testing.T) { test := test t.Run(test.desc, func(t *testing.T) { cri, _ := newTestCRIService() - cri.config = criconfig.Config{ - PluginConfig: criconfig.DefaultConfig(), - } + cri.config = criconfig.DefaultConfig().ImageConfig assert.Equal(t, test.expectSnapshotter, cri.RuntimeSnapshotter(context.Background(), test.runtime)) }) } diff --git a/pkg/cri/server/podsandbox/controller.go b/pkg/cri/server/podsandbox/controller.go index b0f9899af..cb36b4d8a 100644 --- a/pkg/cri/server/podsandbox/controller.go +++ b/pkg/cri/server/podsandbox/controller.go @@ -106,6 +106,7 @@ type ImageService interface { LocalResolve(refOrID string) (imagestore.Image, error) GetImage(id string) (imagestore.Image, error) PullImage(ctx context.Context, name string, creds func(string) (string, string, error), sc *runtime.PodSandboxConfig) (string, error) + RuntimeSnapshotter(ctx context.Context, ociRuntime criconfig.Runtime) string } type Controller struct { diff --git a/pkg/cri/server/podsandbox/helpers.go b/pkg/cri/server/podsandbox/helpers.go index ae47ce29b..015485556 100644 --- a/pkg/cri/server/podsandbox/helpers.go +++ b/pkg/cri/server/podsandbox/helpers.go @@ -33,7 +33,6 @@ import ( "github.com/containerd/containerd/v2/containers" clabels "github.com/containerd/containerd/v2/labels" "github.com/containerd/containerd/v2/oci" - criconfig "github.com/containerd/containerd/v2/pkg/cri/config" crilabels "github.com/containerd/containerd/v2/pkg/cri/labels" imagestore "github.com/containerd/containerd/v2/pkg/cri/store/image" sandboxstore "github.com/containerd/containerd/v2/pkg/cri/store/sandbox" @@ -188,17 +187,6 @@ func (c *Controller) runtimeSpec(id string, baseSpecFile string, opts ...oci.Spe return spec, nil } -// Overrides the default snapshotter if Snapshotter is set for this runtime. -// See https://github.com/containerd/containerd/issues/6657 -func (c *Controller) runtimeSnapshotter(ctx context.Context, ociRuntime criconfig.Runtime) string { - if ociRuntime.Snapshotter == "" { - return c.config.ContainerdConfig.Snapshotter - } - - log.G(ctx).Debugf("Set snapshotter for runtime %s to %s", ociRuntime.Type, ociRuntime.Snapshotter) - return ociRuntime.Snapshotter -} - func getMetadata(ctx context.Context, container containerd.Container) (*sandboxstore.Metadata, error) { // Load sandbox metadata. exts, err := container.Extensions(ctx) diff --git a/pkg/cri/server/podsandbox/sandbox_run.go b/pkg/cri/server/podsandbox/sandbox_run.go index 10570b35b..ec9effb0c 100644 --- a/pkg/cri/server/podsandbox/sandbox_run.go +++ b/pkg/cri/server/podsandbox/sandbox_run.go @@ -138,7 +138,7 @@ func (c *Controller) Start(ctx context.Context, id string) (cin sandbox.Controll snapshotterOpt = append(snapshotterOpt, extraSOpts...) opts := []containerd.NewContainerOpts{ - containerd.WithSnapshotter(c.runtimeSnapshotter(ctx, ociRuntime)), + containerd.WithSnapshotter(c.imageService.RuntimeSnapshotter(ctx, ociRuntime)), customopts.WithNewSnapshot(id, containerdImage, snapshotterOpt...), containerd.WithSpec(spec, specOpts...), containerd.WithContainerLabels(sandboxLabels), diff --git a/pkg/cri/server/restart.go b/pkg/cri/server/restart.go index 052371dd0..5d3784741 100644 --- a/pkg/cri/server/restart.go +++ b/pkg/cri/server/restart.go @@ -21,18 +21,15 @@ import ( "fmt" "os" "path/filepath" - "sync" "time" containerdio "github.com/containerd/containerd/v2/cio" containerd "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/errdefs" - containerdimages "github.com/containerd/containerd/v2/images" criconfig "github.com/containerd/containerd/v2/pkg/cri/config" crilabels "github.com/containerd/containerd/v2/pkg/cri/labels" "github.com/containerd/containerd/v2/pkg/cri/server/podsandbox" "github.com/containerd/containerd/v2/pkg/netns" - "github.com/containerd/containerd/v2/platforms" "github.com/containerd/log" "github.com/containerd/typeurl/v2" "golang.org/x/sync/errgroup" @@ -201,11 +198,9 @@ func (c *criService) recover(ctx context.Context) error { } // Recover all images. - cImages, err := c.client.ListImages(ctx) - if err != nil { - return fmt.Errorf("failed to list images: %w", err) + if err := c.ImageService.CheckImages(ctx); err != nil { + return fmt.Errorf("failed to check images: %w", err) } - c.loadImages(ctx, cImages) // It's possible that containerd containers are deleted unexpectedly. In that case, // we can't even get metadata, we should cleanup orphaned sandbox/container directories @@ -444,44 +439,6 @@ func getNetNS(meta *sandboxstore.Metadata) *netns.NetNS { return netns.LoadNetNS(meta.NetNSPath) } -// loadImages loads images from containerd. -func (c *criService) loadImages(ctx context.Context, cImages []containerd.Image) { - snapshotter := c.config.ContainerdConfig.Snapshotter - var wg sync.WaitGroup - for _, i := range cImages { - wg.Add(1) - i := i - go func() { - defer wg.Done() - ok, _, _, _, err := containerdimages.Check(ctx, i.ContentStore(), i.Target(), platforms.Default()) - if err != nil { - log.G(ctx).WithError(err).Errorf("Failed to check image content readiness for %q", i.Name()) - return - } - if !ok { - log.G(ctx).Warnf("The image content readiness for %q is not ok", i.Name()) - return - } - // Checking existence of top-level snapshot for each image being recovered. - unpacked, err := i.IsUnpacked(ctx, snapshotter) - if err != nil { - log.G(ctx).WithError(err).Warnf("Failed to check whether image is unpacked for image %s", i.Name()) - return - } - if !unpacked { - log.G(ctx).Warnf("The image %s is not unpacked.", i.Name()) - // TODO(random-liu): Consider whether we should try unpack here. - } - if err := c.UpdateImage(ctx, i.Name()); err != nil { - log.G(ctx).WithError(err).Warnf("Failed to update reference for image %q", i.Name()) - return - } - log.G(ctx).Debugf("Loaded image %q", i.Name()) - }() - } - wg.Wait() -} - func cleanupOrphanedIDDirs(ctx context.Context, cntrs []containerd.Container, base string) error { // Cleanup orphaned id directories. dirs, err := os.ReadDir(base) diff --git a/pkg/cri/server/service.go b/pkg/cri/server/service.go index 68cfaeebe..d32654b36 100644 --- a/pkg/cri/server/service.go +++ b/pkg/cri/server/service.go @@ -70,14 +70,14 @@ type ImageService interface { PullImage(ctx context.Context, name string, credentials func(string) (string, string, error), sandboxConfig *runtime.PodSandboxConfig) (string, error) UpdateImage(ctx context.Context, r string) error + CheckImages(ctx context.Context) error + GetImage(id string) (imagestore.Image, error) GetSnapshot(key, snapshotter string) (snapshotstore.Snapshot, error) LocalResolve(refOrID string) (imagestore.Image, error) ImageFSPaths() map[string]string - - //ImageFsInfo(context.Context, *ImageFsInfoRequest) (*ImageFsInfoResponse, error) } // criService implements CRIService. diff --git a/pkg/cri/server/test_config.go b/pkg/cri/server/test_config.go index 74d684905..b86e72e4d 100644 --- a/pkg/cri/server/test_config.go +++ b/pkg/cri/server/test_config.go @@ -33,8 +33,10 @@ var testConfig = criconfig.Config{ PluginConfig: criconfig.PluginConfig{ SandboxImage: testSandboxImage, TolerateMissingHugetlbController: true, + ImageConfig: criconfig.ImageConfig{ + Snapshotter: "overlayfs", + }, ContainerdConfig: criconfig.ContainerdConfig{ - Snapshotter: "overlayfs", DefaultRuntimeName: "runc", Runtimes: map[string]criconfig.Runtime{ "runc": {