Add a pod disruption budget for ingress controllers by Miciah · Pull Request #272 · openshift/cluster-ingress-operator

Miciah · 2019-07-22T21:27:42Z

For each ingress controller, create a pod disruption budget with a maximum of 25% unavailable pods.

manifests/00-cluster-role.yaml: Allow the operator to manage PodDisruptionBudget resources.
pkg/operator/controller/ingress/controller.go (ensureIngressController): Call ensureRouterPodDisruptionBudget.
pkg/operator/controller/ingress/poddisruptionbudget.go: New file.
(ensureRouterPodDisruptionBudget): New method. Ensure the appropriate pod disruption budget exists for the given ingress controller. Get the current one using currentRouterPodDisruptionBudget. If none exists, create one using the desiredRouterPodDisruptionBudget function and createRouterPodDisruptionBudget method. If one already exists, update it if necessary using updateRouterPodDisruptionBudget, or delete it using deleteRouterPodDisruptionBudget if none is desired.
(desiredRouterPodDisruptionBudget): New function.
(currentRouterPodDisruptionBudget): New method.
(createRouterPodDisruptionBudget): New method.
(deleteRouterPodDisruptionBudget): New method.
(updateRouterPodDisruptionBudget): New method. Use podDisruptionBudgetChanged to determine whether an update is needed.
(podDisruptionBudgetChanged): New function.
pkg/operator/controller/names.go (RouterPodDisruptionBudgetName): New function.
test/e2e/operator_test.go (TestPodDisruptionBudgetExists): New test. Verify that a PodDisruptionBudget resource exists for the default ingresscontroller.

This PR is a spin-off from #240.

ironcladlou · 2019-07-22T21:32:22Z

pkg/operator/controller/names.go

+func RouterPodDisruptionBudgetName(ic *operatorv1.IngressController) types.NamespacedName {
+	return types.NamespacedName{
+		Namespace: "openshift-ingress",
+		Name:      "router-" + ic.Name,


using the word "router" in any of our resources was a mistake from the start — could we avoid propagating it further?

We could, but this would be the odd resource out till we renamed other resources, and I am not aware of plans to rename other resources. What name do you propose? Just use the ingresscontroller's name, or prefix "ingress-" or "ingresscontroller-" or similar to the name?

I like "ingress-", but if you want to just stay consistent with router I'm fine with that too at this point

ironcladlou · 2019-07-22T21:33:24Z

Any value in trying to devise an e2e test for this?

Miciah · 2019-07-22T22:09:07Z

Any value in trying to devise an e2e test for this?

Probably. Should it be anything more sophisticated than checking that a pod disruption budget exists for the default ingress controller?

Miciah · 2019-07-23T17:59:34Z

/retest

ironcladlou · 2019-07-23T18:34:13Z

Probably. Should it be anything more sophisticated than checking that a pod disruption budget exists for the default ingress controller?

Would that be much more useful than a unit test?

At the end of the day I think we can trust the disruption budget feature in k8s itself works, so if we know we're setting the right fields on the deployment, maybe that's enough?

Miciah · 2019-07-23T20:45:12Z

Would that be much more useful than a unit test?

At the end of the day I think we can trust the disruption budget feature in k8s itself works, so if we know we're setting the right fields on the deployment, maybe that's enough?

PodDisuptionBudget is a discrete API resource, so it makes sense to make sure it is created, but it makes sense to defer to k8s tests to verify that the general disruption budget feature works properly, I agree.

ironcladlou · 2019-07-26T00:05:15Z

pkg/operator/controller/ingress/poddisruptionbudget.go

+// Boolean indicating whether the PDB was created, and an error value.
+func (r *reconciler) createRouterPodDisruptionBudget(pdb *policyv1beta1.PodDisruptionBudget) (bool, error) {
+	if err := r.client.Create(context.TODO(), pdb); err != nil {
+		return false, err


What if the error is because the resource already exists?

ensureRouterPodDisruptionBudget should call createRouterPodDisruptionBudget only if the resource did not already exist. Morever, returning a non-nil error value should just cause a retry of the reconciliation, which should be reasonably innocuous. We've been removing similar IsAlreadyExists checks in PRs such as #151 and #184.

ironcladlou · 2019-07-26T00:05:59Z

pkg/operator/controller/ingress/poddisruptionbudget.go

+		}
+		return false, err
+	}
+	log.Info("created pod disruption budget", "namespace", pdb.Namespace, "name", pdb.Name)


Says created when it means deleted, and the logging is already happening on the calling side anyway

Deleted. Thanks!

ironcladlou · 2019-07-26T00:06:17Z

pkg/operator/controller/ingress/poddisruptionbudget.go

+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return false, err
+	}
+	log.Info("updated pod disruption budget", "namespace", updated.Namespace, "name", updated.Name)


Logging is happening on the calling side already

ironcladlou · 2019-07-26T00:06:34Z

pkg/operator/controller/ingress/poddisruptionbudget.go

+	if err := r.client.Create(context.TODO(), pdb); err != nil {
+		return false, err
+	}
+	log.Info("created pod disruption budget", "namespace", pdb.Namespace, "name", pdb.Name)


Logging is happening on the calling side already

For each ingress controller, create a pod disruption budget with a maximum of 25% unavailable pods. * manifests/00-cluster-role.yaml: Allow the operator to manage PodDisruptionBudget resources. * pkg/operator/controller/ingress/controller.go (ensureIngressController): Call ensureRouterPodDisruptionBudget. * pkg/operator/controller/ingress/poddisruptionbudget.go: New file. (ensureRouterPodDisruptionBudget): New method. Ensure the appropriate pod disruption budget exists for the given ingress controller. Get the current one using currentRouterPodDisruptionBudget. If none exists, create one using the desiredRouterPodDisruptionBudget function and createRouterPodDisruptionBudget method. If one already exists, update it if necessary using updateRouterPodDisruptionBudget, or delete it using deleteRouterPodDisruptionBudget if none is desired. (desiredRouterPodDisruptionBudget): New function. (currentRouterPodDisruptionBudget): New method. (createRouterPodDisruptionBudget): New method. (deleteRouterPodDisruptionBudget): New method. (updateRouterPodDisruptionBudget): New method. Use podDisruptionBudgetChanged to determine whether an update is needed. (podDisruptionBudgetChanged): New function. * pkg/operator/controller/names.go (RouterPodDisruptionBudgetName): New function. * test/e2e/operator_test.go (TestPodDisruptionBudgetExists): New test. Verify that a PodDisruptionBudget resource exists for the default ingresscontroller.

ironcladlou · 2019-07-26T14:52:48Z

/lgtm

openshift-ci-robot · 2019-07-26T14:52:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah,ironcladlou]

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

openshift-ci-robot requested review from ironcladlou and knobunc July 22, 2019 21:27

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 22, 2019

ironcladlou reviewed Jul 22, 2019

View reviewed changes

Miciah force-pushed the add-poddisruptionbudget branch from f97ff92 to 03f16d6 Compare July 22, 2019 22:34

ironcladlou reviewed Jul 26, 2019

View reviewed changes

Miciah force-pushed the add-poddisruptionbudget branch from 03f16d6 to 35d4f4f Compare July 26, 2019 04:34

openshift-ci-robot assigned ironcladlou Jul 26, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2019

openshift-merge-robot merged commit 35a7942 into openshift:master Jul 26, 2019

Miciah mentioned this pull request Mar 14, 2020

Delete pod disruption budget logic #376

Merged

Conversation

Miciah commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Jul 22, 2019

Uh oh!

Miciah commented Jul 22, 2019

Uh oh!

Miciah commented Jul 23, 2019

Uh oh!

ironcladlou commented Jul 23, 2019

Uh oh!

Miciah commented Jul 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Jul 26, 2019

Uh oh!

openshift-ci-robot commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Miciah commented Jul 22, 2019 •

edited

Loading