Skip to content

feat: use consistent labels - #64

Open
henrichter-sap wants to merge 1 commit into
cobaltcore-dev:mainfrom
henrichter-sap:feat/labels
Open

feat: use consistent labels#64
henrichter-sap wants to merge 1 commit into
cobaltcore-dev:mainfrom
henrichter-sap:feat/labels

Conversation

@henrichter-sap

@henrichter-sap henrichter-sap commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Refactor
    • Standardized Kubernetes resource labels across the operator, model, and EPP components.
    • Improved consistency between deployments, services, and related resource selectors.
    • Added component-specific labeling for clearer resource identification and matching.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Helm label centralization

Layer / File(s) Summary
Define shared and component selectors
helm/thalamus/templates/_helpers.tpl
Adds shared chart metadata labels and model/EPP selector helpers, while narrowing operator selectors to operator-specific labels.
Update model and EPP selectors
helm/thalamus/templates/models.yaml, helm/thalamus/templates/epp.yaml
Deployments, Services, and InferencePool selectors now use component-specific helpers and shared pod labels.
Update operator resource labels
helm/thalamus/templates/operator.yaml
Operator resources and pods combine operator selector labels with shared chart labels.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: varsius

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: standardizing Helm/Kubernetes labels across templates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@henrichter-sap
henrichter-sap marked this pull request as ready for review July 13, 2026 07:58
@henrichter-sap
henrichter-sap requested a review from a team as a code owner July 13, 2026 07:58

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@helm/thalamus/templates/_helpers.tpl`:
- Around line 16-19: Deployment selectors are immutable and the chart currently
changes them across upgrades. Add a pre-upgrade lifecycle hook that deletes the
affected operator, model, and EPP Deployments before recreation; update
helm/thalamus/templates/_helpers.tpl lines 16-19,
helm/thalamus/templates/models.yaml lines 21-23, and
helm/thalamus/templates/epp.yaml lines 107-109 as needed so the hook targets the
existing Deployments and the new selectors remain consistent.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3ed921da-8607-4f57-9c01-35cd0b94fbc4

📥 Commits

Reviewing files that changed from the base of the PR and between 84b2d59 and 04750e6.

📒 Files selected for processing (4)
  • helm/thalamus/templates/_helpers.tpl
  • helm/thalamus/templates/epp.yaml
  • helm/thalamus/templates/models.yaml
  • helm/thalamus/templates/operator.yaml

Comment on lines 16 to 19
{{- define "thalamus.operator.selectorLabels" -}}
app.kubernetes.io/name: {{ include "thalamus.operator.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/component: operator
{{- end }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Deployment selector changes are breaking on upgrade — selectors are immutable.

All three Deployment selectors changed their label keys, which Kubernetes rejects on helm upgrade for existing installations. The root cause is that selector label helpers now emit different keys than the prior hardcoded values.

  • helm/thalamus/templates/_helpers.tpl#L16-L19: thalamus.operator.selectorLabels removed app.kubernetes.io/instance — the operator Deployment selector at operator.yaml line 65 now has fewer keys than before.
  • helm/thalamus/templates/models.yaml#L21-L23: model Deployment matchLabels switched from hardcoded app/llm-d.ai/inference-serving labels to app.kubernetes.io/name + app.kubernetes.io/component.
  • helm/thalamus/templates/epp.yaml#L107-L109: EPP Deployment matchLabels switched from hardcoded inferencepool label to app.kubernetes.io/name + app.kubernetes.io/component.

Migration options:

  1. Fresh install only — document that this version requires helm uninstall + helm install (or manual deletion of affected Deployments).
  2. Lifecycle hook — add a pre-upgrade hook that deletes the old Deployments before the upgrade creates new ones with the new selectors.
  3. Keep old selector keys — if upgrade compatibility is required, preserve the old label keys in the selector helpers alongside the new ones (note: you cannot add keys to an existing selector either, so this only works if the old keys were a subset of the new ones, which they are not here).

Option 1 is simplest for a chart that likely hasn't been widely deployed yet. Option 2 is the cleanest automated approach.

📍 Affects 3 files
  • helm/thalamus/templates/_helpers.tpl#L16-L19 (this comment)
  • helm/thalamus/templates/models.yaml#L21-L23
  • helm/thalamus/templates/epp.yaml#L107-L109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/thalamus/templates/_helpers.tpl` around lines 16 - 19, Deployment
selectors are immutable and the chart currently changes them across upgrades.
Add a pre-upgrade lifecycle hook that deletes the affected operator, model, and
EPP Deployments before recreation; update helm/thalamus/templates/_helpers.tpl
lines 16-19, helm/thalamus/templates/models.yaml lines 21-23, and
helm/thalamus/templates/epp.yaml lines 107-109 as needed so the hook targets the
existing Deployments and the new selectors remain consistent.

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.

1 participant