Support for server side apply #392

redbaron · 2021-12-14T09:13:18Z

First take on server side apply feature (#388) to collect early feedback.

Some highlights:

user provided manifest + some kapp generated annotations is what we are submitting to K8S via PATCH request.
diff calculation is at the core of kapp code paths, so I had to approximate it by diffing existing instances against dry run SSA result. This required calls to K8S much earlier in the code than it was previously, which required some plumbing
because we have to make dry run call, there is a new NewChangeSSA change factory
E2E test runner now supports KAPP_E2E_SSA=1 env var value. When set, --ssa-enabled flag is added to kapp invocation allowing same tests to be run in SSA mode. Tests with rebasing are skipped in SSA mode.
To make debugging e2e cases easier I created embeddable runner, which runs kapps *corbra.Command in the testing process. It is not a full replacement for a kapp command execution, but greatly helped developing this feature as most interesting scenarios come up in e2e and not unit testing.

Unimplemented:

SSA conflict force flag. It is present, but not used anywhere.
conflicting CLI flags are ignored in favour of SSA feature. I think there should be a clear error message if conflicting flags are specificed, like --diff-against-last-applied.

Yet untested (unless there is an existing E2E test covering it):

how changes to namespace are handled
how changes to apiVersion are handled (apps/v1beta1 -> apps/v1)

Features deliberately left outside of this PR:

Ownership transfer as described in docs. There might be situations where user might want to give up ownership of certain fields after initial creation. kapp could assist with it.
Fine grained force conflict resolution. Right now it is all or nothing. There is no fine grained force option in the kubernetes, so it has to be implemented as 2 separate operations: non-SSA patch for conflicting fields + SSA for the rest. I'll create an issue with more detailed proposal once SSA work is merged

100mik · 2021-12-14T09:23:39Z

Thanks for the draft PR @redbaron !
Will definitely go through the changes to get an idea of what's going on.

redbaron · 2021-12-14T19:15:40Z

pkg/kapp/diff/change_factory.go


 	if existingRes != nil {
+		// Strip rebasing "base" object of kapp history so that it never shows up in the diff, regardless
+		// of rebase rules used
+		existingResForRebasing, err = f.NewResourceWithHistory(existingRes).HistorylessResource()


I believe there was a sleeper bug here, which unfolded like following:

By the end of this function existingRes was unconditionally stripped of history with HistorylessResource()

newRes was unconditionally stripped of history with HistorylessResource()

existingResForRebasing has history kept, because it comes from the cluster as is

When building rebasedNewRes using existingResForRebasing + newRes, history was removed due to default rebase rules included in the kapp transplanting history-less annotations from newRes

If we redeploy same object as previous deployment, end result has no spurious diff, because both objects have history stripped

Should provided config had different rebase rules, rebasedNewRes is going to have history annotations from existingResForRebasing, but existingRes wont, generating spurious diff.

Similar situation arises when metadata.managedFields is populated by the cluster. This field is considered history and is stripped in HistorylessResource() call, but default rebase rules have no mentions of it.

Instead of updating default rebase rule, I opted for fixing it at the source. Here I am stripping history from all objects, ensuring it is not going to show up in any diffs regardless of kapp config provided. Technically it is a change in behaviour, hence this comment to highlight it.

will think through it, thanks for call out 👍

to double check -- changes in NewChangeAgainstLastApplied are not really affecting SSA, right? (i might have to ask to separate them out into a diff PR).

cppforlife

looking pretty good so far.

pkg/kapp/cmd/app/apply_flags.go

cppforlife · 2021-12-15T15:45:11Z

pkg/kapp/cmd/app/deploy.go

+			if o.ApplyFlags.ServerSideApply {
+				o.DiffFlags.ChangeSetOpts.Mode = ctldiff.ServerSideApplyChangeSetMode
+			}


not a fan of doing it in RunE, lets move this into Run()... (also going to see if there is a better place for this)

I plan to add flag validation step and return error, rather than silently overwriting values provided by user, so it should be a RunE I believe because it can return error.

I believe we could move these validations to the Run()function as it returns an error and let the command initialisation have

RunE: func(_ *cobra.Command, _ []string) error { return o.Run() }

Moved to PreRunE

pkg/kapp/cmd/app/deploy.go

pkg/kapp/cmd/tools/diff_flags.go

pkg/kapp/diff/change_factory.go

cppforlife · 2021-12-15T15:56:33Z

pkg/kapp/diff/change_factory.go


 	if existingRes != nil {
+		// Strip rebasing "base" object of kapp history so that it never shows up in the diff, regardless
+		// of rebase rules used
+		existingResForRebasing, err = f.NewResourceWithHistory(existingRes).HistorylessResource()


will think through it, thanks for call out 👍

test/e2e/diff_test.go

pkg/kapp/diff/change_set.go

test/e2e/diff_test.go

…ow up in the diff anyway

100mik · 2021-12-16T10:24:11Z

pkg/kapp/cmd/app/deploy.go

+			if o.ApplyFlags.ServerSideApply {
+				o.DiffFlags.ChangeSetOpts.Mode = ctldiff.ServerSideApplyChangeSetMode
+			}


I believe we could move these validations to the Run()function as it returns an error and let the command initialisation have

RunE: func(_ *cobra.Command, _ []string) error { return o.Run() }

pkg/kapp/resources/resources.go

…g up in diff

redbaron · 2021-12-16T15:09:56Z

test/e2e/config_test.go

@@ -313,6 +313,8 @@ apiVersion: v1
 kind: ConfigMap
 metadata:
  name: test-cm
+  annotations:
+    ann1: val1


I've added this to show that not all annotations stripped when object is passed to rebase rules

redbaron · 2021-12-16T15:11:59Z

test/e2e/config_test.go

@@ -351,15 +353,18 @@ data:
 			"values": asYAML(t, map[string]interface{}{
 				"existing": func() interface{} {
 					raw := cm.Raw()
+					removeHistory(raw)


Consequence of https://github.com/vmware-tanzu/carvel-kapp/pull/392/files#r768970039 is that rebase rules don't see history in objects passed to them. I've adjusted test to reflect it.

redbaron · 2021-12-16T15:17:43Z

Hm, all tests are passing locally on Docker Desktop managed 1.21 kubernetes cluster, but TestSSAUpdateSimple fails in CI.

Strategic merge patch is not support for CRDs without schema

redbaron · 2022-01-11T14:59:47Z

OK, it reached the stage where I'd like to propose it for a merge.

I added option to run E2E tests with SSA enabled and test suite now passes with SSA on my 1.21 K8S, except for tests with rebasing. I'll go through diff and add notes on parts worth paying attention to.

TestIgnoreFailingAPIServices fails for me locally even on master, because error message text differs from what test expects.

There is a missing test for --ssa-force option, I'll add it in due course, but otherwise I consider initial implementation complete.
I'll also update PR message to reflect current state shortly.

/cc @cppforlife

cppforlife · 2022-01-11T20:04:51Z

cool cool. ill do a deep dive today/tomorrow on this @redbaron. other folks will also take a look...

cppforlife · 2022-01-13T20:06:09Z

(still reading through the code... hoping to be done tomorrow)

cppforlife

looking pretty good. ive left some nit-picks, and a few "designy" changes to hopefully simplify a little bit of code.

pkg/kapp/cmd/app/deploy_flag_help_sections.go

cppforlife · 2022-01-14T19:04:44Z

test/e2e/update_server_side_test.go

+	// Patch dry run returns merged object with all patch-file fields present
+	err = kubectl.RunWithOptsIntoJSON([]string{"patch", "svc", "redis-primary", "--patch-file", tmpFile.Name(), "--dry-run=client"},
+		RunOpts{IntoNs: true}, &expectedObj)
+	require.NoError(t, err)


seems like a complicated way of determining expected state. is there something simpler that can be done (aka more static/hard-coded into the test)?

test/e2e/update_server_side_test.go

test/e2e/kapp.go

cppforlife · 2022-01-14T21:03:49Z

pkg/kapp/clusterapply/add_or_update_change.go

+	var err error
+
+	if c.aou.opts.ServerSideApply {
+		updatedRes, err = ctlres.WithIdentityAnnotation(c.newRes, func(r ctlres.Resource) (ctlres.Resource, error) {


it seems that we have two types of Patch operations: one where we are working with a resource, and one where we are working with some "minimal/raw" patch. instead of exposing WithIdentityAnnotation outside of IdentifiedResources resources class, lets keep existing Patch method to work with Resource class (instead of []byte) and change it so that it can add annotation on the fly. lets also add PatchRaw which does take []byte. this should result in keeping WithIdentityAnnotation as private.

cppforlife · 2022-01-14T21:19:35Z

pkg/kapp/diff/change_factory.go


 	if existingRes != nil {
+		// Strip rebasing "base" object of kapp history so that it never shows up in the diff, regardless
+		// of rebase rules used
+		existingResForRebasing, err = f.NewResourceWithHistory(existingRes).HistorylessResource()


to double check -- changes in NewChangeAgainstLastApplied are not really affecting SSA, right? (i might have to ask to separate them out into a diff PR).

cppforlife · 2022-01-14T21:22:20Z

pkg/kapp/diff/change_set.go

 type ChangeSetOpts struct {
-	AgainstLastApplied bool
+	Mode ChangeSetMode


after reading through flag "adjusting" above, i think we should just turns this into:

type ChangeSetOpts struct { ServerSideApply bool AgainstLastApplied bool }

anywhere where we are doing a switch based on mode, we should just do:

switch { case opts.ServerSideApply: ... case opts.AgainstLastApplied: ... default: ... }

the reasoning for this, it seems that mode enum introduces quite a bit of gymanistics necessary to compute what it should be.

cppforlife · 2022-01-14T21:23:57Z

pkg/kapp/diff/resource_with_history.go

@@ -218,5 +211,11 @@ func (resourceWithoutHistory) removeAppliedResAnnKeysMods() []ctlres.ResourceMod
 			ResourceMatcher: ctlres.AllMatcher{},
 			Path:            ctlres.NewPathFromStrings([]string{"metadata", "annotations", debugAppliedResDiffFullAnnKey}),
 		},
+		// metadata.managedFields technically is not kapp history, but we want to strip it in same situations we are
+		// stripping kapp history, it is server side apply history after all
+		ctlres.FieldRemoveMod{


without this, was this breaking anything? (if so, just for ssa, or for non-ssa operation?)

Yes, with SSA enabled it was reporting diff on managedFields where it shouldn't

pkg/kapp/resources/mod_string_map_append.go

redbaron · 2022-01-19T10:02:52Z

Thanks for a quick review, I'll make requested changes shortly

Also rename SSAFlags struct fields

cppforlife · 2022-02-03T21:49:02Z

@redbaron lmk, when to take a look at the PR again.

ebma16 · 2024-05-24T07:44:38Z

Hey guys,

Could you give me a brief update on the topic? Are there still plans to support server-side-apply or will there be no further work on this topic for the time being?
I only ask because in my project I need to update some Kubernetes resources partially shared with Carvel as well as with other tools (e.g. Operator SDK) and the other tools support SSA.

praveenrewar · 2024-05-24T12:57:56Z

Hi @ebma16! Unfortunately the maintainers don't have the bandwidth to work on this at the moment, and I am not sure when we will be able to prioritise it. We can definitely prioritise a review if you would be interested in submitting a PR or continuing on this one.

kapp rebase rules should still work well with other tools that support SSA. Could you share the specific problem that you are facing?

ebma16 · 2024-05-24T13:12:28Z

Hi @praveenrewar,
thank's for the update and your quick response!

At the moment I have no problems with the kapp rebase rules. We are still in an early phase of the project and I just did a small test for the interaction between the Java-Operator-SDK (using SSA) and the kapp rebase rules (without SSA). So far I could not find any problems and everything worked. I was just curious about the current status when I read this thread ;)

I will continue with this constellation (kapp without SSA and other tools using SSA). If I notice any problems, I will definitely let you know and continue this thread!

praveenrewar · 2024-05-24T13:43:56Z

Sure, keep us updated. Happy to chat on carvel slack channel too if you need help with anything.

redbaron added 3 commits December 10, 2021 13:02

Adds e2e test for simple server side apply merge case

fd0f790

Embeddable E2E tests

176ebb5

First basic SSA support

20bd0a6

redbaron added 3 commits December 14, 2021 11:11

Fix tests compilation

27e77f8

Make linter happy

6884b1e

Fix diffing logic so that it never diffs managed fields

7de82bd

redbaron commented Dec 14, 2021

View reviewed changes

renuy requested review from cppforlife, rohitagg2020, praveenrewar and 100mik December 15, 2021 06:19

cppforlife reviewed Dec 15, 2021

View reviewed changes

praveenrewar reviewed Dec 15, 2021

View reviewed changes

test/e2e/diff_test.go Show resolved Hide resolved

redbaron added 3 commits December 15, 2021 19:00

Cosmetic changes to address feedback

1f77748

Remove unused field

5ff0cd5

Don't strip history in ChangeFactory.NewChangeSSA because it won't sh…

a1b7202

…ow up in the diff anyway

100mik reviewed Dec 16, 2021

View reviewed changes

redbaron added 4 commits December 16, 2021 12:06

Another fix for NewChangeAgainstLastApplied to prevent history showin…

e7b4682

…g up in diff

Move dryRun to PatchOpts struct

d17a09e

Make RunEmbedded to fail tests on error

868d362

Adjust tests to reflect that history is not passed to rebasing rules

6a42593

redbaron commented Dec 16, 2021

View reviewed changes

redbaron added 5 commits December 16, 2021 19:02

Add SSA support for diff command. Check CLI flags for conflicts

3c0ba71

Add copyright and license header

1e81fce

Run all e2e tests as kapp subprocess. Remove unnecessary error check.

df4a333

Fix diff generation for versioned renamed objects

307bbe9

Support enabling SSA for E2E tests via env var globally

8eb9d07

redbaron added 12 commits January 10, 2022 13:11

env.Namespace more tests

632638e

Skip conflict rebasing test in SSA mode

6af080b

env.Namespace more tests

c01292c

Use JSON merge patch when recording history

c6d868e

Strategic merge patch is not support for CRDs without schema

Add SSASkip and comments

527553b

Run E2E tests in SSA and non-SSA mode

c98601d

Fix nil panic in tests

4301a35

Remove RunEmbedded leftovers in e2e tests

2c4cb43

Use FieldManagerName from CLI args in the AddPlainStrategy

6fd31bd

Add support for ssa-force flag

c9be433

Make linter happy

8a952e0

Remove unused file

3a9a9fc

redbaron requested a review from cppforlife January 11, 2022 14:59

redbaron marked this pull request as ready for review January 11, 2022 15:01

cppforlife reviewed Jan 14, 2022

View reviewed changes

redbaron added 5 commits January 20, 2022 17:53

Smallest nits

389316e

Localize force param value calculation

96fb26c

Also rename SSAFlags struct fields

Duplicate SSAFlags in SSAOpts and move them back to cmd/tools

9795527

Remove dead code

f94e5b3

Don't try to resolve update conflict when using SSA

9c5d0ea

praveenrewar mentioned this pull request Sep 22, 2022

Conflict on weird fields #573

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for server side apply #392

Support for server side apply #392

redbaron commented Dec 14, 2021 •

edited

100mik commented Dec 14, 2021

redbaron Dec 14, 2021 •

edited

cppforlife Dec 15, 2021

cppforlife Jan 14, 2022

cppforlife left a comment

cppforlife Dec 15, 2021

redbaron Dec 15, 2021

100mik Dec 16, 2021

redbaron Dec 16, 2021

cppforlife Dec 15, 2021

100mik Dec 16, 2021

redbaron Dec 16, 2021

redbaron Dec 16, 2021

redbaron commented Dec 16, 2021

redbaron commented Jan 11, 2022

cppforlife commented Jan 11, 2022

cppforlife commented Jan 13, 2022

cppforlife left a comment

cppforlife Jan 14, 2022

cppforlife Jan 14, 2022

cppforlife Jan 14, 2022

cppforlife Jan 14, 2022

cppforlife Jan 14, 2022

redbaron Jan 20, 2022

redbaron commented Jan 19, 2022

cppforlife commented Feb 3, 2022

ebma16 commented May 24, 2024 •

edited

praveenrewar commented May 24, 2024

ebma16 commented May 24, 2024

praveenrewar commented May 24, 2024

Support for server side apply #392

Are you sure you want to change the base?

Support for server side apply #392

Conversation

redbaron commented Dec 14, 2021 • edited

100mik commented Dec 14, 2021

redbaron Dec 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cppforlife left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbaron commented Dec 16, 2021

redbaron commented Jan 11, 2022

cppforlife commented Jan 11, 2022

cppforlife commented Jan 13, 2022

cppforlife left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbaron commented Jan 19, 2022

cppforlife commented Feb 3, 2022

ebma16 commented May 24, 2024 • edited

praveenrewar commented May 24, 2024

ebma16 commented May 24, 2024

praveenrewar commented May 24, 2024

redbaron commented Dec 14, 2021 •

edited

redbaron Dec 14, 2021 •

edited

ebma16 commented May 24, 2024 •

edited