Error when trying to update StatefulSet updateStrategy.type to "OnDelete" from default value

[antaloala]

What happened:

When trying to update the .spec.updateStrategy.type of a StatefulSet object to "OnDelete" it fails if it was not previously set (so default value {"type": "RollingUpdate", "rollingUpdate": { "partition": 0} } applies).
Following message is printed by the failing kubectl command:

The StatefulSet "name-of-the-statefulset-object" is invalid: spec.updateStrategy.rollingUpdate: Invalid value: apps.RollingUpdateStatefulSetStrategy{Partition:0}: only allowed for updateStrategy 'RollingUpdate'

What you expected to happen:

To be able to patch the StatefulSet .spec.updateStrategy stanza to {"updateStrategy": "OnDelete"}

How to reproduce it (as minimally and precisely as possible):

1. Create a StatefulSet object with "kubectl apply -f statefulset-manifest.yaml -n mynamespace" command WITHOUT setting the .spec.updateStrategy stanza in the statefulset manifest
2. Try to update the StatefulSet object using the same kubectl command described above AFTER having edited the statefulset-manifest.yaml file, explicitly setting now ".spec.updateStrategy.type" to "OnDelete"

Second kubectl command fail printing the error message indicated above

Anything else we need to know?:

If before applying step 2 above I am explictly setting the .spec.updateStrategy stanza to {"type": "RollingUpdate", "rollingUpdate": { "partition": 0} } (i.e. explictly setting the default value) then I can run step 2 above without problem
(note it is not enough to explictly set the .spec.updateStrategy.type to RollingUpdate, it is also needed to set the .spec.updateStrategy.rollingUpdate.partitions to 0 to avoid the step 2 above to fail)

I presume the bug is related to API server still trying to apply part of the default value in the .spec.updateStrategy stanza (i.e. the "rollingUpdate.partition:0" part) even when explictly trying to set ".spec.updateStrategy.type" to "OnDelete", this only happening when default values were added (by API server) to the .spec.updateStrategy stanza of the retrieved object (retrieved in order to try to apply the requested patch).

Environment:

• Kubernetes version:
  Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.1", GitCommit:"c4d752765b3bbac2237bf87cf0b1c2e307844666", GitTreeState:"clean", BuildDate:"2020-12-18T12:09:25Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}
  Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.1", GitCommit:"c4d752765b3bbac2237bf87cf0b1c2e307844666", GitTreeState:"clean", BuildDate:"2020-12-18T12:00:47Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}

[antaloala]

/sig api-machinery

[lala123912]

I would like to work on it.
/assign

[lala123912]

I tried the first step, however it failed. could you provide the ymal of the first step? @antaloala

[antaloala]

Thanks @lala123912 for jumping
Whatever valid statefulset manifest not setting the .spec.updateStrategy stanza should work.
I am copying here the one I used in my test (a really simplified one not requiring any headless service manifest nor defining volumeClaimTemplate entries):

apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: web-nginx
  labels:
    app: nginx
spec:
  serviceName: whatever-string-is-ok-for-this-test
  selector:
    matchLabels:
      app: nginx-pods
  replicas: 2
  #  updateStrategy:
  #    type: OnDelete
  template:
    metadata:
      labels:
        app: nginx-pods
    spec:
      containers:
      - name: nginx-container
        image: nginx:latest
        ports:
        - containerPort: 80
          name: web

It includes a commented "updateStrategy" stanza you can uncomment to run step2.

[lala123912]

I use kubectl patch command without retainKeys , the patch file will be merged into the original one, not replace the old update strategy, so the Partition still exists in the new update strategy. Instead, We can use retainKeys to update the type and remove spec.updateStrategy.rollingUpdate. See more details on https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch.

Here is the patch file:

spec:
  updateStrategy:
    $retainKeys:
    - type
    type: OnDelete

then , excute cmd:

 kubectl patch statefulset web-nginx --patch "$(cat patch-file-retainkeys.yaml)"

statefulset.apps/web-nginx patched

describe the sts:

Name:               web-nginx
Namespace:          default
CreationTimestamp:  Tue, 16 Mar 2021 09:55:45 +0800
Selector:           app=nginx-pods
Labels:             app=nginx
Annotations:        <none>
Replicas:           2 desired | 2 total
Update Strategy:    OnDelete
Pods Status:        2 Running / 0 Waiting / 0 Succeeded / 0 Failed
Pod Template:
  Labels:  app=nginx-pods
  Containers:
   nginx-container:
    Image:        nginx:latest
    Port:         80/TCP
    Host Port:    0/TCP
    Environment:  <none>
    Mounts:       <none>
  Volumes:        <none>
Volume Claims:    <none>
Events:
  Type    Reason            Age    From                    Message
  ----    ------            ----   ----                    -------
  Normal  SuccessfulCreate  9m20s  statefulset-controller  create Pod web-nginx-0 in StatefulSet web-nginx successful
  Normal  SuccessfulCreate  9m1s   statefulset-controller  create Pod web-nginx-1 in StatefulSet web-nginx successful

@antaloala

[fedebongio]

/triage accepted

[fedebongio]

Thank you @lala123912 for helping here!

[antaloala]

Thanks @lala123912.
But I understand it is still a bug in API server as the handling of default values in the statefulset's updateStrategy stanza looks wrong, right?

[liggitt]

cc @apelisse, looks related to https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1027-api-unions#normalizing-on-updates

[liggitt]

But I understand it is still a bug in API server as the handling of default values in the statefulset's updateStrategy stanza looks wrong, right?

It's not exactly an API server bug... more an unfortunate interaction between server defaulting and client patching.

The server is defaulting the spec.updateStrategy.rollingUpdate field in the create step.

In the update step, the client is only sending a patch for the spec.updateStrategy.type field, leaving the defaulted rollingUpdate field in place, which is not valid for any type other than RollingUpdate.

You can make the client explicitly clear the updateStrategy.rollingUpdate field either by

1. including it in your original create request, so the client is aware of that field and includes removal of that field in the patch sent to switch to OnDelete

2. explicitly setting it to null in the manifest when switching to OnDelete:

     updateStrategy:
       type: OnDelete
       rollingUpdate: null

There's an unimplemented proposal to have the server clear fields corresponding to other types when switching the type of stanzas like this, but doing so would also clear/ignore explicitly sent incorrect values in manifests like this, which masks errors and seems confusing:

  updateStrategy:
    type: OnDelete
    rollingUpdate:
      partition: 1

[antaloala]

Thanks a lot @liggitt for so clear explanation

[antaloala]

I am the closing It.

[antaloala]

/close

[k8s-ci-robot]

@antaloala: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.