Skip to content

install/kubernetes: add priorityClasses#16933

Closed
aanm wants to merge 1 commit intocilium:masterfrom
aanm:pr/add-priority-classes
Closed

install/kubernetes: add priorityClasses#16933
aanm wants to merge 1 commit intocilium:masterfrom
aanm:pr/add-priority-classes

Conversation

@aanm
Copy link
Copy Markdown
Member

@aanm aanm commented Jul 19, 2021

Since not all Cilium components are required to run on a cluster, this
commit adds priorityClasses into them. With
system-[node|cluster]-critical priority class set, it prevents pods
from being deleted by kubelet, which they are critical for the node
and / or cluster.

Since not all Cilium components are required to run on a cluster, this
commit adds priorityClasses into them. With
system-[node|cluster]-critical priority class set, it prevents pods
from being deleted by kubelet, which they are critical for the node
and / or cluster.

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm added release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.10 labels Jul 19, 2021
@aanm aanm requested a review from a team as a code owner July 19, 2021 23:52
@aanm aanm requested review from a team and errordeveloper July 19, 2021 23:52
@rolinh rolinh self-assigned this Jul 20, 2021
Copy link
Copy Markdown
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

As I point out below, I don't think that Hubble UI should be marked as system critical. I'm also tempted to say that Hubble Relay should not be marked as cluster critical as losing Hubble Relay is not critical to the good operation of the cluster. This is subject to debate though as loosing Relay also means loosing cluster-wide visibility which might come in handy in situations where the cluster is under pressure.

{{- end }}
restartPolicy: Always
{{- if and (or (and (eq .Release.Namespace "kube-system") (gt $k8sMinor "10")) (ge $k8sMinor "17") (gt $k8sMajor "1")) .Values.enableCriticalPriorityClass }}
priorityClassName: system-cluster-critical
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think that Hubble UI should be marked as system-cluster-critical. If the cluster is under pressure, I think evicting Hubble UI should be OK as global visibility should still be provided by Hubble Relay that can be queried using the Hubble CLI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and hubble-relay as well

@gandro
Copy link
Copy Markdown
Member

gandro commented Jul 21, 2021

I just noticed that this overlaps with #16896

@dungdm93 dungdm93 mentioned this pull request Jul 23, 2021
@aanm
Copy link
Copy Markdown
Member Author

aanm commented Jul 23, 2021

closing in favor of #16896

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants