Unsupport atomic to granular schema reconciliation #192

kevindelgado · 2021-03-31T23:10:19Z

Fixes #191 by removing support for atomic to granular schema changes.

Now, when a field on a schema is changed from atomic to granular, the schema will be treated as if it was always granular, potentially causing existing managers to own less fields than they did previously. Previously, they would own all the contents of an atomic list/map field, now they will only own the top level field itself and none of its contents.

This was determined to be less worse than incorrectly assuming empty fields were previously atomic (and causing managers to own more fields than they should).

Following this we plan to:

Document this behavior in upstream kubernetes documentation.
Potentially revisit larger sweeping changes that could enable this support in the future (i.e. making the fieldset aware of atomicity, creating a separate "defaulting" manager to handle defaulted fields atomicity, etc)

k8s-ci-robot · 2021-03-31T23:10:26Z

Hi @kevindelgado. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

kevindelgado · 2021-03-31T23:13:25Z

I went ahead and removed the atomic-to-granular testing that we had. Let me know if you think there's any testing we need to add with this reversion.

It seemed odd to have tests to explicitly prove we don't support this flow, but let me know if you think anything like that should be added to this change.

FWIW testing to confirm no new fields are added to a top level apply manager will be captured in the testing for the Extract changes (where that behavior is needed), but don't seem super relevant here or in any of the schema reconciliation tests.

kevindelgado · 2021-03-31T23:13:40Z

/assign @jpbetz
/cc @apelisse

typed/reconcile_schema.go

apelisse · 2021-04-01T16:34:58Z

merge/schema_change_test.go

 	}
 }
-
-func TestAtomicToGranularSchemaChanges(t *testing.T) {


Can we actually keep the test and update it with the new behavior? If we document what happens when someone goes from atomic to granular, then we should have a test to confirm that behavior.

Seems useful to me as well. Might also be worth adding a comment in the tests explaining the behavior further.

done, let me know if the comments are sufficient

apelisse · 2021-04-01T16:35:29Z

typed/reconcile_schema.go

 func (v *reconcileWithSchemaWalker) doList(t *schema.List) (errs ValidationErrors) {
-	// reconcile lists changed from granular to atomic
+	// reconcile lists changed from granular to atomic.
+	// Note that migrations from atomic to granular are unsuported and will


I'd change from "unsported" to "unrecommended", and I'm not sure I understand the consequences from that sentence. (comment also applies to below for maps)

typed/reconcile_schema_test.go

jpbetz · 2021-04-01T17:29:42Z

merge/schema_change_test.go

+				// Upon changing the schema of struct from
+				// atomic to granular, manager one continues
+				// to own the same fielset as before,
+				// but does not retain ownership of any of the subfields.


Also make it clear that this is a known limitation and not the ideal behavior?

jpbetz · 2021-04-01T17:33:51Z

/lgtm

kevindelgado · 2021-04-01T17:36:23Z

lgtm removed b/c I fixed a comment typo

jpbetz · 2021-04-01T17:43:34Z

/lgtm

apelisse · 2021-04-01T19:56:40Z

typed/reconcile_schema.go

+	// be treated as if they were always granular.
+	//
+	// In this case, the manager that owned the previously atomic field (and all subfields),
+	// will now own just the top-level field and none of the subfields.


apelisse · 2021-04-01T19:57:09Z

/approve

Do we have other plans for SMD or should I cut a release once this is merged?

k8s-ci-robot · 2021-04-01T19:57:17Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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 Mar 31, 2021

k8s-ci-robot requested review from apelisse and jennybuckley March 31, 2021 23:10

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2021

k8s-ci-robot assigned jpbetz Mar 31, 2021

jpbetz reviewed Apr 1, 2021

View reviewed changes

typed/reconcile_schema.go Show resolved Hide resolved

kevindelgado force-pushed the no-atomic-to-granular branch from 2045cd8 to f73880c Compare April 1, 2021 16:47

kevindelgado requested a review from jpbetz April 1, 2021 16:48

apelisse reviewed Apr 1, 2021

View reviewed changes

kevindelgado force-pushed the no-atomic-to-granular branch from f73880c to 326ec8e Compare April 1, 2021 17:20

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2021

kevindelgado requested a review from apelisse April 1, 2021 17:20

jpbetz reviewed Apr 1, 2021

View reviewed changes

kevindelgado force-pushed the no-atomic-to-granular branch from 326ec8e to 460a665 Compare April 1, 2021 17:32

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 1, 2021

kevindelgado requested a review from jpbetz April 1, 2021 17:33

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2021

Unsupport atomic to granular

4278010

kevindelgado force-pushed the no-atomic-to-granular branch from 460a665 to 4278010 Compare April 1, 2021 17:35

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2021

apelisse reviewed Apr 1, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021

k8s-ci-robot merged commit d20b1ce into kubernetes-sigs:master Apr 1, 2021

jpbetz mentioned this pull request Jun 30, 2021

Manual cherry pick of #103318: Bump SMD to v4.1.2 to pick up #102749 fix kubernetes/kubernetes#103319

Merged

sbueringer mentioned this pull request Mar 20, 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

Unsupport atomic to granular schema reconciliation #192

Unsupport atomic to granular schema reconciliation #192

Uh oh!

Conversation

kevindelgado commented Mar 31, 2021

Uh oh!

k8s-ci-robot commented Mar 31, 2021

Uh oh!

kevindelgado commented Mar 31, 2021

Uh oh!

kevindelgado commented Mar 31, 2021

Uh oh!

Uh oh!

apelisse Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

jpbetz Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

kevindelgado Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

apelisse Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

kevindelgado Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpbetz Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindelgado Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

jpbetz commented Apr 1, 2021

Uh oh!

kevindelgado commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpbetz commented Apr 1, 2021

Uh oh!

apelisse Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

apelisse commented Apr 1, 2021

Uh oh!

k8s-ci-robot commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jpbetz Apr 1, 2021 •

edited

Loading

kevindelgado commented Apr 1, 2021 •

edited

Loading