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) +}