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

apiextensions: update CRD strategy #50764

Merged
merged 1 commit into from
Aug 24, 2017
Merged

apiextensions: update CRD strategy #50764

merged 1 commit into from
Aug 24, 2017
+40 −15

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Aug 16, 2017 •
edited
Loading

This PR adds the following changes:

  1. Clear the status of the CRD and set the Generation before creation.
  2. While updating the CRD:
    • ignore changes on status.
    • increase Generation if spec changes.
  3. Don't update objectmeta (except finalizers) when status is updated.

Release note:

CRDs support metadata.generation and implement spec/status split

/cc @sttts @deads2k

@k8s-ci-robot k8s-ci-robot requested review from deads2k and sttts August 16, 2017 12:33
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 16, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @nikhita. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 16, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 16, 2017
@xiangpengzhao
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 16, 2017
@nikhita
Copy link
Member Author

nikhita commented Aug 16, 2017

/retest

2 similar comments
@nikhita
Copy link
Member Author

nikhita commented Aug 16, 2017

/retest

@nikhita
Copy link
Member Author

nikhita commented Aug 16, 2017

/retest

@nikhita
update CRD strategy for status updates
5c35461 
1. Clear the status of the CRD and set the Generation before creation.
2. While updating the CRD:
    - ignore changes on status.
    - increase Generation if spec changes.
3. Don't update objectmeta when status is updated.
    - however, update finalizers. Without this, deletion will timeout.
@nikhita nikhita force-pushed the crd-update-status-strategy branch from 2e6e7db to 5c35461 Compare August 17, 2017 06:15
nikhita
nikhita commented Aug 17, 2017
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go
@@ -87,9 +106,14 @@ func (statusStrategy) PrepareForUpdate(ctx genericapirequest.Context, obj, old r
newObj := obj.(*apiextensions.CustomResourceDefinition)
oldObj := old.(*apiextensions.CustomResourceDefinition)
newObj.Spec = oldObj.Spec

// Status updates are for only for updating status, not objectmeta.
// TODO: Update after ResetObjectMetaForStatus is added to meta/v1.
Copy link
Member Author

@nikhita nikhita Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Deletion won't work without updating finalizers (it will time out). However, this means we are touching some part of objectmeta while updating status...

Also, when ResetObjectMetaForStatus is added (#45552), it will not allow finalizers to be updated.

sttts
sttts reviewed Aug 21, 2017
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go
@@ -46,9 +47,27 @@ func (strategy) NamespaceScoped() bool {
}

func (strategy) PrepareForCreate(ctx genericapirequest.Context, obj runtime.Object) {
crd := obj.(*apiextensions.CustomResourceDefinition)
crd.Status = apiextensions.CustomResourceDefinitionStatus{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reset the status?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't allow end users to set status on creation.

We do that for core objects too: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/replicationcontroller/strategy.go#L64.

sttts
sttts reviewed Aug 21, 2017
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go
}

func (strategy) PrepareForUpdate(ctx genericapirequest.Context, obj, old runtime.Object) {
newCRD := obj.(*apiextensions.CustomResourceDefinition)
oldCRD := old.(*apiextensions.CustomResourceDefinition)
newCRD.Status = oldCRD.Status
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this work without updating the controllers to use the status subresource? Do we have that? Or do we use that already? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go#L170 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

If the controllers use /status already, all this looks fine.

@sttts sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 21, 2017
@sttts
Copy link
Contributor

sttts commented Aug 21, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2017
@sttts
Copy link
Contributor

sttts commented Aug 21, 2017

/assign @deads2k

For approval.

@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2017

/retest

4 similar comments
@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2017

/retest

@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2017

/retest

@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2017

/retest

@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2017

/retest

@deads2k
Copy link
Contributor

deads2k commented Aug 23, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue requirement bypassed by: deads2k

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51047, 48573, 50764, 51092, 50578)

@k8s-github-robot k8s-github-robot merged commit c418e1b into kubernetes:master Aug 24, 2017
@nikhita nikhita deleted the crd-update-status-strategy branch September 11, 2017 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@sttts sttts

@enisoc enisoc

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nikhita @k8s-ci-robot @xiangpengzhao @sttts @deads2k @k8s-github-robot @enisoc