kubectl: `apply --server-side` managed fields migration #112905

alexzielenski · 2022-10-06T21:24:01Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR will transparently take ownership of any client-side-apply fields when --server-side apply is used. If this is not done, then it will not be possible to remove fields previously used by client-side-apply with server-side-apply (without manually patching managed fields yourself).

Previously this change was proposed in a KEP, but during discussion with the SIG we have scoped it down to a bug fix.

Which issue(s) this PR fixes:

addresses issues brought up in the following and likely more bug reports

Fixes #107980
Fixes #108081
Fixes #107417
Fixes #112826
Fixed #99003

Special notes for your reviewer:

Does this PR introduce a user-facing change?

For `kubectl`, `--server-side` now migrates ownership of all fields used by client-side-apply to the specified `--fieldmanager`. This prevents fields previously specified using kubectl from being able to live outside of server-side-apply's management and become undeleteable.

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

- [Discussion]: https://github.com/kubernetes/enhancements/pull/3518#discussion_r984724392

/sig cli
/cc @soltysh @KnVerey @apelisse

alexzielenski · 2022-10-06T21:24:50Z

/hold still doing some manual testing, but sharing this PR to show general approach

alexzielenski

left some comments/discussion points

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go

ardaguclu

I'm very happy to see that this known problem is being fixed.

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go

staging/src/k8s.io/client-go/util/csaupgrade/upgrade_test.go

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go

sftim · 2022-10-10T11:53:22Z

For the release note, can I suggest:

For `kubectl`, `--server-side` now migrates ownership of all fields used by client-side-apply to the specified `--fieldmanager`. This prevents fields previously specified using kubectl from being able to live outside of server-side-apply's management and become undeleteable.

?

fedebongio · 2022-10-11T20:06:20Z

/triage accepted

apelisse · 2022-10-12T20:28:55Z

I would definitely love to have @seans3 review this.

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

seans3

This PR is in a good place. This PR has a lot of useful comments -- we probably need more. It would be useful to describe the migration process high level somewhere. Something like:

field ownership metadata is stored in three places: server-side-manager
fields, client-side-manager fields, and the last_applied_configuration annotation.
The migration merges the client-side-manager fields into the server-side-manager
fields, and removes the last_applied_annotation (if keepLastApplied is false), so
the only remaining metadata is server-side-manager fields.

Warnings will be fine in the places you've enumerated. There are examples in the apply.go file showing how to print warnings.

The one area that we'll need to work on is the tests. Current kubectl apply unit tests are dense, but we'll need to add significant unit tests to ensure proper coverage. As a general rule, we should not have lower unit test coverage numbers after a PR. Including before/after unit test coverage numbers in the PR would be useful. Integration tests will be helpful, but the
focus should be on unit tests. Let's get together to see how to test this.

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

staging/src/k8s.io/client-go/util/csaupgrade/upgrade_test.go

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

apelisse

Mostly good, thanks, missing test-cmd as discussed offline.

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

alexzielenski · 2022-11-04T04:07:38Z

/retest

KnVerey

I have a couple more very minor suggestions, but no concerns. LGTM!

/lgtm
/approve

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

test/cmd/apply.sh

apelisse

/approve

I can lgtm once Katrina's comments have been addressed, thanks

apelisse · 2022-11-04T16:14:24Z

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go

+// the fields in the given set
+//
+// If there is an error decoding one of the fieldsets for any reason, it is ignored
+// and assumed not to match the query.


Maybe add a TODO here that we need to account for subresource. And maybe specifically look for empty subresource in the loop

todo was added here:

kubernetes/staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go

Lines 243 to 245 in 26a6e12

//!TODO: some users may want to migrate subresources.

// should thread through the args at some point.

entry.Subresource == "" &&

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

in discussion with SIG, there is a strong interest in keeping the last-applied-configuration around for a bit longer as other tools transition for of it. This is OK since SSA maintains the annotation on kubectl's behalf on the server-side if it exists migrate client-side-apply fields to SSA when --serverside-side is used kubernetes#107980 kubernetes#108081 kubernetes#107417 kubernetes#112826 add test to make sure only one apply is needed after migration

alexzielenski · 2022-11-04T22:56:04Z

/retest

apelisse

/lgtm

lavalamp · 2022-11-07T17:38:41Z

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go

+			"value": filteredManagers,
+		},
+		{
+			// Use "replace" instead of "test" operation so that etcd rejects with


We don't write this field to etcd so I don't think this explanation is right?

So if we do a json-patch test of resourceVersion, the patch fails to apply without much details. If we set the resourceVersion, then the compare-and-switch etcd performs to write the object fails, and we get a proper 409 conflict. The comment reads fine to me.

I think it's the "etcd rejects" part that threw me off, I thought it was implying that etcd was checking the field instead of this being passed as metadata on the request.

Maybe open an issue about improving error messages from test directives in patches?

We could improve the error messages, but I don't know if the apiserver will ever interpret/return a failure to apply the patch as a 409 Conflict? Also, applying that patch is done from a third-party dependency.

lavalamp · 2022-11-07T17:48:46Z

/approve

for client-go

BenTheElder

/approve
for vendor/modules.txt

k8s-ci-robot · 2022-11-07T17:56:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, BenTheElder, KnVerey, lavalamp

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/kubectl/OWNERS~~ [KnVerey,apelisse,lavalamp]
~~test/cmd/OWNERS~~ [BenTheElder,KnVerey,apelisse,lavalamp]
~~vendor/OWNERS~~ [BenTheElder]

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

alexzielenski · 2022-11-07T23:10:11Z

/hold cancel

k8s-ci-robot · 2022-11-08T00:55:26Z

@alexzielenski: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-integration	`ff5e44f`	link	unknown	`/test pull-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

alexzielenski · 2022-11-08T01:06:15Z

/retest

fjolublar · 2023-05-05T09:48:56Z

Hi @alexzielenski,
Are there any plans to bring this fix to 1.25?

k8s-ci-robot requested a review from apelisse October 6, 2022 21:24

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 6, 2022

k8s-ci-robot requested review from KnVerey and soltysh October 6, 2022 21:24

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2022

alexzielenski commented Oct 6, 2022

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go Show resolved Hide resolved

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go Outdated Show resolved Hide resolved

ardaguclu reviewed Oct 7, 2022

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 11, 2022

apelisse reviewed Oct 12, 2022

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

seans3 reviewed Oct 12, 2022

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

seans3 reviewed Oct 12, 2022

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

seans3 reviewed Oct 12, 2022

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Show resolved Hide resolved

seans3 reviewed Oct 13, 2022

View reviewed changes

apelisse reviewed Oct 14, 2022

View reviewed changes

apelisse reviewed Oct 17, 2022

View reviewed changes

staging/src/k8s.io/client-go/util/csaupgrade/upgrade.go Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go Outdated Show resolved Hide resolved

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 Oct 20, 2022

alexzielenski force-pushed the kubectl-apply-csa-migration branch from ade7e14 to 4651d35 Compare November 4, 2022 03:32

alexzielenski force-pushed the kubectl-apply-csa-migration branch from 4651d35 to 4617f3c Compare November 4, 2022 15:45

KnVerey approved these changes Nov 4, 2022

View reviewed changes

k8s-ci-robot assigned KnVerey Nov 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

apelisse reviewed Nov 4, 2022

View reviewed changes

alexzielenski force-pushed the kubectl-apply-csa-migration branch from 4617f3c to 798efe4 Compare November 4, 2022 21:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

alexzielenski added 2 commits November 4, 2022 14:09

update vendor

ff5e44f

alexzielenski force-pushed the kubectl-apply-csa-migration branch from 798efe4 to ff5e44f Compare November 4, 2022 21:09

apelisse reviewed Nov 4, 2022

View reviewed changes

k8s-ci-robot assigned apelisse Nov 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

lavalamp reviewed Nov 7, 2022

View reviewed changes

BenTheElder reviewed Nov 7, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2022

k8s-ci-robot merged commit 2f837dc into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

alexzielenski deleted the kubectl-apply-csa-migration branch November 16, 2022 18:49

apelisse mentioned this pull request May 15, 2023

Update dependencies to K8s v1.27 kubernetes/kops#15373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl: `apply --server-side` managed fields migration #112905

kubectl: `apply --server-side` managed fields migration #112905

alexzielenski commented Oct 6, 2022 •

edited

alexzielenski commented Oct 6, 2022

alexzielenski left a comment

ardaguclu left a comment

sftim commented Oct 10, 2022

fedebongio commented Oct 11, 2022

apelisse commented Oct 12, 2022 •

edited

seans3 left a comment

apelisse left a comment

alexzielenski commented Nov 4, 2022

KnVerey left a comment

apelisse left a comment

apelisse Nov 4, 2022

alexzielenski Nov 4, 2022

alexzielenski commented Nov 4, 2022

apelisse left a comment

lavalamp Nov 7, 2022

apelisse Nov 7, 2022 •

edited

lavalamp Nov 7, 2022

apelisse Nov 7, 2022 •

edited

lavalamp commented Nov 7, 2022

BenTheElder left a comment

k8s-ci-robot commented Nov 7, 2022

alexzielenski commented Nov 7, 2022

k8s-ci-robot commented Nov 8, 2022

alexzielenski commented Nov 8, 2022

fjolublar commented May 5, 2023

	//!TODO: some users may want to migrate subresources.
	// should thread through the args at some point.
	entry.Subresource == "" &&

kubectl: apply --server-side managed fields migration #112905

kubectl: apply --server-side managed fields migration #112905

Conversation

alexzielenski commented Oct 6, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

alexzielenski commented Oct 6, 2022

alexzielenski left a comment

Choose a reason for hiding this comment

ardaguclu left a comment

Choose a reason for hiding this comment

sftim commented Oct 10, 2022

fedebongio commented Oct 11, 2022

apelisse commented Oct 12, 2022 • edited

seans3 left a comment

Choose a reason for hiding this comment

apelisse left a comment

Choose a reason for hiding this comment

alexzielenski commented Nov 4, 2022

KnVerey left a comment

Choose a reason for hiding this comment

apelisse left a comment

Choose a reason for hiding this comment

apelisse Nov 4, 2022

Choose a reason for hiding this comment

alexzielenski Nov 4, 2022

Choose a reason for hiding this comment

alexzielenski commented Nov 4, 2022

apelisse left a comment

Choose a reason for hiding this comment

lavalamp Nov 7, 2022

Choose a reason for hiding this comment

apelisse Nov 7, 2022 • edited

Choose a reason for hiding this comment

lavalamp Nov 7, 2022

Choose a reason for hiding this comment

apelisse Nov 7, 2022 • edited

Choose a reason for hiding this comment

lavalamp commented Nov 7, 2022

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2022

alexzielenski commented Nov 7, 2022

k8s-ci-robot commented Nov 8, 2022

alexzielenski commented Nov 8, 2022

fjolublar commented May 5, 2023

kubectl: `apply --server-side` managed fields migration #112905

kubectl: `apply --server-side` managed fields migration #112905

alexzielenski commented Oct 6, 2022 •

edited

apelisse commented Oct 12, 2022 •

edited

apelisse Nov 7, 2022 •

edited

apelisse Nov 7, 2022 •

edited