Name each serving-stack Helm release after its chart#314
Open
dennis-upbound wants to merge 1 commit into
Open
Conversation
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>
1fac790 to
126a4d8
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.