diff --git a/pkg/apis/policy/types.go b/pkg/apis/policy/types.go index f7da6c25197..b83ebd390df 100644 --- a/pkg/apis/policy/types.go +++ b/pkg/apis/policy/types.go @@ -42,8 +42,56 @@ type PodDisruptionBudgetSpec struct { // by specifying 0. This is a mutually exclusive setting with "minAvailable". // +optional MaxUnavailable *intstr.IntOrString + + // UnhealthyPodEvictionPolicy defines the criteria for when unhealthy pods + // should be considered for eviction. Current implementation considers healthy pods, + // as pods that have status.conditions item with type="Ready",status="True". + // + // Valid policies are IfHealthyBudget and AlwaysAllow. + // If no policy is specified, the default behavior will be used, + // which corresponds to the IfHealthyBudget policy. + // + // IfHealthyBudget policy means that running pods (status.phase="Running"), + // but not yet healthy can be evicted only if the guarded application is not + // disrupted (status.currentHealthy is at least equal to status.desiredHealthy). + // Healthy pods will be subject to the PDB for eviction. + // + // AlwaysAllow policy means that all running pods (status.phase="Running"), + // but not yet healthy are considered disrupted and can be evicted regardless + // of whether the criteria in a PDB is met. This means perspective running + // pods of a disrupted application might not get a chance to become healthy. + // Healthy pods will be subject to the PDB for eviction. + // + // Additional policies may be added in the future. + // Clients making eviction decisions should disallow eviction of unhealthy pods + // if they encounter an unrecognized policy in this field. + // + // This field is alpha-level. The eviction API uses this field when + // the feature gate PDBUnhealthyPodEvictionPolicy is enabled (disabled by default). + // +optional + UnhealthyPodEvictionPolicy *UnhealthyPodEvictionPolicyType } +// UnhealthyPodEvictionPolicyType defines the criteria for when unhealthy pods +// should be considered for eviction. +// +enum +type UnhealthyPodEvictionPolicyType string + +const ( + // IfHealthyBudget policy means that running pods (status.phase="Running"), + // but not yet healthy can be evicted only if the guarded application is not + // disrupted (status.currentHealthy is at least equal to status.desiredHealthy). + // Healthy pods will be subject to the PDB for eviction. + IfHealthyBudget UnhealthyPodEvictionPolicyType = "IfHealthyBudget" + + // AlwaysAllow policy means that all running pods (status.phase="Running"), + // but not yet healthy are considered disrupted and can be evicted regardless + // of whether the criteria in a PDB is met. This means perspective running + // pods of a disrupted application might not get a chance to become healthy. + // Healthy pods will be subject to the PDB for eviction. + AlwaysAllow UnhealthyPodEvictionPolicyType = "AlwaysAllow" +) + // PodDisruptionBudgetStatus represents information about the status of a // PodDisruptionBudget. Status may trail the actual state of a system. type PodDisruptionBudgetStatus struct { diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index fb529b3d6bc..8f2ad4ff4d3 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -44,6 +44,11 @@ const ( seccompAllowedProfilesAnnotationKey = "seccomp.security.alpha.kubernetes.io/allowedProfileNames" ) +var supportedUnhealthyPodEvictionPolicies = sets.NewString( + string(policy.IfHealthyBudget), + string(policy.AlwaysAllow), +) + type PodDisruptionBudgetValidationOptions struct { AllowInvalidLabelValueInSelector bool } @@ -78,6 +83,10 @@ func ValidatePodDisruptionBudgetSpec(spec policy.PodDisruptionBudgetSpec, opts P allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(spec.Selector, labelSelectorValidationOptions, fldPath.Child("selector"))...) + if spec.UnhealthyPodEvictionPolicy != nil && !supportedUnhealthyPodEvictionPolicies.Has(string(*spec.UnhealthyPodEvictionPolicy)) { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("unhealthyPodEvictionPolicy"), *spec.UnhealthyPodEvictionPolicy, supportedUnhealthyPodEvictionPolicies.List())) + } + return allErrs } diff --git a/pkg/apis/policy/validation/validation_test.go b/pkg/apis/policy/validation/validation_test.go index 0902328d4f3..0f247001775 100644 --- a/pkg/apis/policy/validation/validation_test.go +++ b/pkg/apis/policy/validation/validation_test.go @@ -98,6 +98,63 @@ func TestValidateMinAvailablePodAndMaxUnavailableDisruptionBudgetSpec(t *testing } } +func TestValidateUnhealthyPodEvictionPolicyDisruptionBudgetSpec(t *testing.T) { + c1 := intstr.FromString("10%") + alwaysAllowPolicy := policy.AlwaysAllow + invalidPolicy := policy.UnhealthyPodEvictionPolicyType("Invalid") + + testCases := []struct { + name string + pdbSpec policy.PodDisruptionBudgetSpec + expectErr bool + }{ + { + name: "valid nil UnhealthyPodEvictionPolicy", + pdbSpec: policy.PodDisruptionBudgetSpec{ + MinAvailable: &c1, + UnhealthyPodEvictionPolicy: nil, + }, + expectErr: false, + }, + { + name: "valid UnhealthyPodEvictionPolicy", + pdbSpec: policy.PodDisruptionBudgetSpec{ + MinAvailable: &c1, + UnhealthyPodEvictionPolicy: &alwaysAllowPolicy, + }, + expectErr: false, + }, + { + name: "empty UnhealthyPodEvictionPolicy", + pdbSpec: policy.PodDisruptionBudgetSpec{ + MinAvailable: &c1, + UnhealthyPodEvictionPolicy: new(policy.UnhealthyPodEvictionPolicyType), + }, + expectErr: true, + }, + { + name: "invalid UnhealthyPodEvictionPolicy", + pdbSpec: policy.PodDisruptionBudgetSpec{ + MinAvailable: &c1, + UnhealthyPodEvictionPolicy: &invalidPolicy, + }, + expectErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + errs := ValidatePodDisruptionBudgetSpec(tc.pdbSpec, PodDisruptionBudgetValidationOptions{true}, field.NewPath("foo")) + if len(errs) == 0 && tc.expectErr { + t.Errorf("unexpected success for %v", tc.pdbSpec) + } + if len(errs) != 0 && !tc.expectErr { + t.Errorf("unexpected failure for %v", tc.pdbSpec) + } + }) + } +} + func TestValidatePodDisruptionBudgetStatus(t *testing.T) { const expectNoErrors = false const expectErrors = true diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index dd307cacd54..9139f4358ee 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -625,6 +625,13 @@ const ( // Permits kubelet to run with swap enabled NodeSwap featuregate.Feature = "NodeSwap" + // owner: @mortent, @atiratree, @ravig + // kep: http://kep.k8s.io/3018 + // alpha: v1.26 + // + // Enables PDBUnhealthyPodEvictionPolicy for PodDisruptionBudgets + PDBUnhealthyPodEvictionPolicy featuregate.Feature = "PDBUnhealthyPodEvictionPolicy" + // owner: @haircommander // kep: https://kep.k8s.io/2364 // alpha: v1.23 @@ -1045,6 +1052,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS NodeSwap: {Default: false, PreRelease: featuregate.Alpha}, + PDBUnhealthyPodEvictionPolicy: {Default: false, PreRelease: featuregate.Alpha}, + PodAndContainerStatsFromCRI: {Default: false, PreRelease: featuregate.Alpha}, PodDeletionCost: {Default: true, PreRelease: featuregate.Beta}, diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go index 844c27e475c..d87c2e35c32 100644 --- a/pkg/registry/core/pod/storage/eviction.go +++ b/pkg/registry/core/pod/storage/eviction.go @@ -226,10 +226,24 @@ func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Obje pdb := &pdbs[0] pdbName = pdb.Name - // If the pod is not ready, it doesn't count towards healthy and we should not decrement - if !podutil.IsPodReady(pod) && pdb.Status.CurrentHealthy >= pdb.Status.DesiredHealthy && pdb.Status.DesiredHealthy > 0 { - updateDeletionOptions = true - return nil + // 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. + // Application guarded by the PDB is not disrupted at the moment and deleting unhealthy (unready) pod will not disrupt it. + updateDeletionOptions = true + return nil + } + // confirm no disruptions allowed in checkAndDecrement } refresh := false @@ -264,12 +278,12 @@ func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Obje } // At this point there was either no PDB or we succeeded in decrementing or - // the pod was unready and we have enough healthy replicas + // the pod was unhealthy (unready) and we have enough healthy replicas deleteOptions := originalDeleteOptions // Set deleteOptions.Preconditions.ResourceVersion to ensure - // the pod hasn't been considered ready since we calculated + // the pod hasn't been considered healthy (ready) since we calculated if updateDeletionOptions { // Take a copy so we can compare to client-provied Options later. deleteOptions = deleteOptions.DeepCopy() @@ -351,7 +365,7 @@ func shouldEnforceResourceVersion(pod *api.Pod) bool { return false } // Return true for all other pods to ensure we don't race against a pod becoming - // ready and violating PDBs. + // healthy (ready) and violating PDBs. return true } diff --git a/pkg/registry/core/pod/storage/eviction_test.go b/pkg/registry/core/pod/storage/eviction_test.go index 92cc0a3c082..260b44d9983 100644 --- a/pkg/registry/core/pod/storage/eviction_test.go +++ b/pkg/registry/core/pod/storage/eviction_test.go @@ -19,6 +19,9 @@ package storage import ( "context" "errors" + "fmt" + "reflect" + "strings" "testing" policyv1 "k8s.io/api/policy/v1" @@ -31,21 +34,25 @@ import ( "k8s.io/apimachinery/pkg/watch" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/kubernetes/fake" + featuregatetesting "k8s.io/component-base/featuregate/testing" podapi "k8s.io/kubernetes/pkg/api/pod" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/policy" + "k8s.io/kubernetes/pkg/features" ) func TestEviction(t *testing.T) { testcases := []struct { name string pdbs []runtime.Object + policies []*policyv1.UnhealthyPodEvictionPolicyType eviction *policy.Eviction badNameInURL bool - expectError bool + expectError string expectDeleted bool podPhase api.PodPhase podName string @@ -58,9 +65,10 @@ func TestEviction(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t1", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo needs 0 healthy pods and has 0 currently", podPhase: api.PodRunning, podName: "t1", + policies: []*policyv1.UnhealthyPodEvictionPolicyType{nil, unhealthyPolicyPtr(policyv1.IfHealthyBudget)}, // AlwaysAllow would terminate the pod since Running pods are not guarded by this policy }, { name: "matching pdbs with no disruptions allowed, pod pending", @@ -70,7 +78,7 @@ func TestEviction(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t2", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: false, + expectError: "", podPhase: api.PodPending, expectDeleted: true, podName: "t2", @@ -83,7 +91,7 @@ func TestEviction(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t3", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: false, + expectError: "", podPhase: api.PodSucceeded, expectDeleted: true, podName: "t3", @@ -96,7 +104,7 @@ func TestEviction(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t4", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: false, + expectError: "", podPhase: api.PodFailed, expectDeleted: true, podName: "t4", @@ -132,91 +140,109 @@ func TestEviction(t *testing.T) { }}, badNameInURL: true, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t7", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: "name in URL does not match name in Eviction object: BadRequest", podName: "t7", }, } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) - storage, _, statusStorage, server := newStorage(t) - defer server.Terminate(t) - defer storage.Store.DestroyFunc() - - pod := validNewPod() - pod.Name = tc.podName - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - if _, err := storage.Create(testContext, pod, nil, &metav1.CreateOptions{}); err != nil { - t.Error(err) + for _, unhealthyPodEvictionPolicy := range []*policyv1.UnhealthyPodEvictionPolicyType{nil, unhealthyPolicyPtr(policyv1.IfHealthyBudget), unhealthyPolicyPtr(policyv1.AlwaysAllow)} { + for _, tc := range testcases { + if len(tc.policies) > 0 && !hasUnhealthyPolicy(tc.policies, unhealthyPodEvictionPolicy) { + // unhealthyPodEvictionPolicy is not covered by this test + continue } + t.Run(fmt.Sprintf("%v with %v policy", tc.name, unhealthyPolicyStr(unhealthyPodEvictionPolicy)), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, true)() - if tc.podPhase != "" { - pod.Status.Phase = tc.podPhase - _, _, err := statusStorage.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) - if err != nil { - t.Errorf("Unexpected error: %v", err) + // same test runs multiple times, make copy of objects to have unique ones + evictionCopy := tc.eviction.DeepCopy() + var pdbsCopy []runtime.Object + for _, pdb := range tc.pdbs { + pdbCopy := pdb.DeepCopyObject() + pdbCopy.(*policyv1.PodDisruptionBudget).Spec.UnhealthyPodEvictionPolicy = unhealthyPodEvictionPolicy + pdbsCopy = append(pdbsCopy, pdbCopy) } - } - client := fake.NewSimpleClientset(tc.pdbs...) - evictionRest := newEvictionStorage(storage.Store, client.PolicyV1()) + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) + storage, _, statusStorage, server := newStorage(t) + defer server.Terminate(t) + defer storage.Store.DestroyFunc() - name := pod.Name - if tc.badNameInURL { - name += "bad-name" - } + pod := validNewPod() + pod.Name = tc.podName + pod.Labels = map[string]string{"a": "true"} + pod.Spec.NodeName = "foo" + if _, err := storage.Create(testContext, pod, nil, &metav1.CreateOptions{}); err != nil { + t.Error(err) + } - _, err := evictionRest.Create(testContext, name, tc.eviction, nil, &metav1.CreateOptions{}) - //_, err = evictionRest.Create(testContext, name, tc.eviction, nil, &metav1.CreateOptions{}) - if (err != nil) != tc.expectError { - t.Errorf("expected error=%v, got %v; name %v", tc.expectError, err, pod.Name) - return - } - if tc.badNameInURL { - if err == nil { - t.Error("expected error here, but got nil") + if tc.podPhase != "" { + pod.Status.Phase = tc.podPhase + _, _, err := statusStorage.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + } + + client := fake.NewSimpleClientset(pdbsCopy...) + evictionRest := newEvictionStorage(storage.Store, client.PolicyV1()) + + name := pod.Name + if tc.badNameInURL { + name += "bad-name" + } + + _, err := evictionRest.Create(testContext, name, evictionCopy, nil, &metav1.CreateOptions{}) + gotErr := errToString(err) + if gotErr != tc.expectError { + t.Errorf("error mismatch: expected %v, got %v; name %v", tc.expectError, gotErr, pod.Name) return } - if err.Error() != "name in URL does not match name in Eviction object" { - t.Errorf("got unexpected error: %v", err) + if tc.badNameInURL { + if err == nil { + t.Error("expected error here, but got nil") + return + } + if err.Error() != "name in URL does not match name in Eviction object" { + t.Errorf("got unexpected error: %v", err) + } } - } - if tc.expectError { - return - } - - existingPod, err := storage.Get(testContext, pod.Name, &metav1.GetOptions{}) - if tc.expectDeleted { - if !apierrors.IsNotFound(err) { - t.Errorf("expected to be deleted, lookup returned %#v", existingPod) + if tc.expectError != "" { + return } - return - } else if apierrors.IsNotFound(err) { - t.Errorf("expected graceful deletion, got %v", err) - return - } - if err != nil { - t.Errorf("%#v", err) - return - } + existingPod, err := storage.Get(testContext, pod.Name, &metav1.GetOptions{}) + if tc.expectDeleted { + if !apierrors.IsNotFound(err) { + t.Errorf("expected to be deleted, lookup returned %#v", existingPod) + } + return + } else if apierrors.IsNotFound(err) { + t.Errorf("expected graceful deletion, got %v", err) + return + } - if existingPod.(*api.Pod).DeletionTimestamp == nil { - t.Errorf("expected gracefully deleted pod with deletionTimestamp set, got %#v", existingPod) - } - }) + if err != nil { + t.Errorf("%#v", err) + return + } + + if existingPod.(*api.Pod).DeletionTimestamp == nil { + t.Errorf("expected gracefully deleted pod with deletionTimestamp set, got %#v", existingPod) + } + }) + } } } -func TestEvictionIngorePDB(t *testing.T) { +func TestEvictionIgnorePDB(t *testing.T) { testcases := []struct { name string pdbs []runtime.Object + policies []*policyv1.UnhealthyPodEvictionPolicyType eviction *policy.Eviction - expectError bool + expectError string podPhase api.PodPhase podName string expectedDeleteCount int @@ -231,12 +257,12 @@ func TestEvictionIngorePDB(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t1", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: false, + expectError: "", podPhase: api.PodPending, podName: "t1", expectedDeleteCount: 3, }, - // This test case is critical. If it is removed or broken we may + // This test case is critical. If it is removed or broken we may // regress and allow a pod to be deleted without checking PDBs when the // pod should not be deleted. { @@ -247,10 +273,30 @@ func TestEvictionIngorePDB(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t2", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo needs 0 healthy pods and has 0 currently", podPhase: api.PodPending, podName: "t2", expectedDeleteCount: 1, + policies: []*policyv1.UnhealthyPodEvictionPolicyType{nil, unhealthyPolicyPtr(policyv1.IfHealthyBudget)}, // AlwaysAllow does not continueToPDBs, but straight to deletion + }, + { + name: "pdbs No disruptions allowed, pod pending, first delete conflict, pod becomes running, skip PDB check, conflict", + pdbs: []runtime.Object{&policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, + Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, + }}, + eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t2", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo is still being processed by the server.", + podPhase: api.PodPending, + podName: "t2", + podTerminating: false, + expectedDeleteCount: 2, + prc: &api.PodCondition{ + Type: api.PodReady, + Status: api.ConditionFalse, + }, + policies: []*policyv1.UnhealthyPodEvictionPolicyType{unhealthyPolicyPtr(policyv1.AlwaysAllow)}, // nil, IfHealthyBudget continueToPDBs }, { name: "pdbs disruptions allowed, pod pending, first delete conflict, pod becomes running, continueToPDBs", @@ -260,10 +306,11 @@ func TestEvictionIngorePDB(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 1}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t3", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: false, + expectError: "", podPhase: api.PodPending, podName: "t3", expectedDeleteCount: 2, + policies: []*policyv1.UnhealthyPodEvictionPolicyType{nil, unhealthyPolicyPtr(policyv1.IfHealthyBudget)}, // AlwaysAllow does not continueToPDBs, but straight to deletion if pod not healthy (ready) }, { name: "pod pending, always conflict on delete", @@ -273,7 +320,7 @@ func TestEvictionIngorePDB(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t4", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: `Operation cannot be fulfilled on tests "2": message: Conflict`, podPhase: api.PodPending, podName: "t4", expectedDeleteCount: EvictionsRetry.Steps, @@ -286,7 +333,7 @@ func TestEvictionIngorePDB(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t5", Namespace: "default"}, DeleteOptions: metav1.NewRVDeletionPrecondition("userProvided")}, - expectError: true, + expectError: `Operation cannot be fulfilled on tests "2": message: Conflict`, podPhase: api.PodPending, podName: "t5", expectedDeleteCount: 1, @@ -299,7 +346,7 @@ func TestEvictionIngorePDB(t *testing.T) { Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t6", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(300)}, - expectError: false, + expectError: "", podName: "t6", expectedDeleteCount: 1, podTerminating: true, @@ -310,14 +357,14 @@ func TestEvictionIngorePDB(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, Status: policyv1.PodDisruptionBudgetStatus{ - // This simulates 3 pods desired, our pod healthy, unhealthy pod is not ours. + // This simulates 3 pods expected, our pod healthy, unhealthy pod is not ours. DisruptionsAllowed: 0, CurrentHealthy: 2, DesiredHealthy: 2, }, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t7", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo needs 2 healthy pods and has 2 currently", podName: "t7", expectedDeleteCount: 0, podTerminating: false, @@ -327,20 +374,42 @@ func TestEvictionIngorePDB(t *testing.T) { Status: api.ConditionTrue, }, }, + { + name: "matching pdbs with disruptions allowed, pod running, pod healthy, healthy pod ours, deletes pod by honoring the PDB", + pdbs: []runtime.Object{&policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, + Status: policyv1.PodDisruptionBudgetStatus{ + DisruptionsAllowed: 1, + CurrentHealthy: 3, + DesiredHealthy: 3, + }, + }}, + eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t8", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, + expectError: "", + podName: "t8", + expectedDeleteCount: 1, + podTerminating: false, + podPhase: api.PodRunning, + prc: &api.PodCondition{ + Type: api.PodReady, + Status: api.ConditionTrue, + }, + }, { name: "matching pdbs with no disruptions allowed, pod running, pod unhealthy, unhealthy pod ours", pdbs: []runtime.Object{&policyv1.PodDisruptionBudget{ ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, Status: policyv1.PodDisruptionBudgetStatus{ - // This simulates 3 pods desired, our pod unhealthy + // This simulates 3 pods expected, our pod unhealthy DisruptionsAllowed: 0, CurrentHealthy: 2, DesiredHealthy: 2, }, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t8", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: false, + expectError: "", podName: "t8", expectedDeleteCount: 1, podTerminating: false, @@ -350,6 +419,25 @@ func TestEvictionIngorePDB(t *testing.T) { Status: api.ConditionFalse, }, }, + { + name: "matching pdbs with no disruptions allowed, pod running, pod unhealthy, unhealthy pod ours, skips the PDB check and deletes", + pdbs: []runtime.Object{&policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, + Status: policyv1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0}, + }}, + eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t8", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, + expectError: "", + podName: "t8", + expectedDeleteCount: 1, + podTerminating: false, + podPhase: api.PodRunning, + prc: &api.PodCondition{ + Type: api.PodReady, + Status: api.ConditionFalse, + }, + policies: []*policyv1.UnhealthyPodEvictionPolicyType{unhealthyPolicyPtr(policyv1.AlwaysAllow)}, // nil, IfHealthyBudget would not skip the PDB check + }, { // This case should return the 529 retry error. name: "matching pdbs with no disruptions allowed, pod running, pod unhealthy, unhealthy pod ours, resource version conflict", @@ -357,14 +445,14 @@ func TestEvictionIngorePDB(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, Status: policyv1.PodDisruptionBudgetStatus{ - // This simulates 3 pods desired, our pod unhealthy + // This simulates 3 pods expected, our pod unhealthy DisruptionsAllowed: 0, CurrentHealthy: 2, DesiredHealthy: 2, }, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t9", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo is still being processed by the server.", podName: "t9", expectedDeleteCount: 1, podTerminating: false, @@ -381,14 +469,14 @@ func TestEvictionIngorePDB(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, Status: policyv1.PodDisruptionBudgetStatus{ - // This simulates 3 pods desired, our pod unhealthy + // This simulates 3 pods expected, our pod unhealthy DisruptionsAllowed: 0, CurrentHealthy: 2, DesiredHealthy: 2, }, }}, eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t10", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, - expectError: true, + expectError: "test designed to error: BadRequest", podName: "t10", expectedDeleteCount: 1, podTerminating: false, @@ -400,50 +488,68 @@ func TestEvictionIngorePDB(t *testing.T) { }, } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) - ms := &mockStore{ - deleteCount: 0, + for _, unhealthyPodEvictionPolicy := range []*policyv1.UnhealthyPodEvictionPolicyType{unhealthyPolicyPtr(policyv1.AlwaysAllow), nil, unhealthyPolicyPtr(policyv1.IfHealthyBudget)} { + for _, tc := range testcases { + if len(tc.policies) > 0 && !hasUnhealthyPolicy(tc.policies, unhealthyPodEvictionPolicy) { + // unhealthyPodEvictionPolicy is not covered by this test + continue } + t.Run(fmt.Sprintf("%v with %v policy", tc.name, unhealthyPolicyStr(unhealthyPodEvictionPolicy)), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PDBUnhealthyPodEvictionPolicy, true)() - pod := validNewPod() - pod.Name = tc.podName - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - if tc.podPhase != "" { - pod.Status.Phase = tc.podPhase - } - - if tc.podTerminating { - currentTime := metav1.Now() - pod.ObjectMeta.DeletionTimestamp = ¤tTime - } - - // Setup pod condition - if tc.prc != nil { - if !podapi.UpdatePodCondition(&pod.Status, tc.prc) { - t.Fatalf("Unable to update pod ready condition") + // same test runs 3 times, make copy of objects to have unique ones + evictionCopy := tc.eviction.DeepCopy() + prcCopy := tc.prc.DeepCopy() + var pdbsCopy []runtime.Object + for _, pdb := range tc.pdbs { + pdbCopy := pdb.DeepCopyObject() + pdbCopy.(*policyv1.PodDisruptionBudget).Spec.UnhealthyPodEvictionPolicy = unhealthyPodEvictionPolicy + pdbsCopy = append(pdbsCopy, pdbCopy) } - } - client := fake.NewSimpleClientset(tc.pdbs...) - evictionRest := newEvictionStorage(ms, client.PolicyV1()) + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) + ms := &mockStore{ + deleteCount: 0, + } - name := pod.Name - ms.pod = pod + pod := validNewPod() + pod.Name = tc.podName + pod.Labels = map[string]string{"a": "true"} + pod.Spec.NodeName = "foo" + if tc.podPhase != "" { + pod.Status.Phase = tc.podPhase + } - _, err := evictionRest.Create(testContext, name, tc.eviction, nil, &metav1.CreateOptions{}) - if (err != nil) != tc.expectError { - t.Errorf("expected error=%v, got %v; name %v", tc.expectError, err, pod.Name) - return - } + if tc.podTerminating { + currentTime := metav1.Now() + pod.ObjectMeta.DeletionTimestamp = ¤tTime + } - if tc.expectedDeleteCount != ms.deleteCount { - t.Errorf("expected delete count=%v, got %v; name %v", tc.expectedDeleteCount, ms.deleteCount, pod.Name) - } + // Setup pod condition + if tc.prc != nil { + if !podapi.UpdatePodCondition(&pod.Status, prcCopy) { + t.Fatalf("Unable to update pod ready condition") + } + } - }) + client := fake.NewSimpleClientset(pdbsCopy...) + evictionRest := newEvictionStorage(ms, client.PolicyV1()) + + name := pod.Name + ms.pod = pod + + _, err := evictionRest.Create(testContext, name, evictionCopy, nil, &metav1.CreateOptions{}) + gotErr := errToString(err) + if gotErr != tc.expectError { + t.Errorf("error mismatch: expected %v, got %v; name %v", tc.expectError, gotErr, pod.Name) + return + } + + if tc.expectedDeleteCount != ms.deleteCount { + t.Errorf("expected delete count=%v, got %v; name %v", tc.expectedDeleteCount, ms.deleteCount, pod.Name) + } + }) + } } } @@ -692,3 +798,41 @@ func (ms *mockStore) ConvertToTable(ctx context.Context, object runtime.Object, func (ms *mockStore) Destroy() { } + +func unhealthyPolicyPtr(unhealthyPodEvictionPolicy policyv1.UnhealthyPodEvictionPolicyType) *policyv1.UnhealthyPodEvictionPolicyType { + return &unhealthyPodEvictionPolicy +} + +func unhealthyPolicyStr(unhealthyPodEvictionPolicy *policyv1.UnhealthyPodEvictionPolicyType) string { + if unhealthyPodEvictionPolicy == nil { + return "nil" + } + return string(*unhealthyPodEvictionPolicy) +} + +func hasUnhealthyPolicy(unhealthyPolicies []*policyv1.UnhealthyPodEvictionPolicyType, unhealthyPolicy *policyv1.UnhealthyPodEvictionPolicyType) bool { + for _, p := range unhealthyPolicies { + if reflect.DeepEqual(unhealthyPolicy, p) { + return true + } + } + return false +} + +func errToString(err error) string { + result := "" + if err != nil { + result = err.Error() + if statusErr, ok := err.(*apierrors.StatusError); ok { + result += fmt.Sprintf(": %v", statusErr.ErrStatus.Reason) + if statusErr.ErrStatus.Details != nil { + for _, cause := range statusErr.ErrStatus.Details.Causes { + if !strings.HasSuffix(err.Error(), cause.Message) { + result += fmt.Sprintf(": %v", cause.Message) + } + } + } + } + } + return result +} diff --git a/pkg/registry/policy/poddisruptionbudget/strategy.go b/pkg/registry/policy/poddisruptionbudget/strategy.go index 9be2b55ffd6..4938929cd32 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy.go @@ -26,9 +26,11 @@ 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" ) @@ -68,6 +70,8 @@ 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. @@ -83,6 +87,8 @@ 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. @@ -182,3 +188,23 @@ 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 3aef68575c0..7bc81802002 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy_test.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy_test.go @@ -17,15 +17,176 @@ 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" ) +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) { + defer 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) { + defer 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.FromInt(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 { + defer 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") @@ -210,3 +371,86 @@ 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) { + defer 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/staging/src/k8s.io/api/policy/v1/types.go b/staging/src/k8s.io/api/policy/v1/types.go index 4a03696f000..6aec30b89b2 100644 --- a/staging/src/k8s.io/api/policy/v1/types.go +++ b/staging/src/k8s.io/api/policy/v1/types.go @@ -47,8 +47,56 @@ type PodDisruptionBudgetSpec struct { // by specifying 0. This is a mutually exclusive setting with "minAvailable". // +optional MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty" protobuf:"bytes,3,opt,name=maxUnavailable"` + + // UnhealthyPodEvictionPolicy defines the criteria for when unhealthy pods + // should be considered for eviction. Current implementation considers healthy pods, + // as pods that have status.conditions item with type="Ready",status="True". + // + // Valid policies are IfHealthyBudget and AlwaysAllow. + // If no policy is specified, the default behavior will be used, + // which corresponds to the IfHealthyBudget policy. + // + // IfHealthyBudget policy means that running pods (status.phase="Running"), + // but not yet healthy can be evicted only if the guarded application is not + // disrupted (status.currentHealthy is at least equal to status.desiredHealthy). + // Healthy pods will be subject to the PDB for eviction. + // + // AlwaysAllow policy means that all running pods (status.phase="Running"), + // but not yet healthy are considered disrupted and can be evicted regardless + // of whether the criteria in a PDB is met. This means perspective running + // pods of a disrupted application might not get a chance to become healthy. + // Healthy pods will be subject to the PDB for eviction. + // + // Additional policies may be added in the future. + // Clients making eviction decisions should disallow eviction of unhealthy pods + // if they encounter an unrecognized policy in this field. + // + // This field is alpha-level. The eviction API uses this field when + // the feature gate PDBUnhealthyPodEvictionPolicy is enabled (disabled by default). + // +optional + UnhealthyPodEvictionPolicy *UnhealthyPodEvictionPolicyType `json:"unhealthyPodEvictionPolicy,omitempty" protobuf:"bytes,4,opt,name=unhealthyPodEvictionPolicy"` } +// UnhealthyPodEvictionPolicyType defines the criteria for when unhealthy pods +// should be considered for eviction. +// +enum +type UnhealthyPodEvictionPolicyType string + +const ( + // IfHealthyBudget policy means that running pods (status.phase="Running"), + // but not yet healthy can be evicted only if the guarded application is not + // disrupted (status.currentHealthy is at least equal to status.desiredHealthy). + // Healthy pods will be subject to the PDB for eviction. + IfHealthyBudget UnhealthyPodEvictionPolicyType = "IfHealthyBudget" + + // AlwaysAllow policy means that all running pods (status.phase="Running"), + // but not yet healthy are considered disrupted and can be evicted regardless + // of whether the criteria in a PDB is met. This means perspective running + // pods of a disrupted application might not get a chance to become healthy. + // Healthy pods will be subject to the PDB for eviction. + AlwaysAllow UnhealthyPodEvictionPolicyType = "AlwaysAllow" +) + // PodDisruptionBudgetStatus represents information about the status of a // PodDisruptionBudget. Status may trail the actual state of a system. type PodDisruptionBudgetStatus struct { diff --git a/staging/src/k8s.io/api/policy/v1beta1/types.go b/staging/src/k8s.io/api/policy/v1beta1/types.go index 222b4664b4c..863b2b87323 100644 --- a/staging/src/k8s.io/api/policy/v1beta1/types.go +++ b/staging/src/k8s.io/api/policy/v1beta1/types.go @@ -45,8 +45,56 @@ type PodDisruptionBudgetSpec struct { // by specifying 0. This is a mutually exclusive setting with "minAvailable". // +optional MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty" protobuf:"bytes,3,opt,name=maxUnavailable"` + + // UnhealthyPodEvictionPolicy defines the criteria for when unhealthy pods + // should be considered for eviction. Current implementation considers healthy pods, + // as pods that have status.conditions item with type="Ready",status="True". + // + // Valid policies are IfHealthyBudget and AlwaysAllow. + // If no policy is specified, the default behavior will be used, + // which corresponds to the IfHealthyBudget policy. + // + // IfHealthyBudget policy means that running pods (status.phase="Running"), + // but not yet healthy can be evicted only if the guarded application is not + // disrupted (status.currentHealthy is at least equal to status.desiredHealthy). + // Healthy pods will be subject to the PDB for eviction. + // + // AlwaysAllow policy means that all running pods (status.phase="Running"), + // but not yet healthy are considered disrupted and can be evicted regardless + // of whether the criteria in a PDB is met. This means perspective running + // pods of a disrupted application might not get a chance to become healthy. + // Healthy pods will be subject to the PDB for eviction. + // + // Additional policies may be added in the future. + // Clients making eviction decisions should disallow eviction of unhealthy pods + // if they encounter an unrecognized policy in this field. + // + // This field is alpha-level. The eviction API uses this field when + // the feature gate PDBUnhealthyPodEvictionPolicy is enabled (disabled by default). + // +optional + UnhealthyPodEvictionPolicy *UnhealthyPodEvictionPolicyType `json:"unhealthyPodEvictionPolicy,omitempty" protobuf:"bytes,4,opt,name=unhealthyPodEvictionPolicy"` } +// UnhealthyPodEvictionPolicyType defines the criteria for when unhealthy pods +// should be considered for eviction. +// +enum +type UnhealthyPodEvictionPolicyType string + +const ( + // IfHealthyBudget policy means that running pods (status.phase="Running"), + // but not yet healthy can be evicted only if the guarded application is not + // disrupted (status.currentHealthy is at least equal to status.desiredHealthy). + // Healthy pods will be subject to the PDB for eviction. + IfHealthyBudget UnhealthyPodEvictionPolicyType = "IfHealthyBudget" + + // AlwaysAllow policy means that all running pods (status.phase="Running"), + // but not yet healthy are considered disrupted and can be evicted regardless + // of whether the criteria in a PDB is met. This means perspective running + // pods of a disrupted application might not get a chance to become healthy. + // Healthy pods will be subject to the PDB for eviction. + AlwaysAllow UnhealthyPodEvictionPolicyType = "AlwaysAllow" +) + // PodDisruptionBudgetStatus represents information about the status of a // PodDisruptionBudget. Status may trail the actual state of a system. type PodDisruptionBudgetStatus struct {