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.

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

Server-Side Apply status wiping #99661

Merged
merged 2 commits into from Mar 10, 2021
Merged

Conversation

kevindelgado
Copy link
Contributor

@kevindelgado kevindelgado commented Mar 2, 2021

What type of PR is this?

/kind bug

What this PR does / why we need it:

Implements kubernetes/enhancements#1123
Adds the ResetFieldsProvider interface and implements it for all strategies.

Which issue(s) this PR fixes:

Fixes #75564

Does this PR introduce a user-facing change?

FieldManager no longer owns fields that get reset before the object is persisted (e.g. "status wiping").

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/1123

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress release-note kind/bug size/XXL cncf-cla: yes do-not-merge/needs-sig needs-triage labels Mar 2, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 2, 2021

Hi @kevindelgado. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test needs-priority area/apiserver area/dependency area/test kind/api-change sig/api-machinery sig/auth sig/testing and removed do-not-merge/needs-sig labels Mar 2, 2021
@k8s-ci-robot k8s-ci-robot requested review from apelisse, caesarxuchao and Mar 2, 2021
@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 2, 2021

/assign @liggitt

We need to fix one broken test case in status_test, but wanted to get your eyes on this in the meantime @liggitt to begin reviewing this approach to status wiping.

The first commit is from @kwiesmueller work from back in July #92809
and we've recently added some fixes to broken test cases (second commit in the PR onwards).

cc @apelisse

@apelisse
Copy link
Member

@apelisse apelisse commented Mar 2, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 2, 2021
@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 3, 2021

/test pull-kubernetes-e2e-kind

@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 3, 2021

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot added the approved label Mar 9, 2021
@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 9, 2021

Does this need to have a 1.21 milestone label in order to land in the release @apelisse?

@apelisse
Copy link
Member

@apelisse apelisse commented Mar 9, 2021

I don't think it does, but it wouldn't hurt

@apelisse apelisse added this to the v1.21 milestone Mar 9, 2021
Copy link
Member

@apelisse apelisse left a comment

Looks good, awesome work to both Kevin :-)
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 9, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 9, 2021
@lavalamp
Copy link
Member

@lavalamp lavalamp commented Mar 10, 2021

/milestone v1.21
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 10, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 10, 2021
@lavalamp
Copy link
Member

@lavalamp lavalamp commented Mar 10, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 10, 2021
kevindelgado and others added 2 commits Mar 10, 2021
Adds and implements ResetFieldsProvder interface in order to ensure that
the fieldmanager no longer owns fields that get reset before the object
is persisted.

Co-authored-by: Kevin Wiesmueller <kwiesmul@redhat.com>
Co-authored-by: Kevin Delgado <kevindelgado@google.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 10, 2021
@apelisse
Copy link
Member

@apelisse apelisse commented Mar 10, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 10, 2021
@kwiesmueller
Copy link
Member

@kwiesmueller kwiesmueller commented Mar 10, 2021

Thanks for finishing the job!

@lavalamp lavalamp removed the needs-rebase label Mar 10, 2021
@lavalamp
Copy link
Member

@lavalamp lavalamp commented Mar 10, 2021

I manually removed the "needs-rebase" label, I'm not sure why the bot didn't.

@k8s-ci-robot k8s-ci-robot merged commit b7d23d7 into kubernetes:master Mar 10, 2021
15 of 16 checks passed
@liggitt liggitt moved this from Assigned to API review completed, 1.21 in API Reviews Apr 6, 2021
liggitt
Copy link
Member

@liggitt liggitt commented on a1fac8c Feb 14, 2022

Choose a reason for hiding this comment

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

This is passing in newObj for both args, which means ResetObjectMetaForStatus is a no-op.

Also, this previously allowed updating finalizers via status updates, and ResetObjectMetaForStatus does not allow that. The crd finalizer depends on updating finalizers via a status update API call, so simply fixing this to pass newObj/oldObj correctly will break the CRD finalizer controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apiserver area/dependency area/test cncf-cla: yes kind/api-change kind/bug lgtm needs-priority ok-to-test release-note sig/api-machinery sig/auth sig/testing size/XXL triage/accepted
Projects
API Reviews
API review completed, 1.21
Development

Successfully merging this pull request may close these issues.

8 participants