add status wiping section to apply kep #1123

kwiesmueller · 2019-06-30T19:51:04Z

Address the Status Wiping Issue for Server-Side Apply reported in kubernetes/kubernetes#75564

/sig api-machinery
/wg apply
/cc @apelisse @jennybuckley

kwiesmueller · 2019-06-30T19:53:52Z

@apelisse I started with the section. Not sure about the interface (or mostly where it should be). I spent some time trying to verify how this could work out, but haven't got a finished path yet. Happy for a second look.
Especially where the code should be in the end and how to make it available to fieldManager is on my mind.

keps/sig-api-machinery/0006-apply.md

fejta-bot · 2019-10-10T18:08:19Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

kwiesmueller · 2019-10-12T11:53:23Z

/remove-lifecycle stale

kwiesmueller · 2019-12-10T22:20:53Z

keps/sig-api-machinery/0006-apply.md

+type ResetFieldsProvider interface {
+  // ResetFieldsFor returns a set of fields for the provided version that get reset before persisting the object.
+  // If no fieldset is defined for a version, nil is returned.
+  ResetFieldsFor(version string) *fieldpath.Set


The name ResetFieldsFor may not be ideal. Reset could als be read as action, which it is not in this case.
Haven't found a better name yet, open for suggestions.
WipedFields did not match, as fields don't really get wiped but some of them (like generation) just get modified.

I think naming them method something like ResetList or FieldResetList could be a better name based on my understanding that the function returns the fields that will be reset (please correct me if I am wrong)

Alternatively you could name it along the lines of ResetDryRun since DryRun usually means it does not perform any actions but returns usable data

Not sure about ResetList as it might be misunderstood the same way it currently could.
Maybe it's just me, but as mentioned in my comment to me I could understand people reading ResetAnything and think "oh, it resets things".
English could use "Resetted" :D
DryRun is also not really what it does, as a dry run indicates a run, of the action. This doesn't run anything, just provide information to run.
Also DryRun is used in other areas and I wouldn't want to mix them up.

yeah that makes sense. I was just pitching ideas. I think the name you gave it is good enough

kwiesmueller · 2019-12-10T22:21:56Z

keps/sig-api-machinery/0006-apply.md

+
+    if !diff.IsSame() {
+      // TODO: define good error message
+      return nil, nil, fmt.Errorf("apply contained fields a user should not change:\n%s", diff)


The error should probably be an api error? Like a conflict.

joshmsamuels · 2019-12-17T23:30:05Z

keps/sig-api-machinery/0006-apply.md

+
+var resetFieldsByVersion = map[string]*fieldpath.Set{
+  "v1": fieldpath.NewSet(
+    fieldpath.MakePathOrDie("status"),


How does this method return an error? Maybe you should use fieldpath.MakePath instead since fieldpath.MakePathOrDie panics instead of returning an error

They panic, yes.
This is done when starting the apiserver and if it panics here it indicates a developer error. It's the same we do when building the fieldset we currently wipe from managed fields.
If I remember correctly the only way this could fail is on an invalid symbol on the path. Anyways, it should be nothing that can happen at random but only when the developer made a mistake and it immediately surfaces in every test.

That sounds good to me.

Im still curious where the error is returned. Your use of resetFieldsByVersion has an ok parameter

resetFieldsByVersion is a map.
In golang, if you lookup a key inside a map and that key does not exist you get a zero value.
The additional return (ok) provides a boolean indicating the key exists or not.
https://blog.golang.org/go-maps-in-action

But thanks for pointing out, before I rebased, this was not returning a pointer but now it might not be required anymore to do this and I could directly return the value.

joshmsamuels · 2019-12-17T23:30:49Z

keps/sig-api-machinery/0006-apply.md

+// If no fieldset is defined for a version, nil is returned.
+func (podStrategy) ResetFieldsFor(version string) *fieldpath.Set {
+  set, ok := resetFieldsByVersion[version]
+  if !ok {


consider kloging the error

kwiesmueller · 2020-01-09T21:44:52Z

@apelisse do you think this is good to go?

apelisse · 2020-01-09T23:31:21Z

I think that's correct, thanks
/lgtm

apelisse · 2020-01-09T23:28:27Z

keps/sig-api-machinery/0006-apply.md


 (TODO: update this section with current design)

-What are the caveats to the implementation?


I suspect we could entirely remove that section, but probably in a different PR.

apelisse · 2020-01-09T23:30:38Z

keps/sig-api-machinery/0006-apply.md

+type ResetFieldsProvider interface {
+  // ResetFieldsFor returns a set of fields for the provided version that get reset before persisting the object.
+  // If no fieldset is defined for a version, nil is returned.
+  ResetFieldsFor(version string) *fieldpath.Set


version string is not super explicit, is it APIVersion?

apelisse · 2020-01-09T23:31:52Z

/assign @lavalamp
for approval

lavalamp · 2020-01-10T00:45:55Z

/lgtm
/approve

I think we can work out the exact names in the code change PRs and come back and update the KEP after that.

k8s-ci-robot · 2020-01-10T00:46:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, kwiesmueller, 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:

~~keps/sig-api-machinery/OWNERS~~ [lavalamp]

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 requested review from apelisse and jennybuckley June 30, 2019 19:51

apelisse reviewed Jul 1, 2019

View reviewed changes

keps/sig-api-machinery/0006-apply.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/0006-apply.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/0006-apply.md Outdated Show resolved Hide resolved

jennybuckley mentioned this pull request Jul 9, 2019

Umbrella Issue for Server Side Apply kubernetes/kubernetes#73723

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 10, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 12, 2019

kwiesmueller mentioned this pull request Oct 26, 2019

server-side apply takes ownership of fields that don't get set due to status wiping kubernetes/kubernetes#75564

Closed

This was referenced Dec 9, 2019

add extended remove method for typed values kubernetes-sigs/structured-merge-diff#133

Closed

[WIP] Apply status wiping kubernetes/kubernetes#86083

Closed

Kevin Wiesmüller added 5 commits December 10, 2019 23:06

add status wiping section to apply kep

21dc9c9

Apply: change interface naming and fix formating

3afaf8f

update interface definition to include old Object

81bf4b1

update proposal to explain the new approach

26be126

move status wiping section

012406c

kwiesmueller force-pushed the apply-status-wiping branch from e286092 to 012406c Compare December 10, 2019 22:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 10, 2019

Kevin Wiesmüller added 2 commits December 10, 2019 23:13

add implementation pr to status wiping section

05fce3a

text cleanup

be102e5

kwiesmueller commented Dec 10, 2019

View reviewed changes

Kevin Wiesmüller added 2 commits December 10, 2019 23:22

fix toc

14eed4d

update status wiping section to make apply ignore resetFields

b4bd10c

joshmsamuels reviewed Dec 17, 2019

View reviewed changes

kwiesmueller mentioned this pull request Dec 23, 2019

change apply signature and move it's decoding into handler kubernetes/kubernetes#86560

Merged

kwiesmueller changed the title ~~[WIP] add status wiping section to apply kep~~ add status wiping section to apply kep Jan 9, 2020

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 Jan 9, 2020

k8s-ci-robot assigned apelisse Jan 9, 2020

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

apelisse reviewed Jan 9, 2020

View reviewed changes

k8s-ci-robot assigned lavalamp Jan 9, 2020

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

k8s-ci-robot merged commit 1434952 into kubernetes:master Jan 10, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 10, 2020

kwiesmueller mentioned this pull request Jan 10, 2020

Server-side Apply #555

Closed

kwiesmueller mentioned this pull request Mar 25, 2020

[WIP] Server-Side Apply: Status Wiping/Reset Fields implementation kubernetes/kubernetes#89486

Closed

kwiesmueller mentioned this pull request Apr 22, 2020

[WIP] Server-Side Apply: Alternative Status Wiping/Reset Fields implementation kubernetes/kubernetes#90372

Closed

kwiesmueller mentioned this pull request Jul 5, 2020

[WIP] Server-Side Apply: Status Wiping/Reset Fields implementation kubernetes/kubernetes#92809

Closed

kwiesmueller mentioned this pull request Nov 12, 2020

[WIP] Server-Side Apply: Implement managedFields wiping kubernetes/kubernetes#96498

Closed

kevindelgado mentioned this pull request Mar 2, 2021

Server-Side Apply status wiping kubernetes/kubernetes#99661

Merged


		(TODO: update this section with current design)

		What are the caveats to the implementation?

add status wiping section to apply kep #1123

add status wiping section to apply kep #1123

Uh oh!

Conversation

kwiesmueller commented Jun 30, 2019

Uh oh!

kwiesmueller commented Jun 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fejta-bot commented Oct 10, 2019

Uh oh!

kwiesmueller commented Oct 12, 2019

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!

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!

kwiesmueller commented Jan 9, 2020

Uh oh!

apelisse commented Jan 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Jan 9, 2020

Uh oh!

lavalamp commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lavalamp commented Jan 10, 2020 •

edited

Loading