PDB MaxUnavailable: Disruption Controller Changes

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())
+
+	if pdb.Spec.MaxUnavailable != nil {
+		expectedCount, err = dc.getExpectedScale(pdb, pods)
+		if err != nil {
+			return
+		}
+		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
-			} 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())
+			}
+
+			var minAvailable int
+			minAvailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true)
+			if err != nil {
 				return
 			}
+			desiredHealthy = int32(minAvailable)
 		}
+	}
+	return
+}
 
-		// 2. Add up all the controllers.
-		expectedCount = 0
-		for _, count := range controllerScale {
-			expectedCount += count
-		}
+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
 
-		// 3. Do the math.
-		var dh int
-		dh, err = intstr.GetValueFromIntOrPercent(&pdb.Spec.MinAvailable, int(expectedCount), true)
-		if err != nil {
+	// 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
 		}
-		desiredHealthy = int32(dh)
+	}
+
+	// 2. Add up all the controllers.
+	expectedCount = 0
+	for _, count := range controllerScale {
+		expectedCount += count
 	}
 
 	return

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.