Introduce RequestEviction feature for evicting pods in background (KEP-1397) #1466

ingvagabund · 2024-07-14T17:55:41Z

Implementation of #1354.

When the feature is enabled each pod with descheduler.alpha.kubernetes.io/request-evict-only annotation will have the eviction API error examined for a specific error code/reason and message. If matched eviction of such a pod will be interpreted as initiation of an eviction in background.

The first version of the feature does not provide any way to configure a timeout for pending evictions in background. Neither a timeout for how often a pending evictions in background are checked for timeouts. Both set to 10 minutes by default. The default values can be adjusted based on provided feedback.

TODO:

evictions in background are respested
unit tests
feature gating implemented (EvictionsInBackground as a new feature gate)
e2e integration with kubevirt
feature configuration (currently none)

Refactorings before this PR can be merged:

ingvagabund · 2024-07-18T11:00:24Z

Compared to virt-handler produced by my local kind these are the logs produced by the upstream CI:

{"component":"virt-handler","level":"info","msg":"re-enqueuing VirtualMachineInstance default/vmi-masquerade-3","pos":"vm.go:1712","reason":"server error. command SyncVMI failed: \"LibvirtError(Code=38, Domain=0, Message='Unable to create tap device tap0: Permission denied')\"","timestamp":"2024-07-17T15:01:56.211181Z"}

Could be kubevirt does not like combination of running kind in pod with dind enabled.

ingvagabund · 2024-07-19T07:22:21Z

/retest-required

atiratree · 2024-10-09T09:28:59Z

cmd/descheduler/app/options/options.go

@@ -45,6 +50,10 @@ type DeschedulerServer struct {
 	SecureServing  *apiserveroptions.SecureServingOptionsWithLoopback
 	DisableMetrics bool
 	EnableHTTP2    bool
+	// FeatureGates enabled by the user


Btw, the approach used by upstream is via ComponentGlobalsRegistry. Might be good to consider if it makes sense to introduce that.

Not applicable here as one may create multiple descheduler instances for simulation purposes.

cmd/descheduler/app/server.go

atiratree · 2024-10-09T09:43:30Z

pkg/descheduler/descheduler.go

 		eventRecorder,
+		podInformer,
+		rs.DefaultFeatureGates,


Does it make sense to pass the feature gates as an argument everywhere? Compared to the global variable.

I was keeping in mind the case where the descheduling framework is run as a simulator. E.g. in case two or more framework instances are created in the same run/binary.

pkg/descheduler/descheduler.go

atiratree · 2024-10-09T10:08:52Z

pkg/descheduler/evictions/evictions.go

+						return
+					}
+					// Ignore any pod that does not have eviction in progress
+					if _, exists := newPod.Annotations[EvictionInProgressAnnotationKey]; !exists {


how can we get to this line and delete pods that lost their annotation, when we do

if newPod.Annotations == nil { return } // Ignore pod's that are not subject to an eviction in background if _, exists := newPod.Annotations[EvictionRequestAnnotationKey]; !exists { return }

just above?

If a pod looses its annotation there's no way of knowing whether the annotation was removed by accident or intentionally. E.g. a live migration is no longer needed since some scoring mechanism decided it's more costly to perform a live migration than to drop VM's state and re-create it.

So, shouldn't we remove the check above, or move this check there instead?

Oh. I misunderstood. So it can happen a pod might still have an eviction in background correctly finished. Yet, if EvictionRequestAnnotationKey is removed (e.g. by accident or on purpose) the pod is no longer considered for further processing. EvictionRequestAnnotationKey is the referential one. We might still check for EvictionInProgressAnnotationKey annotation as well just in case there's one. Yet, some implementations might decide to just remove EvictionRequestAnnotationKey annotation to signal an eviction in background was aborted/canceled.

Yet, if EvictionRequestAnnotationKey is removed (e.g. by accident or on purpose) the pod is no longer considered for further processing.

I am not sure if I see all the loose ends here :D

If the pod was in the cache when the Request annotation was removed, then we skip evicting it (thus we process the InProgress annotation)

What's important is to properly remove entry from the cache

which we would not do when evictionAssumed == false and EvictionRequestAnnotationKey is removed

Good catch. Updating the delete event handler to delete every pod that's in the cache: bb084d6

That is much better. I still think it is unfortunate that we do not deschedule pods that had both EvictionRequestAnnotationKey and EvictionInProgressAnnotationKey removed at the same time. If the owner cancels the progress in this way and the pod is not terminating, then the pod will be immune to descheduling.

Removing the pod from the cache during update event just in case the pod is in the cache: 4425072

looks good, thx!

pkg/descheduler/evictions/evictions.go

pkg/framework/profile/profile.go

atiratree · 2024-10-09T11:05:41Z

pkg/descheduler/evictions/evictions.go

@@ -108,6 +423,46 @@ func (pe *PodEvictor) SetClient(client clientset.Interface) {
 	pe.client = client
 }

+func (pe *PodEvictor) evictionRequestsTotal() uint {


I forgot what was the consensus last time, but do we want to introduce new metrics for this feature? https://github.com/kubernetes-sigs/descheduler/blob/master/keps/1397-evictions-in-background/README.md#metrics

We do. Yet, not as part of this PR. I will have a follow up for all other changes.

atiratree · 2024-10-09T11:08:46Z

test/e2e/e2e_evictioninbackground_test.go

+		ObjectMeta: metav1.ObjectMeta{
+			Name: fmt.Sprintf("kubevirtvmi-%v", idx),
+			Annotations: map[string]string{
+				"descheduler.alpha.kubernetes.io/request-evict-only": "",


just to confirm, the feature depends only on the key, and not on the value? Would be good to add a little a bit of the documentation above EvictionRequestAnnotationKey and EvictionInProgressAnnotationKey

That is correct. +1 for a documentation. I still have a blog post to write.

Ok, sounds good. Let's not forget about the constants and update them after as well.

atiratree · 2024-11-14T16:35:39Z

pkg/descheduler/evictions/evictions.go

+						return
+					}
+					// Ignore any pod that does not have eviction in progress
+					if _, exists := newPod.Annotations[EvictionInProgressAnnotationKey]; !exists {


So, shouldn't we remove the check above, or move this check there instead?

atiratree · 2024-11-14T16:40:47Z

pkg/descheduler/evictions/evictions.go

+						if oldPod.Annotations == nil {
+							return
+						}
+						if _, exists := oldPod.Annotations[EvictionInProgressAnnotationKey]; exists {


we can check the presence of a key on nil annotations

The annotation value is currently empty. So oldPod.Annotations[EvictionInProgressAnnotationKey] will always be empty. If, did you mean a different check?

atiratree · 2024-11-14T16:41:48Z

pkg/descheduler/evictions/evictions.go

+		if pod.Annotations != nil {
+			// eviction in background requested
+			if _, exists := pod.Annotations[EvictionRequestAnnotationKey]; exists {


we could simplify this and other checks as well

pkg/framework/profile/profile.go

atiratree · 2024-11-14T17:04:32Z

test/e2e/e2e_evictioninbackground_test.go

+		ObjectMeta: metav1.ObjectMeta{
+			Name: fmt.Sprintf("kubevirtvmi-%v", idx),
+			Annotations: map[string]string{
+				"descheduler.alpha.kubernetes.io/request-evict-only": "",


Ok, sounds good. Let's not forget about the constants and update them after as well.

atiratree · 2024-11-14T17:07:32Z

pkg/descheduler/evictions/evictions.go

+	assumedRequestTimeoutSeconds uint
+}
+
+func newEvictionRequestsCache(assumedRequestTimeoutSeconds uint) *evictionRequestsCache {


It would be nice to introduce unit tests for the cache.

There's one in TestEvictionRequestsCacheCleanup

Ok, that one looks sufficient. Nevertheless from the long term perspective it would be good to test all the cache operations separated from the PodEvictor. We would not have to be concerned what operations the evictor uses and which does not - useful for example when we make changes to the cache in the future.

pkg/framework/profile/profile.go

pkg/descheduler/evictions/evictions.go

pkg/framework/profile/profile.go

atiratree · 2024-11-15T16:40:20Z

pkg/descheduler/evictions/evictions.go

+	assumedRequestTimeoutSeconds uint
+}
+
+func newEvictionRequestsCache(assumedRequestTimeoutSeconds uint) *evictionRequestsCache {


Ok, that one looks sufficient. Nevertheless from the long term perspective it would be good to test all the cache operations separated from the PodEvictor. We would not have to be concerned what operations the evictor uses and which does not - useful for example when we make changes to the cache in the future.

pkg/descheduler/evictions/evictions.go

atiratree · 2024-11-15T17:07:54Z

pkg/descheduler/evictions/evictions.go

+						return
+					}
+					// Ignore any pod that does not have eviction in progress
+					if _, exists := newPod.Annotations[EvictionInProgressAnnotationKey]; !exists {


Yet, if EvictionRequestAnnotationKey is removed (e.g. by accident or on purpose) the pod is no longer considered for further processing.

I am not sure if I see all the loose ends here :D

If the pod was in the cache when the Request annotation was removed, then we skip evicting it (thus we process the InProgress annotation)

pkg/descheduler/evictions/evictions.go

atiratree · 2024-11-18T13:05:43Z

pkg/descheduler/evictions/evictions.go

+						return
+					}
+					// Ignore any pod that does not have eviction in progress
+					if _, exists := newPod.Annotations[EvictionInProgressAnnotationKey]; !exists {


What's important is to properly remove entry from the cache

which we would not do when evictionAssumed == false and EvictionRequestAnnotationKey is removed

pkg/descheduler/evictions/evictions.go

atiratree · 2024-11-19T12:22:32Z

pkg/descheduler/evictions/evictions.go

+func getPodKey(pod *v1.Pod) (string, error) {
+	uid := string(pod.UID)
+	if len(uid) == 0 {
+		return "", errors.New("cannot get cache key for pod with empty UID")


I think this error is quite troublesome. It bubbles up to so many places. Can we remove it?

The UID should be guaranteed on already created pods. Do we ever operate on pods that have not been created yet?

Assuming all pods have the UID set this error will never get returned.

+1 for exploring this avenue and filtering out all pods that does not have their uid set from eviction in a separate PR/improvement.

There should be no need for filtering:

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids

https://github.com/kubernetes/kubernetes/blob/021dbe9d195165cfcbe756c0b6dd5bb943b64a7c/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L151-L159

Replacing the error for missing uid with a panic.

pkg/descheduler/evictions/evictions.go

atiratree · 2024-11-19T14:24:28Z

LGTM, thanks!

ingvagabund · 2024-11-19T14:27:03Z

Squashing all the review commits now

When the feature is enabled each pod with descheduler.alpha.kubernetes.io/request-evict-only annotation will have the eviction API error examined for a specific error code/reason and message. If matched eviction of such a pod will be interpreted as initiation of an eviction in background.

ingvagabund · 2024-11-19T14:30:10Z

@atiratree thank you for all your reviews and patience. This really helped.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 14, 2024

k8s-ci-robot requested review from a7i and JaneLiuL July 14, 2024 17:56

ingvagabund force-pushed the eviction-in-background-code branch 17 times, most recently from 1c05dac to d022bf4 Compare July 17, 2024 14:54

ingvagabund force-pushed the eviction-in-background-code branch 6 times, most recently from 3210921 to 90a6b55 Compare July 19, 2024 07:18

atiratree reviewed Oct 9, 2024

View reviewed changes

ingvagabund mentioned this pull request Nov 12, 2024

DeschedulerServer: new Apply function for applying configuration #1542

Merged

ingvagabund force-pushed the eviction-in-background-code branch from a2b5211 to 7d50a3b Compare November 12, 2024 15:07

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2024

Define EvictionsInBackground feature gate

f115e78

ingvagabund force-pushed the eviction-in-background-code branch from 7d50a3b to 1ee0351 Compare November 14, 2024 12:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2024

atiratree reviewed Nov 14, 2024

View reviewed changes

ingvagabund force-pushed the eviction-in-background-code branch 2 times, most recently from 005f8ea to 17d7518 Compare November 15, 2024 14:36

atiratree reviewed Nov 15, 2024

View reviewed changes

ingvagabund force-pushed the eviction-in-background-code branch from 17d7518 to 6d40fdc Compare November 17, 2024 14:11

ingvagabund mentioned this pull request Nov 17, 2024

Sync with upstream openshift/descheduler#121

Merged

atiratree reviewed Nov 18, 2024

View reviewed changes

ingvagabund mentioned this pull request Nov 19, 2024

Sync with upstream openshift/descheduler#120

Merged

atiratree reviewed Nov 19, 2024

View reviewed changes

ingvagabund force-pushed the eviction-in-background-code branch from 9a3544d to 4425072 Compare November 19, 2024 12:55

atiratree reviewed Nov 19, 2024

View reviewed changes

pkg/descheduler/evictions/evictions.go Outdated Show resolved Hide resolved

ingvagabund force-pushed the eviction-in-background-code branch from 18c833f to cfbb740 Compare November 19, 2024 14:04

atiratree reviewed Nov 19, 2024

View reviewed changes

pkg/descheduler/evictions/evictions.go Show resolved Hide resolved

ingvagabund added 2 commits November 19, 2024 15:28

bump(vendor)

7d4ec60

ingvagabund force-pushed the eviction-in-background-code branch from c45c8cb to 7d4ec60 Compare November 19, 2024 14:29

ingvagabund added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2024

k8s-ci-robot merged commit a4c09bf into kubernetes-sigs:master Nov 19, 2024
8 of 9 checks passed

ingvagabund deleted the eviction-in-background-code branch November 19, 2024 14:58

atiratree mentioned this pull request Dec 9, 2024

New EvictionBlocked PodDisruptionCondition kubernetes/kubernetes#128815

Open

Introduce RequestEviction feature for evicting pods in background (KEP-1397) #1466

Introduce RequestEviction feature for evicting pods in background (KEP-1397) #1466

Conversation

ingvagabund commented Jul 14, 2024 • edited Loading

ingvagabund commented Jul 18, 2024

ingvagabund commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Nov 19, 2024

ingvagabund commented Nov 19, 2024

ingvagabund commented Nov 19, 2024

ingvagabund commented Jul 14, 2024 •

edited

Loading

atiratree Nov 18, 2024 •

edited

Loading

atiratree Nov 18, 2024 •

edited

Loading