Merge pull request kubernetes#128344 from kannon92/revert-128333-eviction-manager

Allow for container fs and image fs to be on the same drive but in a different partition

Author: k8s-ci-robot

pkg/kubelet/container/container_gc.go

@@ -45,8 +45,6 @@ type GC interface {
 	GarbageCollect(ctx context.Context) error
 	// Deletes all unused containers, including containers belonging to pods that are terminated but not deleted
 	DeleteAllUnusedContainers(ctx context.Context) error
-	// IsContainerFsSeparateFromImageFs tells if writeable layer and read-only layer are separate.
-	IsContainerFsSeparateFromImageFs(ctx context.Context) bool
 }
 
 // SourcesReadyProvider knows how to determine if configuration sources are ready
@@ -88,22 +86,3 @@ func (cgc *realContainerGC) DeleteAllUnusedContainers(ctx context.Context) error
 	klog.InfoS("Attempting to delete unused containers")
 	return cgc.runtime.GarbageCollect(ctx, cgc.policy, cgc.sourcesReadyProvider.AllReady(), true)
 }
-
-func (cgc *realContainerGC) IsContainerFsSeparateFromImageFs(ctx context.Context) bool {
-	resp, err := cgc.runtime.ImageFsInfo(ctx)
-	if err != nil {
-		return false
-	}
-	// These fields can be empty if CRI implementation didn't populate.
-	if resp.ContainerFilesystems == nil || resp.ImageFilesystems == nil || len(resp.ContainerFilesystems) == 0 || len(resp.ImageFilesystems) == 0 {
-		return false
-	}
-	// KEP 4191 explains that multiple filesystems for images and containers is not
-	// supported at the moment.
-	// See https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4191-split-image-filesystem#comment-on-future-extensions
-	// for work needed to support multiple filesystems.
-	if resp.ContainerFilesystems[0].FsId != nil && resp.ImageFilesystems[0].FsId != nil {
-		return resp.ContainerFilesystems[0].FsId.Mountpoint != resp.ImageFilesystems[0].FsId.Mountpoint
-	}
-	return false
-}

pkg/kubelet/container/container_gc_test.go

@@ -252,13 +252,20 @@ func (m *managerImpl) synchronize(diskInfoProvider DiskInfoProvider, podFunc Act
 	// build the ranking functions (if not yet known)
 	// TODO: have a function in cadvisor that lets us know if global housekeeping has completed
 	if m.dedicatedImageFs == nil {
-		hasImageFs, splitDiskError := diskInfoProvider.HasDedicatedImageFs(ctx)
-		if splitDiskError != nil {
-			klog.ErrorS(splitDiskError, "Eviction manager: failed to get HasDedicatedImageFs")
-			return nil, fmt.Errorf("eviction manager: failed to get HasDedicatedImageFs: %w", splitDiskError)
+		hasImageFs, imageFsErr := diskInfoProvider.HasDedicatedImageFs(ctx)
+		if imageFsErr != nil {
+			// TODO: This should be refactored to log an error and retry the HasDedicatedImageFs
+			// If we have a transient error this will never be retried and we will not set eviction signals
+			klog.ErrorS(imageFsErr, "Eviction manager: failed to get HasDedicatedImageFs")
+			return nil, fmt.Errorf("eviction manager: failed to get HasDedicatedImageFs: %w", imageFsErr)
 		}
 		m.dedicatedImageFs = &hasImageFs
-		splitContainerImageFs := m.containerGC.IsContainerFsSeparateFromImageFs(ctx)
+		splitContainerImageFs, splitErr := diskInfoProvider.HasDedicatedContainerFs(ctx)
+		if splitErr != nil {
+			// A common error case is when there is no split filesystem
+			// there is an error finding the split filesystem label and we want to ignore these errors
+			klog.ErrorS(splitErr, "eviction manager: failed to check if we have separate container filesystem. Ignoring.")
+		}
 
 		// If we are a split filesystem but the feature is turned off
 		// we should return an error.

pkg/kubelet/eviction/eviction_manager.go

@@ -66,22 +66,27 @@ func (m *mockPodKiller) killPodNow(pod *v1.Pod, evict bool, gracePeriodOverride
 
 // mockDiskInfoProvider is used to simulate testing.
 type mockDiskInfoProvider struct {
-	dedicatedImageFs *bool
+	dedicatedImageFs     *bool
+	dedicatedContainerFs *bool
 }
 
 // HasDedicatedImageFs returns the mocked value
 func (m *mockDiskInfoProvider) HasDedicatedImageFs(_ context.Context) (bool, error) {
 	return ptr.Deref(m.dedicatedImageFs, false), nil
 }
 
+// HasDedicatedContainerFs returns the mocked value
+func (m *mockDiskInfoProvider) HasDedicatedContainerFs(_ context.Context) (bool, error) {
+	return ptr.Deref(m.dedicatedContainerFs, false), nil
+}
+
 // mockDiskGC is used to simulate invoking image and container garbage collection.
 type mockDiskGC struct {
-	err                  error
-	imageGCInvoked       bool
-	containerGCInvoked   bool
-	readAndWriteSeparate bool
-	fakeSummaryProvider  *fakeSummaryProvider
-	summaryAfterGC       *statsapi.Summary
+	err                 error
+	imageGCInvoked      bool
+	containerGCInvoked  bool
+	fakeSummaryProvider *fakeSummaryProvider
+	summaryAfterGC      *statsapi.Summary
 }
 
 // DeleteUnusedImages returns the mocked values.
@@ -102,10 +107,6 @@ func (m *mockDiskGC) DeleteAllUnusedContainers(_ context.Context) error {
 	return m.err
 }
 
-func (m *mockDiskGC) IsContainerFsSeparateFromImageFs(_ context.Context) bool {
-	return m.readAndWriteSeparate
-}
-
 func makePodWithMemoryStats(name string, priority int32, requests v1.ResourceList, limits v1.ResourceList, memoryWorkingSet string) (*v1.Pod, statsapi.PodStats) {
 	pod := newPod(name, priority, []v1.Container{
 		newContainer(name, requests, limits),
@@ -384,7 +385,7 @@ func TestPIDPressure_VerifyPodStatus(t *testing.T) {
 
 		fakeClock := testingclock.NewFakeClock(time.Now())
 		podKiller := &mockPodKiller{}
-		diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: ptr.To(false)}
+		diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: ptr.To(false), dedicatedContainerFs: ptr.To(false)}
 		diskGC := &mockDiskGC{err: nil}
 		nodeRef := &v1.ObjectReference{Kind: "Node", Name: "test", UID: types.UID("test"), Namespace: ""}
 
@@ -562,8 +563,8 @@ func TestDiskPressureNodeFs_VerifyPodStatus(t *testing.T) {
 
 		fakeClock := testingclock.NewFakeClock(time.Now())
 		podKiller := &mockPodKiller{}
-		diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs}
-		diskGC := &mockDiskGC{err: nil, readAndWriteSeparate: tc.writeableSeparateFromReadOnly}
+		diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs, dedicatedContainerFs: &tc.writeableSeparateFromReadOnly}
+		diskGC := &mockDiskGC{err: nil}
 		nodeRef := &v1.ObjectReference{Kind: "Node", Name: "test", UID: types.UID("test"), Namespace: ""}
 
 		config := Config{
@@ -1307,8 +1308,8 @@ func TestDiskPressureNodeFs(t *testing.T) {
 
 			fakeClock := testingclock.NewFakeClock(time.Now())
 			podKiller := &mockPodKiller{}
-			diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs}
-			diskGC := &mockDiskGC{err: nil, readAndWriteSeparate: tc.writeableSeparateFromReadOnly}
+			diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs, dedicatedContainerFs: &tc.writeableSeparateFromReadOnly}
+			diskGC := &mockDiskGC{err: nil}
 			nodeRef := &v1.ObjectReference{Kind: "Node", Name: "test", UID: types.UID("test"), Namespace: ""}
 
 			config := Config{
@@ -1829,7 +1830,7 @@ func TestNodeReclaimFuncs(t *testing.T) {
 
 			fakeClock := testingclock.NewFakeClock(time.Now())
 			podKiller := &mockPodKiller{}
-			diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs}
+			diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs, dedicatedContainerFs: &tc.writeableSeparateFromReadOnly}
 			nodeRef := &v1.ObjectReference{Kind: "Node", Name: "test", UID: types.UID("test"), Namespace: ""}
 
 			config := Config{
@@ -1846,7 +1847,7 @@ func TestNodeReclaimFuncs(t *testing.T) {
 			// This is a constant that we use to test that disk pressure is over. Don't change!
 			diskStatConst := diskStatStart
 			summaryProvider := &fakeSummaryProvider{result: summaryStatsMaker(diskStatStart)}
-			diskGC := &mockDiskGC{fakeSummaryProvider: summaryProvider, err: nil, readAndWriteSeparate: tc.writeableSeparateFromReadOnly}
+			diskGC := &mockDiskGC{fakeSummaryProvider: summaryProvider, err: nil}
 			manager := &managerImpl{
 				clock:                        fakeClock,
 				killPodFunc:                  podKiller.killPodNow,
@@ -2292,8 +2293,8 @@ func TestInodePressureFsInodes(t *testing.T) {
 
 			fakeClock := testingclock.NewFakeClock(time.Now())
 			podKiller := &mockPodKiller{}
-			diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs}
-			diskGC := &mockDiskGC{err: nil, readAndWriteSeparate: tc.writeableSeparateFromReadOnly}
+			diskInfoProvider := &mockDiskInfoProvider{dedicatedImageFs: tc.dedicatedImageFs, dedicatedContainerFs: &tc.writeableSeparateFromReadOnly}
+			diskGC := &mockDiskGC{err: nil}
 			nodeRef := &v1.ObjectReference{Kind: "Node", Name: "test", UID: types.UID("test"), Namespace: ""}
 
 			config := Config{

pkg/kubelet/eviction/eviction_manager_test.go

@@ -75,6 +75,8 @@ type Manager interface {
 type DiskInfoProvider interface {
 	// HasDedicatedImageFs returns true if the imagefs is on a separate device from the rootfs.
 	HasDedicatedImageFs(ctx context.Context) (bool, error)
+	// HasDedicatedContainerFs returns true if the container fs is on a separate device from the rootfs.
+	HasDedicatedContainerFs(ctx context.Context) (bool, error)
 }
 
 // ImageGC is responsible for performing garbage collection of unused images.
@@ -87,8 +89,6 @@ type ImageGC interface {
 type ContainerGC interface {
 	// DeleteAllUnusedContainers deletes all unused containers, even those that belong to pods that are terminated, but not deleted.
 	DeleteAllUnusedContainers(ctx context.Context) error
-	// IsContainerFsSeparateFromImageFs checks if container filesystem is split from image filesystem.
-	IsContainerFsSeparateFromImageFs(ctx context.Context) bool
 }
 
 // KillPodFunc kills a pod.

pkg/kubelet/eviction/types.go

@@ -276,11 +276,11 @@ func (p *cadvisorStatsProvider) ImageFsStats(ctx context.Context) (imageFsRet *s
 	if imageStats == nil || len(imageStats.ImageFilesystems) == 0 || len(imageStats.ContainerFilesystems) == 0 {
 		return nil, nil, fmt.Errorf("missing image stats: %+v", imageStats)
 	}
+
 	splitFileSystem := false
 	imageFs := imageStats.ImageFilesystems[0]
 	containerFs := imageStats.ContainerFilesystems[0]
 	if imageFs.FsId != nil && containerFs.FsId != nil && imageFs.FsId.Mountpoint != containerFs.FsId.Mountpoint {
-		klog.InfoS("Detect Split Filesystem", "ImageFilesystems", imageFs, "ContainerFilesystems", containerFs)
 		splitFileSystem = true
 	}
 	var imageFsInodesUsed *uint64
@@ -312,6 +312,12 @@ func (p *cadvisorStatsProvider) ImageFsStats(ctx context.Context) (imageFsRet *s
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to get container fs info: %w", err)
 	}
+	// ImageFs and ContainerFs could be on different paths on the same device.
+	if containerFsInfo.Device == imageFsInfo.Device {
+		return fsStats, fsStats, nil
+	}
+
+	klog.InfoS("Detect Split Filesystem", "ImageFilesystems", imageStats.ImageFilesystems[0], "ContainerFilesystems", imageStats.ContainerFilesystems[0])
 
 	var containerFsInodesUsed *uint64
 	if containerFsInfo.Inodes != nil && containerFsInfo.InodesFree != nil {

pkg/kubelet/stats/cadvisor_stats_provider.go

@@ -680,7 +680,7 @@ func TestCadvisorSplitImagesFsStats(t *testing.T) {
 		mockRuntime  = containertest.NewMockRuntime(t)
 
 		seed            = 1000
-		imageFsInfo     = getTestFsInfo(seed)
+		imageFsInfo     = getTestFsInfoWithDifferentMount(seed, "image")
 		containerSeed   = 1001
 		containerFsInfo = getTestFsInfo(containerSeed)
 	)
@@ -723,7 +723,59 @@ func TestCadvisorSplitImagesFsStats(t *testing.T) {
 	assert.Equal(containerFsInfo.InodesFree, containerfs.InodesFree)
 	assert.Equal(containerFsInfo.Inodes, containerfs.Inodes)
 	assert.Equal(*containerFsInfo.Inodes-*containerFsInfo.InodesFree, *containerfs.InodesUsed)
+}
 
+func TestCadvisorSameDiskDifferentLocations(t *testing.T) {
+	ctx := context.Background()
+	var (
+		assert       = assert.New(t)
+		mockCadvisor = cadvisortest.NewMockInterface(t)
+		mockRuntime  = containertest.NewMockRuntime(t)
+
+		seed            = 1000
+		imageFsInfo     = getTestFsInfo(seed)
+		containerSeed   = 1001
+		containerFsInfo = getTestFsInfo(containerSeed)
+	)
+	imageFsInfoCRI := &runtimeapi.FilesystemUsage{
+		Timestamp:  imageFsInfo.Timestamp.Unix(),
+		FsId:       &runtimeapi.FilesystemIdentifier{Mountpoint: "images"},
+		UsedBytes:  &runtimeapi.UInt64Value{Value: imageFsInfo.Usage},
+		InodesUsed: &runtimeapi.UInt64Value{Value: *imageFsInfo.Inodes},
+	}
+	containerFsInfoCRI := &runtimeapi.FilesystemUsage{
+		Timestamp:  containerFsInfo.Timestamp.Unix(),
+		FsId:       &runtimeapi.FilesystemIdentifier{Mountpoint: "containers"},
+		UsedBytes:  &runtimeapi.UInt64Value{Value: containerFsInfo.Usage},
+		InodesUsed: &runtimeapi.UInt64Value{Value: *containerFsInfo.Inodes},
+	}
+	imageFsInfoResponse := &runtimeapi.ImageFsInfoResponse{
+		ImageFilesystems:     []*runtimeapi.FilesystemUsage{imageFsInfoCRI},
+		ContainerFilesystems: []*runtimeapi.FilesystemUsage{containerFsInfoCRI},
+	}
+
+	mockCadvisor.EXPECT().ImagesFsInfo().Return(imageFsInfo, nil)
+	mockCadvisor.EXPECT().ContainerFsInfo().Return(containerFsInfo, nil)
+	mockRuntime.EXPECT().ImageFsInfo(ctx).Return(imageFsInfoResponse, nil)
+	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.KubeletSeparateDiskGC, true)
+
+	provider := newCadvisorStatsProvider(mockCadvisor, &fakeResourceAnalyzer{}, mockRuntime, nil, NewFakeHostStatsProvider())
+	stats, containerfs, err := provider.ImageFsStats(ctx)
+	require.NoError(t, err, "imageFsStats should have no error")
+
+	assert.Equal(imageFsInfo.Timestamp, stats.Time.Time)
+	assert.Equal(imageFsInfo.Available, *stats.AvailableBytes)
+	assert.Equal(imageFsInfo.Capacity, *stats.CapacityBytes)
+	assert.Equal(imageFsInfo.InodesFree, stats.InodesFree)
+	assert.Equal(imageFsInfo.Inodes, stats.Inodes)
+	assert.Equal(*imageFsInfo.Inodes-*imageFsInfo.InodesFree, *stats.InodesUsed)
+
+	assert.Equal(imageFsInfo.Timestamp, containerfs.Time.Time)
+	assert.Equal(imageFsInfo.Available, *containerfs.AvailableBytes)
+	assert.Equal(imageFsInfo.Capacity, *containerfs.CapacityBytes)
+	assert.Equal(imageFsInfo.InodesFree, containerfs.InodesFree)
+	assert.Equal(imageFsInfo.Inodes, containerfs.Inodes)
+	assert.Equal(*imageFsInfo.Inodes-*imageFsInfo.InodesFree, *containerfs.InodesUsed)
 }
 
 func TestCadvisorListPodStatsWhenContainerLogFound(t *testing.T) {

pkg/kubelet/stats/cadvisor_stats_provider_test.go

@@ -197,6 +197,20 @@ func (p *Provider) HasDedicatedImageFs(ctx context.Context) (bool, error) {
 	return device != rootFsInfo.Device, nil
 }
 
+// HasDedicatedImageFs returns true if a dedicated image filesystem exists for storing images.
+// KEP Issue Number 4191: Enhanced this to allow for the containers to be separate from images.
+func (p *Provider) HasDedicatedContainerFs(ctx context.Context) (bool, error) {
+	imageFs, err := p.cadvisor.ImagesFsInfo()
+	if err != nil {
+		return false, err
+	}
+	containerFs, err := p.cadvisor.ContainerFsInfo()
+	if err != nil {
+		return false, err
+	}
+	return imageFs.Device != containerFs.Device, nil
+}
+
 func equalFileSystems(a, b *statsapi.FsStats) bool {
 	if a == nil || b == nil {
 		return false