diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index 59d1782562d..6640735dd8c 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/pkg/apis/apps/validation/validation.go @@ -606,6 +606,9 @@ func ValidateDeploymentStatus(status *apps.DeploymentStatus, fldPath *field.Path allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.ReadyReplicas), fldPath.Child("readyReplicas"))...) allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.AvailableReplicas), fldPath.Child("availableReplicas"))...) allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.UnavailableReplicas), fldPath.Child("unavailableReplicas"))...) + if status.TerminatingReplicas != nil { + allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*status.TerminatingReplicas), fldPath.Child("terminatingReplicas"))...) + } if status.CollisionCount != nil { allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*status.CollisionCount), fldPath.Child("collisionCount"))...) } @@ -702,6 +705,9 @@ func ValidateReplicaSetStatus(status apps.ReplicaSetStatus, fldPath *field.Path) allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.ReadyReplicas), fldPath.Child("readyReplicas"))...) allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.AvailableReplicas), fldPath.Child("availableReplicas"))...) allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.ObservedGeneration), fldPath.Child("observedGeneration"))...) + if status.TerminatingReplicas != nil { + allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*status.TerminatingReplicas), fldPath.Child("terminatingReplicas"))...) + } msg := "cannot be greater than status.replicas" if status.FullyLabeledReplicas > status.Replicas { allErrs = append(allErrs, field.Invalid(fldPath.Child("fullyLabeledReplicas"), status.FullyLabeledReplicas, msg)) diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go index 876a1bafaeb..a25da98e3fd 100644 --- a/pkg/apis/apps/validation/validation_test.go +++ b/pkg/apis/apps/validation/validation_test.go @@ -2334,59 +2334,84 @@ func TestValidateDeploymentStatus(t *testing.T) { tests := []struct { name string - replicas int32 - updatedReplicas int32 - readyReplicas int32 - availableReplicas int32 - observedGeneration int64 - collisionCount *int32 + replicas int32 + updatedReplicas int32 + readyReplicas int32 + availableReplicas int32 + terminatingReplicas *int32 + observedGeneration int64 + collisionCount *int32 expectedErr bool }{{ - name: "valid status", - replicas: 3, - updatedReplicas: 3, - readyReplicas: 2, - availableReplicas: 1, - observedGeneration: 2, - expectedErr: false, + name: "valid status", + replicas: 3, + updatedReplicas: 3, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: false, }, { - name: "invalid replicas", - replicas: -1, - updatedReplicas: 2, - readyReplicas: 2, - availableReplicas: 1, - observedGeneration: 2, - expectedErr: true, + name: "valid status with terminating replicas", + replicas: 3, + updatedReplicas: 3, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: ptr.To[int32](5), + observedGeneration: 2, + expectedErr: false, }, { - name: "invalid updatedReplicas", - replicas: 2, - updatedReplicas: -1, - readyReplicas: 2, - availableReplicas: 1, - observedGeneration: 2, - expectedErr: true, + name: "invalid replicas", + replicas: -1, + updatedReplicas: 2, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: true, }, { - name: "invalid readyReplicas", - replicas: 3, - readyReplicas: -1, - availableReplicas: 1, - observedGeneration: 2, - expectedErr: true, + name: "invalid updatedReplicas", + replicas: 2, + updatedReplicas: -1, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: true, }, { - name: "invalid availableReplicas", - replicas: 3, - readyReplicas: 3, - availableReplicas: -1, - observedGeneration: 2, - expectedErr: true, + name: "invalid readyReplicas", + replicas: 3, + readyReplicas: -1, + availableReplicas: 1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: true, }, { - name: "invalid observedGeneration", - replicas: 3, - readyReplicas: 3, - availableReplicas: 3, - observedGeneration: -1, - expectedErr: true, + name: "invalid availableReplicas", + replicas: 3, + readyReplicas: 3, + availableReplicas: -1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: true, + }, { + name: "invalid terminatingReplicas", + replicas: 3, + updatedReplicas: 3, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: ptr.To[int32](-1), + observedGeneration: 2, + expectedErr: true, + }, { + name: "invalid observedGeneration", + replicas: 3, + readyReplicas: 3, + availableReplicas: 3, + terminatingReplicas: nil, + observedGeneration: -1, + expectedErr: true, }, { name: "updatedReplicas greater than replicas", replicas: 3, @@ -2427,12 +2452,13 @@ func TestValidateDeploymentStatus(t *testing.T) { for _, test := range tests { status := apps.DeploymentStatus{ - Replicas: test.replicas, - UpdatedReplicas: test.updatedReplicas, - ReadyReplicas: test.readyReplicas, - AvailableReplicas: test.availableReplicas, - ObservedGeneration: test.observedGeneration, - CollisionCount: test.collisionCount, + Replicas: test.replicas, + UpdatedReplicas: test.updatedReplicas, + ReadyReplicas: test.readyReplicas, + AvailableReplicas: test.availableReplicas, + TerminatingReplicas: test.terminatingReplicas, + ObservedGeneration: test.observedGeneration, + CollisionCount: test.collisionCount, } errs := ValidateDeploymentStatus(&status, field.NewPath("status")) @@ -2735,6 +2761,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas int32 readyReplicas int32 availableReplicas int32 + terminatingReplicas *int32 observedGeneration int64 expectedErr bool @@ -2744,6 +2771,16 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 2, availableReplicas: 1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: false, + }, { + name: "valid status with terminating replicas", + replicas: 3, + fullyLabeledReplicas: 3, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: ptr.To[int32](5), observedGeneration: 2, expectedErr: false, }, { @@ -2752,6 +2789,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 2, availableReplicas: 1, + terminatingReplicas: nil, observedGeneration: 2, expectedErr: true, }, { @@ -2760,6 +2798,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: -1, readyReplicas: 2, availableReplicas: 1, + terminatingReplicas: nil, observedGeneration: 2, expectedErr: true, }, { @@ -2768,6 +2807,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: -1, availableReplicas: 1, + terminatingReplicas: nil, observedGeneration: 2, expectedErr: true, }, { @@ -2776,6 +2816,16 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 3, availableReplicas: -1, + terminatingReplicas: nil, + observedGeneration: 2, + expectedErr: true, + }, { + name: "invalid terminatingReplicas", + replicas: 3, + fullyLabeledReplicas: 3, + readyReplicas: 2, + availableReplicas: 1, + terminatingReplicas: ptr.To[int32](-1), observedGeneration: 2, expectedErr: true, }, { @@ -2784,6 +2834,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 3, availableReplicas: 3, + terminatingReplicas: nil, observedGeneration: -1, expectedErr: true, }, { @@ -2792,6 +2843,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 4, readyReplicas: 3, availableReplicas: 3, + terminatingReplicas: nil, observedGeneration: 1, expectedErr: true, }, { @@ -2800,6 +2852,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 4, availableReplicas: 3, + terminatingReplicas: nil, observedGeneration: 1, expectedErr: true, }, { @@ -2808,6 +2861,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 3, availableReplicas: 4, + terminatingReplicas: nil, observedGeneration: 1, expectedErr: true, }, { @@ -2816,6 +2870,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { fullyLabeledReplicas: 3, readyReplicas: 2, availableReplicas: 3, + terminatingReplicas: nil, observedGeneration: 1, expectedErr: true, }, @@ -2827,6 +2882,7 @@ func TestValidateReplicaSetStatus(t *testing.T) { FullyLabeledReplicas: test.fullyLabeledReplicas, ReadyReplicas: test.readyReplicas, AvailableReplicas: test.availableReplicas, + TerminatingReplicas: test.terminatingReplicas, ObservedGeneration: test.observedGeneration, } diff --git a/pkg/registry/apps/deployment/strategy.go b/pkg/registry/apps/deployment/strategy.go index b3e447c302e..d8ca5ef2c30 100644 --- a/pkg/registry/apps/deployment/strategy.go +++ b/pkg/registry/apps/deployment/strategy.go @@ -31,10 +31,12 @@ import ( genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/api/pod" "k8s.io/kubernetes/pkg/apis/apps" appsvalidation "k8s.io/kubernetes/pkg/apis/apps/validation" + "k8s.io/kubernetes/pkg/features" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) @@ -192,6 +194,7 @@ func (deploymentStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old r oldDeployment := old.(*apps.Deployment) newDeployment.Spec = oldDeployment.Spec newDeployment.Labels = oldDeployment.Labels + dropDisabledStatusFields(&newDeployment.Status, &oldDeployment.Status) } // ValidateUpdate is the default update validation for an end user updating status @@ -203,3 +206,11 @@ func (deploymentStatusStrategy) ValidateUpdate(ctx context.Context, obj, old run func (deploymentStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { return nil } + +// dropDisabledStatusFields removes disabled fields from the deployment status. +func dropDisabledStatusFields(deploymentStatus, oldDeploymentStatus *apps.DeploymentStatus) { + if !utilfeature.DefaultFeatureGate.Enabled(features.DeploymentPodReplacementPolicy) && + (oldDeploymentStatus == nil || oldDeploymentStatus.TerminatingReplicas == nil) { + deploymentStatus.TerminatingReplicas = nil + } +} diff --git a/pkg/registry/apps/deployment/strategy_test.go b/pkg/registry/apps/deployment/strategy_test.go index 68e11e79464..d514783765a 100644 --- a/pkg/registry/apps/deployment/strategy_test.go +++ b/pkg/registry/apps/deployment/strategy_test.go @@ -17,6 +17,7 @@ limitations under the License. package deployment import ( + "k8s.io/utils/ptr" "reflect" "testing" @@ -26,9 +27,12 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" podtest "k8s.io/kubernetes/pkg/api/pod/testing" "k8s.io/kubernetes/pkg/apis/apps" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" ) const ( @@ -62,6 +66,70 @@ func TestStatusUpdates(t *testing.T) { } } +func TestStatusUpdatesWithDeploymentPodReplacementPolicy(t *testing.T) { + tests := []struct { + name string + enableDeploymentPodReplacementPolicy bool + terminatingReplicas *int32 + terminatingReplicasUpdate *int32 + expectedTerminatingReplicas *int32 + }{ + { + name: "should not allow updates when feature gate is disabled", + enableDeploymentPodReplacementPolicy: false, + terminatingReplicas: nil, + terminatingReplicasUpdate: ptr.To[int32](2), + expectedTerminatingReplicas: nil, + }, + { + name: "should allow update when the field is in use when feature gate is disabled", + enableDeploymentPodReplacementPolicy: false, + terminatingReplicas: ptr.To[int32](2), + terminatingReplicasUpdate: ptr.To[int32](5), + expectedTerminatingReplicas: ptr.To[int32](5), + }, + { + name: "should allow updates when feature gate is enabled", + enableDeploymentPodReplacementPolicy: true, + terminatingReplicas: nil, + terminatingReplicasUpdate: ptr.To[int32](2), + expectedTerminatingReplicas: ptr.To[int32](2), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeploymentPodReplacementPolicy, tc.enableDeploymentPodReplacementPolicy) + + ctx := genericapirequest.NewDefaultContext() + validSelector := map[string]string{"a": "b"} + oldDeploy := newDeploymentWithSelectorLabels(validSelector) + oldDeploy.Spec.Replicas = 3 + oldDeploy.Status.Replicas = 3 + oldDeploy.Status.TerminatingReplicas = tc.terminatingReplicas + + newDeploy := newDeploymentWithSelectorLabels(validSelector) + newDeploy.Spec.Replicas = 3 + newDeploy.Status.Replicas = 2 + newDeploy.Status.TerminatingReplicas = tc.terminatingReplicasUpdate + + StatusStrategy.PrepareForUpdate(ctx, newDeploy, oldDeploy) + if newDeploy.Status.Replicas != 2 { + t.Errorf("ReplicaSet status updates should allow change of replicas: %v", newDeploy.Status.Replicas) + } + if !ptr.Equal(newDeploy.Status.TerminatingReplicas, tc.expectedTerminatingReplicas) { + t.Errorf("ReplicaSet status updates failed, expected terminating pods: %v, got: %v", ptr.Deref(tc.expectedTerminatingReplicas, -1), ptr.Deref(newDeploy.Status.TerminatingReplicas, -1)) + } + + errs := StatusStrategy.ValidateUpdate(ctx, newDeploy, oldDeploy) + + if len(errs) != 0 { + t.Errorf("Unexpected error %v", errs) + } + }) + } +} + func newDeployment(labels, annotations map[string]string) *apps.Deployment { return &apps.Deployment{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/registry/apps/replicaset/strategy.go b/pkg/registry/apps/replicaset/strategy.go index 6d090440dee..e795cc73587 100644 --- a/pkg/registry/apps/replicaset/strategy.go +++ b/pkg/registry/apps/replicaset/strategy.go @@ -37,10 +37,12 @@ import ( "k8s.io/apiserver/pkg/registry/rest" apistorage "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/api/pod" "k8s.io/kubernetes/pkg/apis/apps" appsvalidation "k8s.io/kubernetes/pkg/apis/apps/validation" + "k8s.io/kubernetes/pkg/features" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) @@ -231,6 +233,7 @@ func (rsStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.O oldRS := old.(*apps.ReplicaSet) // update is not allowed to set spec newRS.Spec = oldRS.Spec + dropDisabledStatusFields(&newRS.Status, &oldRS.Status) } func (rsStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { @@ -241,3 +244,11 @@ func (rsStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj func (rsStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { return nil } + +// dropDisabledStatusFields removes disabled fields from the replica set status. +func dropDisabledStatusFields(rsStatus, oldRSStatus *apps.ReplicaSetStatus) { + if !utilfeature.DefaultFeatureGate.Enabled(features.DeploymentPodReplacementPolicy) && + (oldRSStatus == nil || oldRSStatus.TerminatingReplicas == nil) { + rsStatus.TerminatingReplicas = nil + } +} diff --git a/pkg/registry/apps/replicaset/strategy_test.go b/pkg/registry/apps/replicaset/strategy_test.go index 01cd2404903..5bf8d46b326 100644 --- a/pkg/registry/apps/replicaset/strategy_test.go +++ b/pkg/registry/apps/replicaset/strategy_test.go @@ -17,15 +17,19 @@ limitations under the License. package replicaset import ( + "k8s.io/utils/ptr" "reflect" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" podtest "k8s.io/kubernetes/pkg/api/pod/testing" "k8s.io/kubernetes/pkg/apis/apps" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" ) const ( @@ -148,6 +152,70 @@ func TestReplicaSetStatusStrategy(t *testing.T) { } } +func TestReplicaSetStatusStrategyWithDeploymentPodReplacementPolicy(t *testing.T) { + tests := []struct { + name string + enableDeploymentPodReplacementPolicy bool + terminatingReplicas *int32 + terminatingReplicasUpdate *int32 + expectedTerminatingReplicas *int32 + }{ + { + name: "should not allow updates when feature gate is disabled", + enableDeploymentPodReplacementPolicy: false, + terminatingReplicas: nil, + terminatingReplicasUpdate: ptr.To[int32](2), + expectedTerminatingReplicas: nil, + }, + { + name: "should allow update when the field is in use when feature gate is disabled", + enableDeploymentPodReplacementPolicy: false, + terminatingReplicas: ptr.To[int32](2), + terminatingReplicasUpdate: ptr.To[int32](5), + expectedTerminatingReplicas: ptr.To[int32](5), + }, + { + name: "should allow updates when feature gate is enabled", + enableDeploymentPodReplacementPolicy: true, + terminatingReplicas: nil, + terminatingReplicasUpdate: ptr.To[int32](2), + expectedTerminatingReplicas: ptr.To[int32](2), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeploymentPodReplacementPolicy, tc.enableDeploymentPodReplacementPolicy) + + ctx := genericapirequest.NewDefaultContext() + validSelector := map[string]string{"a": "b"} + oldRS := newReplicaSetWithSelectorLabels(validSelector) + oldRS.Spec.Replicas = 3 + oldRS.Status.Replicas = 3 + oldRS.Status.TerminatingReplicas = tc.terminatingReplicas + + newRS := newReplicaSetWithSelectorLabels(validSelector) + newRS.Spec.Replicas = 3 + newRS.Status.Replicas = 2 + newRS.Status.TerminatingReplicas = tc.terminatingReplicasUpdate + + StatusStrategy.PrepareForUpdate(ctx, newRS, oldRS) + if newRS.Status.Replicas != 2 { + t.Errorf("ReplicaSet status updates should allow change of replicas: %v", newRS.Status.Replicas) + } + if !ptr.Equal(newRS.Status.TerminatingReplicas, tc.expectedTerminatingReplicas) { + t.Errorf("ReplicaSet status updates failed, expected terminating pods: %v, got: %v", ptr.Deref(tc.expectedTerminatingReplicas, -1), ptr.Deref(newRS.Status.TerminatingReplicas, -1)) + } + + errs := StatusStrategy.ValidateUpdate(ctx, newRS, oldRS) + + if len(errs) != 0 { + t.Errorf("Unexpected error %v", errs) + } + }) + } +} + func TestSelectorImmutability(t *testing.T) { tests := []struct { requestInfo genericapirequest.RequestInfo