Merge pull request #132578 from drigz/actionable-gc-error

Report actionable error when GC fails due to disk pressure

pkg/kubelet/images/image_gc_manager.go

@@ -391,15 +391,22 @@ func (im *realImageGCManager) GarbageCollect(ctx context.Context, beganGC time.T
 		klog.InfoS("Disk usage on image filesystem is over the high threshold, trying to free bytes down to the low threshold", "usage", usagePercent, "highThreshold", im.policy.HighThresholdPercent, "amountToFree", amountToFree, "lowThreshold", im.policy.LowThresholdPercent)
 		remainingImages, freed, err := im.freeSpace(ctx, amountToFree, freeTime, images)
 		if err != nil {
+			// Failed to delete images, eg due to a read-only filesystem.
 			return err
 		}
 
 		im.runPostGCHooks(remainingImages, freeTime)
 
 		if freed < amountToFree {
-			err := fmt.Errorf("Failed to garbage collect required amount of images. Attempted to free %d bytes, but only found %d bytes eligible to free.", amountToFree, freed)
-			im.recorder.Eventf(im.nodeRef, v1.EventTypeWarning, events.FreeDiskSpaceFailed, err.Error())
-			return err
+			// This usually means the disk is full for reasons other than container
+			// images, such as logs, volumes, or other files. However, it could also
+			// be due to an unusually large number or size of in-use container images.
+			message := fmt.Sprintf("Insufficient free disk space on the node's image filesystem (%d%% of %s used). "+
+				"Failed to free sufficient space by deleting unused images (freed %d bytes). "+
+				"Investigate disk usage, as it could be used by active images, logs, volumes, or other data.",
+				usagePercent, formatSize(capacity), freed)
+			im.recorder.Eventf(im.nodeRef, v1.EventTypeWarning, events.FreeDiskSpaceFailed, "%s", message)
+			return fmt.Errorf("%s", message)
 		}
 	}
 
@@ -576,6 +583,31 @@ func (im *realImageGCManager) imagesInEvictionOrder(ctx context.Context, freeTim
 	return images, nil
 }
 
+// formatSize returns a human-readable string for a given size in bytes.
+func formatSize(sizeBytes int64) string {
+	const (
+		KiB = 1024
+		MiB = 1024 * KiB
+		GiB = 1024 * MiB
+		TiB = 1024 * GiB
+	)
+
+	size := float64(sizeBytes)
+
+	switch {
+	case size < KiB:
+		return fmt.Sprintf("%d B", int64(size))
+	case size < MiB:
+		return fmt.Sprintf("%.1f KiB", size/KiB)
+	case size < GiB:
+		return fmt.Sprintf("%.1f MiB", size/MiB)
+	case size < TiB:
+		return fmt.Sprintf("%.1f GiB", size/GiB)
+	default:
+		return fmt.Sprintf("%.1f TiB", size/TiB)
+	}
+}
+
 // If RuntimeClassInImageCriAPI feature gate is enabled, imageRecords
 // are identified by a tuple of (imageId,runtimeHandler) that is passed
 // from ListImages() call. If no runtimehandler is specified in response

pkg/kubelet/images/image_gc_manager_test.go

@@ -645,7 +645,7 @@ func TestGarbageCollectCadvisorFailure(t *testing.T) {
 	manager, _ := newRealImageGCManager(policy, mockStatsProvider)
 
 	mockStatsProvider.EXPECT().ImageFsStats(mock.Anything).Return(&statsapi.FsStats{}, &statsapi.FsStats{}, fmt.Errorf("error"))
-	assert.Error(t, manager.GarbageCollect(ctx, time.Now()))
+	require.Error(t, manager.GarbageCollect(ctx, time.Now()))
 }
 
 func TestGarbageCollectBelowSuccess(t *testing.T) {
@@ -678,19 +678,45 @@ func TestGarbageCollectNotEnoughFreed(t *testing.T) {
 		LowThresholdPercent:  80,
 	}
 	mockStatsProvider := statstest.NewMockProvider(t)
-	manager, fakeRuntime := newRealImageGCManager(policy, mockStatsProvider)
+	fakeRuntime := &containertest.FakeRuntime{}
+	recorder := record.NewFakeRecorder(1)
+	manager := &realImageGCManager{
+		runtime:       fakeRuntime,
+		policy:        policy,
+		imageRecords:  make(map[string]*imageRecord),
+		statsProvider: mockStatsProvider,
+		recorder:      recorder,
+		tracer:        noopoteltrace.NewTracerProvider().Tracer(""),
+	}
 
-	// Expect 95% usage and little of it gets freed.
+	// Initial state: 95% usage, of which 5% is garbage images.
+	capacity := uint64(10 * 1024 * 1024 * 1024)
+	available := capacity / 20 // 5% available, 95% usage
 	imageFs := &statsapi.FsStats{
-		AvailableBytes: ptr.To(uint64(50)),
-		CapacityBytes:  ptr.To(uint64(1000)),
+		AvailableBytes: ptr.To(available),
+		CapacityBytes:  ptr.To(capacity),
 	}
 	mockStatsProvider.EXPECT().ImageFsStats(mock.Anything).Return(imageFs, imageFs, nil)
+	// This image is unused and eligible for deletion.
+	// Its size is less than the required amount to free.
+	imageSize := int64(500 * 1024 * 1024) // 500 MiB
 	fakeRuntime.ImageList = []container.Image{
-		makeImage(0, 50),
+		makeImage(0, imageSize),
 	}
 
-	assert.Error(t, manager.GarbageCollect(ctx, time.Now()))
+	err := manager.GarbageCollect(ctx, time.Now())
+	require.Error(t, err)
+
+	// Check that a warning event was sent
+	expectedEvent := "Warning FreeDiskSpaceFailed Insufficient free disk space on the node's image filesystem" +
+		" (95% of 10.0 GiB used). Failed to free sufficient space by deleting unused images (freed 524288000 bytes)." +
+		" Investigate disk usage, as it could be used by active images, logs, volumes, or other data."
+	select {
+	case event := <-recorder.Events:
+		assert.Equal(t, expectedEvent, event)
+	default:
+		t.Errorf("expected a 'FreeDiskSpaceFailed' event")
+	}
 }
 
 func TestGarbageCollectImageNotOldEnough(t *testing.T) {

pkg/kubelet/kubelet.go

@@ -1610,7 +1610,7 @@ func (kl *Kubelet) StartGarbageCollection() {
 			if prevImageGCFailed {
 				klog.ErrorS(err, "Image garbage collection failed multiple times in a row")
 				// Only create an event for repeated failures
-				kl.recorder.Eventf(kl.nodeRef, v1.EventTypeWarning, events.ImageGCFailed, err.Error())
+				kl.recorder.Event(kl.nodeRef, v1.EventTypeWarning, events.ImageGCFailed, err.Error())
 			} else {
 				klog.ErrorS(err, "Image garbage collection failed once. Stats initialization may not have completed yet")
 			}