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

Support setting priority class name in broker pods #154

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

paulzhang97
Copy link
Contributor

Add priorityClassName to cluster CRD for setting priority class in Redpanda broker pods.

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2024

CLA assistant check
All committers have signed the CLA.

@paulzhang97 paulzhang97 changed the title [DRAFT] Support setting priority class name in broker pods Support setting priority class name in broker pods Jun 6, 2024
@@ -104,6 +104,8 @@ type ClusterSpec struct {
// Replicas determine how big the cluster will be.
// +kubebuilder:validation:Minimum=0
Replicas *int32 `json:"replicas,omitempty"`
// PriorityClassName is the name of PriortyClass.
Copy link

Choose a reason for hiding this comment

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

nit: typo PriorityClass

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can this comment be expanded ever so slightly? Something like PriorityClassName is used to set the PodSpec.PriorityClassName of the redpanda Statefulset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -1071,6 +1071,9 @@ spec:
you can read more in https://kubernetes.io/docs/tasks/run-application/configure-pdb/
x-kubernetes-int-or-string: true
type: object
priorityClassName:
description: PriorityClassName is the name of PriortyClass.
Copy link

Choose a reason for hiding this comment

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

nit: typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

Couple nits but otherwise LGTM

@@ -284,6 +286,25 @@ func (r *Cluster) validateScaling() field.ErrorList {
return allErrs
}

func (r *Cluster) validatePriorityClassName() field.ErrorList {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO there's no need to validate this. This error will eventually be encountered when pods are trying to be scheduled. It's more of the "kubernetes way" to let misconfigurations fail in a backoff loop rather than validate them up front; for better or worse.

I don't have super strong feelings here if the existing validators do something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I agree. Just follow the existing pattern. I will keep it.

@@ -104,6 +104,8 @@ type ClusterSpec struct {
// Replicas determine how big the cluster will be.
// +kubebuilder:validation:Minimum=0
Replicas *int32 `json:"replicas,omitempty"`
// PriorityClassName is the name of PriortyClass.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can this comment be expanded ever so slightly? Something like PriorityClassName is used to set the PodSpec.PriorityClassName of the redpanda Statefulset?

@paulzhang97 paulzhang97 requested a review from sbocinec June 7, 2024 14:26
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

I think with the webhook validation existing deployments would not work as priority class name would be empty string if not provided in cluater cr. As we should not have any more customers using cluster CR I'm fine keeping this implementation

@paulzhang97 paulzhang97 merged commit dd0c3ad into main Jun 7, 2024
5 checks passed
@chrisseto chrisseto deleted the paulz/priority-class branch June 10, 2024 13:52
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

5 participants