🐛 Fix Helm chart: make ClusterRole names unique per namespace - #716
🐛 Fix Helm chart: make ClusterRole names unique per namespace#716clubanderson wants to merge 3 commits into
Conversation
When multiple WVA installations exist on the same cluster in different namespaces (e.g., llm-d-nightly-wva, llm-d-vezio), the chart currently produces identical ClusterRole/ClusterRoleBinding names. This causes Helm ownership conflicts because each installation's annotations claim the same cluster-scoped resources. Add a `clusterResourceName` helper that appends `.Release.Namespace` to the fullname, ensuring each installation's cluster-scoped resources have unique names. Update all 6 ClusterRoles, 6 ClusterRoleBindings, and their cross-references to use the new helper. Example: `workload-variant-autoscaler-manager-role` becomes `workload-variant-autoscaler-llm-d-nightly-wva-manager-role` Namespace-scoped resources (Role, RoleBinding) are unchanged. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Allows nightly dispatch from feature branches to test chart changes before merging to main. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Andrew Anderson <andy@clubanderson.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a Helm chart issue where multiple installations of the Workload Variant Autoscaler (WVA) on the same Kubernetes cluster fail due to ClusterRole/ClusterRoleBinding name collisions. The solution introduces a new clusterResourceName helper that appends the release namespace to resource names, ensuring uniqueness across different namespace installations.
Changes:
- Adds
clusterResourceNamehelper function in_helpers.tplthat appends.Release.Namespaceto the fullname - Updates all 6 ClusterRoles to use namespace-qualified names
- Updates all 6 ClusterRoleBindings to use namespace-qualified names with matching roleRef references
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| charts/workload-variant-autoscaler/templates/_helpers.tpl | Adds new clusterResourceName helper function for namespace-qualified cluster resource naming |
| charts/workload-variant-autoscaler/templates/rbac/role.yaml | Updates manager-role ClusterRole to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/rbac/role_binding.yaml | Updates manager-rolebinding ClusterRoleBinding and its roleRef to use namespace-qualified names |
| charts/workload-variant-autoscaler/templates/rbac/metrics_auth_role.yaml | Updates metrics-auth-role ClusterRole to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/rbac/metrics_auth_role_binding.yaml | Updates metrics-auth-rolebinding ClusterRoleBinding and its roleRef to use namespace-qualified names |
| charts/workload-variant-autoscaler/templates/rbac/metrics_reader_role.yaml | Updates metrics-reader ClusterRole to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/rbac/metrics_reader_role_binding.yaml | Updates metrics-reader-rolebinding ClusterRoleBinding and its roleRef to use namespace-qualified names |
| charts/workload-variant-autoscaler/templates/rbac/prometheus_metrics_auth_role_binding.yaml | Updates prometheus-metrics-auth-rolebinding ClusterRoleBinding and its roleRef to use namespace-qualified names |
| charts/workload-variant-autoscaler/templates/rbac/variantautoscaling_admin_role.yaml | Updates variantautoscaling-admin-role ClusterRole to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/rbac/variantautoscaling_editor_role.yaml | Updates variantautoscaling-editor-role ClusterRole to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/rbac/variantautoscaling_viewer_role.yaml | Updates variantautoscaling-viewer-role ClusterRole to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/manager/wva-clusterrolebinding.yaml | Updates monitoring ClusterRoleBinding to use namespace-qualified name |
| charts/workload-variant-autoscaler/templates/manager/prometheus-clusterrolebinding.yaml | Updates prometheus-adapter-monitoring ClusterRoleBinding to use namespace-qualified name |
| Appends the release namespace to the fullname to ensure uniqueness when multiple | ||
| installations exist on the same cluster in different namespaces. | ||
| */}} | ||
| {{- define "workload-variant-autoscaler.clusterResourceName" -}} | ||
| {{- printf "%s-%s" (include "workload-variant-autoscaler.fullname" .) .Release.Namespace | trunc 63 | trimSuffix "-" }} |
There was a problem hiding this comment.
The clusterResourceName helper appends the namespace to the fullname and then truncates to 63 characters. With long release names and long namespaces, this could result in collisions.
For example, if two namespaces differ only in their last characters (e.g., llm-d-inference-scheduler-pr-12345 and llm-d-inference-scheduler-pr-99999), and the combined fullname + namespace exceeds 63 characters, both could truncate to the same value, defeating the purpose of this fix.
Consider one of these approaches:
- Use a hash suffix of the namespace instead of appending the full namespace (e.g., printf "%s-%s" (include "workload-variant-autoscaler.fullname" .) (.Release.Namespace | sha256sum | trunc 8))
- Truncate the fullname first to leave room for the namespace (e.g., printf "%s-%s" ((include "workload-variant-autoscaler.fullname" .) | trunc 40) .Release.Namespace | trunc 63)
- Document the namespace length limitation in the chart README
Option 1 (hash suffix) would be the most robust as it guarantees uniqueness while staying within the 63-character limit.
| Appends the release namespace to the fullname to ensure uniqueness when multiple | |
| installations exist on the same cluster in different namespaces. | |
| */}} | |
| {{- define "workload-variant-autoscaler.clusterResourceName" -}} | |
| {{- printf "%s-%s" (include "workload-variant-autoscaler.fullname" .) .Release.Namespace | trunc 63 | trimSuffix "-" }} | |
| Appends a hash derived from the release namespace to the fullname to ensure uniqueness | |
| when multiple installations exist on the same cluster in different namespaces. | |
| */}} | |
| {{- define "workload-variant-autoscaler.clusterResourceName" -}} | |
| {{- $fullname := include "workload-variant-autoscaler.fullname" . | trunc 54 | trimSuffix "-" -}} | |
| {{- printf "%s-%s" $fullname (.Release.Namespace | sha256sum | trunc 8) | trunc 63 | trimSuffix "-" }} |
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
When DEPLOY_WVA is true, install.sh deploys WVA separately using the local chart (which supports testing chart changes). Skip the helmfile's WVA release to avoid deploying from the OCI registry with KIND-specific defaults (monitoringNamespace: llm-d-monitoring) that break on OpenShift. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Andrew Anderson <andy@clubanderson.com>
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
|
@lionelvillard @ev-shindin — This PR fixes a root cause issue blocking nightly E2E builds on OpenShift. Multiple WVA installations on the same cluster produce identical ClusterRole names, causing Helm ownership conflicts. This adds a Also includes a fix to skip the helmfile's OCI-based WVA deployment when Would appreciate a review/merge so we can get nightly builds passing. Thanks! |
|
Consolidated into #719 |
- Fix CI E2E: clean up orphaned cluster-scoped WVA resources - Fix Helm chart: make ClusterRole names unique per namespace - Fix broken reusable workflow references (@2b273d6 → @main) Consolidates #715, #716, and #718. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Andrew Anderson <andy@clubanderson.com>
- Fix CI E2E: clean up orphaned cluster-scoped WVA resources - Fix Helm chart: make ClusterRole names unique per namespace - Fix broken reusable workflow references (@2b273d6 → @main) Consolidates #715, #716, and #718. Signed-off-by: Andrew Anderson <andy@clubanderson.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…-d#719) - Fix CI E2E: clean up orphaned cluster-scoped WVA resources - Fix Helm chart: make ClusterRole names unique per namespace - Fix broken reusable workflow references (@2b273d6 → @main) Consolidates llm-d#715, llm-d#716, and llm-d#718. Signed-off-by: Andrew Anderson <andy@clubanderson.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Summary
clusterResourceNamehelper to_helpers.tplthat appends.Release.Namespaceto the fullnameProblem
When multiple WVA installations exist on the same cluster (e.g.,
llm-d-nightly-wva,llm-d-vezio), the chart produces identical ClusterRole names likeworkload-variant-autoscaler-manager-role. Helm stamps these withmeta.helm.sh/release-namespaceannotations, and subsequent installations fail with:Fix
New helper
clusterResourceNameproduces names like:workload-variant-autoscaler-llm-d-nightly-wva-manager-roleworkload-variant-autoscaler-llm-d-vezio-manager-roleEach installation gets its own unique set of ClusterRoles/ClusterRoleBindings.
Namespace-scoped resources (Role, RoleBinding, ServiceAccount, etc.) are unchanged since they are already isolated by namespace.
Test plan
helm templaterenders correctly with namespace suffix in ClusterRole names--namespacevalues produce different ClusterRole nameshelm upgrade --force)