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

Allow users to configure the priorityClassName field in the Helm chart #403

Merged

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Mar 12, 2024

Added a priorityClassName field to the Helm chart so that users can reduce the chances of the approver-policy Pod being evicted (aka preempted) to make way for some other Pod on the node where it is currently running.

I've also written some documentation about why this is useful and why you should use priorityClassName: system-cluster-critical:

The name of the field and its location in the values.yaml file is consistent with trust-manager, csi-driver, csi-driver-spiffy.
But unlike those other charts I have not supplied a default "" value, because that adds distraction to the the generated Deployment and it's unclear what the effect of that empty value is.

$ kubectl explain pod.spec.priorityClassName
KIND:       Pod
VERSION:    v1

FIELD: priorityClassName <string>

DESCRIPTION:
    If specified, indicates the pod's priority. "system-node-critical" and
    "system-cluster-critical" are two special keywords which indicate the
    highest priorities with the former being the highest priority. Any other
    name must be defined by creating a PriorityClass object with that name. If
    not specified, the pod priority will be default or zero if there is no
    default.

Testing

  • Default without priorityClassName displays a warning.
$ helm upgrade cert-manager-approver-policy deploy/charts/approver-policy   --install   --namespace cert-manager  --wait --set app.approveSignerNames="{\
issuers.cert-manager.io/*,clusterissuers.cert-manager.io/*,\
googlecasclusterissuers.cas-issuer.jetstack.io/*,googlecasissuers.cas-issuer.jetstack.io/*,\
awspcaclusterissuers.awspca.cert-manager.io/*,awspcaissuers.awspca.cert-manager.io/*\
}" --set image.tag=v0.13.0
Release "cert-manager-approver-policy" has been upgraded. Happy Helming!
NAME: cert-manager-approver-policy
LAST DEPLOYED: Wed Mar 13 13:42:46 2024
NAMESPACE: cert-manager
STATUS: deployed
REVISION: 12
TEST SUITE: None
NOTES:
⚠️  WARNING: Consider increasing the Helm value `replicaCount` to 2 if you require high availability.
⚠️  WARNING: Consider setting the Helm value `podDisruptionBudget.enabled` to true if you require high availability.
⚠️  WARNING: Consider setting the Helm value `priorityClassName` if you require high availability.

CHART NAME: cert-manager-approver-policy
CHART VERSION: v0.0.0
APP VERSION: v0.0.0

cert-manager-approver-policy is a cert-manager project.

If you're a new user, we recommend that you read the [cert-manager Approval Policy documentation] to learn more.

[cert-manager Approval Policy documentation]: https://cert-manager.io/docs/policy/approval/
  • With priorityClassName, the value is added to the manifests
$ helm template deploy/charts/approver-policy/ --set priorityClassName=system-cluster-critical | grep -C10  priorityClass
        resources:
          {}

        securityContext:
          allowPrivilegeEscalation: false
          capabilities: { drop: ["ALL"] }
          readOnlyRootFilesystem: true

      hostNetwork: false
      dnsPolicy: ClusterFirst
      priorityClassName: "system-cluster-critical"
---
# Source: cert-manager-approver-policy/templates/webhook.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  name: cert-manager-approver-policy
  labels:
    app: cert-manager-approver-policy
    app.kubernetes.io/name: cert-manager-approver-policy
    helm.sh/chart: cert-manager-approver-policy-v0.0.0
  • With an invalid priorityClassName
$ helm upgrade cert-manager-approver-policy deploy/charts/approver-policy   --install   --namespace cert-manager  --wait --set app.approveSignerNames="{\
issuers.cert-manager.io/*,clusterissuers.cert-manager.io/*,\
googlecasclusterissuers.cas-issuer.jetstack.io/*,googlecasissuers.cas-issuer.jetstack.io/*,\
awspcaclusterissuers.awspca.cert-manager.io/*,awspcaissuers.awspca.cert-manager.io/*\
}" --set image.tag=v0.13.0 --set priorityClassName=system-cluster-criticalx

The installation just hangs, and interestingly there is no feedback on the Deployment.

$ kubectl describe deploy -n cert-manager cert-manager-approver-policy
...
  Priority Class Name:  system-cluster-criticalx
Conditions:
  Type             Status  Reason
  ----             ------  ------
  Available        True    MinimumReplicasAvailable
  Progressing      True    NewReplicaSetCreated
  ReplicaFailure   True    FailedCreate
OldReplicaSets:    cert-manager-approver-policy-584c99bfd4 (0/0 replicas created), cert-manager-approver-policy-654fc765d (1/1 replicas created), cert-manager-approver-policy-655975cf4 (0/0 replicas created)
NewReplicaSet:     cert-manager-approver-policy-6f675dbdcd (0/1 replicas created)
Events:
  Type    Reason             Age    From                   Message
  ----    ------             ----   ----                   -------
  Normal  ScalingReplicaSet  10m    deployment-controller  Scaled up replica set cert-manager-approver-policy-584c99bfd4 to 1
  Normal  ScalingReplicaSet  9m36s  deployment-controller  Scaled up replica set cert-manager-approver-policy-654fc765d to 1
  Normal  ScalingReplicaSet  9m22s  deployment-controller  Scaled down replica set cert-manager-approver-policy-584c99bfd4 to 0 from 1
  Normal  ScalingReplicaSet  8m30s  deployment-controller  Scaled up replica set cert-manager-approver-policy-655975cf4 to 1
  Normal  ScalingReplicaSet  8m22s  deployment-controller  Scaled down replica set cert-manager-approver-policy-654fc765d to 0 from 1
  Normal  ScalingReplicaSet  6m19s  deployment-controller  Scaled up replica set cert-manager-approver-policy-654fc765d to 1 from 0
  Normal  ScalingReplicaSet  6m11s  deployment-controller  Scaled down replica set cert-manager-approver-policy-655975cf4 to 0 from 1
  Normal  ScalingReplicaSet  5m9s   deployment-controller  Scaled up replica set cert-manager-approver-policy-6f675dbdcd to 1

But you do see events about the unrecognised priority class

$ kubectl events -n cert-manager
...
34s (x16 over 3m18s)    Warning   FailedCreate        ReplicaSet/cert-manager-approver-policy-6f675dbdcd   Error creating: pods "cert-manager-approver-policy-6f675dbdcd-" is forbidden: no PriorityClass with name system-cluster-criticalx was found
  • With a valid priority class, the installation succeeds in a standard Kind cluster
$ helm upgrade cert-manager-approver-policy deploy/charts/approver-policy   --install   --namespace cert-manager  --wait --set app.approveSignerNames="{\
issuers.cert-manager.io/*,clusterissuers.cert-manager.io/*,\
googlecasclusterissuers.cas-issuer.jetstack.io/*,googlecasissuers.cas-issuer.jetstack.io/*,\
awspcaclusterissuers.awspca.cert-manager.io/*,awspcaissuers.awspca.cert-manager.io/*\
}" --set image.tag=v0.13.0 --set priorityClassName=system-cluster-critical
Release "cert-manager-approver-policy" has been upgraded. Happy Helming!
NAME: cert-manager-approver-policy
LAST DEPLOYED: Wed Mar 13 13:41:30 2024
NAMESPACE: cert-manager
STATUS: deployed
REVISION: 11
TEST SUITE: None
NOTES:
⚠️  WARNING: Consider increasing the Helm value `replicaCount` to 2 if you require high availability.
⚠️  WARNING: Consider setting the Helm value `podDisruptionBudget.enabled` to true if you require high availability.

CHART NAME: cert-manager-approver-policy
CHART VERSION: v0.0.0
APP VERSION: v0.0.0

cert-manager-approver-policy is a cert-manager project.

If you're a new user, we recommend that you read the [cert-manager Approval Policy documentation] to learn more.

[cert-manager Approval Policy documentation]: https://cert-manager.io/docs/policy/approval/

Related Links

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 12, 2024
@wallrj wallrj force-pushed the add-priorityclassname-to-helm-chart branch from 56fd6a6 to dbbd374 Compare March 13, 2024 13:57
Copy link
Member Author

@wallrj wallrj Mar 13, 2024

Choose a reason for hiding this comment

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

image


For more information, see:
* [Guaranteed Scheduling For Critical Add-On Pods](https://kubernetes.io/docs/tasks/administer-cluster/guaranteed-scheduling-critical-addon-pods/)
* [Protect Your Mission-Critical Pods From Eviction With PriorityClass](https://kubernetes.io/blog/2023/01/12/protect-mission-critical-pods-priorityclass/)
Copy link
Member Author

Choose a reason for hiding this comment

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


```yaml
priorityClassName: system-cluster-critical
```
Copy link
Member Author

Choose a reason for hiding this comment

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

The justification for suggesting this priority class is written in:

@wallrj wallrj requested a review from erikgb March 13, 2024 14:41
@wallrj wallrj changed the title WIP: Allow users to configure the priorityClassName field in the Helm chart Allow users to configure the priorityClassName field in the Helm chart Mar 13, 2024
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 13, 2024
Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

One minor comment/question, but this LGTM as it stands.

/lgtm

deploy/charts/approver-policy/values.yaml Outdated Show resolved Hide resolved
@jetstack-bot jetstack-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 14, 2024
@wallrj wallrj force-pushed the add-priorityclassname-to-helm-chart branch from f105b20 to 8755dce Compare March 14, 2024 09:15
@@ -81,6 +81,9 @@ spec:

hostNetwork: {{ (or .Values.app.webhook.hostNetwork .Values.hostNetwork) }}
dnsPolicy: {{ (or .Values.app.webhook.dnsPolicy .Values.dnsPolicy) }}
{{- with .Values.priorityClassName }}
priorityClassName: "{{ . }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
priorityClassName: "{{ . }}"
priorityClassName: {{ . }}

Nit, I don't think quotes are required here and more consistent with the rest of the template.

wallrj and others added 3 commits March 14, 2024 09:19
Signed-off-by: Richard Wall <richard.wall@venafi.com>
Co-authored-by: Erik Godding Boye <egboye@gmail.com>
Signed-off-by: Richard Wall <richard.wall@venafi.com>
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj force-pushed the add-priorityclassname-to-helm-chart branch from 8755dce to d8843ba Compare March 14, 2024 09:20
@wallrj wallrj requested a review from erikgb March 14, 2024 09:21
@erikgb
Copy link
Contributor

erikgb commented Mar 14, 2024

/lgtm

Thanks, Richard!

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2024
@wallrj
Copy link
Member Author

wallrj commented Mar 14, 2024

/approve

@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2024
@jetstack-bot jetstack-bot merged commit 251a436 into cert-manager:main Mar 14, 2024
5 checks passed
@wallrj wallrj deleted the add-priorityclassname-to-helm-chart branch March 14, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants