Add server-side apply mode #651

smuth4 · 2021-11-29T18:44:08Z

Aside from all the benefits listed elsewhere, this would fix issues like prometheus-operator/kube-prometheus#1511.

While testing I did find that it's not entirely a clean conversion when working with objects that have already been applied client-side. Non-functional formatting differences (e.g. "1024Mi" vs "1Gi") or missing default fields will cause the apply to fail and report that it has a field management conflict with "kubectl-client-side-apply". Once a server-side apply succeeds (either by recreating the object or by using --force-conflicts) everything works as expected, just wanted to mention it since it was a bit surprising.

I also kept the code simple as a POC, if desired I can make it a flag similar to --diff-strategy.

Duologic

Is the server-side the default in kubectl (from 1.22)? If not, can we make it optional on Tanka too?

smuth4 · 2021-12-14T20:35:36Z

It's intended to be a general replacement for any client-side apply, but I don't know of any active plans to make it the default for kubectl in particular. I've moved it to be an optional field in spec.json, still defaulting to the normal apply.

Duologic · 2021-12-23T08:35:21Z

docs/docs/server-side-apply.md

+many can be found before an apply with the `validate`
+[diff strategy](/diff-strategy).


This PR already looks great.

It may be a little out-of-scope but I was wondering whether we should make this consistent for both diff and apply.
tk diff could also honor the applyStrategy option if set in spec.json and it could have a CLI flag (replacing diff-strategy=validate): tk diff --diff-strategy=native --server-side (native is the default strategy)

@julienduchesne do you think this makes sense?

I think --diff-strategy=validate should be renamed (or aliased) to --diff-strategy=server, that would be consistent enough IMO. Having both a --server-side and a --(diff|apply)-strategy might get confusing.

Also, the diffStrategy and applyStrategy settings are nice since you can toggle them independently in the spec (someone may wish to apply server-side but do a simple diff, or vice-versa, server-side sometimes has a weird behavior)

julienduchesne

I would add the --apply-strategy flag for consistency between apply and diff. Other than that, this PR looks great!

julienduchesne · 2022-01-05T20:01:56Z

docs/docs/server-side-apply.md

+many can be found before an apply with the `validate`
+[diff strategy](/diff-strategy).


I think --diff-strategy=validate should be renamed (or aliased) to --diff-strategy=server, that would be consistent enough IMO. Having both a --server-side and a --(diff|apply)-strategy might get confusing.

Also, the diffStrategy and applyStrategy settings are nice since you can toggle them independently in the spec (someone may wish to apply server-side but do a simple diff, or vice-versa, server-side sometimes has a weird behavior)

smuth4 · 2022-01-10T22:42:00Z

@julienduchesne @Duologic I ended up adding a 4th distinct diff mode called server, which displays the server-side diff in all it's glory, and is the new default whenever server-side apply is enabled (still override-able by CLI flags and spec.json). I figured being truer to what the apply would actually do was worth having the cruft being shown. I'm not stuck on this behavior though, if you want me to change it back to a simple alias or something, that's not an issue.

I also added the --diff-strategy flag to the apply command since the code seemed to already be set up for that.

fculpo · 2022-01-15T09:53:53Z

Hi, this MR would solve a lot of issue for us too, when can we expect it to land in master ?

julienduchesne · 2022-01-20T14:24:33Z

Hi, this MR would solve a lot of issue for us too, when can we expect it to land in master ?

The changes look good to me overall (I'd like to look more in-depth though)
I'll give it a test drive this week and I think I'd like to do a release around end of next week to integrate jsonnet 0.18, so I'll see if I can integrate this one as well

sh0rez · 2022-01-28T14:17:36Z

Hi!

Aside from all the benefits listed elsewhere, this would fix issues like prometheus-operator/kube-prometheus#1511.

OMG yes, finally!

Before doing an in-depth code review, I got some general questions.

From https://kubernetes.io/blog/2019/01/14/apiserver-dry-run-and-kubectl-diff/:

APIServer dry-run [...]. kubectl diff does exactly what you want by showing the
differences between the current "live" object and the new "dry-run" object.

To me, that reads as if bare kubectl diff already runs server-side. This PR appears to be introducing a new server diff strategy. What is the difference in behavior? And how does it compare to validate?
Currently, this seems to be hidden away after a flag. Given the relevance of described above issue, it sounds to me as if it should be the default behavior, falling back to client side apply depending on version number. Let's enable it from 1.16+, keeping the flag as an override.

julienduchesne · 2022-01-28T15:08:31Z

I think I can answer those

To me, that reads as if bare kubectl diff already runs server-side. This PR appears to be introducing a new server diff strategy. What is the difference in behavior? And how does it compare to validate?

The output is very different between a bare diff and --server-side diff. The server-side diff will show lots of additional fields being modified
validate was added to fix #332. It does a server-side diff to check for errors but then shows the normal non-server-side diff for readability. So the options are essentially:

native: Only diff non server-side
validate: diff server-side to check for errors then diff non-server-side
server Only diff server-side

Currently, this seems to be hidden away after a flag. Given the relevance of described above issue, it sounds to me as if it should be the default behavior, falling back to client side apply depending on version number. Let's enable it from 1.16+, keeping the flag as an override.

That would be kind of breaking. For our own workflows, we'd have to explicitely disable it to get more readable diffs. Also, server-side diff will break on certain manifests that diff and apply just fine in non server-side mode

fculpo · 2022-02-24T08:57:06Z

Hi, any progress on this ?

smuth4 · 2022-03-26T18:49:51Z

I've been using this for a while without issue and would still like to see it get merged. If that last commit to fix pruning complicates things, I can split it out into a separate PR.

tomasfreund · 2022-04-14T15:51:03Z

Hi, is there any progress on this? it would be quite useful for us to have this feature

smuth4 · 2022-04-18T13:06:09Z

I see #695 was merged in with a different approach to server-side support, is this PR still useful? CC @jjo

julienduchesne · 2022-04-25T13:05:44Z

Sorry for the lack of communication on this @smuth4. The other PR sneaked by due it being much smaller. If you still need the config attributes, feel free to rebase and @ me, I'll be sure to take a look this time

julienduchesne · 2022-04-25T13:05:44Z

Sorry for the lack of communication on this @smuth4. The other PR sneaked by due it being much smaller. If you still need the config attributes, feel free to rebase and @ me, I'll be sure to take a look this time

jjo · 2022-04-25T14:13:49Z

Sorry for the lack of communication on this @smuth4. The other PR sneaked by due it being much smaller. If you still need the config attributes, feel free to rebase and @ me, I'll be sure to take a look this time

I much prefer @smuth4 approach, as it's much more sustainable re: future kubectl apply/diff args, totally fine to rollback #695 (the sooner the better).

Also mark the field manager as being tanka

Also add --diff-strategy flag to `apply`

smuth4 · 2022-04-27T12:51:03Z

Thanks for the responses, I've rebased it with the flag rolled back, and if it gets rolled back separately I don't mind rebasing again.

@julienduchesne I think this is pretty much ready for review. There might be room for improvement in cases where a apply --force will fail on the diff but apply perfectly fine (typically on fields that something else tried to manage), but I don't think that's a must-solve issue.

Edit: Immediately after typing that, I realized that I'm incorrect, it would be a problem for anyone who wants to use validate. In the interest of maintain existing workflows first and foremost, I've added --force-conflicts to all server-side diffs, but it could probably stand to be made more dynamic.

julienduchesne

Alright! Let's try this out! Thanks for your work @smuth4!

Duologic reviewed Dec 13, 2021

View reviewed changes

smuth4 force-pushed the serverside branch from 46e60a8 to 2b157fb Compare December 14, 2021 20:32

Duologic reviewed Dec 23, 2021

View reviewed changes

julienduchesne self-requested a review January 3, 2022 01:43

julienduchesne reviewed Jan 5, 2022

View reviewed changes

smuth4 changed the title ~~Apply changes server-side starting in v1.22~~ Add server-side apply mode Jan 10, 2022

julienduchesne force-pushed the serverside branch from 7502209 to dfbd174 Compare January 28, 2022 13:31

julienduchesne requested review from sh0rez and Duologic January 28, 2022 13:38

smuth4 force-pushed the serverside branch from dfbd174 to d8cb183 Compare February 21, 2022 14:34

smuth4 added 7 commits April 26, 2022 19:18

Apply changes server-side starting in v1.22

3491327

Move selection of apply strategy to spec.json

63e691e

Also mark the field manager as being tanka

Add documentation for server-side apply

e61076f

Add client version check for --field-manager flag

8b08520

Refactor diff to add new server strategy

c27c8af

Add --apply-strategy command line flag

ae066f6

Also add --diff-strategy flag to `apply`

Update documentation

867f4e8

smuth4 added 2 commits April 26, 2022 19:23

Change default diff strategy to server in server-side applies

3442c5d

Include server-side created objects when scanning for orphans

f2de2b1

smuth4 force-pushed the serverside branch from 0d03071 to f2de2b1 Compare April 27, 2022 12:50

Default to use --force-conflicts in server-side diffs

47c5e5a

julienduchesne approved these changes Apr 27, 2022

View reviewed changes

julienduchesne merged commit e3922f8 into grafana:main Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add server-side apply mode #651

Add server-side apply mode #651

smuth4 commented Nov 29, 2021

Duologic left a comment

smuth4 commented Dec 14, 2021

Duologic Dec 23, 2021

julienduchesne Jan 5, 2022

julienduchesne left a comment

julienduchesne Jan 5, 2022

smuth4 commented Jan 10, 2022 •

edited

Loading

fculpo commented Jan 15, 2022

julienduchesne commented Jan 20, 2022 •

edited

Loading

sh0rez commented Jan 28, 2022

julienduchesne commented Jan 28, 2022

fculpo commented Feb 24, 2022

smuth4 commented Mar 26, 2022

tomasfreund commented Apr 14, 2022

smuth4 commented Apr 18, 2022 •

edited

Loading

julienduchesne commented Apr 25, 2022

julienduchesne commented Apr 25, 2022

jjo commented Apr 25, 2022

smuth4 commented Apr 27, 2022 •

edited

Loading

julienduchesne left a comment

		many can be found before an apply with the `validate`
		[diff strategy](/diff-strategy).

Add server-side apply mode #651

Add server-side apply mode #651

Conversation

smuth4 commented Nov 29, 2021

Duologic left a comment

Choose a reason for hiding this comment

smuth4 commented Dec 14, 2021

Duologic Dec 23, 2021

Choose a reason for hiding this comment

julienduchesne Jan 5, 2022

Choose a reason for hiding this comment

julienduchesne left a comment

Choose a reason for hiding this comment

julienduchesne Jan 5, 2022

Choose a reason for hiding this comment

smuth4 commented Jan 10, 2022 • edited Loading

fculpo commented Jan 15, 2022

julienduchesne commented Jan 20, 2022 • edited Loading

sh0rez commented Jan 28, 2022

julienduchesne commented Jan 28, 2022

fculpo commented Feb 24, 2022

smuth4 commented Mar 26, 2022

tomasfreund commented Apr 14, 2022

smuth4 commented Apr 18, 2022 • edited Loading

julienduchesne commented Apr 25, 2022

julienduchesne commented Apr 25, 2022

jjo commented Apr 25, 2022

smuth4 commented Apr 27, 2022 • edited Loading

julienduchesne left a comment

Choose a reason for hiding this comment

smuth4 commented Jan 10, 2022 •

edited

Loading

julienduchesne commented Jan 20, 2022 •

edited

Loading

smuth4 commented Apr 18, 2022 •

edited

Loading

smuth4 commented Apr 27, 2022 •

edited

Loading