Chart: Deploy `PodDisruptionBudget` with KEDA. #11032

StuxxNet · 2024-02-27T18:20:01Z

What this PR does / why we need it:

Currently we deploy the PDB only if Keda is enabled. This PR tries to address this adding a new entry to the "if" statement to make it consider Keda also.

Types of changes

New feature (non-breaking change which adds functionality)

Which issue/s this PR fixes

fixes #10083

How Has This Been Tested?

The testing was done by creating a fresh deployment of the Ingress Nginx in a brand new Kind cluster running Kubernetes 1.29. Below you can see that the corresponding PDB object was created when the values for Keda were passed to the chart:

➜  ingress-nginx git:(feat/ISSUE-10083-add-pdb-when-keda-in-place) ✗ kubectl get pods 
NAME                                        READY   STATUS    RESTARTS   AGE
ingress-nginx-controller-76bf7c9746-h4kmk   1/1     Running   0          12s
ingress-nginx-controller-76bf7c9746-qv82w   1/1     Running   0          15m

➜  ingress-nginx git:(feat/ISSUE-10083-add-pdb-when-keda-in-place) ✗ kubectl get scaledobject
NAME                       SCALETARGETKIND      SCALETARGETNAME            MIN   MAX   TRIGGERS     AUTHENTICATION   READY   ACTIVE   FALLBACK   PAUSED    AGE
ingress-nginx-controller   apps/v1.Deployment   ingress-nginx-controller   2     11    prometheus                    False   False    False      Unknown   15m

➜  ingress-nginx git:(feat/ISSUE-10083-add-pdb-when-keda-in-place) ✗ kubectl get pdb 
NAME                       MIN AVAILABLE   MAX UNAVAILABLE   ALLOWED DISRUPTIONS   AGE
ingress-nginx-controller   1               N/A               1                     41s

➜  ingress-nginx git:(feat/ISSUE-10083-add-pdb-when-keda-in-place) ✗ kubectl drain kind-control-plane --ignore-daemonsets 
node/kind-control-plane already cordoned
Warning: ignoring DaemonSet-managed Pods: kube-system/kindnet-bbq8w, kube-system/kube-proxy-f9xh7
evicting pod local-path-storage/local-path-provisioner-7577fdbbfb-trlrz
evicting pod default/ingress-nginx-controller-76bf7c9746-j64mh
evicting pod keda/keda-operator-5c77476f9b-jkrdh
evicting pod default/ingress-nginx-controller-76bf7c9746-p6wcn
evicting pod keda/keda-admission-webhooks-6987d68f4c-m4mr2
evicting pod kube-system/coredns-76f75df574-f498s
evicting pod kube-system/coredns-76f75df574-zrx2q
evicting pod keda/keda-operator-metrics-apiserver-7c4675769c-96bht
error when evicting pods/"ingress-nginx-controller-76bf7c9746-p6wcn" -n "default" (will retry after 5s): Cannot evict pod as it would violate the pod's disruption budget.
pod/keda-operator-5c77476f9b-jkrdh evicted
pod/keda-admission-webhooks-6987d68f4c-m4mr2 evicted
evicting pod default/ingress-nginx-controller-76bf7c9746-p6wcn
error when evicting pods/"ingress-nginx-controller-76bf7c9746-p6wcn" -n "default" (will retry after 5s): Cannot evict pod as it would violate the pod's disruption budget.
pod/coredns-76f75df574-zrx2q evicted
pod/coredns-76f75df574-f498s evicted

Checklist:

I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.

linux-foundation-easycla · 2024-02-27T18:20:05Z

The committers listed above are authorized under a signed CLA.

✅ login: StuxxNet / name: Ramon Borges (5f194a7, 29c4237, 6480e14, b869ebc, 97a4f84, 17ee81c)

k8s-ci-robot · 2024-02-27T18:20:10Z

Welcome @StuxxNet!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-02-27T18:20:11Z

Hi @StuxxNet. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2024-02-27T18:20:19Z

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	`17ee81c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/65ef54ae39f1a1000880a1d6

rikatz · 2024-02-27T18:24:16Z

/ok-to-test

…s is greater than 1

Gacko · 2024-02-27T20:27:51Z

/assign

Gacko · 2024-02-27T20:35:45Z

/hold

Gacko · 2024-02-27T20:42:11Z

/retitle Chart: Deploy PodDisruptionBudget with KEDA.

Gacko · 2024-02-27T20:43:16Z

charts/ingress-nginx/tests/controller-keda_pdb_test.yaml

+          value: RELEASE-NAME-ingress-nginx-controller
+      - equal:
+          path: spec.minAvailable
+          value: 1


Suggested change

value: 1

value: 1

This suggestions adds the requite line feed at the end of the file.

Also this was addressed in the last commit where the file was renamed :)

Gacko · 2024-02-27T20:43:37Z

charts/ingress-nginx/tests/controller-keda_pdb_test.yaml

+          value: RELEASE-NAME-ingress-nginx-controller
+      - equal:
+          path: spec.minAvailable
+          value: 1


This suggestions adds the requite line feed at the end of the file.

Gacko · 2024-02-27T20:44:25Z

charts/ingress-nginx/tests/controller-keda_pdb_test.yaml

Could you please rename the file to charts/ingress-nginx/tests/controller-poddisruptionbudget_test.yaml. We used to have them named like the resources they are testing.

Hey! May you please check if the naming is correct now? :D

charts/ingress-nginx/templates/controller-poddisruptionbudget.yaml

Gacko · 2024-02-27T21:09:22Z

charts/ingress-nginx/tests/controller-poddisruptionbudget_test.yaml

@@ -0,0 +1,22 @@
+suite: Controller > Keda > PDB


Suggested change

suite: Controller > Keda > PDB

suite: Controller > PodDisruptionBudget

Also addressed!

charts/ingress-nginx/templates/controller-poddisruptionbudget.yaml

charts/ingress-nginx/tests/controller-poddisruptionbudget_test.yaml

…yaml Co-authored-by: Marco Ebert <marco_ebert@icloud.com>

….yaml Co-authored-by: Marco Ebert <marco_ebert@icloud.com>

StuxxNet · 2024-03-11T19:01:04Z

@Gacko Hey! Thanks for the review. Indeed it looks way much simple now than it was before. Thanks for it! I just accepted the proposed changes :D

Gacko · 2024-03-11T19:05:09Z

/unhold
/approve
/cherry-pick release-1.10

Thanks a lot for your contribution and the nice collaboration! :)

k8s-infra-cherrypick-robot · 2024-03-11T19:05:12Z

@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.

In response to this:

/unhold
/approve
/cherry-pick release-1.10

Thanks a lot for your contribution and the nice collaboration! :)

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.

Gacko · 2024-03-11T19:05:37Z

/kind bug
/priority backlog
/triage accepted

cpanato

Comment for clarification

cpanato · 2024-03-11T21:01:06Z

charts/ingress-nginx/templates/controller-poddisruptionbudget.yaml

+{{- if .Values.controller.autoscaling.enabled }}
+{{- $replicas = .Values.controller.autoscaling.minReplicas }}
+{{- else if .Values.controller.keda.enabled }}
+{{- $replicas = .Values.controller.keda.minReplicas }}


Don't we need to add some docs for this?

What if the user enable both by mistake?
Can we try to use the OR instead or that does not work?

/hold for clarification

Hm, yeah, I get your point. HPA and KEDA being mutually exclusive always was a problem and looking at the particular resource templates, we are not even preventing that in there.

So right now what you see above is pretty close the best we can get without making the conditions too complex and unreadable.

I created an issue for improving and aligning that everywhere. But for now, I'd just go on and merge this PR. Aligning all these conditions is out of scope here.

That's the issue I created: #10814

cpanato

Sgtm
/approve
/lgtm
/unhold

Thanks for this change!

Thanks @Gacko for the clarification

k8s-ci-robot · 2024-03-11T21:11:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, Gacko, StuxxNet

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:

~~charts/ingress-nginx/OWNERS~~ [Gacko,cpanato]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-infra-cherrypick-robot · 2024-03-11T21:15:21Z

@Gacko: new pull request created: #11105

In response to this:

/unhold
/approve
/cherry-pick release-1.10

Thanks a lot for your contribution and the nice collaboration! :)

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.

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 27, 2024

k8s-ci-robot added area/helm Issues or PRs related to helm charts needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 27, 2024

k8s-ci-robot requested review from cpanato and Gacko February 27, 2024 18:20

k8s-ci-robot added needs-priority size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 27, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 27, 2024

feat: deploy PDB if Keda is enabled and the minimum amount of replica…

29c4237

…s is greater than 1

rikatz mentioned this pull request Feb 27, 2024

feat: Updated PDB template to support Keda #10136

Closed

10 tasks

feat: add the corresponding unit-test to check PDB deployment with Keda

5f194a7

StuxxNet force-pushed the feat/ISSUE-10083-add-pdb-when-keda-in-place branch from e12c9d3 to 5f194a7 Compare February 27, 2024 18:38

k8s-ci-robot assigned Gacko Feb 27, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2024

k8s-ci-robot changed the title ~~Deploy PodDisruptionBudget if Keda is enabled~~ Chart: Deploy PodDisruptionBudget with KEDA. Feb 27, 2024

Gacko reviewed Feb 27, 2024

View reviewed changes

chore: update the test-case suite name to the new format

b869ebc

Gacko reviewed Mar 11, 2024

View reviewed changes

charts/ingress-nginx/templates/controller-poddisruptionbudget.yaml Outdated Show resolved Hide resolved

charts/ingress-nginx/tests/controller-poddisruptionbudget_test.yaml Outdated Show resolved Hide resolved

StuxxNet and others added 2 commits March 11, 2024 19:59

Update charts/ingress-nginx/templates/controller-poddisruptionbudget.…

6480e14

…yaml Co-authored-by: Marco Ebert <marco_ebert@icloud.com>

Update charts/ingress-nginx/tests/controller-poddisruptionbudget_test…

17ee81c

….yaml Co-authored-by: Marco Ebert <marco_ebert@icloud.com>

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 11, 2024

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 11, 2024

cpanato reviewed Mar 11, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2024

cpanato approved these changes Mar 11, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2024

k8s-ci-robot assigned cpanato Mar 11, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2024

k8s-ci-robot merged commit b5c4476 into kubernetes:main Mar 11, 2024
17 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Mar 11, 2024

Chart: Deploy PodDisruptionBudget with KEDA. #11105

Merged

StuxxNet deleted the feat/ISSUE-10083-add-pdb-when-keda-in-place branch March 11, 2024 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chart: Deploy `PodDisruptionBudget` with KEDA. #11032

Chart: Deploy `PodDisruptionBudget` with KEDA. #11032

StuxxNet commented Feb 27, 2024 •

edited

linux-foundation-easycla bot commented Feb 27, 2024 •

edited

k8s-ci-robot commented Feb 27, 2024

k8s-ci-robot commented Feb 27, 2024

netlify bot commented Feb 27, 2024 •

edited

rikatz commented Feb 27, 2024

Gacko commented Feb 27, 2024

Gacko commented Feb 27, 2024

Gacko commented Feb 27, 2024

Gacko Feb 27, 2024

Gacko Feb 27, 2024

StuxxNet Feb 27, 2024

Gacko Feb 27, 2024

Gacko Feb 27, 2024

StuxxNet Feb 27, 2024

Gacko Feb 27, 2024

StuxxNet Feb 27, 2024

StuxxNet commented Mar 11, 2024

Gacko commented Mar 11, 2024

k8s-infra-cherrypick-robot commented Mar 11, 2024

Gacko commented Mar 11, 2024

cpanato left a comment

cpanato Mar 11, 2024

Gacko Mar 11, 2024

Gacko Mar 11, 2024

cpanato left a comment

k8s-ci-robot commented Mar 11, 2024

k8s-infra-cherrypick-robot commented Mar 11, 2024

	suite: Controller > Keda > PDB
	suite: Controller > PodDisruptionBudget

Chart: Deploy PodDisruptionBudget with KEDA. #11032

Chart: Deploy PodDisruptionBudget with KEDA. #11032

Conversation

StuxxNet commented Feb 27, 2024 • edited

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Feb 27, 2024 • edited

k8s-ci-robot commented Feb 27, 2024

k8s-ci-robot commented Feb 27, 2024

netlify bot commented Feb 27, 2024 • edited

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

rikatz commented Feb 27, 2024

Gacko commented Feb 27, 2024

Gacko commented Feb 27, 2024

Gacko commented Feb 27, 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

StuxxNet commented Mar 11, 2024

Gacko commented Mar 11, 2024

k8s-infra-cherrypick-robot commented Mar 11, 2024

Gacko commented Mar 11, 2024

cpanato left a 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

cpanato left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 11, 2024

k8s-infra-cherrypick-robot commented Mar 11, 2024

Chart: Deploy `PodDisruptionBudget` with KEDA. #11032

Chart: Deploy `PodDisruptionBudget` with KEDA. #11032

StuxxNet commented Feb 27, 2024 •

edited

linux-foundation-easycla bot commented Feb 27, 2024 •

edited

netlify bot commented Feb 27, 2024 •

edited