Fix a bug where StatefulSets continued to update forever

[chitoku-k]

This PR fixes a bug in controller that caused StatefulSets, owned by a Tenant resource, to be updated forever when the feature gate MaxUnavailableStatefulSet is not enabled, by removing the feature.

The operator currently sets the following in spec.updateStrategy:

type: RollingUpdate
rollingUpdate: 
  maxUnavailable: 1

However, kube-apiserver updates the StatefulSet the way below as it is feature gated, and updates it with another generation:

type: RollingUpdate
rollingUpdate:
  partition: 0

The operator immediately receives this update and after five seconds it thinks the StatefulSet still does not match the expected status and continues this flow forever, which can be observed by kubectl get statefulsets.apps -w.

With this patch, maxUnavailable defaults to null and partition to 0. I confirmed that it solved this issue on kube-apiserver v1.27.4.

[jiuker]

No bug here. According this

// RollingUpdateStatefulSetStrategy is used to communicate parameter for RollingUpdateStatefulSetStrategyType.
type RollingUpdateStatefulSetStrategy struct {
	// Partition indicates the ordinal at which the StatefulSet should be partitioned
	// for updates. During a rolling update, all pods from ordinal Replicas-1 to
	// Partition are updated. All pods from ordinal Partition-1 to 0 remain untouched.
	// This is helpful in being able to do a canary based deployment. The default value is 0.
	// +optional
	Partition *int32 `json:"partition,omitempty" protobuf:"varint,1,opt,name=partition"`
	// The maximum number of pods that can be unavailable during the update.
	// Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%).
	// Absolute number is calculated from percentage by rounding up. This can not be 0.
	// Defaults to 1. This field is alpha-level and is only honored by servers that enable the
	// MaxUnavailableStatefulSet feature. The field applies to all pods in the range 0 to
	// Replicas-1. That means if there is any unavailable pod in the range 0 to Replicas-1, it
	// will be counted towards MaxUnavailable.
	// +optional
	MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty" protobuf:"varint,2,opt,name=maxUnavailable"`
}

Default is 1. Operator set or not set . They are the same. And I do your change as my cluster, nothing different. If your sts updated always, you should check the minio-operator's log and minio's log.

[chitoku-k]

@jiuker
What version is your kube-apiserver? What did your kube-apiserver respond to your StatefulSet, especially about maxUnavailable? Did you test with the Tenant with pools that were less than or equal to 4 servers too? I observe this field being removed even when I edited a StatefulSet resource manually with kubectl edit.

I checked the logs of minio-operator and tenants as well as audit logs from kube-apiserver and found that the minio-operator updates StatefulSets every five seconds.

[jiuker]

What version is your kube-apiserver? What did your kube-apiserver respond to your StatefulSet, especially about maxUnavailable? Did you test with the Tenant with pools that were less than or equal to 4 servers too? I observe this field being removed even when I edited a StatefulSet resource manually with kubectl edit.

I checked the logs of minio-operator and tenants as well as audit logs from kube-apiserver and found that the minio-operator updates StatefulSets every five seconds.

This field is deleted, it should be done by K8S itself, not dropped by minio-operator. @chitoku-k I test 4x4, 6x1,1x4. they are the same.

[chitoku-k]

@jiuker
I understand that Kubernetes removes that field but the one who compares the existing StatefulSet and the expected StatefulSet to determine whether it wants to update the StatefulSet is the operator, specifically poolSSMatchesSpec(). The received StatefulSet does not have maxUnavailable because Kubernetes has removed it and the expected StatefulSet contains maxUnavailable ; how does the current implementation of poolSSMatchesSpec() return true to indicate the operator does not need updating it then?

[jiuker]

@jiuker I understand that Kubernetes removes that field but the one who compares the existing StatefulSet and the expected StatefulSet to determine whether it wants to update the StatefulSet is the operator, specifically poolSSMatchesSpec(). The received StatefulSet does not have maxUnavailable because Kubernetes has removed it and the expected StatefulSet contains maxUnavailable ; how does the current implementation of poolSSMatchesSpec() return true to indicate the operator does not need updating it then?

It doesn't matter, because the sts to the list loop are also not equipped with this field. It does not cause a loop by itself. @chitoku-k And you're describing a scenario that doesn't have a particular action, in other words, any environment will trigger it. But we haven't encountered it in testing or production.

[chitoku-k]

@jiuker

It doesn't matter, because the sts to the list loop are also not equipped with this field. It does not cause a loop by itself.

It causes an update by itself at least on kube-apiserver 1.27.4 even with a single command as below:

kubectl patch statefulsets.apps STS_NAME \
  --patch='{"spec":{"updateStrategy":{"type":"RollingUpdate","rollingUpdate":{"maxUnavailable":1}}}}'

And you're describing a scenario that doesn't have a particular action, in other words, any environment will trigger it.

Sorry but I don't get what you meant here. What action did you refer to?

[jiuker]

@chitoku-k I think you can remove this field for PDB will do that. Maybe there is no generate version update for kubeclt get sts -w.

[chitoku-k]

@jiuker
After some more investigation, when I enabled the feature gate MaxUnavailableStatefulSet (alpha), this problem did not reproduce, so I'm wondering you may have configured your environment that way.
https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

[jiuker]

LGTM

[shtripat]

lgtm