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

Set priority class for the critical Flux components #3802

Merged
merged 1 commit into from
May 2, 2023

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Apr 16, 2023

This PR marks source-controller, kustomize-controller and helm-controller as system-cluster-critical. This priority class will reduce the chances of Flux controllers being evicted before other non-critical workloads and prevents the pods from being permanently unavailable.

Note that this does not prevent evictions, if a node is under pressure, the Flux pods will be evicted before pods marked as system-node-critical which is the highest available priority.

Fixes #3800

@stefanprodan stefanprodan added area/bootstrap Bootstrap related issues and pull requests area/install Install and uninstall related issues and pull requests labels Apr 16, 2023
@stefanprodan stefanprodan requested a review from a team April 16, 2023 09:13
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Why did you choose to leave out IRC and IAC?

Otherwise lgtm.

@stefanprodan
Copy link
Member Author

Why did you choose to leave out IRC and IAC?

Please see my comment here #3800 (comment)

@@ -4,3 +4,6 @@
- op: add
path: /spec/template/spec/serviceAccountName
value: helm-controller
- op: add
path: /spec/template/spec/priorityClassName
value: system-cluster-critical
Copy link
Contributor

@darkowlzz darkowlzz Apr 17, 2023

Choose a reason for hiding this comment

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

I've been trying to reason about how is this different from setting security context which is set on each of the individual component's deployments.
Priority class seems to be a common thing and not have any dependency on bootstrap related configurations like the service account and events-address that are set above.
I don't see anything wrong, but just wondering how to decide what should be added as a bootstrap patch and what should go to each of the individual component's deployment manifests.

Copy link
Member Author

Choose a reason for hiding this comment

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

wondering how to decide what should be added as a bootstrap patch and what should go to each of the individual component's deployment manifests.

I think the deployment manifests in each repo should contain the minimum needed to run the e2e tests. In the future, I really hope to create a dedicated repo for Flux distribution written in cuelang and drop all the kustomize overlays & templating done in the CLI.

Mark source-controller, kustomize-controller and helm-controller as system-cluster-critical.
This will reduce the chances of Flux controllers being evicted before other non-critical workloads.

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
@stefanprodan stefanprodan merged commit 73e2f56 into main May 2, 2023
8 checks passed
@stefanprodan stefanprodan deleted the system-cluster-critical branch May 2, 2023 15:56
@stefanprodan stefanprodan mentioned this pull request May 3, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bootstrap Bootstrap related issues and pull requests area/install Install and uninstall related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: include priorityClassName: system-node-critical to controller deployments
6 participants