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

Helm chart: Add priorityClass support for operator #1423

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

varet80
Copy link
Contributor

@varet80 varet80 commented Jan 29, 2023

With the following change I am enabled the ability for the operator to be scheduled before minio clusters (if desired)

In case of restarts and or cases where required a full restart. We need operator to be scheduled before minio pods.
This way Pods can find the operator and startup properly.

priorityClass allows this more (https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/)

@pjuarezd
Copy link
Member

pjuarezd commented Feb 28, 2023

the tests failing are not related to this PR changes, there is no harm in allow set a priorityClass for operator deployment in the helm chart, already tested it.

@allanrogerr
Copy link
Contributor

Tests

Assign in Operator helm chart

operator:
  priorityClassName: first

Test deploy.

Ensure file /testing/deploy-tenant.sh is edited to install_operator helm. Once cluster is up, apply the priority class yaml below

./testing/deploy-tenant.sh

Observe tenant pods for priority

k -n minio-operator get pod/minio-operator-686b95f945-6sswx -o jsonpath="{.spec.priorityClassName}"
first

Validate operator and tenants work as expected

--

Create a priorty class

cat <<EOF | kubectl apply -f -
apiVersion: scheduling.k8s.io/v1
kind: PriorityClass
metadata:
  name: first
value: 1000000
globalDefault: false
description: "Test priority class"
EOF

@allanrogerr
Copy link
Contributor

Works as expected @varet80

@pjuarezd
Copy link
Member

Thank you @allanrogerr, could you approve the PR please?

Comment on lines +43 to +45
{{- with .Values.operator.priorityClassName }}
priorityClassName: {{ . }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

For uniformity, I would suggest:

Suggested change
{{- with .Values.operator.priorityClassName }}
priorityClassName: {{ . }}
{{- end }}
{{- with .Values.operator.priorityClassName }}
priorityClassName:
{{- toYaml . | nindent 8 }}
{{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Your thoughts?

Copy link
Member

@pjuarezd pjuarezd Feb 28, 2023

Choose a reason for hiding this comment

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

yeah!, same I though yesterday, but tried that and creates a carriage return that seems unecesary, ie:

priorityClassName:
        prioriryClass

The current code outpus as follows, that is more natural.

priorityClassName: className

@pjuarezd pjuarezd merged commit 6c2cb23 into minio:master Feb 28, 2023
pjuarezd pushed a commit to pjuarezd/operator that referenced this pull request Mar 2, 2023
Add priorityClass support for operator helm chart
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.

None yet

3 participants