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

Jump to bottom

make ObjectReference field ownership granular #110495

Merged
merged 3 commits into from Aug 3, 2022
Merged

make ObjectReference field ownership granular #110495

merged 3 commits into from Aug 3, 2022

Conversation

alexzielenski
Copy link
Contributor

@alexzielenski alexzielenski commented Jun 9, 2022 •
edited

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

This change changes the structType of ObjectReference from atomic (atomic added in v1.22 #100684).

Something this PR is particularly concerned with is the transition path from users of 1.22 with atomic ownership, to having granular field management setting. An integration test was added to ensure the expected behavior.

Without this fix, users of SSA will find since the object is atomic, unspecified fields are cleared by the apiserver when applying changes to objects. This is a problem because there are often multiple agents writing to an ObjectReference. For example, in the case of PersistentVolume's claimRef, the uid field and others might be populated by kube-controller-manager but the name or namespace fields would be specified by a user.

Which issue(s) this PR fixes:

fluxcd/flux2#2827
issues with this behavior reported in this comment

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Changes ownership semantics of PersistentVolume's spec.claimRef from `atomic` to `granular`.

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

Note for Server-Side Apply Users: Following this change, existing atomic owners of PersistentVolumeClaim `spec.claimRef` fields will not own any of the nested fields. Nested fields will have whatever owner may be already specified in managed fields, or whoever claims the field first.

/assign @apelisse

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/code-generation area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 9, 2022
@apelisse
Copy link
Member

apelisse commented Jun 9, 2022

Thanks @alexzielenski, I'm curious what @liggitt will say :-)

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@alexzielenski alexzielenski force-pushed the atomic-objectreference branch 2 times, most recently from ba33159 to b3c1f22 Compare June 10, 2022 21:51
@aojea
Copy link
Member

aojea commented Jun 12, 2022 via email

liggitt
liggitt reviewed Jun 14, 2022
View reviewed changes
staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go Outdated
@@ -5248,7 +5248,7 @@ var schemaYAML = typed.YAMLObject(`types:
- name: uid
type:
scalar: string
elementRelationship: atomic
elementRelationship: separable
Copy link
Member

@liggitt liggitt Jun 14, 2022 •
edited

Choose a reason for hiding this comment

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

what are the implications for client-go consumers that compiled in old versions of this?

@liggitt
Copy link
Member

liggitt commented Jun 14, 2022

I see the case for this being a bug in PV, but most of the other uses still seem like atomic is correct.

Can we drop the structType and keep the other uses atomic by decorating the field at point of use?

@liggitt
Copy link
Member

liggitt commented Jun 14, 2022 •
edited

Alternatively, can we keep structType as atomic and override the PV use to be granular by decorating the field... because this is a core type, changing structType will have implications for all out-of-tree consumers that compile this into their APIs as well

@apelisse
Copy link
Member

I see the case for this being a bug in PV, but most of the other uses still seem like atomic is correct.

Wouldn't you agree that we can't generally assume the UID will be set by the same controller/actor as the gvk?

@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2022
@alexzielenski alexzielenski changed the title [WIP] make ObjectReference field ownership granular make ObjectReference field ownership granular Jun 22, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2022
@apelisse
Copy link
Member

Since we don't want to flap the atomicity/granularity of ObjectReference too often, we've decided to both revert this AND update atomicity at the field-level together.

@alexzielenski alexzielenski mentioned this pull request Jul 30, 2022
Merged
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubectl sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Aug 3, 2022
@alexzielenski
Copy link
Contributor Author

alexzielenski commented Aug 3, 2022 •
edited

PR has been updated to use new SSA feature for a field-level specification to override a type-level structType. Now PersistentVolumeSpec.claimRef uses a granular ObjectReference, despite ObjectReference being marked as atomic.

A regression test has been added for the specific case of PersistentVolume showing that modifications to its claimRef do not wipe out previously set fields, and that managed fields reflects granular ownership.

cc @liggitt @apelisse

@liggitt
Copy link
Member

liggitt commented Aug 3, 2022

/lgtm
/approve

thanks for going the extra mile for compatibility here, and for the test coverage

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, liggitt

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2022
@liggitt
Copy link
Member

liggitt commented Aug 3, 2022

/milestone v1.25

adding to the milestone, since this is a bugfix for a regression in behavior that we're likely to try to backport

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit a0e7027 into kubernetes:master Aug 3, 2022
@alexzielenski alexzielenski deleted the atomic-objectreference branch November 16, 2022 18:49
@janetkuo
Copy link
Member

janetkuo commented Nov 1, 2023

Is this ever backported to older versions? @alexzielenski

@alexzielenski
Copy link
Contributor Author

alexzielenski commented Nov 1, 2023 •
edited

I don't think so, and I'm not sure that we would pick now to versions earlier than 1.25 when it was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@apelisse apelisse apelisse left review comments

@deads2k deads2k Awaiting requested review from deads2k

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@liggitt liggitt

@apelisse apelisse

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@alexzielenski @apelisse @k8s-triage-robot @aojea @liggitt @fedebongio @k8s-ci-robot @janetkuo