From ce48d4fb5cbccd58611d8db4954f21a6e4ef68c6 Mon Sep 17 00:00:00 2001 From: Anirudh Date: Wed, 10 May 2017 04:52:33 -0700 Subject: [PATCH] PDB MaxUnavailable: Disruption Controller Changes --- pkg/controller/disruption/disruption.go | 132 +++++++++++-------- pkg/controller/disruption/disruption_test.go | 91 +++++++++++-- 2 files changed, 157 insertions(+), 66 deletions(-) diff --git a/pkg/controller/disruption/disruption.go b/pkg/controller/disruption/disruption.go index 0ce105cb592..116b5d50361 100644 --- a/pkg/controller/disruption/disruption.go +++ b/pkg/controller/disruption/disruption.go @@ -516,64 +516,88 @@ func (dc *DisruptionController) getExpectedPodCount(pdb *policy.PodDisruptionBud // permitted controller configurations (specifically, considering it an error // if a pod covered by a PDB has 0 controllers or > 1 controller) should be // handled the same way for integer and percentage minAvailable - if pdb.Spec.MinAvailable.Type == intstr.Int { - desiredHealthy = pdb.Spec.MinAvailable.IntVal - expectedCount = int32(len(pods)) - } else if pdb.Spec.MinAvailable.Type == intstr.String { - // When the user specifies a fraction of pods that must be available, we - // use as the fraction's denominator - // SUM_{all c in C} scale(c) - // where C is the union of C_p1, C_p2, ..., C_pN - // and each C_pi is the set of controllers controlling the pod pi - // k8s only defines what will happens when 0 or 1 controllers control a - // given pod. We explicitly exclude the 0 controllers case here, and we - // report an error if we find a pod with more than 1 controller. Thus in - // practice each C_pi is a set of exactly 1 controller. - - // A mapping from controllers to their scale. - controllerScale := map[types.UID]int32{} - - // 1. Find the controller(s) for each pod. If any pod has 0 controllers, - // that's an error. If any pod has more than 1 controller, that's also an - // error. - for _, pod := range pods { - controllerCount := 0 - for _, finder := range dc.finders() { - var controllers []controllerAndScale - controllers, err = finder(pod) - if err != nil { - return - } - for _, controller := range controllers { - controllerScale[controller.UID] = controller.scale - controllerCount++ - } - } - if controllerCount == 0 { - err = fmt.Errorf("asked for percentage, but found no controllers for pod %q", pod.Name) - dc.recorder.Event(pdb, v1.EventTypeWarning, "NoControllers", err.Error()) - return - } else if controllerCount > 1 { - err = fmt.Errorf("pod %q has %v>1 controllers", pod.Name, controllerCount) - dc.recorder.Event(pdb, v1.EventTypeWarning, "TooManyControllers", err.Error()) - return - } - } - - // 2. Add up all the controllers. - expectedCount = 0 - for _, count := range controllerScale { - expectedCount += count - } - - // 3. Do the math. - var dh int - dh, err = intstr.GetValueFromIntOrPercent(&pdb.Spec.MinAvailable, int(expectedCount), true) + if pdb.Spec.MaxUnavailable != nil { + expectedCount, err = dc.getExpectedScale(pdb, pods) if err != nil { return } - desiredHealthy = int32(dh) + var maxUnavailable int + maxUnavailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MaxUnavailable, int(expectedCount), true) + if err != nil { + return + } + desiredHealthy = expectedCount - int32(maxUnavailable) + if desiredHealthy < 0 { + desiredHealthy = 0 + } + } else if pdb.Spec.MinAvailable != nil { + if pdb.Spec.MinAvailable.Type == intstr.Int { + desiredHealthy = pdb.Spec.MinAvailable.IntVal + expectedCount = int32(len(pods)) + } else if pdb.Spec.MinAvailable.Type == intstr.String { + expectedCount, err = dc.getExpectedScale(pdb, pods) + if err != nil { + return + } + + var minAvailable int + minAvailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true) + if err != nil { + return + } + desiredHealthy = int32(minAvailable) + } + } + return +} + +func (dc *DisruptionController) getExpectedScale(pdb *policy.PodDisruptionBudget, pods []*v1.Pod) (expectedCount int32, err error) { + // When the user specifies a fraction of pods that must be available, we + // use as the fraction's denominator + // SUM_{all c in C} scale(c) + // where C is the union of C_p1, C_p2, ..., C_pN + // and each C_pi is the set of controllers controlling the pod pi + + // k8s only defines what will happens when 0 or 1 controllers control a + // given pod. We explicitly exclude the 0 controllers case here, and we + // report an error if we find a pod with more than 1 controller. Thus in + // practice each C_pi is a set of exactly 1 controller. + + // A mapping from controllers to their scale. + controllerScale := map[types.UID]int32{} + + // 1. Find the controller(s) for each pod. If any pod has 0 controllers, + // that's an error. If any pod has more than 1 controller, that's also an + // error. + for _, pod := range pods { + controllerCount := 0 + for _, finder := range dc.finders() { + var controllers []controllerAndScale + controllers, err = finder(pod) + if err != nil { + return + } + for _, controller := range controllers { + controllerScale[controller.UID] = controller.scale + controllerCount++ + } + } + if controllerCount == 0 { + err = fmt.Errorf("found no controllers for pod %q", pod.Name) + dc.recorder.Event(pdb, v1.EventTypeWarning, "NoControllers", err.Error()) + return + } else if controllerCount > 1 { + err = fmt.Errorf("pod %q has %v>1 controllers", pod.Name, controllerCount) + dc.recorder.Event(pdb, v1.EventTypeWarning, "TooManyControllers", err.Error()) + return + } + } + + // 2. Add up all the controllers. + expectedCount = 0 + for _, count := range controllerScale { + expectedCount += count } return diff --git a/pkg/controller/disruption/disruption_test.go b/pkg/controller/disruption/disruption_test.go index b729cf528aa..cd472ea3588 100644 --- a/pkg/controller/disruption/disruption_test.go +++ b/pkg/controller/disruption/disruption_test.go @@ -35,6 +35,8 @@ import ( policy "k8s.io/kubernetes/pkg/apis/policy/v1beta1" informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/externalversions" "k8s.io/kubernetes/pkg/controller" + + "github.com/Azure/go-autorest/autorest/to" ) type pdbStates map[string]policy.PodDisruptionBudget @@ -141,7 +143,7 @@ func newSelFooBar() *metav1.LabelSelector { return newSel(map[string]string{"foo": "bar"}) } -func newPodDisruptionBudget(t *testing.T, minAvailable intstr.IntOrString) (*policy.PodDisruptionBudget, string) { +func newMinAvailablePodDisruptionBudget(t *testing.T, minAvailable intstr.IntOrString) (*policy.PodDisruptionBudget, string) { pdb := &policy.PodDisruptionBudget{ TypeMeta: metav1.TypeMeta{APIVersion: api.Registry.GroupOrDie(v1.GroupName).GroupVersion.String()}, @@ -152,7 +154,7 @@ func newPodDisruptionBudget(t *testing.T, minAvailable intstr.IntOrString) (*pol ResourceVersion: "18", }, Spec: policy.PodDisruptionBudgetSpec{ - MinAvailable: minAvailable, + MinAvailable: &minAvailable, Selector: newSelFooBar(), }, } @@ -165,6 +167,29 @@ func newPodDisruptionBudget(t *testing.T, minAvailable intstr.IntOrString) (*pol return pdb, pdbName } +func newMaxUnavailablePodDisruptionBudget(t *testing.T, maxUnavailable intstr.IntOrString) (*policy.PodDisruptionBudget, string) { + pdb := &policy.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{APIVersion: api.Registry.GroupOrDie(v1.GroupName).GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "foobar", + Namespace: metav1.NamespaceDefault, + ResourceVersion: "18", + }, + Spec: policy.PodDisruptionBudgetSpec{ + MaxUnavailable: &maxUnavailable, + Selector: newSelFooBar(), + }, + } + + pdbName, err := controller.KeyFunc(pdb) + if err != nil { + t.Fatalf("Unexpected error naming pdb %q: %v", pdb.Name, err) + } + + return pdb, pdbName +} + func newPod(t *testing.T, name string) (*v1.Pod, string) { pod := &v1.Pod{ TypeMeta: metav1.TypeMeta{APIVersion: api.Registry.GroupOrDie(v1.GroupName).GroupVersion.String()}, @@ -304,7 +329,7 @@ func add(t *testing.T, store cache.Store, obj interface{}) { func TestNoSelector(t *testing.T) { dc, ps := newFakeDisruptionController() - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromInt(3)) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromInt(3)) pdb.Spec.Selector = &metav1.LabelSelector{} pod, _ := newPod(t, "yo-yo-yo") @@ -322,7 +347,7 @@ func TestNoSelector(t *testing.T) { func TestUnavailable(t *testing.T) { dc, ps := newFakeDisruptionController() - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromInt(3)) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromInt(3)) add(t, dc.pdbStore, pdb) dc.sync(pdbName) @@ -346,12 +371,54 @@ func TestUnavailable(t *testing.T) { ps.VerifyPdbStatus(t, pdbName, 0, 3, 3, 4, map[string]metav1.Time{}) } +// Verify that an integer MaxUnavailable won't +// allow a disruption for pods with no controller. +func TestIntegerMaxUnavailable(t *testing.T) { + dc, ps := newFakeDisruptionController() + + pdb, pdbName := newMaxUnavailablePodDisruptionBudget(t, intstr.FromInt(1)) + add(t, dc.pdbStore, pdb) + dc.sync(pdbName) + // This verifies that when a PDB has 0 pods, disruptions are not allowed. + ps.VerifyDisruptionAllowed(t, pdbName, 0) + + pod, _ := newPod(t, "naked") + add(t, dc.podStore, pod) + dc.sync(pdbName) + + ps.VerifyDisruptionAllowed(t, pdbName, 0) +} + +// Verify that an integer MaxUnavailable will recompute allowed disruptions when the scale of +// the selected pod's controller is modified. +func TestIntegerMaxUnavailableWithScaling(t *testing.T) { + dc, ps := newFakeDisruptionController() + + pdb, pdbName := newMaxUnavailablePodDisruptionBudget(t, intstr.FromInt(2)) + add(t, dc.pdbStore, pdb) + + rs, _ := newReplicaSet(t, 7) + add(t, dc.rsStore, rs) + + pod, _ := newPod(t, "pod") + add(t, dc.podStore, pod) + dc.sync(pdbName) + ps.VerifyPdbStatus(t, pdbName, 0, 1, 5, 7, map[string]metav1.Time{}) + + // Update scale of ReplicaSet and check PDB + rs.Spec.Replicas = to.Int32Ptr(5) + update(t, dc.rsStore, rs) + + dc.sync(pdbName) + ps.VerifyPdbStatus(t, pdbName, 0, 1, 3, 5, map[string]metav1.Time{}) +} + // Create a pod with no controller, and verify that a PDB with a percentage // specified won't allow a disruption. func TestNakedPod(t *testing.T) { dc, ps := newFakeDisruptionController() - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromString("28%")) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromString("28%")) add(t, dc.pdbStore, pdb) dc.sync(pdbName) // This verifies that when a PDB has 0 pods, disruptions are not allowed. @@ -368,7 +435,7 @@ func TestNakedPod(t *testing.T) { func TestReplicaSet(t *testing.T) { dc, ps := newFakeDisruptionController() - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromString("20%")) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromString("20%")) add(t, dc.pdbStore, pdb) rs, _ := newReplicaSet(t, 10) @@ -387,7 +454,7 @@ func TestMultipleControllers(t *testing.T) { dc, ps := newFakeDisruptionController() - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromString("1%")) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromString("1%")) add(t, dc.pdbStore, pdb) for i := 0; i < podCount; i++ { @@ -429,7 +496,7 @@ func TestReplicationController(t *testing.T) { dc, ps := newFakeDisruptionController() // 34% should round up to 2 - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromString("34%")) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromString("34%")) add(t, dc.pdbStore, pdb) rc, _ := newReplicationController(t, 3) rc.Spec.Selector = labels @@ -470,7 +537,7 @@ func TestStatefulSetController(t *testing.T) { dc, ps := newFakeDisruptionController() // 34% should round up to 2 - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromString("34%")) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromString("34%")) add(t, dc.pdbStore, pdb) ss, _ := newStatefulSet(t, 3) add(t, dc.ssStore, ss) @@ -518,7 +585,7 @@ func TestTwoControllers(t *testing.T) { const minimumOne int32 = 4 // integer minimum with one controller const minimumTwo int32 = 7 // integer minimum with two controllers - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromString("28%")) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromString("28%")) add(t, dc.pdbStore, pdb) rc, _ := newReplicationController(t, collectionSize) rc.Spec.Selector = rcLabels @@ -605,7 +672,7 @@ func TestTwoControllers(t *testing.T) { // Test pdb doesn't exist func TestPDBNotExist(t *testing.T) { dc, _ := newFakeDisruptionController() - pdb, _ := newPodDisruptionBudget(t, intstr.FromString("67%")) + pdb, _ := newMinAvailablePodDisruptionBudget(t, intstr.FromString("67%")) add(t, dc.pdbStore, pdb) if err := dc.sync("notExist"); err != nil { t.Errorf("Unexpected error: %v, expect nil", err) @@ -615,7 +682,7 @@ func TestPDBNotExist(t *testing.T) { func TestUpdateDisruptedPods(t *testing.T) { dc, ps := newFakeDisruptionController() dc.recheckQueue = workqueue.NewNamedDelayingQueue("pdb-queue") - pdb, pdbName := newPodDisruptionBudget(t, intstr.FromInt(1)) + pdb, pdbName := newMinAvailablePodDisruptionBudget(t, intstr.FromInt(1)) currentTime := time.Now() pdb.Status.DisruptedPods = map[string]metav1.Time{ "p1": {Time: currentTime}, // Should be removed, pod deletion started.