Skip to content
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

Evictable pods & gentle label/annotation handling #13180

Merged
merged 15 commits into from
Mar 20, 2024

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented Mar 14, 2024

What this PR does / why we need it:
The Cluster-Autoscaler does not evict Pods that use non-memory emptyDirs, unless they have a special annotation (see here). This PR is meant to add this special annotation to all Pods (i.e. PodTemplateSpecs in Deployments, StatefulSets) that make use of emptyDirs. This applies to both our own Helm charts, as well as addons and all the Go code that reconciles such resources.

In the process of doing this I was coming from the "I could have tested this on a KKP dev system, if only KKP let me manually patch the pod templates grrrrr" standpoint, and so I changed the reconciling code to not overwrite all the labels/annotations in the pod template, but to only merge the required ones into the existing ones.

Also there was a fun bug in the Azure CCM code, which accidentally used openstack labels. A harmless bug,as there can never be both openstack and azure CCMs in the same cluster namespace, but still fun. To prevent such mistakes and to slim down the code a bit, the labelling of CCM deployments is now mostly taken care of by the wrapper code.

The S3 exporter was also blocking the cluster-autoscaler because pods in the kube-system namespace are treated differently. To allow it to be evictable, a PDB has to be provided. The same applies to the Vertical Pod Autoscaler (VPA).

NB: Setting .Name or .Namespace is never required or useful in our reconciling framework. I removed some leftovers from older code.

What type of PR is this?
/kind feature
/kind design

Does this PR introduce a user-facing change? Then add your Release Note here:

Improve compatibility with cluster-autoscaler 1.27.1+: Pods using temporary volumes are now marked as evictable.

Documentation:

NONE

@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. docs/none Denotes a PR that doesn't need documentation (changes). kind/feature Categorizes issue or PR as related to a new feature. kind/design Categorizes issue or PR as related to design. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. do-not-merge/code-freeze Indicates that a PR should not merge because it has not been approved for code freeze yet. sig/app-management Denotes a PR or issue as being assigned to SIG App Management. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 14, 2024
@xrstf xrstf self-assigned this Mar 14, 2024
@xrstf xrstf changed the title Evictable pods & gentle label handling Evictable pods & gentle label/annotation handling Mar 14, 2024
@embik embik removed the do-not-merge/code-freeze Indicates that a PR should not merge because it has not been approved for code freeze yet. label Mar 15, 2024
@xrstf
Copy link
Contributor Author

xrstf commented Mar 18, 2024

/override pre-kubermatic-e2e-vsphere-ubuntu-1.29
/override pre-kubermatic-e2e-vsphere-ubuntu-1.29-customfolder
/override pre-kubermatic-e2e-vsphere-ubuntu-1.29-datastore-cluster

vSphere has issues.

@kubermatic-bot
Copy link
Contributor

@xrstf: Overrode contexts on behalf of xrstf: pre-kubermatic-e2e-vsphere-ubuntu-1.29, pre-kubermatic-e2e-vsphere-ubuntu-1.29-customfolder, pre-kubermatic-e2e-vsphere-ubuntu-1.29-datastore-cluster

In response to this:

/override pre-kubermatic-e2e-vsphere-ubuntu-1.29
/override pre-kubermatic-e2e-vsphere-ubuntu-1.29-customfolder
/override pre-kubermatic-e2e-vsphere-ubuntu-1.29-datastore-cluster

vSphere has issues.

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.

@kron4eg
Copy link
Member

kron4eg commented Mar 18, 2024

Did you consider setting CA flag --skip-nodes-with-local-storage=false? By default it's true.

@xrstf
Copy link
Contributor Author

xrstf commented Mar 18, 2024

Did you consider setting CA flag --skip-nodes-with-local-storage=false? By default it's true.

You mean setting it to false? That seems risky, I cannot make this decision for any kind of Pod running in any cluster. KKP should IMHO not assume the autoscaler will just ignore local volumes.

@xrstf xrstf requested a review from kron4eg March 20, 2024 15:52
@kron4eg
Copy link
Member

kron4eg commented Mar 20, 2024

/approve
/lgtm

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2024
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 29c3638c4f346a8c61cba09bf6cab52c4811762c

@xrstf
Copy link
Contributor Author

xrstf commented Mar 20, 2024

/approve

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kron4eg, xrstf

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2024
@kubermatic-bot kubermatic-bot merged commit 4d4b55e into kubermatic:main Mar 20, 2024
30 checks passed
@kubermatic-bot kubermatic-bot added this to the KKP 2.25 milestone Mar 20, 2024
@xrstf
Copy link
Contributor Author

xrstf commented Mar 20, 2024

/cherrypick release/v2.25

@kubermatic-bot
Copy link
Contributor

@xrstf: new pull request created: #13196

In response to this:

/cherrypick release/v2.25

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.

@xrstf
Copy link
Contributor Author

xrstf commented Mar 20, 2024

/cherrypick release/v2.24

@xrstf xrstf deleted the evictable-pods branch March 20, 2024 21:53
@kubermatic-bot
Copy link
Contributor

@xrstf: #13180 failed to apply on top of branch "release/v2.24":

Applying: add http-probe tempVolume to safe volume list
Applying: patch apiserver
Applying: be more careful with setting/overwriting label sets
Using index info to reconstruct a base tree...
M	pkg/resources/apiserver/deployment.go
M	pkg/resources/apiserver/is-running.go
M	pkg/resources/cloudcontroller/aws.go
M	pkg/resources/cloudcontroller/azure.go
M	pkg/resources/cloudcontroller/deployment.go
M	pkg/resources/cloudcontroller/digitalocean.go
A	pkg/resources/cloudcontroller/gcp.go
M	pkg/resources/cloudcontroller/hetzner.go
M	pkg/resources/cloudcontroller/openstack.go
M	pkg/resources/cloudcontroller/vsphere.go
M	pkg/resources/dns/dns.go
M	pkg/resources/etcd/statefulset.go
M	pkg/resources/kubernetes-dashboard/deployment.go
M	pkg/resources/machinecontroller/deployment.go
M	pkg/resources/metrics-server/deployment.go
M	pkg/resources/operatingsystemmanager/deployment.go
M	pkg/resources/operatingsystemmanager/webhook.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/resources/operatingsystemmanager/webhook.go
Auto-merging pkg/resources/operatingsystemmanager/deployment.go
Auto-merging pkg/resources/metrics-server/deployment.go
Auto-merging pkg/resources/machinecontroller/deployment.go
Auto-merging pkg/resources/kubernetes-dashboard/deployment.go
Auto-merging pkg/resources/etcd/statefulset.go
Auto-merging pkg/resources/dns/dns.go
Auto-merging pkg/resources/cloudcontroller/vsphere.go
CONFLICT (content): Merge conflict in pkg/resources/cloudcontroller/vsphere.go
Auto-merging pkg/resources/cloudcontroller/openstack.go
Auto-merging pkg/resources/cloudcontroller/hetzner.go
CONFLICT (modify/delete): pkg/resources/cloudcontroller/gcp.go deleted in HEAD and modified in be more careful with setting/overwriting label sets. Version be more careful with setting/overwriting label sets of pkg/resources/cloudcontroller/gcp.go left in tree.
Auto-merging pkg/resources/cloudcontroller/digitalocean.go
Auto-merging pkg/resources/cloudcontroller/deployment.go
CONFLICT (content): Merge conflict in pkg/resources/cloudcontroller/deployment.go
Auto-merging pkg/resources/cloudcontroller/azure.go
Auto-merging pkg/resources/cloudcontroller/aws.go
Auto-merging pkg/resources/apiserver/is-running.go
Auto-merging pkg/resources/apiserver/deployment.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 be more careful with setting/overwriting label sets
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release/v2.24

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/none Denotes a PR that doesn't need documentation (changes). kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/app-management Denotes a PR or issue as being assigned to SIG App Management. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants