Skip to content

Conversation

mikkeloscar
Copy link
Contributor

@mikkeloscar mikkeloscar commented May 15, 2024

Proof of Concept of porting the "kubectl apply" code from deployment-service to also utilize it in CLM. Basically this means making the kubectl apply operation via Go instead of calling the external kubectl binary.

The motivation for this change is that we currently restrict how many executions of kubectl apply we do concurrently as it's hard to control the memory. This has the side effect of limiting how many clusters we can process in parallel. The idea is that by having this in go we save memory by not having to run another process and we thus can do more in parallel.

The feature is behind a config-item kubectl_apply_inline: true which enables us to configure it only in a few clusters as a start to validate that it behaves as expected and gives the intended benefits.

TODO

  • Roll out behind a config-item to test in selected clusters first
  • Check if we still need to control concurrency.

@mikkeloscar mikkeloscar added do-not-merge minor Minor changes, e.g. low risk config updates, changes that do not introduce a new API call. labels May 15, 2024
@mikkeloscar mikkeloscar force-pushed the kubectl-apply branch 2 times, most recently from 20f40d1 to f2cd750 Compare October 24, 2024 14:44
@mikkeloscar mikkeloscar marked this pull request as ready for review January 27, 2025 11:01
@mikkeloscar mikkeloscar changed the title [PoC] In-code kubectl apply In-code kubectl apply Jan 27, 2025
@mikkeloscar mikkeloscar force-pushed the kubectl-apply branch 2 times, most recently from 31b264b to ec9ed09 Compare February 4, 2025 08:21
@mikkeloscar mikkeloscar force-pushed the kubectl-apply branch 2 times, most recently from 99333c1 to 982953b Compare February 12, 2025 21:48
@mikkeloscar mikkeloscar force-pushed the kubectl-apply branch 3 times, most recently from 77377f8 to b0bd34f Compare February 19, 2025 11:00
@demonCoder95
Copy link
Member

Check if we still need to control concurrency.

Does this require us to run any tests? I think it's fine to rollout this change and observe after a while, to check this concurrency requirement.

Otherwise, PR looks good 👍

@mikkeloscar
Copy link
Contributor Author

Check if we still need to control concurrency.

Does this require us to run any tests? I think it's fine to rollout this change and observe after a while, to check this concurrency requirement.

Yes, this we can best observe after rolling it out and enabling it in some clusters.

@mikkeloscar
Copy link
Contributor Author

👍

@mikkeloscar
Copy link
Contributor Author

👍

Signed-off-by: Mikkel Oscar Lyderik Larsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Minor changes, e.g. low risk config updates, changes that do not introduce a new API call.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants