Merge pull request #123428 from atiratree/UnhealthyPodEvictionPolicy-GA

promote PDBUnhealthyPodEvictionPolicy to GA

pkg/features/kube_features.go

@@ -526,6 +526,7 @@ const (
 	// kep: http://kep.k8s.io/3018
 	// alpha: v1.26
 	// beta: v1.27
+	// GA: v1.31
 	//
 	// Enables PDBUnhealthyPodEvictionPolicy for PodDisruptionBudgets
 	PDBUnhealthyPodEvictionPolicy featuregate.Feature = "PDBUnhealthyPodEvictionPolicy"
@@ -1095,7 +1096,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
 
 	NodeSwap: {Default: true, PreRelease: featuregate.Beta},
 
-	PDBUnhealthyPodEvictionPolicy: {Default: true, PreRelease: featuregate.Beta},
+	PDBUnhealthyPodEvictionPolicy: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.33
 
 	PersistentVolumeLastPhaseTransitionTime: {Default: true, PreRelease: featuregate.Beta},
 

pkg/registry/core/pod/storage/eviction.go

@@ -230,13 +230,11 @@ func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Obje
 		// IsPodReady is the current implementation of IsHealthy
 		// If the pod is healthy, it should be guarded by the PDB.
 		if !podutil.IsPodReady(pod) {
-			if feature.DefaultFeatureGate.Enabled(features.PDBUnhealthyPodEvictionPolicy) {
 			if pdb.Spec.UnhealthyPodEvictionPolicy != nil && *pdb.Spec.UnhealthyPodEvictionPolicy == policyv1.AlwaysAllow {
 				// Delete the unhealthy pod, it doesn't count towards currentHealthy and desiredHealthy and we should not decrement disruptionsAllowed.
 				updateDeletionOptions = true
 				return nil
 			}
-			}
 			// default nil and IfHealthyBudget policy
 			if pdb.Status.CurrentHealthy >= pdb.Status.DesiredHealthy && pdb.Status.DesiredHealthy > 0 {
 				// Delete the unhealthy pod, it doesn't count towards currentHealthy and desiredHealthy and we should not decrement disruptionsAllowed.

pkg/registry/core/pod/storage/eviction_test.go

@@ -165,8 +165,6 @@ func TestEviction(t *testing.T) {
 				continue
 			}
 			t.Run(fmt.Sprintf("%v with %v policy", tc.name, unhealthyPolicyStr(unhealthyPodEvictionPolicy)), func(t *testing.T) {
-				featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, true)
-
 				// same test runs multiple times, make copy of objects to have unique ones
 				evictionCopy := tc.eviction.DeepCopy()
 				var pdbsCopy []runtime.Object
@@ -530,8 +528,6 @@ func TestEvictionIgnorePDB(t *testing.T) {
 				continue
 			}
 			t.Run(fmt.Sprintf("%v with %v policy", tc.name, unhealthyPolicyStr(unhealthyPodEvictionPolicy)), func(t *testing.T) {
-				featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, true)
-
 				// same test runs 3 times, make copy of objects to have unique ones
 				evictionCopy := tc.eviction.DeepCopy()
 				prcCopy := tc.prc.DeepCopy()

pkg/registry/policy/poddisruptionbudget/strategy.go

@@ -26,11 +26,9 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/storage/names"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/apis/policy"
 	"k8s.io/kubernetes/pkg/apis/policy/validation"
-	"k8s.io/kubernetes/pkg/features"
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 )
 
@@ -70,8 +68,6 @@ func (podDisruptionBudgetStrategy) PrepareForCreate(ctx context.Context, obj run
 	podDisruptionBudget.Status = policy.PodDisruptionBudgetStatus{}
 
 	podDisruptionBudget.Generation = 1
-
-	dropDisabledFields(&podDisruptionBudget.Spec, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -87,8 +83,6 @@ func (podDisruptionBudgetStrategy) PrepareForUpdate(ctx context.Context, obj, ol
 	if !apiequality.Semantic.DeepEqual(oldPodDisruptionBudget.Spec, newPodDisruptionBudget.Spec) {
 		newPodDisruptionBudget.Generation = oldPodDisruptionBudget.Generation + 1
 	}
-
-	dropDisabledFields(&newPodDisruptionBudget.Spec, &oldPodDisruptionBudget.Spec)
 }
 
 // Validate validates a new PodDisruptionBudget.
@@ -188,23 +182,3 @@ func hasInvalidLabelValueInLabelSelector(pdb *policy.PodDisruptionBudget) bool {
 	}
 	return false
 }
-
-// dropDisabledFields removes disabled fields from the pod disruption budget spec.
-// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pod disruption budget spec.
-func dropDisabledFields(pdbSpec, oldPDBSpec *policy.PodDisruptionBudgetSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.PDBUnhealthyPodEvictionPolicy) {
-		if !unhealthyPodEvictionPolicyInUse(oldPDBSpec) {
-			pdbSpec.UnhealthyPodEvictionPolicy = nil
-		}
-	}
-}
-
-func unhealthyPodEvictionPolicyInUse(oldPDBSpec *policy.PodDisruptionBudgetSpec) bool {
-	if oldPDBSpec == nil {
-		return false
-	}
-	if oldPDBSpec.UnhealthyPodEvictionPolicy != nil {
-		return true
-	}
-	return false
-}

pkg/registry/policy/poddisruptionbudget/strategy_test.go

@@ -17,176 +17,16 @@ limitations under the License.
 package poddisruptionbudget
 
 import (
-	"reflect"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/kubernetes/pkg/apis/policy"
-	"k8s.io/kubernetes/pkg/features"
+	"k8s.io/utils/ptr"
 )
 
-type unhealthyPodEvictionPolicyStrategyTestCase struct {
-	name                                                       string
-	enableUnhealthyPodEvictionPolicy                           bool
-	disablePDBUnhealthyPodEvictionPolicyFeatureGateAfterCreate bool
-	unhealthyPodEvictionPolicy                                 *policy.UnhealthyPodEvictionPolicyType
-	expectedUnhealthyPodEvictionPolicy                         *policy.UnhealthyPodEvictionPolicyType
-	expectedValidationErr                                      bool
-	updateUnhealthyPodEvictionPolicy                           *policy.UnhealthyPodEvictionPolicyType
-	expectedUpdateUnhealthyPodEvictionPolicy                   *policy.UnhealthyPodEvictionPolicyType
-	expectedValidationUpdateErr                                bool
-}
-
 func TestPodDisruptionBudgetStrategy(t *testing.T) {
-	tests := map[string]bool{
-		"PodDisruptionBudget strategy with PDBUnhealthyPodEvictionPolicy feature gate disabled": false,
-		"PodDisruptionBudget strategy with PDBUnhealthyPodEvictionPolicy feature gate enabled":  true,
-	}
-
-	for name, enableUnhealthyPodEvictionPolicy := range tests {
-		t.Run(name, func(t *testing.T) {
-			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, enableUnhealthyPodEvictionPolicy)
-			testPodDisruptionBudgetStrategy(t)
-		})
-	}
-
-	healthyPolicyTests := []unhealthyPodEvictionPolicyStrategyTestCase{
-		{
-			name:                             "PodDisruptionBudget strategy with FeatureGate disabled should remove unhealthyPodEvictionPolicy",
-			enableUnhealthyPodEvictionPolicy: false,
-			unhealthyPodEvictionPolicy:       unhealthyPolicyPtr(policy.IfHealthyBudget),
-			updateUnhealthyPodEvictionPolicy: unhealthyPolicyPtr(policy.IfHealthyBudget),
-		},
-		{
-			name:                             "PodDisruptionBudget strategy with FeatureGate disabled should remove invalid unhealthyPodEvictionPolicy",
-			enableUnhealthyPodEvictionPolicy: false,
-			unhealthyPodEvictionPolicy:       unhealthyPolicyPtr("Invalid"),
-			updateUnhealthyPodEvictionPolicy: unhealthyPolicyPtr("Invalid"),
-		},
-		{
-			name:                             "PodDisruptionBudget strategy with FeatureGate enabled",
-			enableUnhealthyPodEvictionPolicy: true,
-		},
-		{
-			name:                                     "PodDisruptionBudget strategy with FeatureGate enabled should respect unhealthyPodEvictionPolicy",
-			enableUnhealthyPodEvictionPolicy:         true,
-			unhealthyPodEvictionPolicy:               unhealthyPolicyPtr(policy.AlwaysAllow),
-			expectedUnhealthyPodEvictionPolicy:       unhealthyPolicyPtr(policy.AlwaysAllow),
-			updateUnhealthyPodEvictionPolicy:         unhealthyPolicyPtr(policy.IfHealthyBudget),
-			expectedUpdateUnhealthyPodEvictionPolicy: unhealthyPolicyPtr(policy.IfHealthyBudget),
-		},
-		{
-			name:                             "PodDisruptionBudget strategy with FeatureGate enabled should fail invalid unhealthyPodEvictionPolicy",
-			enableUnhealthyPodEvictionPolicy: true,
-			unhealthyPodEvictionPolicy:       unhealthyPolicyPtr("Invalid"),
-			expectedValidationErr:            true,
-		},
-		{
-			name:                             "PodDisruptionBudget strategy with FeatureGate enabled should fail invalid unhealthyPodEvictionPolicy when updated",
-			enableUnhealthyPodEvictionPolicy: true,
-			updateUnhealthyPodEvictionPolicy: unhealthyPolicyPtr("Invalid"),
-			expectedValidationUpdateErr:      true,
-		},
-		{
-			name:                             "PodDisruptionBudget strategy with unhealthyPodEvictionPolicy should be updated when feature gate is disabled",
-			enableUnhealthyPodEvictionPolicy: true,
-			disablePDBUnhealthyPodEvictionPolicyFeatureGateAfterCreate: true,
-			unhealthyPodEvictionPolicy:                                 unhealthyPolicyPtr(policy.AlwaysAllow),
-			expectedUnhealthyPodEvictionPolicy:                         unhealthyPolicyPtr(policy.AlwaysAllow),
-			updateUnhealthyPodEvictionPolicy:                           unhealthyPolicyPtr(policy.IfHealthyBudget),
-			expectedUpdateUnhealthyPodEvictionPolicy:                   unhealthyPolicyPtr(policy.IfHealthyBudget),
-		},
-		{
-			name:                             "PodDisruptionBudget strategy with unhealthyPodEvictionPolicy should not be updated to invalid when feature gate is disabled",
-			enableUnhealthyPodEvictionPolicy: true,
-			disablePDBUnhealthyPodEvictionPolicyFeatureGateAfterCreate: true,
-			unhealthyPodEvictionPolicy:                                 unhealthyPolicyPtr(policy.AlwaysAllow),
-			expectedUnhealthyPodEvictionPolicy:                         unhealthyPolicyPtr(policy.AlwaysAllow),
-			updateUnhealthyPodEvictionPolicy:                           unhealthyPolicyPtr("Invalid"),
-			expectedValidationUpdateErr:                                true,
-			expectedUpdateUnhealthyPodEvictionPolicy:                   unhealthyPolicyPtr(policy.AlwaysAllow),
-		},
-	}
-
-	for _, tc := range healthyPolicyTests {
-		t.Run(tc.name, func(t *testing.T) {
-			testPodDisruptionBudgetStrategyWithUnhealthyPodEvictionPolicy(t, tc)
-		})
-	}
-}
-
-func testPodDisruptionBudgetStrategyWithUnhealthyPodEvictionPolicy(t *testing.T, tc unhealthyPodEvictionPolicyStrategyTestCase) {
-	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, tc.enableUnhealthyPodEvictionPolicy)
-	ctx := genericapirequest.NewDefaultContext()
-	if !Strategy.NamespaceScoped() {
-		t.Errorf("PodDisruptionBudget must be namespace scoped")
-	}
-	if Strategy.AllowCreateOnUpdate() {
-		t.Errorf("PodDisruptionBudget should not allow create on update")
-	}
-
-	validSelector := map[string]string{"a": "b"}
-	minAvailable := intstr.FromInt32(3)
-	pdb := &policy.PodDisruptionBudget{
-		ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
-		Spec: policy.PodDisruptionBudgetSpec{
-			MinAvailable:               &minAvailable,
-			Selector:                   &metav1.LabelSelector{MatchLabels: validSelector},
-			UnhealthyPodEvictionPolicy: tc.unhealthyPodEvictionPolicy,
-		},
-	}
-
-	Strategy.PrepareForCreate(ctx, pdb)
-	errs := Strategy.Validate(ctx, pdb)
-	if len(errs) != 0 {
-		if !tc.expectedValidationErr {
-			t.Errorf("Unexpected error validating %v", errs)
-		}
-		return // no point going further when we have invalid PDB
-	}
-	if len(errs) == 0 && tc.expectedValidationErr {
-		t.Errorf("Expected error validating")
-	}
-	if !reflect.DeepEqual(pdb.Spec.UnhealthyPodEvictionPolicy, tc.expectedUnhealthyPodEvictionPolicy) {
-		t.Errorf("Unexpected UnhealthyPodEvictionPolicy set: expected %v, got %v", tc.expectedUnhealthyPodEvictionPolicy, pdb.Spec.UnhealthyPodEvictionPolicy)
-	}
-	if tc.disablePDBUnhealthyPodEvictionPolicyFeatureGateAfterCreate {
-		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, false)
-	}
-
-	newPdb := &policy.PodDisruptionBudget{
-		ObjectMeta: metav1.ObjectMeta{Name: pdb.Name, Namespace: pdb.Namespace},
-		Spec:       pdb.Spec,
-	}
-	if tc.updateUnhealthyPodEvictionPolicy != nil {
-		newPdb.Spec.UnhealthyPodEvictionPolicy = tc.updateUnhealthyPodEvictionPolicy
-	}
-
-	// Nothing in Spec changes: OK
-	Strategy.PrepareForUpdate(ctx, newPdb, pdb)
-	errs = Strategy.ValidateUpdate(ctx, newPdb, pdb)
-
-	if len(errs) != 0 {
-		if !tc.expectedValidationUpdateErr {
-			t.Errorf("Unexpected error updating PodDisruptionBudget %v", errs)
-		}
-		return // no point going further when we have invalid PDB
-	}
-	if len(errs) == 0 && tc.expectedValidationUpdateErr {
-		t.Errorf("Expected error updating PodDisruptionBudget")
-	}
-	if !reflect.DeepEqual(newPdb.Spec.UnhealthyPodEvictionPolicy, tc.expectedUpdateUnhealthyPodEvictionPolicy) {
-		t.Errorf("Unexpected UnhealthyPodEvictionPolicy set: expected %v, got %v", tc.expectedUpdateUnhealthyPodEvictionPolicy, newPdb.Spec.UnhealthyPodEvictionPolicy)
-	}
-}
-
-func testPodDisruptionBudgetStrategy(t *testing.T) {
 	ctx := genericapirequest.NewDefaultContext()
 	if !Strategy.NamespaceScoped() {
 		t.Errorf("PodDisruptionBudget must be namespace scoped")
@@ -202,6 +42,7 @@ func testPodDisruptionBudgetStrategy(t *testing.T) {
 		Spec: policy.PodDisruptionBudgetSpec{
 			MinAvailable:               &minAvailable,
 			Selector:                   &metav1.LabelSelector{MatchLabels: validSelector},
+			UnhealthyPodEvictionPolicy: ptr.To(policy.AlwaysAllow),
 		},
 	}
 
@@ -256,6 +97,25 @@ func testPodDisruptionBudgetStrategy(t *testing.T) {
 	if len(errs) != 0 {
 		t.Errorf("Expected no error updating replacing MinAvailable with MaxUnavailable on poddisruptionbudgets.")
 	}
+
+	// Changing UnhealthyPodEvictionPolicy? OK
+	newPdb.Spec.UnhealthyPodEvictionPolicy = ptr.To(policy.IfHealthyBudget)
+	Strategy.PrepareForUpdate(ctx, newPdb, pdb)
+	errs = Strategy.ValidateUpdate(ctx, newPdb, pdb)
+	if len(errs) != 0 {
+		t.Errorf("Expected no error on changing UnhealthyPodEvictionPolicy on poddisruptionbudgets.")
+	}
+	if *newPdb.Spec.UnhealthyPodEvictionPolicy != policy.IfHealthyBudget {
+		t.Errorf("Unexpected UnhealthyPodEvictionPolicy: expected %v, got %v", *newPdb.Spec.UnhealthyPodEvictionPolicy, policy.IfHealthyBudget)
+	}
+
+	// Changing to invalid UnhealthyPodEvictionPolicy.
+	newPdb.Spec.UnhealthyPodEvictionPolicy = ptr.To(policy.UnhealthyPodEvictionPolicyType("invalid"))
+	Strategy.PrepareForUpdate(ctx, newPdb, pdb)
+	errs = Strategy.ValidateUpdate(ctx, newPdb, pdb)
+	if len(errs) == 0 {
+		t.Errorf("Expected error on changing to invalid UnhealthyPodEvictionPolicy on poddisruptionbudgets.")
+	}
 }
 
 func TestPodDisruptionBudgetStatusStrategy(t *testing.T) {
@@ -371,86 +231,3 @@ func TestPodDisruptionBudgetStatusValidationByApiVersion(t *testing.T) {
 		})
 	}
 }
-
-func TestDropDisabledFields(t *testing.T) {
-	tests := map[string]struct {
-		oldSpec                          *policy.PodDisruptionBudgetSpec
-		newSpec                          *policy.PodDisruptionBudgetSpec
-		expectNewSpec                    *policy.PodDisruptionBudgetSpec
-		enableUnhealthyPodEvictionPolicy bool
-	}{
-		"disabled clears unhealthyPodEvictionPolicy": {
-			enableUnhealthyPodEvictionPolicy: false,
-			oldSpec:                          nil,
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(nil),
-		},
-		"disabled does not allow updating unhealthyPodEvictionPolicy": {
-			enableUnhealthyPodEvictionPolicy: false,
-			oldSpec:                          specWithUnhealthyPodEvictionPolicy(nil),
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(nil),
-		},
-		"disabled preserves old unhealthyPodEvictionPolicy when both old and new have it": {
-			enableUnhealthyPodEvictionPolicy: false,
-			oldSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-		},
-		"disabled allows updating unhealthyPodEvictionPolicy": {
-			enableUnhealthyPodEvictionPolicy: false,
-			oldSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.AlwaysAllow)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.AlwaysAllow)),
-		},
-		"enabled preserve unhealthyPodEvictionPolicy": {
-			enableUnhealthyPodEvictionPolicy: true,
-			oldSpec:                          nil,
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-		},
-		"enabled allows updating unhealthyPodEvictionPolicy": {
-			enableUnhealthyPodEvictionPolicy: true,
-			oldSpec:                          specWithUnhealthyPodEvictionPolicy(nil),
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-		},
-		"enabled preserve unhealthyPodEvictionPolicy when both old and new have it": {
-			enableUnhealthyPodEvictionPolicy: true,
-			oldSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-		},
-		"enabled updates unhealthyPodEvictionPolicy": {
-			enableUnhealthyPodEvictionPolicy: true,
-			oldSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.IfHealthyBudget)),
-			newSpec:                          specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.AlwaysAllow)),
-			expectNewSpec:                    specWithUnhealthyPodEvictionPolicy(unhealthyPolicyPtr(policy.AlwaysAllow)),
-		},
-	}
-
-	for name, tc := range tests {
-		t.Run(name, func(t *testing.T) {
-			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, tc.enableUnhealthyPodEvictionPolicy)
-
-			oldSpecBefore := tc.oldSpec.DeepCopy()
-			dropDisabledFields(tc.newSpec, tc.oldSpec)
-			if !reflect.DeepEqual(tc.newSpec, tc.expectNewSpec) {
-				t.Error(cmp.Diff(tc.newSpec, tc.expectNewSpec))
-			}
-			if !reflect.DeepEqual(tc.oldSpec, oldSpecBefore) {
-				t.Error(cmp.Diff(tc.oldSpec, oldSpecBefore))
-			}
-		})
-	}
-}
-
-func unhealthyPolicyPtr(unhealthyPodEvictionPolicy policy.UnhealthyPodEvictionPolicyType) *policy.UnhealthyPodEvictionPolicyType {
-	return &unhealthyPodEvictionPolicy
-}
-
-func specWithUnhealthyPodEvictionPolicy(unhealthyPodEvictionPolicy *policy.UnhealthyPodEvictionPolicyType) *policy.PodDisruptionBudgetSpec {
-	return &policy.PodDisruptionBudgetSpec{
-		UnhealthyPodEvictionPolicy: unhealthyPodEvictionPolicy,
-	}
-}

test/e2e/apps/disruption.go

@@ -38,6 +38,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/json"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
@@ -45,9 +46,11 @@ import (
 	"k8s.io/client-go/util/retry"
 	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 	"k8s.io/kubernetes/test/e2e/framework"
+	e2epod "k8s.io/kubernetes/test/e2e/framework/pod"
 	e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper"
 	imageutils "k8s.io/kubernetes/test/utils/image"
 	admissionapi "k8s.io/pod-security-admission/api"
+	"k8s.io/utils/ptr"
 )
 
 // schedulingTimeout is longer specifically because sometimes we need to wait
@@ -301,7 +304,7 @@ var _ = SIGDescribe("DisruptionController", func() {
 			}
 
 			if c.maxUnavailable.String() != "" {
-				createPDBMaxUnavailableOrDie(ctx, cs, ns, defaultName, c.maxUnavailable)
+				createPDBMaxUnavailableOrDie(ctx, cs, ns, defaultName, c.maxUnavailable, nil)
 			}
 
 			// Locate a running pod.
@@ -375,7 +378,7 @@ var _ = SIGDescribe("DisruptionController", func() {
 
 		ginkgo.By("Trying to evict the same pod we tried earlier which should now be evictable")
 		waitForPodsOrDie(ctx, cs, ns, 3)
-		waitForPdbToObserveHealthyPods(ctx, cs, ns, 3)
+		waitForPdbToObserveHealthyPods(ctx, cs, ns, 3, 2)
 		err = cs.CoreV1().Pods(ns).EvictV1(ctx, e)
 		framework.ExpectNoError(err) // the eviction is now allowed
 
@@ -414,6 +417,112 @@ var _ = SIGDescribe("DisruptionController", func() {
 		framework.ExpectNoError(err) // the eviction is now allowed
 	})
 
+	unhealthyPodEvictionPolicyCases := []struct {
+		name                          string
+		unhealthyPodEvictionPolicy    *policyv1.UnhealthyPodEvictionPolicyType
+		podsShouldBecomeReadyFirst    bool
+		expectedSuccesfulPodEvictions int
+	}{
+		{
+			name:                          "should evict ready pods with Default UnhealthyPodEvictionPolicy",
+			unhealthyPodEvictionPolicy:    nil,
+			podsShouldBecomeReadyFirst:    true,
+			expectedSuccesfulPodEvictions: 1,
+		},
+		{
+			name:                          "should evict ready pods with IfHealthyBudget UnhealthyPodEvictionPolicy",
+			unhealthyPodEvictionPolicy:    ptr.To(policyv1.IfHealthyBudget),
+			podsShouldBecomeReadyFirst:    true,
+			expectedSuccesfulPodEvictions: 1,
+		},
+		{
+			name:                          "should evict ready pods with AlwaysAllow UnhealthyPodEvictionPolicy",
+			unhealthyPodEvictionPolicy:    ptr.To(policyv1.AlwaysAllow),
+			podsShouldBecomeReadyFirst:    true,
+			expectedSuccesfulPodEvictions: 1,
+		},
+		{
+			name:                          "should not evict unready pods with Default UnhealthyPodEvictionPolicy",
+			unhealthyPodEvictionPolicy:    nil,
+			podsShouldBecomeReadyFirst:    false,
+			expectedSuccesfulPodEvictions: 0,
+		},
+		{
+			name:                          "should not evict unready pods with IfHealthyBudget UnhealthyPodEvictionPolicy",
+			unhealthyPodEvictionPolicy:    ptr.To(policyv1.IfHealthyBudget),
+			podsShouldBecomeReadyFirst:    false,
+			expectedSuccesfulPodEvictions: 0,
+		},
+		{
+			name:                          "should evict unready pods with AlwaysAllow UnhealthyPodEvictionPolicy",
+			unhealthyPodEvictionPolicy:    ptr.To(policyv1.AlwaysAllow),
+			podsShouldBecomeReadyFirst:    false,
+			expectedSuccesfulPodEvictions: 3,
+		},
+	}
+	for i := range unhealthyPodEvictionPolicyCases {
+		tc := unhealthyPodEvictionPolicyCases[i]
+
+		framework.It(tc.name, func(ctx context.Context) {
+			ginkgo.By("Creating a pdb")
+			createPDBMaxUnavailableOrDie(ctx, cs, ns, defaultName, intstr.FromInt32(1), tc.unhealthyPodEvictionPolicy)
+
+			ginkgo.By("Creating a replica set")
+			rsName := "test-rs-with-delayed-ready"
+			replicas := int32(3)
+			rs := newRS(rsName, replicas, defaultLabels, WebserverImageName, WebserverImage, nil)
+			rs.Labels["name"] = rsName
+			initialDelaySeconds := framework.PodStartTimeout.Seconds() + 30
+			if tc.podsShouldBecomeReadyFirst {
+				initialDelaySeconds = 0
+			}
+			rs.Spec.Template.Spec.Containers[0].ReadinessProbe = &v1.Probe{
+				ProbeHandler: v1.ProbeHandler{
+					HTTPGet: &v1.HTTPGetAction{
+						Path: "/index.html",
+						Port: intstr.IntOrString{IntVal: 80},
+					},
+				},
+				InitialDelaySeconds: int32(initialDelaySeconds),
+			}
+			_, err := cs.AppsV1().ReplicaSets(ns).Create(ctx, rs, metav1.CreateOptions{})
+			framework.ExpectNoError(err, "Creating replica set %q in namespace %q", rs.Name, ns)
+
+			if tc.podsShouldBecomeReadyFirst {
+				ginkgo.By("Wait for pods to be running and ready")
+				waitForPodsOrDie(ctx, cs, ns, int(replicas))
+				waitForPdbToObserveHealthyPods(ctx, cs, ns, replicas, replicas-1)
+			} else {
+				ginkgo.By("Wait for pods to be running and not ready")
+				err := e2epod.VerifyPodsRunning(ctx, cs, ns, rsName, false, replicas)
+				framework.ExpectNoError(err)
+				waitForPdbToObserveHealthyPods(ctx, cs, ns, 0, replicas-1)
+			}
+
+			ginkgo.By("Try to evict all pods guarded by a PDB")
+			podList, err := cs.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{})
+			framework.ExpectNoError(err)
+
+			evictedPods := sets.New[string]()
+			for _, pod := range podList.Items {
+				e := &policyv1.Eviction{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      pod.Name,
+						Namespace: ns,
+					},
+				}
+				err = cs.CoreV1().Pods(ns).EvictV1(ctx, e)
+				if err == nil {
+					evictedPods.Insert(pod.Name)
+				}
+			}
+			gomega.Expect(evictedPods).Should(gomega.HaveLen(tc.expectedSuccesfulPodEvictions))
+			_, err = e2epod.WaitForPods(ctx, cs, ns, metav1.ListOptions{}, e2epod.Range{NoneMatching: true}, framework.PodDeleteTimeout, "evicted pods should be deleted", func(pod *v1.Pod) bool {
+				return evictedPods.Has(pod.Name) && pod.DeletionTimestamp == nil
+			})
+			framework.ExpectNoError(err)
+		})
+	}
 })
 
 func createPDBMinAvailableOrDie(ctx context.Context, cs kubernetes.Interface, ns string, name string, minAvailable intstr.IntOrString, labels map[string]string) {
@@ -433,7 +542,7 @@ func createPDBMinAvailableOrDie(ctx context.Context, cs kubernetes.Interface, ns
 	waitForPdbToBeProcessed(ctx, cs, ns, name)
 }
 
-func createPDBMaxUnavailableOrDie(ctx context.Context, cs kubernetes.Interface, ns string, name string, maxUnavailable intstr.IntOrString) {
+func createPDBMaxUnavailableOrDie(ctx context.Context, cs kubernetes.Interface, ns string, name string, maxUnavailable intstr.IntOrString, unhealthyPodEvictionPolicy *policyv1.UnhealthyPodEvictionPolicyType) {
 	pdb := policyv1.PodDisruptionBudget{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
@@ -442,6 +551,7 @@ func createPDBMaxUnavailableOrDie(ctx context.Context, cs kubernetes.Interface,
 		Spec: policyv1.PodDisruptionBudgetSpec{
 			Selector:                   &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
 			MaxUnavailable:             &maxUnavailable,
+			UnhealthyPodEvictionPolicy: unhealthyPodEvictionPolicy,
 		},
 	}
 	_, err := cs.PolicyV1().PodDisruptionBudgets(ns).Create(ctx, &pdb, metav1.CreateOptions{})
@@ -670,7 +780,7 @@ func waitForPdbToBeDeleted(ctx context.Context, cs kubernetes.Interface, ns stri
 	framework.ExpectNoError(err, "Waiting for the pdb to be deleted in namespace %s", ns)
 }
 
-func waitForPdbToObserveHealthyPods(ctx context.Context, cs kubernetes.Interface, ns string, healthyCount int32) {
+func waitForPdbToObserveHealthyPods(ctx context.Context, cs kubernetes.Interface, ns string, healthyCount, desiredHealthy int32) {
 	ginkgo.By("Waiting for the pdb to observed all healthy pods")
 	err := wait.PollUntilContextTimeout(ctx, framework.Poll, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) {
 		pdb, err := cs.PolicyV1().PodDisruptionBudgets(ns).Get(ctx, "foo", metav1.GetOptions{})
@@ -680,6 +790,9 @@ func waitForPdbToObserveHealthyPods(ctx context.Context, cs kubernetes.Interface
 		if pdb.Status.CurrentHealthy != healthyCount {
 			return false, nil
 		}
+		if pdb.Status.DesiredHealthy != desiredHealthy {
+			return false, nil
+		}
 		return true, nil
 	})
 	framework.ExpectNoError(err, "Waiting for the pdb in namespace %s to observed %d healthy pods", ns, healthyCount)

test/integration/evictions/evictions_test.go

@@ -434,32 +434,22 @@ func TestEvictionWithFinalizers(t *testing.T) {
 // TestEvictionWithUnhealthyPodEvictionPolicy tests eviction with a PDB that has a UnhealthyPodEvictionPolicy
 func TestEvictionWithUnhealthyPodEvictionPolicy(t *testing.T) {
 	cases := map[string]struct {
-		enableUnhealthyPodEvictionPolicy bool
 		unhealthyPodEvictionPolicy *policyv1.UnhealthyPodEvictionPolicyType
 		isPodReady                 bool
 	}{
-		"UnhealthyPodEvictionPolicy disabled and policy not set": {
-			enableUnhealthyPodEvictionPolicy: false,
-			unhealthyPodEvictionPolicy:       nil,
-			isPodReady:                       true,
-		},
 		"UnhealthyPodEvictionPolicy enabled but policy not set": {
-			enableUnhealthyPodEvictionPolicy: true,
 			unhealthyPodEvictionPolicy: nil,
 			isPodReady:                 true,
 		},
 		"UnhealthyPodEvictionPolicy enabled but policy set to IfHealthyBudget with ready pod": {
-			enableUnhealthyPodEvictionPolicy: true,
 			unhealthyPodEvictionPolicy: unhealthyPolicyPtr(policyv1.IfHealthyBudget),
 			isPodReady:                 true,
 		},
 		"UnhealthyPodEvictionPolicy enabled but policy set to AlwaysAllow with ready pod": {
-			enableUnhealthyPodEvictionPolicy: true,
 			unhealthyPodEvictionPolicy: unhealthyPolicyPtr(policyv1.AlwaysAllow),
 			isPodReady:                 true,
 		},
 		"UnhealthyPodEvictionPolicy enabled but policy set to AlwaysAllow with unready pod": {
-			enableUnhealthyPodEvictionPolicy: true,
 			unhealthyPodEvictionPolicy: unhealthyPolicyPtr(policyv1.AlwaysAllow),
 			isPodReady:                 false,
 		},
@@ -467,7 +457,6 @@ func TestEvictionWithUnhealthyPodEvictionPolicy(t *testing.T) {
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			tCtx := ktesting.Init(t)
-			featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, tc.enableUnhealthyPodEvictionPolicy)
 			closeFn, rm, informers, _, clientSet := rmSetup(tCtx, t)
 			defer closeFn()