diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index f84825e8e9f..0b060d68d81 100644 --- a/pkg/features/kube_features.go +++ b/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}, diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go index c24a27c1f7f..4082c805c4f 100644 --- a/pkg/registry/core/pod/storage/eviction.go +++ b/pkg/registry/core/pod/storage/eviction.go @@ -230,12 +230,10 @@ 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 - } + 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 { diff --git a/pkg/registry/core/pod/storage/eviction_test.go b/pkg/registry/core/pod/storage/eviction_test.go index 308e3c69c46..2c8a5e695fd 100644 --- a/pkg/registry/core/pod/storage/eviction_test.go +++ b/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() diff --git a/pkg/registry/policy/poddisruptionbudget/strategy.go b/pkg/registry/policy/poddisruptionbudget/strategy.go index 4938929cd32..9be2b55ffd6 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy.go +++ b/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 -} diff --git a/pkg/registry/policy/poddisruptionbudget/strategy_test.go b/pkg/registry/policy/poddisruptionbudget/strategy_test.go index 9dc72625ceb..f81f0e2f7b3 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy_test.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy_test.go @@ -17,112 +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") @@ -138,70 +42,7 @@ func testPodDisruptionBudgetStrategyWithUnhealthyPodEvictionPolicy(t *testing.T, 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") - } - 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: 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, - } -} diff --git a/test/e2e/apps/disruption.go b/test/e2e/apps/disruption.go index 690f4d85adc..d32da5255cb 100644 --- a/test/e2e/apps/disruption.go +++ b/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,15 +542,16 @@ 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, Namespace: ns, }, Spec: policyv1.PodDisruptionBudgetSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - MaxUnavailable: &maxUnavailable, + 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) diff --git a/test/integration/evictions/evictions_test.go b/test/integration/evictions/evictions_test.go index 4ab080e4108..a5491c5e00e 100644 --- a/test/integration/evictions/evictions_test.go +++ b/test/integration/evictions/evictions_test.go @@ -434,40 +434,29 @@ 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 *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: nil, + isPodReady: true, }, "UnhealthyPodEvictionPolicy enabled but policy set to IfHealthyBudget with ready pod": { - enableUnhealthyPodEvictionPolicy: true, - unhealthyPodEvictionPolicy: unhealthyPolicyPtr(policyv1.IfHealthyBudget), - isPodReady: 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: unhealthyPolicyPtr(policyv1.AlwaysAllow), + isPodReady: true, }, "UnhealthyPodEvictionPolicy enabled but policy set to AlwaysAllow with unready pod": { - enableUnhealthyPodEvictionPolicy: true, - unhealthyPodEvictionPolicy: unhealthyPolicyPtr(policyv1.AlwaysAllow), - isPodReady: false, + unhealthyPodEvictionPolicy: unhealthyPolicyPtr(policyv1.AlwaysAllow), + isPodReady: false, }, } 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()