-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add annotation to ignore local storage volume during scale down #5594
feat: add annotation to ignore local storage volume during scale down #5594
Conversation
Test suite is failing because the tests haven't been updated for the new annotation yet. |
@@ -38,6 +38,8 @@ const ( | |||
// PodSafeToEvictKey - annotation that ignores constraints to evict a pod like not being replicated, being on | |||
// kube-system namespace or having a local storage. | |||
PodSafeToEvictKey = "cluster-autoscaler.kubernetes.io/safe-to-evict" | |||
// IgnoreLocalStorageVolumeKey - annotation that ignores (doesn't block on) a local storage volume during scale down | |||
IgnoreLocalStorageVolumeKey = "cluster-autoscaler.kubernetes.io/ignore-local-storage-volume" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better naming suggestions are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"safe-to-evict" is a widely recognized concept at this point, maybe we could reuse it, e.g. cluster-autoscaler.kubernetes.io/local-volume-safe-to-evict
? I know we're not technically evicting the volume, but I think it conveys the intent better. Or maybe include "scale down" and "blocking" instead, like cluster-autoscaler.kubernetes.io/scale-down-non-blocking-local-volume
? Just "ignore" doesn't tell much about what purpose it's being ignored for.
On a slightly related note, I don't think we should restrict ourselves to just one volume here, what if there's more that should be ignored? WDYT about making the value of the annotation a comma-separated list of volume names instead?
@@ -115,6 +115,7 @@ func TestGetPodsToMove(t *testing.T) { | |||
Spec: apiv1.PodSpec{ | |||
Volumes: []apiv1.Volume{ | |||
{ | |||
Name: "empty-vol", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added volume name because it is a required field in Kubernetes. I realized the tests in this files were failing because I wasn't handling the case for empty volume name here. This has been fixed in this PR here.
I thought about removing the volume name above since I am handling the case for empty volume name but I kept it since this makes the manifest closer to actual manifest on Kubernetes.
suraj@suraj:~/Downloads$ kubectl explain pod.spec.volumes | grep name
Volume represents a named volume in a pod that may be accessed by any
name <string> -required-
name of the volume. Must be a DNS_LABEL and unique within the pod. More
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
PersistentVolumeClaim in the same namespace. More info:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, that makes sense.
@@ -136,6 +137,7 @@ func TestGetPodsToMove(t *testing.T) { | |||
Spec: apiv1.PodSpec{ | |||
Volumes: []apiv1.Volume{ | |||
{ | |||
Name: "my-repo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask for a review on this before I go and test it on a live cluster 🙏 Asking this because I think it's a small change and I think the code should work. If possible, I want to address the comments first, get an OK and go test things on a live cluster. This would save me one (or more) round(s) of manual testing 🙇 |
Hi! We already have an annotation that you can use to mark a pod as "never blocking scale down" - |
@towca had the same question myself. Quoting a snippet from @ldemailly's comment from here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I get it, it makes a lot of sense for sidecars, thanks for the explanation. Overall the PR looks good, I just had a couple comments. Also, could you please squash the intermediate commits? I think this is all pretty self-contained and 1 commit for the whole PR should suffice.
@@ -115,6 +115,7 @@ func TestGetPodsToMove(t *testing.T) { | |||
Spec: apiv1.PodSpec{ | |||
Volumes: []apiv1.Volume{ | |||
{ | |||
Name: "empty-vol", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, that makes sense.
@@ -489,6 +509,16 @@ func TestDrain(t *testing.T) { | |||
expectBlockingPod: &BlockingPod{Pod: emptydirPod, Reason: LocalStorageRequested}, | |||
expectDaemonSetPods: []*apiv1.Pod{}, | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider also including test cases for pod with local volume(s) and:
- The annotation value being empty (=no local volumes match, pod is blocking)
- The annotation value not matching any local volumes on the pod (=pod is blocking)
- The annotation value only matching one local volume on the pod, but there is another one without the annotation (=pod is still blocking)
- [if we have multiple values available] Multiple annotation values matching all local volumes on the pod (=pod is not blocking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of them make sense. I came up with a couple more after reading your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the new test cases. Thank you for the suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@@ -38,6 +38,8 @@ const ( | |||
// PodSafeToEvictKey - annotation that ignores constraints to evict a pod like not being replicated, being on | |||
// kube-system namespace or having a local storage. | |||
PodSafeToEvictKey = "cluster-autoscaler.kubernetes.io/safe-to-evict" | |||
// IgnoreLocalStorageVolumeKey - annotation that ignores (doesn't block on) a local storage volume during scale down | |||
IgnoreLocalStorageVolumeKey = "cluster-autoscaler.kubernetes.io/ignore-local-storage-volume" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"safe-to-evict" is a widely recognized concept at this point, maybe we could reuse it, e.g. cluster-autoscaler.kubernetes.io/local-volume-safe-to-evict
? I know we're not technically evicting the volume, but I think it conveys the intent better. Or maybe include "scale down" and "blocking" instead, like cluster-autoscaler.kubernetes.io/scale-down-non-blocking-local-volume
? Just "ignore" doesn't tell much about what purpose it's being ignored for.
On a slightly related note, I don't think we should restrict ourselves to just one volume here, what if there's more that should be ignored? WDYT about making the value of the annotation a comma-separated list of volume names instead?
/assign @towca |
I think
I think P.S.: Thank you for the suggestions. |
I like the idea. 👍 Let me think about it. Thank you for the suggestion! |
Wanted to squash but after seeing your comments I think I need to add more commits. Do you mind if I do squashing at the end before we merge the PR? |
@@ -38,6 +39,8 @@ const ( | |||
// PodSafeToEvictKey - annotation that ignores constraints to evict a pod like not being replicated, being on | |||
// kube-system namespace or having a local storage. | |||
PodSafeToEvictKey = "cluster-autoscaler.kubernetes.io/safe-to-evict" | |||
// SafeToEvictLocalVolumeKey - annotation that ignores (doesn't block on) a local storage volume during node scale down | |||
SafeToEvictLocalVolumeKey = "cluster-autoscaler.kubernetes.io/safe-to-evict-local-volume" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the annotation key here to safe-to-evict-local-volume
instead of the suggested local-volume-safe-to-evict
because I think it's easier to read and remember. This is of course my thinking. Happy to change it based on review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotation key name is being discussed in this comment: #5594 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name LGTM, but I'd make it plural if we allow multiple volumes - safe-to-evict-local-volumes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to safe-to-evict-local-volumes
@towca I think I'm done from my end. Waiting for your review. |
Everything looks good, thanks for the contribution! I have one more small nit if you agree and want to fix, otherwise feel free to unhold the PR. /lgtm |
- this is so that scale down is not blocked on local storage volume - for pods where it is okay to ignore local storage volume Signed-off-by: vadasambar <[email protected]> fix: tests failing - there was a problem in the logic Signed-off-by: vadasambar <[email protected]> test: add unit test for `IgnoreLocalStorageVolumeKey` Signed-off-by: vadasambar <[email protected]> refactor: use `IgnoreLocalStorageVolumeKey` in tests instead of hardcoding the annotation Signed-off-by: vadasambar <[email protected]> fix: wording for test name - `pod with EmptyDir but IgnoreLocalStorageVolumeKey annotation` -> `pod with EmptyDir and IgnoreLocalStorageVolumeKey annotation` Signed-off-by: vadasambar <[email protected]> fix: simulator drain tests failing - set local storage vol name (required) Signed-off-by: vadasambar <[email protected]> refactor: add support for multiple vals in `safe-to-evict-local-volume` annotation - add more unit tests Signed-off-by: vadasambar <[email protected]> refactor: rename ignore local vol key `safe-to-evict-local-volume` -> `safe-to-evict-local-volumes` - abtract code to process annotation into a separate fn - shorten name for test cases Signed-off-by: vadasambar <[email protected]> docs: update FAQ with info about `safe-to-evict-local-volumes` annotation Signed-off-by: vadasambar <[email protected]> docs: add the FAQ for `safe-to-evict-local-volumes` annotation Signed-off-by: vadasambar <[email protected]> docs: fix formatting for `safe-to-evict-local-volumes` in FAQ Signed-off-by: vadasambar <[email protected]> docs: format the `safe-to-evict-local-volumes` as a bullet Signed-off-by: vadasambar <[email protected]> docs: fix `Unless` -> `unless` to make it consistent with other lines Signed-off-by: vadasambar <[email protected]> test: add an extra test for mismatching local vol value in annotation Signed-off-by: vadasambar <[email protected]> docs: make the wording clearer - for `safe-to-evict-local-volumes` annotation Signed-off-by: vadasambar <[email protected]>
c37f4fd
to
b663f13
Compare
Thank you for the review. I have updated the PR to address the last comment. |
/unhold |
/lgtm |
@vadasambar: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@towca need a |
Thanks again! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: towca, vadasambar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Nice feature @vadasambar! @MaciekPytel any idea on timeline for this to get released? It doesn't seem to be in latest which was cut about a month ago. |
You can find it in 1.27 which will be released soon (check |
@vadasambar @towca what do we think about allowing |
I think there is a room for improvement in the annotation.
In 1, if you add Unless there is a specific usecase, I don't see a lot of value in adding I guess what you want is |
Signed-off-by: vadasambar [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
Some pods like istio sidecars have local storage which block node scale down. We would like to have the ability to ignore local storage volumes on certain pods provided the right annotation of the form
cluster-autoscaler.kubernetes.io/ignore-local-storage-volume: <volume-name>
is present on the pod.Which issue(s) this PR fixes:
Fixes #3947
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: