Skip to content

Conversation

lukeviens
Copy link
Contributor

Adding two new default (higher) priority classes to the backend and jobs-runners pods, in order to eliminate crash loops during upgrades due to un-ran jobs (namely db migrations).

Currently, this change will only be applied when the global “priorityClassName” is not set. If it is, these default values will be overridden. In the future, it might be useful to create per-pod PriorityClass overrides in order to enable similar behavior, while still allowing a global priorityClassName.

@lukeviens lukeviens requested a review from Copilot July 14, 2025 20:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces higher default priorities for Retool jobs-runner and backend pods when no global priorityClassName is specified, aiming to prevent upgrade crash loops by ensuring critical jobs (like database migrations) complete.

  • Adds two new PriorityClass resources (jobs-runner and backend) in priorityclass.yaml
  • Updates deployment_jobs.yaml and deployment_backend.yaml to conditionally use the new defaults or the global override
  • Bumps Helm chart version from 6.6.0 to 6.6.1

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
charts/retool/templates/priorityclass.yaml New PriorityClass definitions for jobs-runner and backend pods
charts/retool/templates/deployment_jobs.yaml Conditional templating for jobs-runner priorityClassName
charts/retool/templates/deployment_backend.yaml Conditional templating for backend priorityClassName
charts/retool/Chart.yaml Helm chart version bump to 6.6.1
Comments suppressed due to low confidence (3)

charts/retool/templates/deployment_jobs.yaml:49

  • The priorityClassName templating logic is duplicated across multiple deployment templates. Consider extracting this into a shared helper (e.g., in _helpers.tpl) to reduce duplication and simplify future updates.
      {{- else }}

charts/retool/templates/deployment_backend.yaml:51

  • The same defaulting logic appears here as in deployment_jobs.yaml. Extract this into a helper function to keep the chart DRY and easier to maintain.
      priorityClassName: {{ printf "%s-backend-priority" (include "retool.fullname" .) | quote }}

charts/retool/templates/priorityclass.yaml:1

  • There’s currently no test coverage for rendering these PriorityClass resources when priorityClassName is unset. Consider adding a Helm template unit test to ensure this conditional outputs the expected YAML.
{{- if not .Values.priorityClassName }}

@lukeviens lukeviens requested a review from JatinNanda July 14, 2025 20:54
Copy link
Contributor

@JatinNanda JatinNanda left a comment

Choose a reason for hiding this comment

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

nice

@@ -0,0 +1,23 @@
{{- if not .Values.priorityClassName }}
Copy link
Contributor

Choose a reason for hiding this comment

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

worth a comment explaining why we're intentionally deprioritizing backend here

@lukeviens lukeviens force-pushed the lv/jobs-runner-priority branch from bbd3f36 to 77f003e Compare July 14, 2025 22:19
Copy link
Contributor

@ryanartecona ryanartecona left a comment

Choose a reason for hiding this comment

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

I'd prob add a new field in values to disable this new priorityclass behavior, in case some customer has an env that doesn't support it (conflicts, various internal policies, whatever) or they just don't want it. previously, PriorityClass was strictly opt-in, now we're making it default on (when no priorityClassName specified) or overridable (if priorityClassName is specified) but no way to actually just opt out.

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.

3 participants