Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for server side apply #392

Open
wants to merge 49 commits into
base: develop
Choose a base branch
from
Open

Conversation

redbaron
Copy link
Contributor

@redbaron redbaron commented Dec 14, 2021

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
Copy link
Contributor

100mik commented Dec 14, 2021

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


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()
Copy link
Contributor Author

@redbaron redbaron Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will think through it, thanks for call out 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@cppforlife cppforlife left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking pretty good so far.

pkg/kapp/cmd/app/apply_flags.go Outdated Show resolved Hide resolved
Comment on lines 56 to 58
if o.ApplyFlags.ServerSideApply {
o.DiffFlags.ChangeSetOpts.Mode = ctldiff.ServerSideApplyChangeSetMode
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to PreRunE

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/tools/diff_flags.go Outdated Show resolved Hide resolved
pkg/kapp/diff/change_factory.go Outdated Show resolved Hide resolved

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will think through it, thanks for call out 👍

test/e2e/diff_test.go Outdated Show resolved Hide resolved
pkg/kapp/diff/change_set.go Show resolved Hide resolved
Comment on lines 56 to 58
if o.ApplyFlags.ServerSideApply {
o.DiffFlags.ChangeSetOpts.Mode = ctldiff.ServerSideApplyChangeSetMode
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@@ -313,6 +313,8 @@ apiVersion: v1
kind: ConfigMap
metadata:
name: test-cm
annotations:
ann1: val1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@redbaron
Copy link
Contributor Author

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

@redbaron redbaron marked this pull request as ready for review January 11, 2022 15:01
@cppforlife
Copy link
Contributor

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

@cppforlife
Copy link
Contributor

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

Copy link
Contributor

@cppforlife cppforlife left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines +123 to +126
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
test/e2e/kapp.go Outdated Show resolved Hide resolved
test/e2e/kapp.go Show resolved Hide resolved
var err error

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines 22 to +23
type ChangeSetOpts struct {
AgainstLastApplied bool
Mode ChangeSetMode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkg/kapp/resources/mod_string_map_append.go Outdated Show resolved Hide resolved
@redbaron
Copy link
Contributor Author

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

@cppforlife
Copy link
Contributor

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

@ebma16
Copy link

ebma16 commented May 24, 2024

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
Copy link
Member

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
Copy link

ebma16 commented May 24, 2024

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants