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 additional apply mechanism -- server side apply #388

Open
redbaron opened this issue Dec 8, 2021 · 8 comments
Open

Support additional apply mechanism -- server side apply #388

redbaron opened this issue Dec 8, 2021 · 8 comments
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request

Comments

@redbaron
Copy link
Contributor

redbaron commented Dec 8, 2021

Describe the problem/challenge you have
I'd like make use of server side apply feature, bypassing rebaseRule entirely.

Describe the solution you'd like

An optional support for server-side apply, enabled by the --server-side flag.

Anything else you would like to add:

When server-side apply is in use rebaseRules are entirely bypassed. Diff is generated using dry run and diffing result with kubernetes objects currently in cluster to get a most accurate preview of changes.

Should server side apply generate a conflict, it can be resolved in kapp favour with --force flag.

kapp.k14s.io/original annotation remains populated to support subsequent kapp invocations without --server-side flag. Default rebaseRules probably need to be updated to keep cluster-managed metadata.managedFields path.

Related issues: #43

Server side apply doc: https://kubernetes.io/docs/reference/using-api/server-side-apply/


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@redbaron redbaron added carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request labels Dec 8, 2021
@100mik
Copy link
Contributor

100mik commented Dec 8, 2021

Interesting suggestion! We were working on having a --dry-run feature which would probably complement the server side validation which just made it to alpha I believe. We did run into a few edge cases like where users were deploying CRDs and CRs using the same manifest.

Will definitely explore the Server Side apply feature to see how we can leverage it. 🥂
Worth noting that we do generate diffs using the resource on the cluster when there are changes 🤔
Using the --diff-changes (or -c) flag will help you see the exact changes being applied before you confirm them.

@100mik 100mik added the discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution label Dec 8, 2021
@cppforlife cppforlife removed the carvel triage This issue has not yet been reviewed for validity label Dec 8, 2021
@cppforlife
Copy link
Contributor

it looks like server side apply and it's conflict resolution framework is better approach

kapp tries to provide a guarantee that what gets applied results in same outcome regardless of what happened on the server. in server side apply world, for example, keys that are set by something else on the server side will never be unset by future apply that do not touch that section. this definitely "fights" against general philosophy of kapp. but lets take that convo to this other thread you started.

(copying my comment from #385 (comment))

@cppforlife cppforlife changed the title Support for a server side apply Support additional apply mechanism -- server side apply Dec 8, 2021
@redbaron
Copy link
Contributor Author

redbaron commented Dec 8, 2021

keys that are set by something else on the server side will never be unset by future apply that do not touch that section

This is exactly what I want though. I want deploy tool to be concerned only with values provided to it to deploy, not anything else added by other actors, which is very well aligned with server side apply approach.

Given your strong preference for explicit rebase as it is now, would you still concider a PR implementing SSA?

Worth noting that we do generate diffs using the resource on the cluster when there are changes 🤔

Simple diff between resources to apply and cluster state would show deletion of any fields added outside of kapp, right? When applied with server side apply, those fields wont actually be deleted (because they never been in the kapp). Showing diff between dryrun and cluster state shows expected changes more accurately.

@cppforlife
Copy link
Contributor

Given your strong preference for explicit rebase as it is now, would you still concider a PR implementing SSA?

yes, we would (let us know if you want to work on the PR). we definitely would like to support it as an alternative apply mechanism -- just not as default.

This is exactly what I want though. I want deploy tool to be concerned only with values provided to it to deploy, not anything else added by other actors, which is very well aligned with server side apply approach.

yeah, fair enough -- if you are aware of the side effects thats really your call to make for your environments.

i was just pointing out that this specifically enables your cluster to "drift away" from your desired config. desired config doesnt just state which fields should exist, but also implicitly states which fields should not exist. 2 clusters that are deployed from the same config, in reality may not be actually configured the same way.

@redbaron
Copy link
Contributor Author

redbaron commented Dec 8, 2021

yes, we would (let us know if you want to work on the PR). we definitely would like to support it as an alternative apply mechanism -- just not as default.

I am embedding kapp into our deploy tool (not as CLI) and started working on SSA for it. I don't think it is going to be too much extra work to code it such that it is upstreamable. I outlined rough user visible plan in the opening comment.

@renuy renuy added carvel accepted This issue should be considered for future work and that the triage process has been completed and removed discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution labels Dec 9, 2021
@ohookins
Copy link

Our use case is not so much around better/different conflict-resolution methods, but in the case of CRDs that exceed the client-side apply limit (~256kB) you cannot apply these client side at all. We need server-side apply in these cases as there's no other way to apply them.

@praveenrewar
Copy link
Member

client-side apply limit (~256kB)

@ohookins Are you talking about the annotation limit? Are you facing this issue with kapp? I know that kubectl adds the original yaml as an annotation value to the object and hence the error might come up, but if you are using kapp then kapp would not add such an annotation if it's exceeding the limit. Could you confirm the behaviour that you are observing?

@ohookins
Copy link

No, it's nothing to do with annotations. Some CRDs for example are just huge by design, and cannot be applied client side no matter what (they exceed 256kB). Without an option for server side apply there's no way to use Kapp with these CRDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request
Projects
Status: To Triage
Development

No branches or pull requests

6 participants