Server side defaulting of values prevents diff patches from working #34292

pwittrock · 2016-10-07T02:26:58Z

Server side defaults are easily differentiated from other values set within the kubernetes system (e.g. DeploymentStatus). kubectl apply records the applied config as an annotation on the objects that it creates so that it can perform a 3-way diff to merge local changes into the live config without erasing other items such as the status.

Steps to reproduce:

Use kubectl apply -f to create a deployment using this config:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: nginx-deployment
spec:
  replicas: 3
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx
        image: nginx
        ports:
        - containerPort: 80

Update the config spec adding

strategy:
    type: "Recreate"

Apply the new config.

Result:

The Deployment "nginx-deployment" is invalid.
spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate'

This is caused because the server defaults spec.strategy.rollingUpdate but this is invisible to the client and cannot be differentiated from other changes made by the server that the client should ignore (e.g. status changes).

This can be seen for any field whose default is derived from another field such as Deployment labels and selectors (derived from the PodTemplate labels).

Solving this will require at least some changes on the server side. One possible solution would be to:

Have the server record which derived fields that it adds / updates
Have the client then merge these changes into the last-applied-config used by diff patch so they are deleted
Have the server re-perform defaulting / normalization on the resulting object from the PATCH

Thoughts / Comments / Suggestions?

The text was updated successfully, but these errors were encountered:

adohe-zz · 2016-10-07T12:28:53Z

I believe we have discussed this on this issue #26202

pwittrock · 2016-10-07T18:52:34Z

@adohe Yeah this is the root cause of #26202, but impacts a number of different cases

lavalamp · 2016-10-07T20:14:45Z

Did you mean, "not easily differentiated"?

lavalamp · 2016-10-07T20:23:45Z

My suggested workflow is something like:

Add a POST dry-run mode to apiserver, so clients can discover the result of the defaulting step.

Then, at apply time, we can compute some series like this:

diff := new - orig // compute the patch

orig_default := dryRun(orig)
cur_default := get()
cur = cur_default - orig_default // remove defaults that haven't changed

applied = cur + diff
applied_default = dryRun(applied)
final_patch = applied_default - cur_default
patch(final_patch)

You need to come up with a test suite to make sure the procedure you implement handles all the cases. I'm not convinced the above does.

The defaulting system can change. I think in that case we need to keep the old value over an update unless the user specifically changes it.
Other components in the system can make changes which we wish to keep.

bgrant0607 · 2016-11-01T07:09:12Z

@pwittrock @lavalamp Before we do anything about this, we need a design proposal PR.

Just handling defaults is not sufficient. See #1178 and #17333, the apply design description in #15894, and https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#late-initialization.

We should enumerate the problematic defaults.

Label selector defaulting is obviously one. We may want to reconsider both the current defaulting approach (#14961) and allowing updates (#26202).

We've also discussed adding discriminators for all oneof/unions (e.g., in #6979). That would enable us to clear all other fields when the discriminator changed. For backward compatibility, we'd also set the discriminator value from the one set field (and continue to return an error in the case of ambiguity). Representing these unions using OpenAPI extensions would also facilitate better validation and diff'ing.

If there are other cases of multiple mutable fields set from the same value, we should consider what to do about them on a case-by-case basis. Some may have been introduced due to lack of fine-grain API versioning (#34508).

That said, we should support dry run in the apiserver, though I wasn't thinking of using it for this purpose (#12143 more generally).

In the meantime, apply --force #16569 could maybe be used as a workaround.

mengqiy · 2016-12-15T00:18:37Z

We should enumerate the problematic defaults.

@andronat After examining all the defaulting in defaults.go, I have enumerated them in this doc. Feel free to edit it. People in the kubernetes-dev google group have permission to edit it, make sure you are in the group.

mengqiy · 2016-12-15T04:32:21Z

Label selector defaulting is obviously one. We may want to reconsider both the current defaulting approach (#14961) and allowing updates (#26202).

I will draft a proposal that we will use a similar approach like batch/job.

We've also discussed adding discriminators for all oneof/unions (e.g., in #6979). That would enable us to clear all other fields when the discriminator changed.

I will draft another proposal for using discriminators. I guess it should be use in version like v2alpha1, since we can't change API when they are GA.

andronat · 2016-12-15T04:37:58Z

Thanks @ymqytw! This is really nice! I'll take a look asap (probably this weekend). Sorry I was very busy those last couple of weeks.

bgrant0607 · 2017-01-27T17:22:54Z

Ref #24198

bgrant0607 · 2017-03-08T17:48:26Z

Also related: #25424

0xmichalis · 2017-06-24T20:46:15Z

/sig api-machinery

@apelisse

Automatic merge from submit-queue (batch tested with PRs 50919, 51410, 50099, 51300, 50296) Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy. With the new value in `patchStrategy`, the patch will include an optional directive that will tell the apiserver to clear defaulted fields and update. This will resolve issue like #34292 (comment) and similar issue caused by defaulting in volume. The change is [backward compatible](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md#version-skew). The proposal for this new patch strategy is in https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md The implementation to support the new patch strategy's logic is in #44597 and has been merged in 1.7. ```release-note Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy. ``` /assign @apelisse /assign @janetkuo for deployment change /assign @saad-ali for volume change

@apelisse

Automatic merge from submit-queue (batch tested with PRs 50919, 51410, 50099, 51300, 50296) Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy. With the new value in `patchStrategy`, the patch will include an optional directive that will tell the apiserver to clear defaulted fields and update. This will resolve issue like kubernetes/kubernetes#34292 (comment) and similar issue caused by defaulting in volume. The change is [backward compatible](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md#version-skew). The proposal for this new patch strategy is in https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md The implementation to support the new patch strategy's logic is in #44597 and has been merged in 1.7. ```release-note Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy. ``` /assign @apelisse /assign @janetkuo for deployment change /assign @saad-ali for volume change

fejta-bot · 2017-12-30T08:45:02Z

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.

Prevent issues from auto-closing with an /lifecycle frozen comment.

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

bgrant0607 · 2018-01-23T02:25:15Z

/remove-lifecycle stale

fejta-bot · 2018-06-14T01:36:27Z

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

bgrant0607 · 2018-06-15T03:08:12Z

Another apply-related issue
cc @apelisse

julianvmodesto · 2020-01-28T02:45:48Z

/wg apply

pwittrock added area/kubectl team/ux labels Oct 7, 2016

pwittrock assigned lavalamp and mengqiy Oct 7, 2016

lavalamp removed their assignment Oct 7, 2016

mengqiy mentioned this issue Oct 7, 2016

kubectl apply umbrella issue #15894

Closed

pwittrock added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 13, 2016

pwittrock mentioned this issue Oct 20, 2016

kubectl apply umbrella issue tracker #35234

Open

32 tasks

mengqiy mentioned this issue Oct 21, 2016

remove defaulting for deployment strategy #35343

Closed

bgrant0607 added the area/app-lifecycle label Nov 17, 2016

bgrant0607 mentioned this issue Nov 21, 2016

StatefulSets with the same selector interfere with each other #36859

Closed

bgrant0607 added the area/declarative-configuration label Dec 8, 2016

mengqiy mentioned this issue Dec 22, 2016

Proposal: Add Discriminators in All Unions/OneOf APIs kubernetes/community#229

Closed

This was referenced Mar 8, 2017

Write proposal for controller pod management: adoption, orphaning, ownership, etc. (aka controllers v2) #14961

Open

Workload API v1 requirements umbrella issue #42752

Closed

Field-level versioning #34508

Open

k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label May 31, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 24, 2017

k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jun 24, 2017

mengqiy mentioned this issue Aug 8, 2017

Add retainKeys to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy #50296

Merged

crimsonfaith91 mentioned this issue Aug 16, 2017

Controller selectors should be immutable after creation #50808

Closed

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

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

spiffxp removed team/ux (deprecated - do not use) labels Mar 15, 2018

tnozicka mentioned this issue Apr 6, 2018

Support API CREATE with dry run #62197

Closed

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

bgrant0607 added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 15, 2018

bgrant0607 unassigned mengqiy Jun 15, 2018

liggitt mentioned this issue Dec 18, 2018

Convert existing PVs to use volume topology #72139

Closed

apelisse mentioned this issue May 2, 2019

Add unions in core types #77370

Closed

k8s-ci-robot added the wg/apply label Jan 28, 2020

k8s-ci-robot added wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. and removed wg/apply labels Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server side defaulting of values prevents diff patches from working #34292

Server side defaulting of values prevents diff patches from working #34292

pwittrock commented Oct 7, 2016 •

edited

Loading

adohe-zz commented Oct 7, 2016

pwittrock commented Oct 7, 2016

lavalamp commented Oct 7, 2016

lavalamp commented Oct 7, 2016

bgrant0607 commented Nov 1, 2016

mengqiy commented Dec 15, 2016

mengqiy commented Dec 15, 2016

andronat commented Dec 15, 2016

bgrant0607 commented Jan 27, 2017

bgrant0607 commented Mar 8, 2017

0xmichalis commented Jun 24, 2017

fejta-bot commented Dec 30, 2017

bgrant0607 commented Jan 23, 2018

fejta-bot commented Jun 14, 2018

bgrant0607 commented Jun 15, 2018

julianvmodesto commented Jan 28, 2020

Server side defaulting of values prevents diff patches from working #34292

Server side defaulting of values prevents diff patches from working #34292

Comments

pwittrock commented Oct 7, 2016 • edited Loading

adohe-zz commented Oct 7, 2016

pwittrock commented Oct 7, 2016

lavalamp commented Oct 7, 2016

lavalamp commented Oct 7, 2016

bgrant0607 commented Nov 1, 2016

mengqiy commented Dec 15, 2016

mengqiy commented Dec 15, 2016

andronat commented Dec 15, 2016

bgrant0607 commented Jan 27, 2017

bgrant0607 commented Mar 8, 2017

0xmichalis commented Jun 24, 2017

fejta-bot commented Dec 30, 2017

bgrant0607 commented Jan 23, 2018

fejta-bot commented Jun 14, 2018

bgrant0607 commented Jun 15, 2018

julianvmodesto commented Jan 28, 2020

pwittrock commented Oct 7, 2016 •

edited

Loading