Support granular<->atomic schema changes #170

jpbetz · 2020-08-12T22:47:10Z

WARNING: All merge type changes are backward incompatible! The functionality this PR provides is intended to reduce the pain caused should a merge type change need to be made (in a alpha API for example, or in a CRD where the author has control of all clients and is able to ensure the change is safe). This should be used with caution.

Enables developers to add/remove these tags to Kubernetes types, by reconciling existing managed field data with the schema change:

atomic tags: +listType=atomic, +structType=atomic, +mapType=atomic
granular tags: +listType=map with +listMapKey, +listType=set, +mapType=granular

The corresponding x-kubernetes OpenAPI annotations for CRDs are also supported.

Need to do more careful benchmarking in k8s, but a simple benchmark in SMD suggests a 3.8% bump in CPU utilization and less than a 0.5% bump in allocations. The low allocation number is expected because the traversal is read-only unless there is an actual reconciliation change to apply afterward.

Before:

BenchmarkMultipleApplierRecursiveRealConversion 	   11803	   1547320 ns/op	  708431 B/op	    7441 allocs/op

After:

BenchmarkMultipleApplierRecursiveRealConversion 	   10000	   1606807 ns/op	  711170 B/op	    7471 allocs/op

Unblocks kubernetes/kubernetes#92913 and kubernetes/kubernetes#91256.

xref kubernetes/kubernetes#93901 which enables structType=atomic usage in k8s

jpbetz · 2020-08-12T22:48:19Z

@apelisse I'll look into if what is required to have an atomic clear nested managed fields from other field managers when applied.

apelisse · 2020-08-13T04:02:42Z

Also, the second apply does not require a force, but probably should when it clears the ownership of the fields owned by the first applier

Correct. If we look at both cases (going from atomic to non-atomic and vice versa), both should probably generate a conflict:

changing an atomic list that already had items MUST be a conflict
adding an item to a list that used to be atomic should maybe generate a conflict. Note that we also need to somehow transfer ownership of existing items to the owner of the former atomic list.

Trying to see what happens on HA clusters that have multiple versions ... I don't think it has additional consequences beside the typical "things are blurry during transition".

merge/schema_change_test.go

apelisse · 2020-08-16T05:29:23Z

Let me know when I should take a look, thanks :-)

jpbetz · 2020-09-23T02:52:43Z

@apelisse This is ready for review.

jpbetz · 2020-09-23T03:01:15Z

@apelisse to close the loop on the case of handling atomic->granular schema changes. I was concerned that it might be possible to mis-interpret a field set that is already granular as atomic if the fieldset only owned the root of a map/list but not any of it's children (i.e.{ .: {}}). I added a test case for this and it does what you had hoped-- it drops ownership of the entire map once no children are owned. So it appears the case I was concerned with cannot happen. It looks like lists are handled the same way. This is great because it means if we ever see {} we know for sure the field was atomically owned when the fieldset was written.

jpbetz · 2020-09-23T20:18:21Z

@jennybuckley if you have any extra review bandwidth, this one could use some extra eyes

apelisse

Looks good, a couple of minor questions

apelisse · 2020-09-23T22:15:55Z

merge/update.go

 	return newObject, managers, nil
 }

+// reconcileManagedFieldsWithSchemaChanges reconciles the managed fields with any changes to the


Super nit: can you move that below "update/apply" methods.

typed/reconcile_schema.go

apelisse · 2020-09-28T19:53:24Z

merge/update.go

+		if err != nil {
+			return nil, err
+		}
+		reconciled, err := typed.ReconcileFieldSetWithSchema(versionedSet.Set(), tv)


I'm curious if we really need the live object for that? Can't we fix each manager set by merely looking at the current type information?

Not asking you to change anything, just curious if we considered that and if that's any better.

The reason we need the live type is that when we go from atomic to granular, we need to add to the fieldset all the fields that exist in the live object under the field that was previously atomic. We don't know exactly what they are just from the schema, because we don't know which fields are absent, or which map keys or list items are present.

apelisse · 2020-09-28T22:14:22Z

typed/reconcile_schema.go

+	return result, nil
+}
+
+func prefixWithPath(prefix fieldpath.Path, set *fieldpath.Set) *fieldpath.Set {


That makes me a little sad, there's probably a better way to do that :-)

I've switched to constructing the new field path in a more direct way that doesn't require the prefixing.

apelisse · 2020-09-28T22:24:26Z

merge/schema_change_test.go

+				  string: "b"
+			`,
+			APIVersion: "v1",
+			Managed: fieldpath.ManagedFields{


jpbetz · 2020-09-29T19:16:01Z

Thanks for the review @apelisse! Feedback applied.

apelisse · 2020-09-29T21:16:58Z

/lgtm
/approve

k8s-ci-robot · 2020-09-29T21:17:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, jpbetz

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:

~~OWNERS~~ [apelisse]

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

apelisse · 2020-09-29T21:16:45Z

typed/reconcile_schema.go

 	return node, ok
 }

+func descendToPath(node *fieldpath.Set, path fieldpath.Path) *fieldpath.Set {


Nit for next time we look at it, this code should probably live in fieldpath rather than here.

apelisse · 2020-09-29T21:58:42Z

We also probably need to cherry-pick this in as many versions as we can :-(

jpbetz · 2020-09-30T03:20:52Z

We also probably need to cherry-pick this in as many versions as we can :-(

Roger that.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 12, 2020

k8s-ci-robot requested review from apelisse and jennybuckley August 12, 2020 22:47

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 12, 2020

jpbetz mentioned this pull request Aug 12, 2020

Treat LabelSelectors as atomic by Server Side Apply to fix #92913 kubernetes/kubernetes#93901

Merged

apelisse reviewed Aug 13, 2020

View reviewed changes

merge/schema_change_test.go Show resolved Hide resolved

jpbetz force-pushed the test-atomic-introduction branch from 382e16c to 82b7e0f Compare August 14, 2020 21:33

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 14, 2020

jpbetz force-pushed the test-atomic-introduction branch from 82b7e0f to e965b91 Compare August 25, 2020 22:02

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

jpbetz force-pushed the test-atomic-introduction branch 5 times, most recently from 411eaee to eb03fad Compare September 19, 2020 04: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 Sep 19, 2020

jpbetz changed the title ~~[WIP] Add test for changing struct from granular to atomic~~ [WIP] Support granular<->atomic schema changes Sep 21, 2020

jpbetz mentioned this pull request Sep 21, 2020

Merge patches to ImagePullSecrets for Service Accounts kubernetes/kubernetes#91256

Closed

jpbetz force-pushed the test-atomic-introduction branch 5 times, most recently from 5157704 to 9664c45 Compare September 23, 2020 02:20

jpbetz force-pushed the test-atomic-introduction branch 2 times, most recently from 1042d0a to b4bff7c Compare September 23, 2020 02:29

jpbetz force-pushed the test-atomic-introduction branch from b4bff7c to c4315fe Compare September 23, 2020 03:04

Add atomic<->granular schema change fixup

90b911a

jpbetz force-pushed the test-atomic-introduction branch from c4315fe to 90b911a Compare September 23, 2020 03:08

apelisse reviewed Sep 28, 2020

View reviewed changes

Apply feedback

9a22ec6

jpbetz force-pushed the test-atomic-introduction branch from f1d2e49 to 9a22ec6 Compare September 29, 2020 19:17

k8s-ci-robot assigned apelisse Sep 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2020

k8s-ci-robot merged commit 0bcdf88 into kubernetes-sigs:master Sep 29, 2020

apelisse reviewed Sep 29, 2020

View reviewed changes

jpbetz mentioned this pull request Sep 30, 2020

Ignore fieldset paths not in schema when reconciling #175

Merged

jpbetz mentioned this pull request Jan 13, 2021

Make selectors atomic kubernetes/kubernetes#97989

Merged

jpbetz mentioned this pull request Mar 2, 2021

graduate CSIServiceAccountToken to beta kubernetes/kubernetes#99298

Merged

jpbetz mentioned this pull request Apr 10, 2023

support node taints owned by different fieldManagers kubernetes/kubernetes#117142

Open

thockin mentioned this pull request Jul 18, 2024

Make ServiceBackendPort an atomic struct kubernetes/kubernetes#126207

Merged

jpbetz mentioned this pull request Jul 18, 2024

Using server-side-apply for an Ingress failed when updating IngressBackend from port number to port name kubernetes/kubernetes#125869

Closed

VannTen mentioned this pull request Nov 19, 2024

Make node taints manageable by differents field managers (server-side apply) kubernetes/kubernetes#128866

Open

5 tasks

sbueringer mentioned this pull request Feb 18, 2025

ClusterClass Upgrade Fails Due to ManagedFields Behavior Change in CAPI v1.6 → v1.8 kubernetes-sigs/cluster-api#11857

Closed

sbueringer mentioned this pull request Jul 10, 2025

✨ Enable ssatags KAL linter kubernetes-sigs/cluster-api#12470

Merged

Support granular<->atomic schema changes #170

Support granular<->atomic schema changes #170

Conversation

jpbetz commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpbetz commented Aug 12, 2020

Uh oh!

apelisse commented Aug 13, 2020

Uh oh!

Uh oh!

apelisse commented Aug 16, 2020

Uh oh!

jpbetz commented Sep 23, 2020

Uh oh!

jpbetz commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpbetz commented Sep 23, 2020

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

jpbetz commented Sep 29, 2020

Uh oh!

apelisse commented Sep 29, 2020

Uh oh!

k8s-ci-robot commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Sep 29, 2020

Uh oh!

jpbetz commented Sep 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jpbetz commented Aug 12, 2020 •

edited

Loading

jpbetz commented Sep 23, 2020 •

edited

Loading