apiextensions: add subresources for custom resources by nikhita · Pull Request #55168 · kubernetes/kubernetes

nikhita · 2017-11-06T14:34:38Z

Fixes #38113
Fixes #58778

Related:

Proposal: Proposal: SubResources for CustomResources community#913
Docs: Add docs for CRD features for 1.10 website#7439
For custom resources to work with kubectl scale: kubectl scale: support Unstructured objects #58283

Add types:

Add CustomResourceSubresources type to CRD.
- Fix proto generation for CustomResourceSubresourceStatus: Fix protobuf generation for empty struct #55970.
Add feature gate for CustomResourceSubresources.
- Update CRD strategy: if feature gate is disabled, this feature is dropped (i.e. set to nil).
Add validation for CustomResourceSubresources:
- SpecReplicasPath should not be empty and should be a valid json path under .spec. If there is no value under the given path in the custom resource, the /scale subresource will return an error on GET.
- StatusReplicasPath should not be empty and should be a valid json path under .status. If there is no value under the given path in the custom resource, the status replica value in the /scale subresource will default to 0.
- If present, LabelSelectorPath is optional and should be a valid json path under .status. If there is no value under LabelSelectorPath in the custom resource, the status label selector value in the /scale subresource will default to the empty string.
- If CustomResourceSubresources feature gate is enabled, only properties is allowed under the root schema for CRD validation.

Add status and scale subresources:

Use helper functions from apimachinery/pkg/apis/meta/v1/unstructured/helpers.go.
- Improve error handling: apimachinery: improve error handling for unstructured helpers #56563, unstructured helpers: print path in error #58215.
Introduce Registry interface for storage.
Update storage:
- Introduce CustomResourceStorage which acts as storage for the custom resource and its status and scale subresources. Note: storage for status and scale is only enabled when the feature gate is enabled and the respective fields are enabled in the CRD.
- Introduce StatusREST and its New(), Get() and Update() methods.
- Introduce ScaleREST and its New(), Get() and Update() methods.
  - Get and Update use the json paths from the CRD and use it to return an autoscaling/v1.Scale object.
Update strategy:
- In PrepareForCreate,
  - Clear .status.
  - Set .metadata.generation = 1
- In PrepareForUpdate,
  - Do not update .status.
    - If both the old and new objects have .status and it is changed, set it back to its old value.
    - If the old object has a .status but the new object doesn't, set it to the old value.
    - If old object did not have a .status but the new object does, delete it.
  - Increment generation if spec changes i.e. in the following cases:
    - If both the old and new objects had .spec and it changed.
    - If the old object did not have .spec but the new object does.
    - If the old object had a .spec but the new object doesn't.
- In Validate and ValidateUpdate,
  - ensure that values at specReplicasPath and statusReplicasPath are >=0 and < maxInt32.
  - make sure there are no errors in getting the value at all the paths.
- Introduce statusStrategy with its methods.
  - In PrepareForUpdate:
    - Do not update .spec.
      - If both the old and new objects have .spec and it is changed, set it back to its old value.
      - If the old object has a .spec but the new object doesn't, set it to the old value.
      - If old object did not have a .spec but the new object does, delete it.
    - Do not update .metadata.
  - In ValidateStatusUpdate:
    - For CRD validation, validate only under .status.
    - Validate value at statusReplicasPath and labelSelectorPath as above.
Plug into the custom resource handler:
- Store all three storage - customResource, status and scale in crdInfo.
- Use the storage as per the subresource in the request.
- Use the validator as per the subresource (for status, only use the schema for status, if present).
- Serve the endpoint as per the subresource - see serveResource, serveStatus and serveScale.
Update discovery by adding the /status and /scale resources, if enabled.

Add tests:

Add unit tests in etcd_test.go.
Add integration tests.
- In subresources_test.go, use the polymporphic scale client to get and update Scale.
- Add a test to check everything works fine with yaml in yaml_test.go.

Release note:

`/status` and `/scale` subresources are added for custom resources.

nikhita · 2017-11-06T14:35:26Z

/assign @sttts @deads2k @enisoc

sttts · 2017-11-06T14:41:40Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

do we use typographical quotes?

sttts · 2017-11-06T14:42:31Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

This should be fixed when proto generation is fixed. Currently, update-generated-protobuf fails on CustomResourceSubResourceStatus

sttts · 2017-11-06T14:44:08Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

better: field.Invalid

sttts · 2017-11-06T14:46:08Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/helpers.go

we have public unstructured helpers now. Would they work for you? apimachinery/pkg/apis/meta/v1/unstructured/helpers.go

yes, they probably should. I'll try with them.

sttts · 2017-11-06T14:48:01Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

This makes every CR part of kubectl get all. Wondering, do we have a way to opt-in to that group in a CRD?

Wondering, do we have a way to opt-in to that group in a CRD?

Maybe another field in the CRD? 🤔

sttts · 2017-11-06T14:48:37Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

err.Error()?

sttts · 2017-11-06T14:50:20Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

not capital Scale?

sttts · 2017-11-06T14:50:43Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

would make all of the paths explicit here

To have something like this:

scaleJSONPath["specReplicasPath"] = crd.Spec.SubResources.Scale.SpecReplicasPath scaleJSONPath["statusReplicasPath"] = crd.Spec.SubResources.Scale.StatusReplicasPath scaleJSONPath["labelSelectorPath"] = crd.Spec.SubResources.Scale.LabelSelectorPath

would make all of the paths explicit here

👍

3 string fields would do as well?

sttts · 2017-11-06T14:52:13Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

isn't it mandatory?

sttts · 2017-11-06T14:57:41Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go

These tests should go into the CRD validation.

This validates the "custom resource". So I think it should not go in the CRD strategy and should instead remain the in the CR strategy.

@sttts WDYT?

hmm, I think customResource["oneOf"] does not make sense here. We have to check that OneOf is not used in the root of the JSON schema in the CRD.

sttts · 2017-11-06T15:01:55Z

cmd/kube-controller-manager/app/autoscaling.go

@DirectXMan12 ^^

:-(. It was fun while it lasted

DirectXMan12 · 2017-11-06T16:29:45Z

@kubernetes/sig-autoscaling-api-reviews

DirectXMan12

You're using the scheme used internally to the scale client as your internal version, which I'd say is not ok.

Other comments inline

DirectXMan12 · 2017-11-06T16:33:47Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

why is StatusReplicas optional? If it defaults to something otherwise, you should probably write that here (same with the other optional fields)

If I remember right, the idea was that a scale subresource could exist without the scale being reflected in the status. So there is no defaulting of this field.

Adding on to what @sttts said, the user cannot write to the ~~/status~~ .status of the scale object at all. It is completely read-only for the user. Only a controller has write access to the status fields. So we can leave it empty and not have it default to anything. This also matches our plan with CRDs.

DirectXMan12 · 2017-11-06T16:41:56Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

This was intended to be purely internal to the scale client. The internal types aren't supposed to be used by other people.

Make them internal.

you mean change the name? Because they are marked as having the version be __internal.

We don't make any use of that, but Go 1.5+ has packages called internal which can only be imported if your package and the internal one shares a common root. Does not belong into this PR, but I really think we should move a lot of internal code into internal packages. These client schemes are candidates for that. So I agree, we probably should not use the scale scheme here in the registry.

Uses external type for autoscaling now. 👍

DirectXMan12 · 2017-11-06T16:43:15Z

staging/src/k8s.io/client-go/scale/scheme/validation/validation.go

I don't see what purpose this serves. This is never supposed to be used as an API server type.

nikhita · 2017-11-09T16:01:37Z

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go

This needs fixing. Will do that.

Oops, done in 6f36088. Should be fine after a squash.

sttts · 2018-02-21T20:41:56Z

/lgtm

sttts · 2018-02-22T07:00:10Z

/lgtm

anubhakushwaha · 2018-02-22T08:59:09Z

@nikhita @sttts Just a minor comment update required here I think in the comment it should be "3" instead of "5", let me know if I am wrong as it may lead to false test.

sttts · 2018-02-22T16:43:33Z

/lgtm

lavalamp · 2018-02-22T17:39:32Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

Per discussion in slack:

This should be DefaultScaleGroupVersion, as it's not intended to prevent the server from serving additional versions. The version of polymorphic subresources is intended to be whatever the client of the subresource wants it to be. We haven't built this out because we don't have any multi-versioned subresources yet.

The default is probably always going to be autoscaling/v1/scale.

In the unlikely event we do change the default, we could always add this field at that time and backfill.

Therefore, let's remove this field, it's not necessary at the moment.

Sorry to not have figured this out yesterday when I asked for changes!

sttts · 2018-02-22T17:58:42Z

/lgtm

lavalamp · 2018-02-22T18:04:12Z

/approve

k8s-ci-robot · 2018-02-22T18:04:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, deads2k, lavalamp, nikhita, sttts

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~Godeps/OWNERS~~ [cblecker,lavalamp]
~~api/OWNERS~~ [lavalamp]
~~cmd/kube-controller-manager/OWNERS~~ [deads2k,lavalamp]
~~hack/OWNERS~~ [cblecker,deads2k,lavalamp,sttts]
~~pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k,lavalamp,sttts]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k,lavalamp,sttts]
~~vendor/OWNERS~~ [cblecker,lavalamp]

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

k8s-github-robot · 2018-02-22T20:36:47Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-22T21:37:35Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

sttts · 2018-02-22T22:13:04Z

🎉

tamalsaha · 2018-02-23T01:19:08Z

Thanks @nikhita @sttts and everyone who helped to get this added!

idvoretskyi · 2018-02-23T02:06:50Z

@nikhita @sttts can you please link this to an issue in https://github.com/kubernetes/features

nikhita · 2018-02-23T04:15:02Z

@idvoretskyi It is a part of kubernetes/enhancements#95.

sttts · 2018-04-03T11:13:13Z

/lgtm
/approve

yue9944882 · 2019-06-13T13:36:22Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+func (r *crdHandler) serveResource(w http.ResponseWriter, req *http.Request, requestInfo *apirequest.RequestInfo, crdInfo *crdInfo, terminating bool, supportedTypes []string) http.HandlerFunc {
+	requestScope := crdInfo.requestScope
+	storage := crdInfo.storage.CustomResource
+	minRequestTimeout := 1 * time.Minute


@nikhita do you know what's the point of setting a constant timeout here?

k8s-ci-robot assigned deads2k, enisoc and sttts Nov 6, 2017

sttts reviewed Nov 6, 2017

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go Outdated

Copy link

Contributor

sttts Nov 6, 2017

Choose a reason for hiding this comment

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

not capital Scale?

sttts reviewed Nov 6, 2017

View reviewed changes

k8s-ci-robot added sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 6, 2017

DirectXMan12 suggested changes Nov 6, 2017

View reviewed changes

nikhita force-pushed the customresources-subresources branch from b716144 to b80000b Compare November 9, 2017 15:51

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 9, 2017

nikhita commented Nov 9, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2017

nikhita force-pushed the customresources-subresources branch from b80000b to 30fe1d1 Compare November 13, 2017 12:40

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2017

lavalamp reviewed Feb 22, 2018

View reviewed changes

nikhita added 2 commits February 22, 2018 23:26

add subresources for custom resources

6fbe815

update generated files

55ce3de

enisoc mentioned this pull request Feb 23, 2018

Support status subresource and ObservedGeneration. GoogleCloudPlatform/metacontroller#26

Closed

nilebox mentioned this pull request Feb 26, 2018

Support Generation/ObservedGeneration atlassian/smith#102

Closed

This was referenced Mar 13, 2018

[1.9] bump(go-openapi/validate): d509235108fcf6ab4913d2dcb3a2260c0db2108e #61106

Closed

Proposal: SubResources for CustomResources kubernetes/community#913

Merged

prydie mentioned this pull request Apr 17, 2018

Fail to scale cluster after few days oracle/mysql-operator#72

Closed

nikhita mentioned this pull request Apr 18, 2018

Promote CustomResourceSubresources to beta in 1.11 #62786

Closed

asymmetric mentioned this pull request May 7, 2018

Use Status field of Habitat object chef-boneyard/habitat-operator#200

Closed

nikhita mentioned this pull request Jun 1, 2018

Subresources for Custom Resources kubernetes/enhancements#571

Closed

tamalsaha mentioned this pull request Aug 15, 2018

metadata.Generation of crds should change for annotations/labels #67428

Closed

alecmerdler mentioned this pull request Nov 1, 2018

[WIP] Update Status on Operator Custom Resources prometheus-operator/prometheus-operator#2074

Closed

deads2k mentioned this pull request Mar 8, 2019

make kubectl scale work without a GET if a precodition isn't requested #75210

Merged

yue9944882 reviewed Jun 13, 2019

View reviewed changes

tjamet mentioned this pull request Sep 6, 2024

fix(appset): Fix perpetual appset reconciliation argoproj/argo-cd#19822

Merged

14 tasks

Conversation

nikhita commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikhita commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikhita Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented Nov 6, 2017

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikhita Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nikhita commented Nov 6, 2017 •

edited

Loading

nikhita Nov 6, 2017 •

edited

Loading

nikhita Nov 9, 2017 •

edited

Loading