From 5b9e4f1e8830b51a266ab8cb9138655e2a29545b Mon Sep 17 00:00:00 2001 From: David McCormick Date: Tue, 16 Oct 2018 11:15:34 +0100 Subject: [PATCH 1/2] Rebase allow updates to pdbs to latest upstream master --- pkg/apis/policy/validation/validation.go | 7 +- pkg/apis/policy/validation/validation_test.go | 15 +-- .../poddisruptionbudget/strategy_test.go | 18 +-- test/e2e/apps/BUILD | 1 + test/e2e/apps/disruption.go | 110 ++++++++++++++---- 5 files changed, 105 insertions(+), 46 deletions(-) diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index b3c90ade846..386d809231a 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -19,7 +19,6 @@ package validation import ( "fmt" "path/filepath" - "reflect" "regexp" "strings" @@ -43,14 +42,10 @@ func ValidatePodDisruptionBudget(pdb *policy.PodDisruptionBudget) field.ErrorLis } func ValidatePodDisruptionBudgetUpdate(pdb, oldPdb *policy.PodDisruptionBudget) field.ErrorList { - allErrs := field.ErrorList{} - restoreGeneration := pdb.Generation pdb.Generation = oldPdb.Generation - if !reflect.DeepEqual(pdb.Spec, oldPdb.Spec) { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to poddisruptionbudget spec are forbidden.")) - } + allErrs := ValidatePodDisruptionBudgetSpec(pdb.Spec, field.NewPath("spec")) allErrs = append(allErrs, ValidatePodDisruptionBudgetStatus(pdb.Status, field.NewPath("status"))...) pdb.Generation = restoreGeneration diff --git a/pkg/apis/policy/validation/validation_test.go b/pkg/apis/policy/validation/validation_test.go index 82b52aaaa5c..259f929b275 100644 --- a/pkg/apis/policy/validation/validation_test.go +++ b/pkg/apis/policy/validation/validation_test.go @@ -140,12 +140,10 @@ func TestValidatePodDisruptionBudgetUpdate(t *testing.T) { generations: []int64{int64(2), int64(3)}, specs: []policy.PodDisruptionBudgetSpec{ { - MinAvailable: &c1, - MaxUnavailable: &c2, + MinAvailable: &c1, }, { - MinAvailable: &c1, - MaxUnavailable: &c2, + MinAvailable: &c1, }, }, status: []policy.PodDisruptionBudgetStatus{ @@ -163,7 +161,7 @@ func TestValidatePodDisruptionBudgetUpdate(t *testing.T) { ok: true, }, { - name: "only update pdb spec", + name: "update pdb spec causing clash", generations: []int64{int64(2), int64(3)}, specs: []policy.PodDisruptionBudgetSpec{ { @@ -192,7 +190,6 @@ func TestValidatePodDisruptionBudgetUpdate(t *testing.T) { MaxUnavailable: &c2, }, { - MinAvailable: &c1, MaxUnavailable: &c3, }, }, @@ -208,7 +205,7 @@ func TestValidatePodDisruptionBudgetUpdate(t *testing.T) { DesiredHealthy: 3, }, }, - ok: false, + ok: true, }, } @@ -219,9 +216,9 @@ func TestValidatePodDisruptionBudgetUpdate(t *testing.T) { pdb.Spec = tc.specs[1] pdb.Generation = tc.generations[1] - oldPdb.Status = tc.status[1] + pdb.Status = tc.status[1] - errs := ValidatePodDisruptionBudgetUpdate(oldPdb, pdb) + errs := ValidatePodDisruptionBudgetUpdate(pdb, oldPdb) if tc.ok && len(errs) > 0 { t.Errorf("[%d:%s] unexpected errors: %v", i, tc.name, errs) } else if !tc.ok && len(errs) == 0 { diff --git a/pkg/registry/policy/poddisruptionbudget/strategy_test.go b/pkg/registry/policy/poddisruptionbudget/strategy_test.go index 579e28c9690..f8ff2c7ef63 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy_test.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy_test.go @@ -68,30 +68,32 @@ func TestPodDisruptionBudgetStrategy(t *testing.T) { t.Errorf("Unexpected error updating PodDisruptionBudget.") } - // Changing the selector? No. + // Changing the selector? OK newPdb.Spec.Selector = &metav1.LabelSelector{MatchLabels: map[string]string{"a": "bar"}} Strategy.PrepareForUpdate(ctx, newPdb, pdb) errs = Strategy.ValidateUpdate(ctx, newPdb, pdb) - if len(errs) == 0 { - t.Errorf("Expected a validation error since updates are disallowed on poddisruptionbudgets.") + if len(errs) != 0 { + t.Errorf("Expected no error on changing selector on poddisruptionbudgets.") } newPdb.Spec.Selector = pdb.Spec.Selector - // Changing MinAvailable? Also no. + // Changing MinAvailable? OK newMinAvailable := intstr.FromString("28%") newPdb.Spec.MinAvailable = &newMinAvailable Strategy.PrepareForUpdate(ctx, newPdb, pdb) errs = Strategy.ValidateUpdate(ctx, newPdb, pdb) - if len(errs) == 0 { - t.Errorf("Expected a validation error since updates are disallowed on poddisruptionbudgets.") + if len(errs) != 0 { + t.Errorf("Expected no error updating MinAvailable on poddisruptionbudgets.") } + // Changing MinAvailable to MaxAvailable? OK maxUnavailable := intstr.FromString("28%") newPdb.Spec.MaxUnavailable = &maxUnavailable + newPdb.Spec.MinAvailable = nil Strategy.PrepareForUpdate(ctx, newPdb, pdb) errs = Strategy.ValidateUpdate(ctx, newPdb, pdb) - if len(errs) == 0 { - t.Errorf("Expected a validation error since updates are disallowed on poddisruptionbudgets.") + if len(errs) != 0 { + t.Errorf("Expected no error updating replacing MinAvailable with MaxUnavailable on poddisruptionbudgets.") } } diff --git a/test/e2e/apps/BUILD b/test/e2e/apps/BUILD index 2b1d22ce6ca..1b4190f9762 100644 --- a/test/e2e/apps/BUILD +++ b/test/e2e/apps/BUILD @@ -59,6 +59,7 @@ go_library( "//staging/src/k8s.io/client-go/kubernetes/scheme:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", "//staging/src/k8s.io/client-go/tools/watch:go_default_library", + "//staging/src/k8s.io/client-go/util/retry:go_default_library", "//test/e2e/common:go_default_library", "//test/e2e/framework:go_default_library", "//test/e2e/framework/deployment:go_default_library", diff --git a/test/e2e/apps/disruption.go b/test/e2e/apps/disruption.go index 645b0b87cd5..61245742e76 100644 --- a/test/e2e/apps/disruption.go +++ b/test/e2e/apps/disruption.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" + "k8s.io/client-go/util/retry" "k8s.io/kubernetes/test/e2e/framework" e2elog "k8s.io/kubernetes/test/e2e/framework/log" imageutils "k8s.io/kubernetes/test/utils/image" @@ -164,23 +165,8 @@ var _ = SIGDescribe("DisruptionController", func() { } // Locate a running pod. - var pod v1.Pod - err := wait.PollImmediate(framework.Poll, schedulingTimeout, func() (bool, error) { - podList, err := cs.CoreV1().Pods(ns).List(metav1.ListOptions{}) - if err != nil { - return false, err - } - - for i := range podList.Items { - if podList.Items[i].Status.Phase == v1.PodRunning { - pod = podList.Items[i] - return true, nil - } - } - - return false, nil - }) - framework.ExpectNoError(err) + pod, err := locateRunningPod(cs, ns) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) e := &policy.Eviction{ ObjectMeta: metav1.ObjectMeta{ @@ -190,10 +176,6 @@ var _ = SIGDescribe("DisruptionController", func() { } if c.shouldDeny { - // Since disruptionAllowed starts out false, wait at least 60s hoping that - // this gives the controller enough time to have truly set the status. - time.Sleep(timeout) - err = cs.CoreV1().Pods(ns).Evict(e) gomega.Expect(err).Should(gomega.MatchError("Cannot evict pod as it would violate the pod's disruption budget.")) } else { @@ -215,6 +197,34 @@ var _ = SIGDescribe("DisruptionController", func() { } }) } + + ginkgo.It("should block an eviction until the PDB is updated to allow it", func() { + ginkgo.By("Creating a pdb that targets all three pods in a test replica set") + createPDBMinAvailableOrDie(cs, ns, intstr.FromInt(3)) + createReplicaSetOrDie(cs, ns, 3, false) + + ginkgo.By("First trying to evict a pod which shouldn't be evictable") + pod, err := locateRunningPod(cs, ns) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + waitForPodsOrDie(cs, ns, 3) // make sure that they are running and so would be evictable with a different pdb + e := &policy.Eviction{ + ObjectMeta: metav1.ObjectMeta{ + Name: pod.Name, + Namespace: ns, + }, + } + err = cs.CoreV1().Pods(ns).Evict(e) + gomega.Expect(err).Should(gomega.MatchError("Cannot evict pod as it would violate the pod's disruption budget.")) + + ginkgo.By("Updating the pdb to allow a pod to be evicted") + updatePDBMinAvailableOrDie(cs, ns, intstr.FromInt(2)) + + ginkgo.By("Trying to evict the same pod we tried earlier which should now be evictable") + waitForPodsOrDie(cs, ns, 3) + err = cs.CoreV1().Pods(ns).Evict(e) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) // the eviction is now allowed + }) }) func createPDBMinAvailableOrDie(cs kubernetes.Interface, ns string, minAvailable intstr.IntOrString) { @@ -229,7 +239,8 @@ func createPDBMinAvailableOrDie(cs kubernetes.Interface, ns string, minAvailable }, } _, err := cs.PolicyV1beta1().PodDisruptionBudgets(ns).Create(&pdb) - framework.ExpectNoError(err) + framework.ExpectNoError(err, "Waiting for the pdb to be created with minAvailable %d in namespace %s", minAvailable.IntVal, ns) + waitForPdbToBeProcessed(cs, ns) } func createPDBMaxUnavailableOrDie(cs kubernetes.Interface, ns string, maxUnavailable intstr.IntOrString) { @@ -244,7 +255,25 @@ func createPDBMaxUnavailableOrDie(cs kubernetes.Interface, ns string, maxUnavail }, } _, err := cs.PolicyV1beta1().PodDisruptionBudgets(ns).Create(&pdb) - framework.ExpectNoError(err) + framework.ExpectNoError(err, "Waiting for the pdb to be created with maxUnavailable %d in namespace %s", maxUnavailable.IntVal, ns) + waitForPdbToBeProcessed(cs, ns) +} + +func updatePDBMinAvailableOrDie(cs kubernetes.Interface, ns string, minAvailable intstr.IntOrString) { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + old, err := cs.PolicyV1beta1().PodDisruptionBudgets(ns).Get("foo", metav1.GetOptions{}) + if err != nil { + return err + } + old.Spec.MinAvailable = &minAvailable + if _, err := cs.PolicyV1beta1().PodDisruptionBudgets(ns).Update(old); err != nil { + return err + } + return nil + }) + + framework.ExpectNoError(err, "Waiting for the pdb update to be processed in namespace %s", ns) + waitForPdbToBeProcessed(cs, ns) } func createPodsOrDie(cs kubernetes.Interface, ns string, n int) { @@ -335,3 +364,38 @@ func createReplicaSetOrDie(cs kubernetes.Interface, ns string, size int32, exclu _, err := cs.AppsV1().ReplicaSets(ns).Create(rs) framework.ExpectNoError(err, "Creating replica set %q in namespace %q", rs.Name, ns) } + +func locateRunningPod(cs kubernetes.Interface, ns string) (pod *v1.Pod, err error) { + ginkgo.By("locating a running pod") + err = wait.PollImmediate(framework.Poll, schedulingTimeout, func() (bool, error) { + podList, err := cs.CoreV1().Pods(ns).List(metav1.ListOptions{}) + if err != nil { + return false, err + } + + for i := range podList.Items { + if podList.Items[i].Status.Phase == v1.PodRunning { + pod = &podList.Items[i] + return true, nil + } + } + + return false, nil + }) + return pod, err +} + +func waitForPdbToBeProcessed(cs kubernetes.Interface, ns string) { + ginkgo.By("Waiting for the pdb to be processed") + err := wait.PollImmediate(framework.Poll, schedulingTimeout, func() (bool, error) { + pdb, err := cs.PolicyV1beta1().PodDisruptionBudgets(ns).Get("foo", metav1.GetOptions{}) + if err != nil { + return false, err + } + if pdb.Status.ObservedGeneration < pdb.Generation { + return false, nil + } + return true, nil + }) + framework.ExpectNoError(err, "Waiting for the pdb to be processed in namespace %s", ns) +} From 3537eed826c5e3d4b329d73e156e45ec165fe513 Mon Sep 17 00:00:00 2001 From: David McCormick Date: Fri, 26 Apr 2019 10:36:54 +0100 Subject: [PATCH 2/2] Remove the generation altering code - validate an update for a PDB by running ValidatePodDisruptionBudget only. --- pkg/apis/policy/validation/validation.go | 11 -- pkg/apis/policy/validation/validation_test.go | 105 ------------------ .../policy/poddisruptionbudget/strategy.go | 4 +- 3 files changed, 1 insertion(+), 119 deletions(-) diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index 386d809231a..2d880a20968 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -41,17 +41,6 @@ func ValidatePodDisruptionBudget(pdb *policy.PodDisruptionBudget) field.ErrorLis return allErrs } -func ValidatePodDisruptionBudgetUpdate(pdb, oldPdb *policy.PodDisruptionBudget) field.ErrorList { - restoreGeneration := pdb.Generation - pdb.Generation = oldPdb.Generation - - allErrs := ValidatePodDisruptionBudgetSpec(pdb.Spec, field.NewPath("spec")) - allErrs = append(allErrs, ValidatePodDisruptionBudgetStatus(pdb.Status, field.NewPath("status"))...) - - pdb.Generation = restoreGeneration - return allErrs -} - func ValidatePodDisruptionBudgetSpec(spec policy.PodDisruptionBudgetSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} diff --git a/pkg/apis/policy/validation/validation_test.go b/pkg/apis/policy/validation/validation_test.go index 259f929b275..c390097bcc3 100644 --- a/pkg/apis/policy/validation/validation_test.go +++ b/pkg/apis/policy/validation/validation_test.go @@ -122,111 +122,6 @@ func TestValidatePodDisruptionBudgetStatus(t *testing.T) { } } -func TestValidatePodDisruptionBudgetUpdate(t *testing.T) { - c1 := intstr.FromString("10%") - c2 := intstr.FromInt(1) - c3 := intstr.FromInt(2) - oldPdb := &policy.PodDisruptionBudget{} - pdb := &policy.PodDisruptionBudget{} - testCases := []struct { - generations []int64 - name string - specs []policy.PodDisruptionBudgetSpec - status []policy.PodDisruptionBudgetStatus - ok bool - }{ - { - name: "only update status", - generations: []int64{int64(2), int64(3)}, - specs: []policy.PodDisruptionBudgetSpec{ - { - MinAvailable: &c1, - }, - { - MinAvailable: &c1, - }, - }, - status: []policy.PodDisruptionBudgetStatus{ - { - PodDisruptionsAllowed: 10, - CurrentHealthy: 5, - ExpectedPods: 2, - }, - { - PodDisruptionsAllowed: 8, - CurrentHealthy: 5, - DesiredHealthy: 3, - }, - }, - ok: true, - }, - { - name: "update pdb spec causing clash", - generations: []int64{int64(2), int64(3)}, - specs: []policy.PodDisruptionBudgetSpec{ - { - MaxUnavailable: &c2, - }, - { - MinAvailable: &c1, - MaxUnavailable: &c3, - }, - }, - status: []policy.PodDisruptionBudgetStatus{ - { - PodDisruptionsAllowed: 10, - }, - { - PodDisruptionsAllowed: 10, - }, - }, - ok: false, - }, - { - name: "update spec and status", - generations: []int64{int64(2), int64(3)}, - specs: []policy.PodDisruptionBudgetSpec{ - { - MaxUnavailable: &c2, - }, - { - MaxUnavailable: &c3, - }, - }, - status: []policy.PodDisruptionBudgetStatus{ - { - PodDisruptionsAllowed: 10, - CurrentHealthy: 5, - ExpectedPods: 2, - }, - { - PodDisruptionsAllowed: 8, - CurrentHealthy: 5, - DesiredHealthy: 3, - }, - }, - ok: true, - }, - } - - for i, tc := range testCases { - oldPdb.Spec = tc.specs[0] - oldPdb.Generation = tc.generations[0] - oldPdb.Status = tc.status[0] - - pdb.Spec = tc.specs[1] - pdb.Generation = tc.generations[1] - pdb.Status = tc.status[1] - - errs := ValidatePodDisruptionBudgetUpdate(pdb, oldPdb) - if tc.ok && len(errs) > 0 { - t.Errorf("[%d:%s] unexpected errors: %v", i, tc.name, errs) - } else if !tc.ok && len(errs) == 0 { - t.Errorf("[%d:%s] expected errors: %v", i, tc.name, errs) - } - } -} - func TestValidatePodSecurityPolicy(t *testing.T) { validPSP := func() *policy.PodSecurityPolicy { return &policy.PodSecurityPolicy{ diff --git a/pkg/registry/policy/poddisruptionbudget/strategy.go b/pkg/registry/policy/poddisruptionbudget/strategy.go index 0ee30f3b9d7..5a4f8dc2122 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy.go @@ -83,9 +83,7 @@ func (podDisruptionBudgetStrategy) AllowCreateOnUpdate() bool { // ValidateUpdate is the default update validation for an end user. func (podDisruptionBudgetStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - validationErrorList := validation.ValidatePodDisruptionBudget(obj.(*policy.PodDisruptionBudget)) - updateErrorList := validation.ValidatePodDisruptionBudgetUpdate(obj.(*policy.PodDisruptionBudget), old.(*policy.PodDisruptionBudget)) - return append(validationErrorList, updateErrorList...) + return validation.ValidatePodDisruptionBudget(obj.(*policy.PodDisruptionBudget)) } // AllowUnconditionalUpdate is the default update policy for PodDisruptionBudget objects. Status update should