Revert "Use Update() instead of UpdateStatus()"

[stevekuznetsov]

This reverts commit 6ec5070.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

fixes #11093

/assign @cjwagner @fejta @munnerz
/cc @BenTheElder @krzyzacy

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• prow/OWNERS [stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fejta]

Doesn't this require 1.13?

[stevekuznetsov]

No, I require a basic amount of reading comprehension 🤦‍♂️

The page just said 1.13 had it in beta, not that it transitioned to beta in 1.13

[fejta]

Can you find a doc that lists when it went to beta? I tried but cannot find it. We can't use alpha apis in GKE (or if we do, our cluster gets periodically deleted)

[stevekuznetsov]

@munnerz had the links for that

So, some fun facts:

when you update the CRD definition to include a status subresource, update calls will no longer update status at all
when you change to updatestatus without declaring a status subresource, those calls will fail

@nikhita is there a reasonable way to roll out a change like this without downtime?

[fejta]

do both an Update and UpdateStatus call?

[nikhita]

Can you find a doc that lists when it went to beta?

I can confirm that this has been in beta from 1.11. :)

(Though I agree it'd be neat to show since when it has been in beta in the official docs)

• Official docs: https://v1-11.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#advanced-topics

• Enhancements issue: Subresources for Custom Resources enhancements#571 (comment)

• source code: https://github.com/kubernetes/kubernetes/blob/fcb0d60d4c8d6018572b5e908ab933869e8b9067/staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go#L39

@stevekuznetsov Re Update and UpdateStatus, the (breaking) change in behaviour is expected because the controller should be able to write the status, and only read the spec and users should be able to write the spec, but only read the status.

I would argue that if the controller is also updating the spec or vice-versa, then that behaviour should be fixed. :)

[stevekuznetsov]

@nikhita the breaking issue is as such:

• today, controller calls Update() and intends to update /status
• today, CRD has no /status subresource

I see these timelines:

---

1. update controllers to call UpdateStatus()
2. update CRD to have /status subresource

This fails as the controllers will error on their UpdateStatus() calls while the CRD has no /status subresource

---

1. update CRD to have /status subresource
2. update controllers to call UpdateStatus()

This fails as the controllers will hot-loop on their Update() calls while they cannot affect the CRD's /status field. Our controllers for instance will attempt to abort every single ProwJob in the cluster in a hot-loop.

[munnerz]

Our controllers for instance will attempt to abort every single ProwJob in the cluster in a hot-loop.

Will the controller's rate limiters not kick in and start backing-off processing these items?

It's not really an ideal solution, but at least if you update the CRDs first, you will have the controllers returning errors (and thus rate limiting) whereas with the first strategy, the call to Update() will just silently throw away the status information.

Very much interested in the resolution here, as I want to move cert-manager to use /status too and need to somehow manage this as part of our upgrade process 😅

[stevekuznetsov]

Our "controllers" are just for loops so there are no rate limiters 🤦‍♂️

[stevekuznetsov]

If you version the CRD can you add the /status only to a newer version?

[munnerz]

Looks like you might be able to.. kubectl explain crd.spec.versions.subresources:

KIND:     CustomResourceDefinition
VERSION:  apiextensions.k8s.io/v1beta1

RESOURCE: subresources <Object>

DESCRIPTION:
     Subresources describes the subresources for CustomResource Top-level and
     per-version subresources are mutually exclusive. Per-version subresources
     must not all be set to identical values (top-level subresources should be
     used instead) This field is alpha-level and is only honored by servers that
     enable the CustomResourceWebhookConversion feature.

     CustomResourceSubresources defines the status and scale subresources for
     CustomResources.

FIELDS:
   scale	<Object>
     Scale denotes the scale subresource for CustomResources

   status	<>
     Status denotes the status subresource for CustomResources

[munnerz]

This field is alpha-level and is only honored by servers that enable the CustomResourceWebhookConversion feature.

🤔 not sure if versioning for subresources is available as far back as 1.11 however...

[fejta]

Seems pretty clear here:

1. Deploy a vesion which calls both UpdateStatus and Update, set this flag to default
2. Update CRD to have a status field
3. Tune flag to disable the update call
4. Remove the logic to call Update in 6mo

[stevekuznetsov]

/hold

Can tackle later

[k8s-ci-robot]

@stevekuznetsov: PR needs rebase.

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.

[k8s-ci-robot]

@stevekuznetsov: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-test-infra-lint	a1bc15a	link	/test pull-test-infra-lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[stevekuznetsov]

Not super urgent and high risk for disruption

/close

[k8s-ci-robot]

@stevekuznetsov: Closed this PR.

In response to this:

Not super urgent and high risk for disruption

/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.