Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Promote minReadySeconds to GA #110896

Merged
merged 3 commits into from
Jul 14, 2022
Merged

Promote minReadySeconds to GA #110896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9144250
Promote minReadySeconds to GA
ravisantoshgudimetla Jun 30, 2022
05f20dc
api: Promote statefulset MinReadySeconds to GA
ravisantoshgudimetla Jul 1, 2022
9ab807f
Generated: minReadySeconds for STS
ravisantoshgudimetla Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions api/openapi-spec/v3/apis__apps__v1_openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@
"description": "A StatefulSetSpec is the specification of a StatefulSet.",
"properties": {
"minReadySeconds": {
"description": "Minimum number of seconds for which a newly created pod should be ready without any of its container crashing for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready) This is an alpha field and requires enabling StatefulSetMinReadySeconds feature gate.",
"description": "Minimum number of seconds for which a newly created pod should be ready without any of its container crashing for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)",
"format": "int32",
"type": "integer"
},
Expand Down Expand Up @@ -1144,7 +1144,7 @@
"properties": {
"availableReplicas": {
"default": 0,
"description": "Total number of available pods (ready for at least minReadySeconds) targeted by this statefulset. This is a beta field and enabled/disabled by StatefulSetMinReadySeconds feature gate.",
"description": "Total number of available pods (ready for at least minReadySeconds) targeted by this statefulset.",
"format": "int32",
"type": "integer"
},
Expand Down
2 changes: 0 additions & 2 deletions pkg/apis/apps/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ type StatefulSetSpec struct {
// Minimum number of seconds for which a newly created pod should be ready
// without any of its container crashing for it to be considered available.
// Defaults to 0 (pod will be considered available as soon as it is ready)
// This is an alpha field and requires enabling StatefulSetMinReadySeconds feature gate.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt - Should I open backports to make this beta in previous releases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably so, since the API docs are incorrect

// +optional
MinReadySeconds int32

Expand Down Expand Up @@ -256,7 +255,6 @@ type StatefulSetStatus struct {
Conditions []StatefulSetCondition

// Total number of available pods (ready for at least minReadySeconds) targeted by this statefulset.
// This is a beta field and requires enabling StatefulSetMinReadySeconds feature gate.
// +optional
AvailableReplicas int32
}
Expand Down
39 changes: 14 additions & 25 deletions pkg/apis/apps/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,8 @@ func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, op
allErrs = append(allErrs, ValidatePersistentVolumeClaimRetentionPolicy(spec.PersistentVolumeClaimRetentionPolicy, fldPath.Child("persistentVolumeClaimRetentionPolicy"))...)

allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...)
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) {
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...)
}
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...)

if spec.Selector == nil {
allErrs = append(allErrs, field.Required(fldPath.Child("selector"), ""))
} else {
Expand Down Expand Up @@ -175,19 +174,14 @@ func ValidateStatefulSetUpdate(statefulSet, oldStatefulSet *apps.StatefulSet, op
// statefulset updates aren't super common and general updates are likely to be touching spec, so we'll do this
// deep copy right away. This avoids mutating our inputs
newStatefulSetClone := statefulSet.DeepCopy()
newStatefulSetClone.Spec.Replicas = oldStatefulSet.Spec.Replicas // +k8s:verify-mutation:reason=clone
newStatefulSetClone.Spec.Template = oldStatefulSet.Spec.Template // +k8s:verify-mutation:reason=clone
newStatefulSetClone.Spec.UpdateStrategy = oldStatefulSet.Spec.UpdateStrategy // +k8s:verify-mutation:reason=clone
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) {
newStatefulSetClone.Spec.MinReadySeconds = oldStatefulSet.Spec.MinReadySeconds // +k8s:verify-mutation:reason=clone
}
newStatefulSetClone.Spec.Replicas = oldStatefulSet.Spec.Replicas // +k8s:verify-mutation:reason=clone
newStatefulSetClone.Spec.Template = oldStatefulSet.Spec.Template // +k8s:verify-mutation:reason=clone
newStatefulSetClone.Spec.UpdateStrategy = oldStatefulSet.Spec.UpdateStrategy // +k8s:verify-mutation:reason=clone
newStatefulSetClone.Spec.MinReadySeconds = oldStatefulSet.Spec.MinReadySeconds // +k8s:verify-mutation:reason=clone

newStatefulSetClone.Spec.PersistentVolumeClaimRetentionPolicy = oldStatefulSet.Spec.PersistentVolumeClaimRetentionPolicy // +k8s:verify-mutation:reason=clone
if !apiequality.Semantic.DeepEqual(newStatefulSetClone.Spec, oldStatefulSet.Spec) {
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden"))
} else {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy' and 'persistentVolumeClaimRetentionPolicy' are forbidden"))
}
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden"))
}

return allErrs
Expand All @@ -201,9 +195,7 @@ func ValidateStatefulSetStatus(status *apps.StatefulSetStatus, fieldPath *field.
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.ReadyReplicas), fieldPath.Child("readyReplicas"))...)
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.CurrentReplicas), fieldPath.Child("currentReplicas"))...)
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.UpdatedReplicas), fieldPath.Child("updatedReplicas"))...)
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) {
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.AvailableReplicas), fieldPath.Child("availableReplicas"))...)
}
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.AvailableReplicas), fieldPath.Child("availableReplicas"))...)
if status.ObservedGeneration != nil {
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*status.ObservedGeneration), fieldPath.Child("observedGeneration"))...)
}
Expand All @@ -221,15 +213,12 @@ func ValidateStatefulSetStatus(status *apps.StatefulSetStatus, fieldPath *field.
if status.UpdatedReplicas > status.Replicas {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("updatedReplicas"), status.UpdatedReplicas, msg))
}
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) {
if status.AvailableReplicas > status.Replicas {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("availableReplicas"), status.AvailableReplicas, msg))
}
if status.AvailableReplicas > status.ReadyReplicas {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("availableReplicas"), status.AvailableReplicas, "cannot be greater than readyReplicas"))
}
if status.AvailableReplicas > status.Replicas {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("availableReplicas"), status.AvailableReplicas, msg))
}
if status.AvailableReplicas > status.ReadyReplicas {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("availableReplicas"), status.AvailableReplicas, "cannot be greater than status.readyReplicas"))
}

return allErrs
}

Expand Down
117 changes: 37 additions & 80 deletions pkg/apis/apps/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,39 +690,28 @@ func generateStatefulSetSpec(minSeconds int32) *apps.StatefulSetSpec {
// TestValidateStatefulSetMinReadySeconds tests the StatefulSet Spec's minReadySeconds field
func TestValidateStatefulSetMinReadySeconds(t *testing.T) {
testCases := map[string]struct {
ss *apps.StatefulSetSpec
enableMinReadySeconds bool
expectErr bool
ss *apps.StatefulSetSpec
expectErr bool
}{
"valid : minReadySeconds enabled, zero": {
ss: generateStatefulSetSpec(0),
enableMinReadySeconds: true,
expectErr: false,
ss: generateStatefulSetSpec(0),
expectErr: false,
},
"invalid : minReadySeconds enabled, negative": {
ss: generateStatefulSetSpec(-1),
enableMinReadySeconds: true,
expectErr: true,
ss: generateStatefulSetSpec(-1),
expectErr: true,
},
"valid : minReadySeconds enabled, very large value": {
ss: generateStatefulSetSpec(2147483647),
enableMinReadySeconds: true,
expectErr: false,
ss: generateStatefulSetSpec(2147483647),
expectErr: false,
},
"invalid : minReadySeconds enabled, large negative": {
ss: generateStatefulSetSpec(-2147483648),
enableMinReadySeconds: true,
expectErr: true,
},
"valid : minReadySeconds disabled, we don't validate anything": {
ss: generateStatefulSetSpec(-2147483648),
enableMinReadySeconds: false,
expectErr: false,
ss: generateStatefulSetSpec(-2147483648),
expectErr: true,
},
}
for tcName, tc := range testCases {
t.Run(tcName, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StatefulSetMinReadySeconds, tc.enableMinReadySeconds)()
errs := ValidateStatefulSetSpec(tc.ss, field.NewPath("spec", "minReadySeconds"),
corevalidation.PodValidationOptions{})
if tc.expectErr && len(errs) == 0 {
Expand All @@ -739,16 +728,15 @@ func TestValidateStatefulSetStatus(t *testing.T) {
observedGenerationMinusOne := int64(-1)
collisionCountMinusOne := int32(-1)
tests := []struct {
name string
replicas int32
readyReplicas int32
currentReplicas int32
updatedReplicas int32
availableReplicas int32
enableMinReadySeconds bool
observedGeneration *int64
collisionCount *int32
expectedErr bool
name string
replicas int32
readyReplicas int32
currentReplicas int32
updatedReplicas int32
availableReplicas int32
observedGeneration *int64
collisionCount *int32
expectedErr bool
}{
{
name: "valid status",
Expand Down Expand Up @@ -833,64 +821,33 @@ func TestValidateStatefulSetStatus(t *testing.T) {
expectedErr: true,
},
{
name: "invalid: number of available replicas",
replicas: 3,
readyReplicas: 3,
currentReplicas: 2,
availableReplicas: int32(-1),
expectedErr: true,
enableMinReadySeconds: true,
},
{
name: "invalid: available replicas greater than replicas",
replicas: 3,
readyReplicas: 3,
currentReplicas: 2,
availableReplicas: int32(4),
expectedErr: true,
enableMinReadySeconds: true,
},
{
name: "invalid: available replicas greater than ready replicas",
replicas: 3,
readyReplicas: 2,
currentReplicas: 2,
availableReplicas: int32(3),
expectedErr: true,
enableMinReadySeconds: true,
},
{
name: "minReadySeconds flag not set, no validation: number of available replicas",
replicas: 3,
readyReplicas: 3,
currentReplicas: 2,
availableReplicas: int32(-1),
expectedErr: false,
enableMinReadySeconds: false,
name: "invalid: number of available replicas",
replicas: 3,
readyReplicas: 3,
currentReplicas: 2,
availableReplicas: int32(-1),
expectedErr: true,
},
{
name: "minReadySeconds flag not set, no validation: available replicas greater than replicas",
replicas: 3,
readyReplicas: 3,
currentReplicas: 2,
availableReplicas: int32(4),
expectedErr: false,
enableMinReadySeconds: false,
name: "invalid: available replicas greater than replicas",
replicas: 3,
readyReplicas: 3,
currentReplicas: 2,
availableReplicas: int32(4),
expectedErr: true,
},
{
name: "minReadySeconds flag not set, no validation: available replicas greater than ready replicas",
replicas: 3,
readyReplicas: 2,
currentReplicas: 2,
availableReplicas: int32(3),
expectedErr: false,
enableMinReadySeconds: false,
name: "invalid: available replicas greater than ready replicas",
replicas: 3,
readyReplicas: 2,
currentReplicas: 2,
availableReplicas: int32(3),
expectedErr: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StatefulSetMinReadySeconds, test.enableMinReadySeconds)()
status := apps.StatefulSetStatus{
Replicas: test.replicas,
ReadyReplicas: test.readyReplicas,
Expand Down
6 changes: 2 additions & 4 deletions pkg/controller/statefulset/stateful_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"k8s.io/apimachinery/pkg/labels"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait"
utilfeature "k8s.io/apiserver/pkg/util/feature"
appsinformers "k8s.io/client-go/informers/apps/v1"
coreinformers "k8s.io/client-go/informers/core/v1"
clientset "k8s.io/client-go/kubernetes"
Expand All @@ -43,7 +42,6 @@ import (
podutil "k8s.io/kubernetes/pkg/api/v1/pod"
"k8s.io/kubernetes/pkg/controller"
"k8s.io/kubernetes/pkg/controller/history"
"k8s.io/kubernetes/pkg/features"

"k8s.io/klog/v2"
)
Expand Down Expand Up @@ -232,7 +230,7 @@ func (ssc *StatefulSetController) updatePod(old, cur interface{}) {
// TODO: MinReadySeconds in the Pod will generate an Available condition to be added in
// the Pod status which in turn will trigger a requeue of the owning replica set thus
// having its status updated with the newly available replica.
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) && !podutil.IsPodReady(oldPod) && podutil.IsPodReady(curPod) && set.Spec.MinReadySeconds > 0 {
if !podutil.IsPodReady(oldPod) && podutil.IsPodReady(curPod) && set.Spec.MinReadySeconds > 0 {
klog.V(2).Infof("StatefulSet %s will be enqueued after %ds for availability check", set.Name, set.Spec.MinReadySeconds)
// Add a second to avoid milliseconds skew in AddAfter.
// See https://github.com/kubernetes/kubernetes/issues/39785#issuecomment-279959133 for more info.
Expand Down Expand Up @@ -485,7 +483,7 @@ func (ssc *StatefulSetController) syncStatefulSet(ctx context.Context, set *apps
}
klog.V(4).Infof("Successfully synced StatefulSet %s/%s successful", set.Namespace, set.Name)
// One more sync to handle the clock skew. This is also helping in requeuing right after status update
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) && set.Spec.MinReadySeconds > 0 && status != nil && status.AvailableReplicas != *set.Spec.Replicas {
if set.Spec.MinReadySeconds > 0 && status != nil && status.AvailableReplicas != *set.Spec.Replicas {
ssc.enqueueSSAfter(set, time.Duration(set.Spec.MinReadySeconds)*time.Second)
}

Expand Down
18 changes: 5 additions & 13 deletions pkg/controller/statefulset/stateful_set_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,14 +298,10 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
if isRunningAndReady(pods[i]) {
status.ReadyReplicas++
// count the number of running and available replicas
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) {
if isRunningAndAvailable(pods[i], set.Spec.MinReadySeconds) {
status.AvailableReplicas++
}
} else {
// If the featuregate is not enabled, all the ready replicas should be considered as available replicas
status.AvailableReplicas = status.ReadyReplicas
if isRunningAndAvailable(pods[i], set.Spec.MinReadySeconds) {
status.AvailableReplicas++
}

}

// count the number of current and update replicas
Expand Down Expand Up @@ -462,9 +458,7 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
// If we have a Pod that has been created but is not available we can not make progress.
// We must ensure that all for each Pod, when we create it, all of its predecessors, with respect to its
// ordinal, are Available.
// TODO: Since available is superset of Ready, once we have this featuregate enabled by default, we can remove the
// isRunningAndReady block as only Available pods should be brought down.
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) && !isRunningAndAvailable(replicas[i], set.Spec.MinReadySeconds) && monotonic {
if !isRunningAndAvailable(replicas[i], set.Spec.MinReadySeconds) && monotonic {
ravisantoshgudimetla marked this conversation as resolved.
Show resolved Hide resolved
klog.V(4).InfoS("StatefulSet is waiting for Pod to be Available",
"statefulSet", klog.KObj(set), "pod", klog.KObj(replicas[i]))
return &status, nil
Expand Down Expand Up @@ -525,9 +519,7 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
return &status, nil
}
// if we are in monotonic mode and the condemned target is not the first unhealthy Pod, block.
// TODO: Since available is superset of Ready, once we have this featuregate enabled by default, we can remove the
// isRunningAndReady block as only Available pods should be brought down.
if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) && !isRunningAndAvailable(condemned[target], set.Spec.MinReadySeconds) && monotonic && condemned[target] != firstUnhealthyPod {
if !isRunningAndAvailable(condemned[target], set.Spec.MinReadySeconds) && monotonic && condemned[target] != firstUnhealthyPod {
klog.V(4).InfoS("StatefulSet is waiting for Pod to be Available prior to scale down",
"statefulSet", klog.KObj(set), "pod", klog.KObj(firstUnhealthyPod))
return &status, nil
Expand Down
Loading