diff --git a/pkg/kubelet/images/image_gc_manager.go b/pkg/kubelet/images/image_gc_manager.go index f1f80eb5518..4d41c5d8c2b 100644 --- a/pkg/kubelet/images/image_gc_manager.go +++ b/pkg/kubelet/images/image_gc_manager.go @@ -69,7 +69,7 @@ type StatsProvider interface { type ImageGCManager interface { // Applies the garbage collection policy. Errors include being unable to free // enough space as per the garbage collection policy. - GarbageCollect(ctx context.Context) error + GarbageCollect(ctx context.Context, beganGC time.Time) error // Start async garbage collection of images. Start() @@ -308,7 +308,7 @@ func (im *realImageGCManager) detectImages(ctx context.Context, detectTime time. return imagesInUse, nil } -func (im *realImageGCManager) GarbageCollect(ctx context.Context) error { +func (im *realImageGCManager) GarbageCollect(ctx context.Context, beganGC time.Time) error { ctx, otelSpan := im.tracer.Start(ctx, "Images/GarbageCollect") defer otelSpan.End() @@ -318,7 +318,7 @@ func (im *realImageGCManager) GarbageCollect(ctx context.Context) error { return err } - images, err = im.freeOldImages(ctx, images, freeTime) + images, err = im.freeOldImages(ctx, images, freeTime, beganGC) if err != nil { return err } @@ -369,10 +369,16 @@ func (im *realImageGCManager) GarbageCollect(ctx context.Context) error { return nil } -func (im *realImageGCManager) freeOldImages(ctx context.Context, images []evictionInfo, freeTime time.Time) ([]evictionInfo, error) { +func (im *realImageGCManager) freeOldImages(ctx context.Context, images []evictionInfo, freeTime, beganGC time.Time) ([]evictionInfo, error) { if im.policy.MaxAge == 0 { return images, nil } + + // Wait until the MaxAge has passed since the Kubelet has started, + // or else we risk prematurely garbage collecting images. + if freeTime.Sub(beganGC) <= im.policy.MaxAge { + return images, nil + } var deletionErrors []error remainingImages := make([]evictionInfo, 0) for _, image := range images { diff --git a/pkg/kubelet/images/image_gc_manager_test.go b/pkg/kubelet/images/image_gc_manager_test.go index 4dc5816ade2..cf4f0f54afc 100644 --- a/pkg/kubelet/images/image_gc_manager_test.go +++ b/pkg/kubelet/images/image_gc_manager_test.go @@ -659,7 +659,7 @@ func TestGarbageCollectBelowLowThreshold(t *testing.T) { } mockStatsProvider.EXPECT().ImageFsStats(gomock.Any()).Return(imageStats, imageStats, nil) - assert.NoError(t, manager.GarbageCollect(ctx)) + assert.NoError(t, manager.GarbageCollect(ctx, time.Now())) } func TestGarbageCollectCadvisorFailure(t *testing.T) { @@ -674,7 +674,7 @@ func TestGarbageCollectCadvisorFailure(t *testing.T) { manager, _ := newRealImageGCManager(policy, mockStatsProvider) mockStatsProvider.EXPECT().ImageFsStats(gomock.Any()).Return(&statsapi.FsStats{}, &statsapi.FsStats{}, fmt.Errorf("error")) - assert.NotNil(t, manager.GarbageCollect(ctx)) + assert.NotNil(t, manager.GarbageCollect(ctx, time.Now())) } func TestGarbageCollectBelowSuccess(t *testing.T) { @@ -699,7 +699,7 @@ func TestGarbageCollectBelowSuccess(t *testing.T) { makeImage(0, 450), } - assert.NoError(t, manager.GarbageCollect(ctx)) + assert.NoError(t, manager.GarbageCollect(ctx, time.Now())) } func TestGarbageCollectNotEnoughFreed(t *testing.T) { @@ -723,7 +723,7 @@ func TestGarbageCollectNotEnoughFreed(t *testing.T) { makeImage(0, 50), } - assert.NotNil(t, manager.GarbageCollect(ctx)) + assert.NotNil(t, manager.GarbageCollect(ctx, time.Now())) } func TestGarbageCollectImageNotOldEnough(t *testing.T) { @@ -824,14 +824,15 @@ func TestGarbageCollectImageTooOld(t *testing.T) { // First GC round should not GC remaining image, as it was used too recently. assert := assert.New(t) - images, err = manager.freeOldImages(ctx, images, fakeClock.Now()) + oldStartTime := fakeClock.Now() + images, err = manager.freeOldImages(ctx, images, oldStartTime, oldStartTime) require.NoError(t, err) assert.Len(images, 1) assert.Len(fakeRuntime.ImageList, 2) // move clock by a millisecond past maxAge duration, then 1 image will be garbage collected fakeClock.Step(policy.MaxAge + 1) - images, err = manager.freeOldImages(ctx, images, fakeClock.Now()) + images, err = manager.freeOldImages(ctx, images, fakeClock.Now(), oldStartTime) require.NoError(t, err) assert.Len(images, 0) assert.Len(fakeRuntime.ImageList, 1) @@ -879,8 +880,10 @@ func TestGarbageCollectImageMaxAgeDisabled(t *testing.T) { require.Equal(t, len(images), 1) assert.Len(fakeRuntime.ImageList, 2) + oldStartTime := fakeClock.Now() + // First GC round should not GC remaining image, as it was used too recently. - images, err = manager.freeOldImages(ctx, images, fakeClock.Now()) + images, err = manager.freeOldImages(ctx, images, oldStartTime, oldStartTime) require.NoError(t, err) assert.Len(images, 1) assert.Len(fakeRuntime.ImageList, 2) @@ -888,7 +891,7 @@ func TestGarbageCollectImageMaxAgeDisabled(t *testing.T) { // Move clock by a lot, and the images should continue to not be garbage colleced // See https://stackoverflow.com/questions/25065055/what-is-the-maximum-time-time-in-go fakeClock.SetTime(time.Unix(1<<63-62135596801, 999999999)) - images, err = manager.freeOldImages(ctx, images, fakeClock.Now()) + images, err = manager.freeOldImages(ctx, images, fakeClock.Now(), oldStartTime) require.NoError(t, err) assert.Len(images, 1) assert.Len(fakeRuntime.ImageList, 2) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index dbf8f554c07..46034ff756e 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1433,16 +1433,19 @@ func (kl *Kubelet) StartGarbageCollection() { } }, ContainerGCPeriod, wait.NeverStop) - // when the high threshold is set to 100, stub the image GC manager - if kl.kubeletConfiguration.ImageGCHighThresholdPercent == 100 { - klog.V(2).InfoS("ImageGCHighThresholdPercent is set 100, Disable image GC") + // when the high threshold is set to 100, and the max age is 0 (or the max age feature is disabled) + // stub the image GC manager + if kl.kubeletConfiguration.ImageGCHighThresholdPercent == 100 && + (!utilfeature.DefaultFeatureGate.Enabled(features.ImageMaximumGCAge) || kl.kubeletConfiguration.ImageMaximumGCAge.Duration == 0) { + klog.V(2).InfoS("ImageGCHighThresholdPercent is set 100 and ImageMaximumGCAge is 0, Disable image GC") return } prevImageGCFailed := false + beganGC := time.Now() go wait.Until(func() { ctx := context.Background() - if err := kl.imageManager.GarbageCollect(ctx); err != nil { + if err := kl.imageManager.GarbageCollect(ctx, beganGC); err != nil { if prevImageGCFailed { klog.ErrorS(err, "Image garbage collection failed multiple times in a row") // Only create an event for repeated failures diff --git a/test/e2e_node/image_gc_test.go b/test/e2e_node/image_gc_test.go new file mode 100644 index 00000000000..118c078285e --- /dev/null +++ b/test/e2e_node/image_gc_test.go @@ -0,0 +1,117 @@ +/* +Copyright 2024 The Kubernetes 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 e2enode + +import ( + "context" + "time" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + internalapi "k8s.io/cri-api/pkg/apis" + runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1" + kubefeatures "k8s.io/kubernetes/pkg/features" + kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config" + "k8s.io/kubernetes/test/e2e/framework" + e2epod "k8s.io/kubernetes/test/e2e/framework/pod" + admissionapi "k8s.io/pod-security-admission/api" + + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +const ( + // Kubelet GC's on a frequency of once every 5 minutes + // Add a little leeway to give it time. + checkGCUntil time.Duration = 6 * time.Minute + checkGCFreq time.Duration = 30 * time.Second +) + +var _ = SIGDescribe("ImageGarbageCollect", framework.WithSerial(), framework.WithNodeFeature("GarbageCollect"), func() { + f := framework.NewDefaultFramework("image-garbage-collect-test") + f.NamespacePodSecurityLevel = admissionapi.LevelPrivileged + var is internalapi.ImageManagerService + ginkgo.BeforeEach(func() { + var err error + _, is, err = getCRIClient() + framework.ExpectNoError(err) + }) + ginkgo.AfterEach(func() { + framework.ExpectNoError(PrePullAllImages()) + }) + ginkgo.Context("when ImageMaximumGCAge is set", func() { + tempSetCurrentKubeletConfig(f, func(ctx context.Context, initialConfig *kubeletconfig.KubeletConfiguration) { + initialConfig.ImageMaximumGCAge = metav1.Duration{Duration: time.Duration(time.Minute * 1)} + initialConfig.ImageMinimumGCAge = metav1.Duration{Duration: time.Duration(time.Second * 1)} + if initialConfig.FeatureGates == nil { + initialConfig.FeatureGates = make(map[string]bool) + } + initialConfig.FeatureGates[string(kubefeatures.ImageMaximumGCAge)] = true + }) + ginkgo.It("should GC unused images", func(ctx context.Context) { + pod := innocentPod() + e2epod.NewPodClient(f).CreateBatch(ctx, []*v1.Pod{pod}) + + _, err := is.PullImage(context.Background(), &runtimeapi.ImageSpec{Image: agnhostImage}, nil, nil) + framework.ExpectNoError(err) + + allImages, err := is.ListImages(context.Background(), &runtimeapi.ImageFilter{}) + framework.ExpectNoError(err) + + e2epod.NewPodClient(f).DeleteSync(ctx, pod.ObjectMeta.Name, metav1.DeleteOptions{}, e2epod.DefaultPodDeletionTimeout) + + // Even though the image gc max timing is less, we are bound by the kubelet's + // ImageGCPeriod, which is hardcoded to 5 minutes. + gomega.Eventually(ctx, func() int { + gcdImageList, err := is.ListImages(context.Background(), &runtimeapi.ImageFilter{}) + framework.ExpectNoError(err) + return len(gcdImageList) + }, checkGCUntil, checkGCFreq).Should(gomega.BeNumerically("<", len(allImages))) + }) + ginkgo.It("should not GC unused images prematurely", func(ctx context.Context) { + pod := innocentPod() + e2epod.NewPodClient(f).CreateBatch(ctx, []*v1.Pod{pod}) + + _, err := is.PullImage(context.Background(), &runtimeapi.ImageSpec{Image: agnhostImage}, nil, nil) + framework.ExpectNoError(err) + + allImages, err := is.ListImages(context.Background(), &runtimeapi.ImageFilter{}) + framework.ExpectNoError(err) + + e2epod.NewPodClient(f).DeleteSync(ctx, pod.ObjectMeta.Name, metav1.DeleteOptions{}, e2epod.DefaultPodDeletionTimeout) + + restartKubelet(true) + waitForKubeletToStart(ctx, f) + + // Wait until the maxAge of the image after the kubelet is restarted to ensure it doesn't + // GC too early. + gomega.Consistently(ctx, func() int { + gcdImageList, err := is.ListImages(context.Background(), &runtimeapi.ImageFilter{}) + framework.ExpectNoError(err) + return len(gcdImageList) + }, 50*time.Second, 10*time.Second).Should(gomega.Equal(len(allImages))) + + // Even though the image gc max timing is less, we are bound by the kubelet's + // ImageGCPeriod, which is hardcoded to 5 minutes. + gomega.Eventually(ctx, func() int { + gcdImageList, err := is.ListImages(context.Background(), &runtimeapi.ImageFilter{}) + framework.ExpectNoError(err) + return len(gcdImageList) + }, checkGCUntil, checkGCFreq).Should(gomega.BeNumerically("<", len(allImages))) + }) + }) +}) diff --git a/test/e2e_node/util.go b/test/e2e_node/util.go index 680f0f3889f..27fdfba32f0 100644 --- a/test/e2e_node/util.go +++ b/test/e2e_node/util.go @@ -74,6 +74,7 @@ import ( var startServices = flag.Bool("start-services", true, "If true, start local node services") var stopServices = flag.Bool("stop-services", true, "If true, stop local node services after running tests") var busyboxImage = imageutils.GetE2EImage(imageutils.BusyBox) +var agnhostImage = imageutils.GetE2EImage(imageutils.Agnhost) const ( // Kubelet internal cgroup name for node allocatable cgroup. @@ -233,7 +234,10 @@ func updateKubeletConfig(ctx context.Context, f *framework.Framework, kubeletCon ginkgo.By("Starting the kubelet") startKubelet() + waitForKubeletToStart(ctx, f) +} +func waitForKubeletToStart(ctx context.Context, f *framework.Framework) { // wait until the kubelet health check will succeed gomega.Eventually(ctx, func() bool { return kubeletHealthCheck(kubeletHealthCheckURL)