-
Notifications
You must be signed in to change notification settings - Fork 93
fix(bundler): recognize OCP component aliases in DRA rollout, annotation, and remedy paths #2346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5f41dd0
ea746e9
423fd3d
550b08d
3783583
bc34d14
762c666
ed97605
a5fac94
c879000
6c3ab8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,7 @@ The source of truth is [`recipes/registry.yaml`](https://github.com/NVIDIA/aicr/ | |
| | **cert-manager-ocp-olm** | OLM installer for cert-manager on OpenShift. Creates the OperatorGroup and Subscription resources that install the certified cert-manager Operator via the Operator Lifecycle Manager. Paired with `cert-manager-ocp`. OCP-specific. | [cert-manager (Certified)](https://catalog.redhat.com/software/container-stacks/detail/5ec3f5a5eebc3d6acb0ee71c) | | ||
| | **cert-manager-ocp** | cert-manager CertManager CR for OpenShift. The operand Deployments (controller, cainjector, webhook) land in a hardcoded `cert-manager` namespace regardless of the operator's own namespace. Deployed after `cert-manager-ocp-olm`. OCP-specific. | [cert-manager](https://github.com/cert-manager/cert-manager) | | ||
| | **prometheus-adapter-ocp** | Prometheus Adapter for OpenShift. Reuses the same upstream chart as `prometheus-adapter`, pointed at OCP's built-in Thanos Querier instead of kube-prometheus-stack (which stays disabled on OCP). No certified OCP operator exists for this component. OCP-specific. | [prometheus-adapter](https://github.com/kubernetes-sigs/prometheus-adapter) | | ||
| | **nvidia-dra-driver-gpu-ocp** | NVIDIA DRA GPU driver for OpenShift. Reuses the same upstream chart as `nvidia-dra-driver-gpu`, with an added SCC RoleBinding granting the kubelet-plugin DaemonSet the host device access OCP's default restricted-v2 SCC forbids. No certified OCP operator exists for this component. OCP-specific. Known limitation: some GPU-driver rollout protections and remedy hints do not yet cover the OCP aliases (`gpu-operator-ocp`, `nvidia-dra-driver-gpu-ocp`) — the deployer's stale-NVML migration wait/restart, driver-version annotation injection, and the driver-absent remedy's `gpuoperator:`/`dradriver:` override keys; tracked in [#2136](https://github.com/NVIDIA/aicr/issues/2136). | [NVIDIA DRA Driver](https://github.com/kubernetes-sigs/dra-driver-nvidia-gpu) | | ||
| | **nvidia-dra-driver-gpu-ocp** | NVIDIA DRA GPU driver for OpenShift. Reuses the same upstream chart as `nvidia-dra-driver-gpu`, with an added SCC RoleBinding granting the kubelet-plugin DaemonSet the host device access OCP's default restricted-v2 SCC forbids. No certified OCP operator exists for this component. OCP-specific. Known limitation: the driver-version annotation injected onto the DRA pod templates falls back to the `gpu-operator-ocp-olm` Subscription channel, which changes on a channel re-pin but not on every in-channel OLM auto-upgrade — so the stale-NVML rollout gate (#973) can still miss an in-channel driver bump on OCP; tracked in [#2135](https://github.com/NVIDIA/aicr/issues/2135). | [NVIDIA DRA Driver](https://github.com/kubernetes-sigs/dra-driver-nvidia-gpu) | | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor — "Fixes: #2135" closes the issue the docs + comment still cite as the OPEN tracker Not addressed since the prior review. This catalog row and the mirroring comment at Blast radius: User-facing catalog page + a durable code comment. An OCP operator hitting stale NVML after an in-channel auto-upgrade follows the link, finds a closed issue, and concludes it's a resolved bug rather than a documented standing limitation. Fix: File a narrower follow-up issue for the in-channel-auto-upgrade gap and repoint both this row and the bundler.go:2954 comment at it; or drop |
||
| | **k8s-nim-operator-ocp** | NVIDIA NIM Operator for OpenShift. Reuses the same upstream chart as `k8s-nim-operator`, with OCP-specific RBAC. Requires `cert-manager-ocp` for admission-webhook TLS. OCP-specific. | [K8s NIM Operator](https://github.com/NVIDIA/k8s-nim-operator) | | ||
|
|
||
| ## How Components Are Selected | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,15 @@ type ComponentData struct { | |
| IsOCI bool | ||
| Tag string // Git ref for Kustomize-typed components (tag/branch/commit) | ||
| Path string // Path within the repository to the kustomization | ||
|
|
||
| // DriverOperatorManaged is true when the bundle's effective values | ||
| // select an operator-managed NVIDIA driver — gpu-operator's or | ||
| // gpu-operator-ocp's driver.enabled is true. deploy.sh's DRA | ||
| // migration-wait block (see #2135, #973) uses this to tell "driver | ||
| // is host-managed" apart from "driver is operator-managed but the | ||
| // DaemonSet/node-label migration signal isn't observable yet", | ||
| // which live cluster state alone cannot distinguish. | ||
| DriverOperatorManaged bool | ||
| } | ||
|
|
||
| // compile-time interface check | ||
|
|
@@ -265,6 +274,42 @@ func (g *Generator) Generate(ctx context.Context, outputDir string) (*deployer.O | |
|
|
||
| // buildComponentDataList builds a sorted list of ComponentData from the recipe. | ||
| // It validates that all component names are safe for use as directory names. | ||
| // driverOperatorManaged reports whether this bundle's effective values | ||
| // select an operator-managed NVIDIA driver: gpu-operator's or | ||
| // gpu-operator-ocp's driver.enabled is true. Checks both component names | ||
| // since only one is ever enabled in a given recipe (see | ||
| // pkg/bundler/bundler.go's gpuOperatorComponentNames for the canonical | ||
| // list this mirrors). | ||
| // gpuOperatorComponentName and gpuOperatorOCPComponentName are this | ||
| // package's copy of the canonical/OCP gpu-operator component names (a | ||
| // 4th duplicate alongside pkg/bundler/bundler.go, pkg/bundler/validations | ||
| // /checks.go, and their override-key constants — this package cannot | ||
| // import pkg/bundler due to the dependency cycle noted at | ||
| // componentOverrideKeys' godoc equivalent). Named here, rather than an | ||
| // inline literal, so a `grep gpuOperatorOCPComponentName` across the repo | ||
| // surfaces every copy that needs updating together. | ||
| const ( | ||
| gpuOperatorComponentName = "gpu-operator" | ||
| gpuOperatorOCPComponentName = "gpu-operator-ocp" | ||
| ) | ||
|
|
||
| func (g *Generator) driverOperatorManaged() bool { | ||
| for _, name := range []string{gpuOperatorComponentName, gpuOperatorOCPComponentName} { | ||
| values, ok := g.ComponentValues[name] | ||
| if !ok { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Nitpick — driverOperatorManaged() gpu-operator-ocp branch untested The Blast radius: OCP operator-managed driver bundles — the DRA restart gate would not engage. Fix: Add a table case with |
||
| continue | ||
| } | ||
| driver, ok := values["driver"].(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if enabled, ok := driver["enabled"].(bool); ok && enabled { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // Only the fields consumed by the orchestration templates are populated. | ||
| func (g *Generator) buildComponentDataList() ([]ComponentData, error) { | ||
| // Sort by deployment order | ||
|
|
@@ -273,6 +318,8 @@ func (g *Generator) buildComponentDataList() ([]ComponentData, error) { | |
| g.RecipeResult.DeploymentOrder, | ||
| ) | ||
|
|
||
| driverOperatorManaged := g.driverOperatorManaged() | ||
|
|
||
| components := make([]ComponentData, 0, len(sorted)) | ||
| for _, ref := range sorted { | ||
| if !deployer.IsSafePathComponent(ref.Name) { | ||
|
|
@@ -283,14 +330,15 @@ func (g *Generator) buildComponentDataList() ([]ComponentData, error) { | |
| chartName := ref.EffectiveChart() | ||
|
|
||
| components = append(components, ComponentData{ | ||
| Name: ref.Name, | ||
| Namespace: ref.Namespace, | ||
| Repository: ref.Source, | ||
| ChartName: chartName, | ||
| Version: ref.Version, | ||
| IsOCI: strings.HasPrefix(ref.Source, "oci://"), | ||
| Tag: ref.Tag, | ||
| Path: ref.Path, | ||
| Name: ref.Name, | ||
| Namespace: ref.Namespace, | ||
| Repository: ref.Source, | ||
| ChartName: chartName, | ||
| Version: ref.Version, | ||
| IsOCI: strings.HasPrefix(ref.Source, "oci://"), | ||
| Tag: ref.Tag, | ||
| Path: ref.Path, | ||
| DriverOperatorManaged: driverOperatorManaged, | ||
| }) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Minor — "Fixes: #2135" closes the issue the docs + comment cite as the tracker for the remaining limitation
The PR body says
Fixes: #2135, so merging closes #2135. But this "Known limitation" row (and the mirroring comment atpkg/bundler/bundler.go:2954, "See #2135") both cite #2135 as the tracker for the still-open in-channel OLM auto-upgrade gap — the annotation keys off the Subscription channel string, which stays stable across in-channel CSV bumps underinstallPlanApproval: Automatic. After merge the link resolves to a closed issue, implying the limitation is resolved when it isn't. This is the same defect class mchmarny already got fixed once, when the row cited (now-closed) #2136.Blast radius: User-facing catalog page + a durable code comment. An OCP operator hitting stale NVML after an in-channel auto-upgrade follows the link, finds a closed issue, and concludes it's a resolved bug rather than a documented standing limitation.
Fix: File a narrower follow-up issue for the in-channel-auto-upgrade gap and repoint both the catalog row and the bundler.go:2954 comment at it; or drop
Fixes: #2135from the PR body so #2135 stays open as the tracker. This is the one item worth doing before merge.