Name each serving-stack Helm release after its chart#314
Merged
dennis-upbound merged 1 commit intoJul 1, 2026
Merged
Conversation
dennis-upbound
force-pushed
the
serving-stack-sa-name-length
branch
from
1fac790 to
126a4d8
Compare
negz
reviewed
Jun 30, 2026
Comment on lines
+145
to
+164
| The release is named "mp-<chart>" via the crossplane.io/external-name | ||
| annotation, which provider-helm uses verbatim as the Helm release name. Left | ||
| unset, the release would inherit the composed resource's generated name | ||
| (<inferencecluster>-<hash>) - long and unpredictable, and the source of a | ||
| whole class of failures: charts derive their resource names from the release | ||
| name, truncate their own fullname to 63 chars, then *append* suffixes like | ||
| "-service-account-kubeletplugin", so the final name can exceed 63. Kubernetes | ||
| label values are capped at 63 chars, so anything that copies such a name into | ||
| a label value rejects it - e.g. Cilium keys a per-pod CiliumIdentity on the | ||
| ServiceAccount name and the pod then never gets a network sandbox. See issue | ||
| #215. | ||
|
|
||
| A fixed "mp-<chart>" name keeps every derived name well under 63 regardless | ||
| of the InferenceCluster name: the longest is the DRA driver's kubelet-plugin | ||
| ServiceAccount, "mp-dra-driver-nvidia-gpu-service-account-kubeletplugin" at | ||
| 54 chars. Each chart runs once per workload cluster in its own namespace, so | ||
| "mp-<chart>" is a unique, stable release name (stable across chart-version | ||
| upgrades, so provider-helm upgrades in place rather than reinstalling). The | ||
| composed resource keeps its own generated metadata.name, unique in the | ||
| control plane. The "mp-" prefix is reserved for releases this function owns.""" |
Collaborator
There was a problem hiding this comment.
Nit: This information is important but the comment feels ~3x as long as it needs to be to communicate it.
There was a problem hiding this comment.
Pull request overview
This PR fixes Helm chart-derived Kubernetes resource names exceeding the 63-character label-value limit (notably breaking NVIDIA DRA driver pods on Cilium clusters) by making provider-helm use short, stable Helm release names that do not depend on the generated composed-resource name.
Changes:
- Set each Helm
Release’scrossplane.io/external-nametomp-<chart>so provider-helm uses a short, predictable Helm release name. - Add test coverage to ensure release names remain within Helm’s 53-character limit and that worst-case ServiceAccount name suffixing stays within 63 characters even with an oversized ServingStack/cluster name.
- Update existing test fixtures to include the new
crossplane.io/external-nameannotation expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| functions/compose-serving-stack/function/fn.py | Forces stable mp-<chart> Helm release names via crossplane.io/external-name to prevent downstream 63-char overflows. |
| functions/compose-serving-stack/tests/test_fn.py | Updates expected Release metadata and adds a regression test that validates name-length constraints independent of XR name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
negz
reviewed
Jun 30, 2026
| ] | ||
| self.assertNotIn("nvidiaDriverRoot", dra_values) | ||
|
|
||
| async def test_release_names_short_regardless_of_inferencecluster_name(self) -> None: |
Collaborator
There was a problem hiding this comment.
Do we need this new test? The cases above cover the behavior and would capture a regression.
negz
approved these changes
Jun 30, 2026
negz
left a comment
negz
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @dennis-upbound! Few nits but looks good overall.
dennis-upbound
force-pushed
the
serving-stack-sa-name-length
branch
from
126a4d8 to
813bc8d
Compare
provider-helm names each release after the composed resource, whose generated name is <inferencecluster>-<hash>. That name flows into every chart's fullname and from there into the chart's resource names. A chart truncates its fullname to 63 chars and then appends suffixes - the NVIDIA DRA driver's kubelet-plugin ServiceAccount is <fullname>-service-account-kubeletplugin - so the final name can exceed 63 even when the fullname did not. Kubernetes label values are capped at 63 chars, so a consumer that copies the name into a label value rejects it: on Cilium clusters the per-pod CiliumIdentity keys on the ServiceAccount name, so the DRA driver's kubelet-plugin pod never gets a network sandbox and no GPU is bound. node-feature-discovery, lws, and the kube-prometheus-stack node-exporter subchart overflow the same way once the name is long enough. _helm_release now sets crossplane.io/external-name to mp-<chart>, which provider-helm uses verbatim as the release name. Naming each release for its chart makes every derived name short and independent of the InferenceCluster name; the longest is the DRA driver's kubelet-plugin ServiceAccount at 54 of 63 chars. The mp- prefix reserves a release-name namespace for releases this function owns, so an mp-cert-manager release can't adopt or collide with a cert-manager release a user already runs in that namespace. The name is stable across chart-version upgrades, so provider-helm upgrades in place. The composed resource keeps its generated metadata.name, so control-plane uniqueness is unchanged. A new test runs the function with an oversized InferenceCluster name and asserts every release is named mp-<chart>, so the release name - and every name a chart derives from it - stays within the 63-char label limit no matter how long the InferenceCluster name is. This changes release identity. On an existing install provider-helm installs fresh releases under the new names and orphans the old <inferencecluster>-<hash> releases, so migrating a running stack needs a teardown and reinstall. Fixes modelplaneai#215. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
dennis-upbound
force-pushed
the
serving-stack-sa-name-length
branch
from
813bc8d to
ec6bb40
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Fixes #215.
provider-helm names each Helm release after the composed resource, whose generated name is
<inferencecluster>-<hash>. That name is long, unpredictable, and length-capped, and it flows into every chart's fullname and from there into the names of the resources the chart creates. A chart truncates its own fullname to 63 chars and then appends suffixes — the NVIDIA DRA driver's kubelet-plugin ServiceAccount is<fullname>-service-account-kubeletplugin— so the final name can exceed 63 even when the fullname did not. Kubernetes label values are capped at 63 chars, so any consumer that copies such a name into a label value rejects it. On Cilium clusters the per-pod CiliumIdentity keys on the ServiceAccount name, so the DRA driver's kubelet-plugin pod never gets a network sandbox, never schedules, and no GPU is ever bound.This isn't specific to the DRA driver. Rendering every chart at a realistic release name shows the DRA driver overflows at essentially any InferenceCluster name, while
node-feature-discoveryandlwsoverflow once the name is a little longer, and the kube-prometheus-stack node-exporter subchart overflows the same way (it ignores the chart'sfullnameOverrideand uses the release name directly). The shared cause is the release name, so the fix belongs there rather than per chart._helm_releasenow setscrossplane.io/external-nametomp-<chart>, which provider-helm uses verbatim as the release name. Each release is named for its chart (mp-dra-driver-nvidia-gpu) instead of the InferenceCluster, so every derived name is short and independent of the InferenceCluster name: the budget below holds for any deployment, not only the ones tested. Themp-prefix reserves a release-name namespace for the releases this function owns; without it a release namedcert-managerwould adopt or collide with acert-managerrelease a user already runs in that namespace, andmp-cert-managercannot.mp-node-feature-discoverymp-dra-driver-nvidia-gpu-service-account-kubeletpluginThe DRA driver is the binding constraint at 54/63: it appends the longest suffix (
-service-account-kubeletplugin, 30 chars) to a release name that already contains the chart name. Every other chart is smaller. The release name is stable across chart-version upgrades, so provider-helm upgrades in place rather than treating each version as a new release, and the composed resource keeps its generated, hashedmetadata.name, so managed-resource uniqueness in the control plane is unchanged.This does change release identity. On an existing install provider-helm installs fresh releases under the new
mp-<chart>names and orphans the old<inferencecluster>-<hash>releases, so migrating a running stack means a teardown and reinstall. That's acceptable pre-1.0, and the Cilium clusters this fixes are non-functional today regardless.I have:
nix flake check(or./nix.sh flake check) and made sure it passes.git commit -s.