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
make ObjectReference field ownership granular #110495
make ObjectReference field ownership granular #110495
Conversation
Thanks @alexzielenski, I'm curious what @liggitt will say :-) |
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. |
ba33159
to
b3c1f22
Compare
Kmews*
El jue., 9 jun. 2022 23:16, Alex Zielenski ***@***.***>
escribió:
… 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
<#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.
Which issue(s) this PR fixes:
fluxcd/flux2#2827 <fluxcd/flux2#2827>
Fixes #
Special notes for your reviewer: Does this PR introduce a user-facing
change?
Changes ownership semantics of ObjectReference types 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 `ObjectReference` fields will not own any of the nested fields. Nested fields will have whatever owner is specified in managed fields.
/assign @apelisse <https://github.com/apelisse>
------------------------------
You can view, comment on, or merge this pull request online at:
#110495
Commit Summary
- 1dcdb62
<1dcdb62>
make ObjectReference field ownership granular
File Changes
(11 files <https://github.com/kubernetes/kubernetes/pull/110495/files>)
- *M* api/openapi-spec/swagger.json
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-ce7b64e72add3d44567b11ab1a4b1e6bfc9345a86b4a352283140e4974776c4d>
(2)
- *M* api/openapi-spec/v3/api__v1_openapi.json
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-32d8d3f65b972f670c1bfc3cb75b072ec9424642ebbaf328bdeccdc919c78c84>
(2)
- *M* api/openapi-spec/v3/apis__batch__v1_openapi.json
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-a9f8b0686411291b143dc526ce4ae478903dbfbe9c2d5831331404c5fa5d5f4f>
(2)
- *M* api/openapi-spec/v3/apis__discovery.k8s.io__v1_openapi.json
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-9804a76b3af29bddc0fda57eaa2df6c058bf224dfa3ea81cf8357653e8f983d5>
(2)
- *M* api/openapi-spec/v3/apis__events.k8s.io__v1_openapi.json
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-7af1765cc467c5fe08d1bd40b6bdba6795e198142607d123c348bad1200bc72d>
(2)
- *M* api/openapi-spec/v3/apis__storage.k8s.io__v1_openapi.json
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-940eff8ea5539eaef866e270d95a472c30b147f3efae5f377857c22fe82b7788>
(2)
- *M* pkg/generated/openapi/zz_generated.openapi.go
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-9f4c1466e676f9e733cae72d369ffd5ff37f446116c98562807cf3bfb872ae95>
(2)
- *M* staging/src/k8s.io/api/core/v1/generated.proto
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-22a65693efec8e1bbaef93a230cc438a588551f78f5df8e10a6a4d8cabc9df7e>
(2)
- *M* staging/src/k8s.io/api/core/v1/types.go
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-5ffbebb40a29f8e491f4ff1e07c618eb3edcc0d5f00a1a71445272d6767d7883>
(2)
- *M*
staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-d7743328e88b67741ee281243ae21b0b2911a65d13c3aad9938451d717eb3765>
(2)
- *M* test/integration/apiserver/apply/apply_test.go
<https://github.com/kubernetes/kubernetes/pull/110495/files#diff-b629323ef67f91837456deae39526a0e81aa0a5397465ff546e6845211afb13d>
(209)
Patch Links:
- https://github.com/kubernetes/kubernetes/pull/110495.patch
- https://github.com/kubernetes/kubernetes/pull/110495.diff
—
Reply to this email directly, view it on GitHub
<#110495>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRGXIKVLUZ2RWUCSMQO2SLVOJNLTANCNFSM5YLNKKVQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@@ -5248,7 +5248,7 @@ var schemaYAML = typed.YAMLObject(`types: | |||
- name: uid | |||
type: | |||
scalar: string | |||
elementRelationship: atomic | |||
elementRelationship: separable |
There was a problem hiding this comment.
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?
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? |
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 |
Wouldn't you agree that we can't generally assume the UID will be set by the same controller/actor as the gvk? |
/triage accepted |
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. |
d3f8ba7
to
bd648f3
Compare
PR has been updated to use new SSA feature for a field-level specification to override a type-level 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. |
/lgtm thanks for going the extra mile for compatibility here, and for the test coverage |
[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 |
/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 |
Is this ever backported to older versions? @alexzielenski |
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. |
What type of PR is this?
/kind bug
/kind regression
What this PR does / why we need it:
This change changes the
structType
ofObjectReference
fromatomic
(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 ofPersistentVolume
'sclaimRef
, theuid
field and others might be populated bykube-controller-manager
but thename
ornamespace
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/assign @apelisse