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

feat(provider/kubernetes): support for kubectl server-side-apply strategy #5989

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

a7i
Copy link
Contributor

@a7i a7i commented Aug 4, 2023

kubernetes server-side apply (SSA) was released back in 1.14 and became GA In 1.22. This new strategy will use the new merging algorithm, as well as tracking field ownership at the kubernetes api-server

fixes: spinnaker/spinnaker#6865

@dbyron-sf
Copy link
Contributor

Still wrapping my head around the approach, but it very well could be OK. Would you please add some integration tests to get a bit more verification that it works properly?

@a7i
Copy link
Contributor Author

a7i commented Aug 5, 2023

Still wrapping my head around the approach, but it very well could be OK. Would you please add some integration tests to get a bit more verification that it works properly?

Thanks for the suggestion @dbyron-sf I have implemented two integration tests per your suggestion (client-side vs server-side apply)

…tegy

kubernetes server-side apply (SSA) was released back in 1.14 and became GA In 1.22. This new strategy will use the new merging algorithm, as well as tracking field ownership at the kubernetes api-server

Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
@jasonmcintosh
Copy link
Member

@a7i Any chance of getting the annotation changed so it supports the force/no-force option? E.g. the strategy.spinnaker.io/server-side-apply: force option? I'd love to get this in! My thought is that this should eventually be the "default" for applies, and the force would also be defaulted to on. BUT for the next release or two check for the annotation and eventually migrate to the server side apply as the default unless disabled. SO support three options:

  • strategy.spinnaker.io/server-side-apply: force - default if server side apply is set at all maybe (though lean towards requiring force if set)
  • strategy.spinnaker.io/server-side-apply: no-force - Allow server side apply but WITHOUT forcing. I question this based on docs, but think it could be good to support
  • strategy.spinnaker.io/server-side-apply: disabled - Something that in the future would disable server side applies IF this becomes the default. Probably don't need this today, just the force/no-force options, but if we wanted server side applies to be the default it's always nice to have fallback options in case there's a change in behavior (which... would NOT surprise me)

@a7i
Copy link
Contributor Author

a7i commented Nov 16, 2023

@jasonmcintosh thanks for following up. I took a stab at it, and it just seemed a bit messy to have yet another annotation. Also, one more arg that I had to pass around for deployment, etc. I also really dislike the idea of annotations as an API but it's the existing pattern so not much I can do about that.
I'll take another look.

@jasonmcintosh
Copy link
Member

Yeah the goal is ONE annotation just with values for it. This would match how ALB ingress controllers work. It's not... "ideal" but not sure there's a great answer without that to pass this kinda thing around without changing it on deploy stages.

@a7i
Copy link
Contributor Author

a7i commented Nov 17, 2023

Yeah the goal is ONE annotation just with values for it. This would match how ALB ingress controllers work. It's not... "ideal" but not sure there's a great answer without that to pass this kinda thing around without changing it on deploy stages.

Let me know how you feel about the new commit I pushed

Annotation Behavior
strategy.spinnaker.io/server-side-apply: 'false' Defaults to client-side Apply
strategy.spinnaker.io/server-side-apply: 'true' Uses Server-Side Apply (i.e. kubectl apply --server-side=true
strategy.spinnaker.io/server-side-apply: 'force-conflicts' Uses Server-Side Apply w/ Force (i.e. kubectl apply --server-side=true --force-conflicts=true

Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
@jasonmcintosh
Copy link
Member

Think this should work well, allowing all the options. LONG TERM, as said, would love it if "Server side apply w/force" was just the default, another release or two for that default ;)

@a7i
Copy link
Contributor Author

a7i commented Nov 17, 2023

Think this should work well, allowing all the options. LONG TERM, as said, would love it if "Server side apply w/force" was just the default, another release or two for that default ;)

How do you feel about field-managers? should that be another annotation?

for example, apply a Deployment but let HPA handle the replicas.

kubectl apply -f nginx-deployment-replicas-only.yaml \
  --server-side --field-manager=handover-to-hpa

@jasonmcintosh
Copy link
Member

For. the MOMENT probably an annotation. There's two sorta strategies that come into play

  • Do it as a pipeline argument, aka "overrideNamespace"
  • Annotation driven - for this particular resource, do X (eg disabling versioning)
    ARGUABLY (to my mind)... some of this would be better off NOT as an annotation but as an option on the stage itself similar to overrideNamespace. It's just... more work/trickier/etc. on the stage AND you lose a lot of granularity of control with the advantage that you can apply these without changing manifests (though I'd argue defining it in the manifest is a bit more DRY). As much as I like stage overrides, annotations offer a LOT of capability as well that can be done PER resource. E.g. "For a configmap do a server side apply but for a deployment don't". That CAN be critical as we've seen VERY different behavior on server side applies vs. kubectl applies.

It'd be interesting to have more conversations with the Cloud SIG on direction on this long term and impacts.

@jasonmcintosh
Copy link
Member

Merging this - but would love a docs update on the force changes ;) Thanks!!

@jasonmcintosh jasonmcintosh added the ready to merge Approved and ready for a merge label Nov 17, 2023
@mergify mergify bot added the auto merged Merged automatically by a bot label Nov 17, 2023
@a7i
Copy link
Contributor Author

a7i commented Nov 17, 2023

@jasonmcintosh spinnaker/spinnaker.io#363

@mergify mergify bot merged commit ce121b3 into spinnaker:master Nov 17, 2023
16 checks passed
@a7i a7i deleted the kubectl-ssa branch December 1, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for a merge target-release/1.33
Projects
None yet
5 participants