koordlet: kill container after calling eviction api success (#1759)

Signed-off-by: j4ckstraw <j4ckstraw@foxmail.com>

pkg/koordlet/qosmanager/framework/config.go

@@ -27,6 +27,7 @@ type Config struct {
 	MemoryEvictIntervalSeconds int
 	MemoryEvictCoolTimeSeconds int
 	CPUEvictCoolTimeSeconds    int
+	OnlyEvictByAPI             bool
 	QOSExtensionCfg            *QOSExtensionConfig
 }
 
@@ -38,6 +39,7 @@ func NewDefaultConfig() *Config {
 		MemoryEvictIntervalSeconds: 1,
 		MemoryEvictCoolTimeSeconds: 4,
 		CPUEvictCoolTimeSeconds:    20,
+		OnlyEvictByAPI:             false,
 		QOSExtensionCfg:            &QOSExtensionConfig{FeatureGates: map[string]bool{}},
 	}
 }
@@ -49,5 +51,6 @@ func (c *Config) InitFlags(fs *flag.FlagSet) {
 	fs.IntVar(&c.MemoryEvictIntervalSeconds, "memory-evict-interval-seconds", c.MemoryEvictIntervalSeconds, "evict be pod(memory) interval by seconds")
 	fs.IntVar(&c.MemoryEvictCoolTimeSeconds, "memory-evict-cool-time-seconds", c.MemoryEvictCoolTimeSeconds, "cooling time: memory next evict time should after lastEvictTime + MemoryEvictCoolTimeSeconds")
 	fs.IntVar(&c.CPUEvictCoolTimeSeconds, "cpu-evict-cool-time-seconds", c.CPUEvictCoolTimeSeconds, "cooltime: CPU next evict time should after lastEvictTime + CPUEvictCoolTimeSeconds")
+	fs.BoolVar(&c.OnlyEvictByAPI, "only-evict-by-api", c.OnlyEvictByAPI, "only evict pod if call eviction api successed")
 	c.QOSExtensionCfg.InitFlags(fs)
 }

pkg/koordlet/qosmanager/framework/config_test.go

@@ -31,6 +31,7 @@ func Test_NewDefaultConfig(t *testing.T) {
 		MemoryEvictIntervalSeconds: 1,
 		MemoryEvictCoolTimeSeconds: 4,
 		CPUEvictCoolTimeSeconds:    20,
+		OnlyEvictByAPI:             false,
 		QOSExtensionCfg:            &QOSExtensionConfig{FeatureGates: map[string]bool{}},
 	}
 	defaultConfig := NewDefaultConfig()
@@ -47,6 +48,7 @@ func Test_InitFlags(t *testing.T) {
 		"--memory-evict-cool-time-seconds=8",
 		"--cpu-evict-cool-time-seconds=40",
 		"--qos-extension-plugins=test-plugin=true",
+		"--only-evict-by-api=false",
 	}
 	fs := flag.NewFlagSet(cmdArgs[0], flag.ExitOnError)
 
@@ -57,6 +59,7 @@ func Test_InitFlags(t *testing.T) {
 		MemoryEvictIntervalSeconds int
 		MemoryEvictCoolTimeSeconds int
 		CPUEvictCoolTimeSeconds    int
+		OnlyEvictByAPI             bool
 		QOSExtensionCfg            *QOSExtensionConfig
 	}
 	type args struct {
@@ -76,6 +79,7 @@ func Test_InitFlags(t *testing.T) {
 				MemoryEvictIntervalSeconds: 2,
 				MemoryEvictCoolTimeSeconds: 8,
 				CPUEvictCoolTimeSeconds:    40,
+				OnlyEvictByAPI:             false,
 				QOSExtensionCfg:            &QOSExtensionConfig{FeatureGates: map[string]bool{"test-plugin": true}},
 			},
 			args: args{fs: fs},
@@ -90,6 +94,7 @@ func Test_InitFlags(t *testing.T) {
 				MemoryEvictIntervalSeconds: tt.fields.MemoryEvictIntervalSeconds,
 				MemoryEvictCoolTimeSeconds: tt.fields.MemoryEvictCoolTimeSeconds,
 				CPUEvictCoolTimeSeconds:    tt.fields.CPUEvictCoolTimeSeconds,
+				OnlyEvictByAPI:             tt.fields.OnlyEvictByAPI,
 				QOSExtensionCfg:            tt.fields.QOSExtensionCfg,
 			}
 			c := NewDefaultConfig()

pkg/koordlet/qosmanager/framework/context.go

@@ -62,7 +62,7 @@ func (r *Evictor) Start(stopCh <-chan struct{}) error {
 
 func (r *Evictor) EvictPodsIfNotEvicted(evictPods []*corev1.Pod, node *corev1.Node, reason string, message string) {
 	for _, evictPod := range evictPods {
-		r.evictPodIfNotEvicted(evictPod, node, reason, message)
+		r.EvictPodIfNotEvicted(evictPod, node, reason, message)
 	}
 }
 
@@ -74,16 +74,18 @@ func (r *Evictor) IsPodEvicted(pod *corev1.Pod) bool {
 	return evicted
 }
 
-func (r *Evictor) evictPodIfNotEvicted(evictPod *corev1.Pod, node *corev1.Node, reason string, message string) {
+func (r *Evictor) EvictPodIfNotEvicted(evictPod *corev1.Pod, node *corev1.Node, reason string, message string) bool {
 	_, evicted := r.podsEvicted.Get(string(evictPod.UID))
 	if evicted {
 		klog.V(5).Infof("Pod has been evicted! podID: %v, evict reason: %s", evictPod.UID, reason)
-		return
+		return true
 	}
 	success := r.evictPod(evictPod, reason, message)
 	if success {
 		_ = r.podsEvicted.SetDefault(string(evictPod.UID), evictPod.UID)
 	}
+
+	return success
 }
 
 func (r *Evictor) evictPod(evictPod *corev1.Pod, reason string, message string) bool {

pkg/koordlet/qosmanager/framework/context_test.go

@@ -32,11 +32,101 @@ import (
 
 	apiext "github.com/koordinator-sh/koordinator/apis/extension"
 	"github.com/koordinator-sh/koordinator/pkg/koordlet/qosmanager/helpers"
+	"github.com/koordinator-sh/koordinator/pkg/koordlet/resourceexecutor"
 	mock_statesinformer "github.com/koordinator-sh/koordinator/pkg/koordlet/statesinformer/mockstatesinformer"
 	"github.com/koordinator-sh/koordinator/pkg/koordlet/util/testutil"
 	"github.com/koordinator-sh/koordinator/pkg/util"
 )
 
+func Test_EvictPodIfNotEvicted(t *testing.T) {
+	testpod := testutil.MockTestPod(apiext.QoSBE, "test_be_pod")
+	testnode := testutil.MockTestNode("80", "120G")
+
+	type args struct {
+		pod    *corev1.Pod
+		node   *corev1.Node
+		reason string
+	}
+	type wants struct {
+		result           bool
+		evictObjectError bool
+		eventReason      string
+	}
+
+	tests := []struct {
+		name   string
+		pod    *corev1.Pod
+		node   *corev1.Node
+		args   args
+		wanted wants
+	}{
+		{
+			name: "evict ok",
+			pod:  testpod,
+			node: testnode,
+			args: args{
+				pod:    testpod,
+				node:   testnode,
+				reason: resourceexecutor.EvictPodByNodeMemoryUsage,
+			},
+			wanted: wants{
+				result:           true,
+				evictObjectError: false,
+				eventReason:      helpers.EvictPodSuccess,
+			},
+		},
+		{
+			name: "evict not found",
+			node: testnode,
+			args: args{
+				pod:    testpod,
+				node:   testnode,
+				reason: resourceexecutor.EvictPodByNodeMemoryUsage,
+			},
+			wanted: wants{
+				result:           false,
+				evictObjectError: true,
+				eventReason:      helpers.EvictPodFail,
+			},
+		},
+		// TODO add test for evict failed by forbidden
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// prepare
+			ctl := gomock.NewController(t)
+			defer ctl.Finish()
+			fakeRecorder := &testutil.FakeRecorder{}
+			client := clientsetfake.NewSimpleClientset()
+			r := NewEvictor(client, fakeRecorder, policyv1beta1.SchemeGroupVersion.Version)
+			stop := make(chan struct{})
+			err := r.podsEvicted.Run(stop)
+			assert.NoError(t, err)
+			defer func() { stop <- struct{}{} }()
+
+			if tt.pod != nil {
+				_, err = client.CoreV1().Pods(tt.pod.Namespace).Create(context.TODO(), tt.pod, metav1.CreateOptions{})
+				assert.NoError(t, err)
+			}
+
+			got := r.EvictPodIfNotEvicted(tt.args.pod, tt.args.node, tt.args.reason, "")
+			assert.Equal(t, tt.wanted.result, got)
+
+			getEvictObject, err := client.Tracker().Get(testutil.PodsResource, tt.args.pod.Namespace, tt.args.pod.Name)
+			if tt.wanted.evictObjectError {
+				assert.Error(t, err)
+				assert.Nil(t, getEvictObject)
+				assert.Equal(t, tt.wanted.eventReason, fakeRecorder.EventReason, "expect evict failed event! but got %s", fakeRecorder.EventReason)
+			} else {
+				assert.NoError(t, err)
+				assert.NotNil(t, getEvictObject)
+				assert.Equal(t, tt.wanted.eventReason, fakeRecorder.EventReason, "expect evict success event! but got %s", fakeRecorder.EventReason)
+			}
+		})
+	}
+}
+
 func Test_EvictPodsIfNotEvicted(t *testing.T) {
 	// test data
 	pod := testutil.MockTestPod(apiext.QoSBE, "test_be_pod")

pkg/koordlet/qosmanager/plugins/cpuevict/cpu_evict.go

@@ -58,6 +58,7 @@ type cpuEvictor struct {
 	metricCache           metriccache.MetricCache
 	evictor               *framework.Evictor
 	lastEvictTime         time.Time
+	onlyEvictByAPI        bool
 }
 
 func New(opt *framework.Options) framework.QOSStrategy {
@@ -68,6 +69,7 @@ func New(opt *framework.Options) framework.QOSStrategy {
 		statesInformer:        opt.StatesInformer,
 		metricCache:           opt.MetricCache,
 		lastEvictTime:         time.Now(),
+		onlyEvictByAPI:        opt.Config.OnlyEvictByAPI,
 	}
 }
 
@@ -295,24 +297,30 @@ func (c *cpuEvictor) killAndEvictBEPodsRelease(node *corev1.Node, bePodInfos []*
 		node.Name, cpuNeedMilliRelease)
 
 	cpuMilliReleased := int64(0)
-	var killedPods []*corev1.Pod
+	hasKillPods := false
 	for _, bePod := range bePodInfos {
 		if cpuMilliReleased >= cpuNeedMilliRelease {
 			break
 		}
 
-		podKillMsg := fmt.Sprintf("%s, kill pod: %s", message, util.GetPodKey(bePod.pod))
-		helpers.KillContainers(bePod.pod, podKillMsg)
-
-		killedPods = append(killedPods, bePod.pod)
-		cpuMilliReleased = cpuMilliReleased + bePod.milliRequest
-
-		klog.V(5).Infof("cpuEvict pick pod %s to evict", util.GetPodKey(bePod.pod))
+		if c.onlyEvictByAPI {
+			if c.evictor.EvictPodIfNotEvicted(bePod.pod, node, resourceexecutor.EvictPodByBECPUSatisfaction, message) {
+				cpuMilliReleased = cpuMilliReleased + bePod.milliRequest
+				klog.V(5).Infof("cpuEvict pick pod %s to evict", util.GetPodKey(bePod.pod))
+				hasKillPods = true
+			} else {
+				klog.V(5).Infof("cpuEvict pick pod %s to evict, failed", util.GetPodKey(bePod.pod))
+			}
+		} else {
+			podKillMsg := fmt.Sprintf("%s, kill pod: %s", message, util.GetPodKey(bePod.pod))
+			helpers.KillContainers(bePod.pod, podKillMsg)
+			cpuMilliReleased = cpuMilliReleased + bePod.milliRequest
+			klog.V(5).Infof("cpuEvict pick pod %s to evict", util.GetPodKey(bePod.pod))
+			hasKillPods = true
+		}
 	}
 
-	c.evictor.EvictPodsIfNotEvicted(killedPods, node, resourceexecutor.EvictPodByBECPUSatisfaction, message)
-
-	if len(killedPods) > 0 {
+	if hasKillPods {
 		c.lastEvictTime = time.Now()
 	}
 	klog.V(5).Infof("killAndEvictBEPodsRelease finished! cpuNeedMilliRelease(%d) cpuMilliReleased(%d)",

pkg/koordlet/qosmanager/plugins/cpuevict/cpu_evict_test.go

@@ -576,12 +576,71 @@ func Test_killAndEvictBEPodsRelease(t *testing.T) {
 
 	// evict subresource will not be creat or update in client go testing, check evict object
 	// https://github.com/kubernetes/client-go/blob/v0.28.7/testing/fixture.go#L117
-	assert.True(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[0].pod))
-	assert.True(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[1].pod))
+
+	// evict by API is false, so podsEvicted cache will not record killed pod
+	assert.False(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[0].pod))
+	assert.False(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[1].pod))
 	assert.False(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[2].pod))
+}
 
-	assert.True(t, cpuEvictor.lastEvictTime.After(time.Now().Add(-5*time.Second)), "checkLastTime")
+func Test_killAndEvictBEPodsReleaseWithEvictionAPIOnly(t *testing.T) {
+	podEvictInfosSorted := []*podEvictCPUInfo{
+		{
+			pod:          mockBEPodForCPUEvict("pod_be_3_priority10", 16*1000, 10),
+			milliRequest: 16 * 1000,
+		},
+		{
+			pod:          mockBEPodForCPUEvict("pod_be_2_priority100", 16*1000, 100),
+			milliRequest: 16 * 1000,
+		},
+		{
+			pod:          mockBEPodForCPUEvict("pod_be_1_priority100", 16*1000, 100),
+			milliRequest: 16 * 1000,
+		},
+	}
+
+	ctl := gomock.NewController(t)
+	defer ctl.Finish()
+
+	fakeRecorder := &testutil.FakeRecorder{}
+	client := clientsetfake.NewSimpleClientset()
+
+	stop := make(chan struct{})
+	evictor := framework.NewEvictor(client, fakeRecorder, policyv1beta1.SchemeGroupVersion.Version)
+	evictor.Start(stop)
+	defer func() { stop <- struct{}{} }()
 
+	node := testutil.MockTestNode("100", "500G")
+	runtime.DockerHandler = handler.NewFakeRuntimeHandler()
+	// create pod
+	var containers []*critesting.FakeContainer
+	for _, podInfo := range podEvictInfosSorted {
+		pod := podInfo.pod
+		_, _ = client.CoreV1().Pods(pod.Namespace).Create(context.TODO(), pod, metav1.CreateOptions{})
+		for _, containerStatus := range pod.Status.ContainerStatuses {
+			_, containerId, _ := util.ParseContainerId(containerStatus.ContainerID)
+			fakeContainer := &critesting.FakeContainer{
+				SandboxID:       string(pod.UID),
+				ContainerStatus: runtimeapi.ContainerStatus{Id: containerId},
+			}
+			containers = append(containers, fakeContainer)
+		}
+	}
+	runtime.DockerHandler.(*handler.FakeRuntimeHandler).SetFakeContainers(containers)
+
+	cpuEvictor := &cpuEvictor{
+		evictor:        evictor,
+		onlyEvictByAPI: true,
+		lastEvictTime:  time.Now().Add(-5 * time.Minute),
+	}
+
+	cpuEvictor.killAndEvictBEPodsRelease(node, podEvictInfosSorted, 18*1000)
+
+	// evict subresource will not be creat or update in client go testing, check evict object
+	// https://github.com/kubernetes/client-go/blob/v0.28.7/testing/fixture.go#L117
+	assert.True(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[0].pod))
+	assert.True(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[1].pod))
+	assert.False(t, cpuEvictor.evictor.IsPodEvicted(podEvictInfosSorted[2].pod))
 }
 
 func Test_isSatisfactionConfigValid(t *testing.T) {

pkg/koordlet/qosmanager/plugins/memoryevict/memory_evict.go

@@ -32,6 +32,7 @@ import (
 	"github.com/koordinator-sh/koordinator/pkg/koordlet/qosmanager/helpers"
 	"github.com/koordinator-sh/koordinator/pkg/koordlet/resourceexecutor"
 	"github.com/koordinator-sh/koordinator/pkg/koordlet/statesinformer"
+	"github.com/koordinator-sh/koordinator/pkg/util"
 )
 
 const (
@@ -50,6 +51,7 @@ type memoryEvictor struct {
 	metricCache           metriccache.MetricCache
 	evictor               *framework.Evictor
 	lastEvictTime         time.Time
+	onlyEvictByAPI        bool
 }
 
 type podInfo struct {
@@ -64,6 +66,7 @@ func New(opt *framework.Options) framework.QOSStrategy {
 		metricCollectInterval: opt.MetricAdvisorConfig.CollectResUsedInterval,
 		statesInformer:        opt.StatesInformer,
 		metricCache:           opt.MetricCache,
+		onlyEvictByAPI:        opt.Config.OnlyEvictByAPI,
 	}
 }
 
@@ -164,24 +167,36 @@ func (m *memoryEvictor) killAndEvictBEPods(node *corev1.Node, podMetrics map[str
 	bePodInfos := m.getSortedBEPodInfos(podMetrics)
 	message := fmt.Sprintf("killAndEvictBEPods for node, need to release memory: %v", memoryNeedRelease)
 	memoryReleased := int64(0)
-
-	var killedPods []*corev1.Pod
+	hasKillPods := false
 	for _, bePod := range bePodInfos {
 		if memoryReleased >= memoryNeedRelease {
 			break
 		}
 
-		killMsg := fmt.Sprintf("%v, kill pod: %v", message, bePod.pod.Name)
-		helpers.KillContainers(bePod.pod, killMsg)
-		killedPods = append(killedPods, bePod.pod)
-		if bePod.memUsed != 0 {
-			memoryReleased += int64(bePod.memUsed)
+		if m.onlyEvictByAPI {
+			if m.evictor.EvictPodIfNotEvicted(bePod.pod, node, resourceexecutor.EvictPodByNodeMemoryUsage, message) {
+				hasKillPods = true
+				if bePod.memUsed != 0 {
+					memoryReleased += int64(bePod.memUsed)
+				}
+				klog.V(5).Infof("memoryEvict pick pod %s to evict", util.GetPodKey(bePod.pod))
+			} else {
+				klog.V(5).Infof("memoryEvict pick pod %s to evict", util.GetPodKey(bePod.pod))
+			}
+		} else {
+			killMsg := fmt.Sprintf("%v, kill pod: %v", message, bePod.pod.Name)
+			helpers.KillContainers(bePod.pod, killMsg)
+			hasKillPods = true
+			if bePod.memUsed != 0 {
+				memoryReleased += int64(bePod.memUsed)
+			}
+			klog.V(5).Infof("memoryEvict pick pod %s to evict", util.GetPodKey(bePod.pod))
 		}
 	}
+	if hasKillPods {
+		m.lastEvictTime = time.Now()
+	}
 
-	m.evictor.EvictPodsIfNotEvicted(killedPods, node, resourceexecutor.EvictPodByNodeMemoryUsage, message)
-
-	m.lastEvictTime = time.Now()
 	klog.Infof("killAndEvictBEPods completed, memoryNeedRelease(%v) memoryReleased(%v)", memoryNeedRelease, memoryReleased)
 }