Skip to content

Commit a4be19a

Browse files
authored
fix(bundler): honor bundlers query param; driver-managed cleanup (#1606)
Signed-off-by: Mark Chmarny <mark@chmarny.com>
1 parent 480b7ff commit a4be19a

12 files changed

Lines changed: 524 additions & 30 deletions

File tree

DEVELOPMENT.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,26 @@ Verify a bundle with `aicr verify <dir>`. Update the trusted root cache with
559559
| Build failures | Run `make tidy` to update dependencies |
560560
| K8s connection fails | Check `~/.kube/config` or `KUBECONFIG` env |
561561

562+
### Redeploying on Driver-Managed Clusters
563+
564+
On clusters where gpu-operator installs the NVIDIA kernel driver
565+
(`driver.enabled: true` — e.g. EKS; GKE COS is host-managed and unaffected),
566+
running `make cleanup` (`tools/cleanup`) can leave the `nvidia_uvm` kernel
567+
module wedged mid-unload on GPU nodes. The symptom appears on the *next*
568+
install: the driver validation init container crashes
569+
(`Init:CrashLoopBackOff`, logs show `failed to create device node
570+
nvidia-uvm`). The cleanup script detects driver-managed mode (via the
571+
`nvidia-driver-daemonset` or `driver.enabled` in the release values) and
572+
prints this guidance; the fix is to reboot the GPU nodes before reinstalling:
573+
574+
```bash
575+
kubectl cordon <gpu-node> # each GPU node
576+
# reboot the instances, e.g. on EKS:
577+
aws ec2 reboot-instances --instance-ids <ids>
578+
# wait for the nodes to report Ready, then
579+
kubectl uncordon <gpu-node>
580+
```
581+
562582
### Debugging Tests
563583

564584
```bash

api/aicr/v1/server.yaml

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -742,8 +742,13 @@ paths:
742742
in: query
743743
required: false
744744
description: >
745-
Comma-delimited list of bundler types to execute.
746-
If not specified, all registered bundlers are executed.
745+
Comma-delimited list of recipe component names to bundle.
746+
If not specified, all enabled components are bundled.
747+
Components not listed are skipped as if disabled (their
748+
dependency edges are treated as satisfied externally).
749+
A name the recipe does not declare, or one that is disabled
750+
(by the recipe or by a "set" parameter enabled=false
751+
override), is rejected with HTTP 400.
747752
schema:
748753
type: string
749754
examples:
@@ -1020,7 +1025,7 @@ paths:
10201025
type: string
10211026
format: binary
10221027
"400":
1023-
description: Invalid request (invalid recipe or bundler type)
1028+
description: Invalid request (invalid recipe or bundlers filter)
10241029
headers:
10251030
X-Request-Id:
10261031
$ref: "#/components/headers/RequestIdResponse"
@@ -1037,15 +1042,19 @@ paths:
10371042
requestId: "550e8400-e29b-41d4-a716-446655440000"
10381043
timestamp: "2025-01-15T10:30:00Z"
10391044
retryable: false
1040-
invalidBundler:
1041-
summary: Invalid bundler type
1045+
unknownBundler:
1046+
summary: Bundlers filter names an undeclared component
10421047
value:
10431048
code: INVALID_REQUEST
1044-
message: "Invalid bundler type"
1045-
details:
1046-
bundler: "invalid-bundler"
1047-
error: "unsupported bundle type: invalid-bundler"
1048-
valid: ["gpu-operator", "network-operator", "nodewright-operator", "nvsentinel", "cert-manager", "nvidia-dra-driver-gpu"]
1049+
message: 'unknown component "invalid-bundler" in bundlers filter; recipe declares: gpu-operator, network-operator, cert-manager'
1050+
requestId: "550e8400-e29b-41d4-a716-446655440000"
1051+
timestamp: "2025-01-15T10:30:00Z"
1052+
retryable: false
1053+
disabledBundler:
1054+
summary: Bundlers filter names a disabled component
1055+
value:
1056+
code: INVALID_REQUEST
1057+
message: 'component "cert-manager" is disabled and cannot be selected via the bundlers filter'
10491058
requestId: "550e8400-e29b-41d4-a716-446655440000"
10501059
timestamp: "2025-01-15T10:30:00Z"
10511060
retryable: false
@@ -1554,8 +1563,10 @@ components:
15541563
type: array
15551564
items:
15561565
type: string
1557-
enum: [gpu-operator, network-operator, nodewright-operator, nvsentinel, cert-manager, nvidia-dra-driver-gpu]
15581566
description: >
1559-
Optional list of bundler types to execute.
1560-
If not specified, all registered bundlers are executed.
1567+
Optional list of recipe component names to bundle.
1568+
If not specified, all enabled components are bundled.
1569+
Superseded by the "bundlers" query parameter, which accepts
1570+
enabled component names the recipe declares; unknown or
1571+
disabled names are rejected with HTTP 400.
15611572
example: [gpu-operator, network-operator]

docs/integrator/index.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,10 @@ curl "http://aicrd.aicr.svc/v1/recipe?service=eks&accelerator=h100"
5454
- name: Generate bundles
5555
# recipe.json is the fully-hydrated RecipeResult from the GET above; the
5656
# bundle endpoint adopts it as-is and bundles all of its componentRefs.
57-
# (The `bundlers` query param is not currently honored — see
58-
# https://github.com/NVIDIA/aicr/issues/1531. There is no supported way to
59-
# bundle a subset via the API today; hand-trimming componentRefs drops
60-
# required dependencies, so bundle the full result.)
57+
# To bundle a subset, add the `bundlers` query parameter (comma-delimited
58+
# component names, e.g. "?bundlers=gpu-operator,network-operator") rather
59+
# than hand-trimming componentRefs, which drops required dependencies.
60+
# Unknown or disabled component names are rejected with HTTP 400.
6161
run: |
6262
curl -X POST "http://aicrd.aicr.svc/v1/bundle" \
6363
-H "Content-Type: application/json" \

docs/user/api-reference.md

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ Generate deployment bundles from a recipe.
367367

368368
| Parameter | Type | Default | Description |
369369
|-----------|------|---------|-------------|
370-
| `bundlers` | string | (all) | Comma-delimited list of bundler types to execute. **Not currently honored — all bundlers run regardless ([#1531](https://github.com/NVIDIA/aicr/issues/1531)).** |
370+
| `bundlers` | string | (all) | Comma-delimited list of recipe component names to bundle (e.g. `gpu-operator,network-operator`). Whitespace around names is trimmed. Components not listed are skipped as if disabled (their dependency edges are treated as satisfied externally). A name the recipe does not declare, or one that is disabled (by the recipe or a `set` `enabled=false` override), is rejected with HTTP 400. |
371371
| `set` | string[] | | Value overrides (format: `bundler:path.to.field=value`). Repeat for multiple. |
372372
| `dynamic` | string[] | | Declare value paths as install-time parameters (format: `component:path.to.field`). Repeat for multiple. Supported with `deployer=helm`, `deployer=argocd-helm`, `deployer=flux`, and `deployer=helmfile`. |
373373
| `system-node-selector` | string[] | | Node selectors for system components (format: `key=value`). Repeat for multiple. |
@@ -386,7 +386,7 @@ The request body is the recipe (RecipeResult) directly. No wrapper object needed
386386

387387
#### Components
388388

389-
These are the recipe **components** in [`recipes/registry.yaml`](https://github.com/NVIDIA/aicr/blob/main/recipes/registry.yaml). (The `bundlers` query parameter that would select a subset is currently ignored — all run regardless, #1531.) The registry is the authoritative source — see the [component catalog](component-catalog.md) for the full, current list with detailed descriptions. The table below is illustrative of commonly used components:
389+
These are the recipe **components** in [`recipes/registry.yaml`](https://github.com/NVIDIA/aicr/blob/main/recipes/registry.yaml) — the names the `bundlers` query parameter accepts (a request may only name components the recipe declares). The registry is the authoritative source — see the [component catalog](component-catalog.md) for the full, current list with detailed descriptions. The table below is illustrative of commonly used components:
390390

391391
| Component | Description |
392392
|-----------|-------------|
@@ -436,12 +436,13 @@ These are the recipe **components** in [`recipes/registry.yaml`](https://github.
436436
> a few component fields shown) — use a generated `RecipeResult`, not these
437437
> literals.
438438
>
439-
> The `bundlers` query parameter is **not currently honored** — all bundlers run
440-
> regardless ([#1531](https://github.com/NVIDIA/aicr/issues/1531)). There is no
441-
> supported way to bundle a subset via the API today: hand-trimming
442-
> `componentRefs` is unsafe (it silently drops required dependencies and breaks
443-
> deployers like Helmfile on dangling `dependencyRefs`). Bundle the full
444-
> hydrated result.
439+
> To bundle a subset of the recipe's components, use the `bundlers` query
440+
> parameter (e.g. `?bundlers=gpu-operator,network-operator`) rather than
441+
> hand-trimming `componentRefs` — trimming the body silently drops required
442+
> dependencies and breaks deployers like Helmfile on dangling
443+
> `dependencyRefs`. The filter prunes those edges safely (a filtered-out
444+
> dependency is assumed satisfied externally) and rejects unknown or disabled
445+
> component names with HTTP 400.
445446
446447
```shell
447448
# Basic: pipe recipe to bundle

pkg/bundler/bundler.go

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -798,7 +798,8 @@ func (b *DefaultBundler) getTypedValueOverridesForComponent(componentName string
798798
}
799799

800800
// filterEnabledComponents resolves the set of components to bundle by applying
801-
// recipe-level overrides.enabled and bundle-time --set enabled toggles, then
801+
// recipe-level overrides.enabled, bundle-time --set enabled toggles, and the
802+
// positive bundlers component-name filter (config.WithBundlers, #1531), then
802803
// returns the enabled refs (with dangling dependency edges pruned) alongside
803804
// the deployment order filtered to those refs.
804805
//
@@ -845,6 +846,46 @@ func (b *DefaultBundler) filterEnabledComponents(recipeResult *recipe.RecipeResu
845846
enabledSet[ref.Name] = struct{}{}
846847
}
847848

849+
// Apply the positive component-name filter (POST /v1/bundle ?bundlers=…,
850+
// config.WithBundlers). Requested names must be declared AND enabled —
851+
// an unknown name is a typo the operator needs to hear about, and a
852+
// disabled one mirrors the --set re-enable rejection above (the recipe
853+
// author disabled it because the platform provides it). Enabled
854+
// components outside the requested set are skipped exactly like
855+
// disabled ones, so the dependency-edge pruning below treats them as
856+
// satisfied externally. See #1531.
857+
if b.Config != nil {
858+
if requested := b.Config.Bundlers(); len(requested) > 0 {
859+
requestedSet := make(map[string]struct{}, len(requested))
860+
for _, name := range requested {
861+
if _, declared := declaredSet[name]; !declared {
862+
declaredNames := make([]string, 0, len(recipeResult.ComponentRefs))
863+
for _, ref := range recipeResult.ComponentRefs {
864+
declaredNames = append(declaredNames, ref.Name)
865+
}
866+
return nil, nil, errors.New(errors.ErrCodeInvalidRequest, fmt.Sprintf(
867+
"unknown component %q in bundlers filter; recipe declares: %s",
868+
name, strings.Join(declaredNames, ", ")))
869+
}
870+
if _, enabled := enabledSet[name]; !enabled {
871+
return nil, nil, errors.New(errors.ErrCodeInvalidRequest, fmt.Sprintf(
872+
"component %q is disabled and cannot be selected via the bundlers filter", name))
873+
}
874+
requestedSet[name] = struct{}{}
875+
}
876+
kept := make([]recipe.ComponentRef, 0, len(requestedSet))
877+
for _, ref := range enabledRefs {
878+
if _, ok := requestedSet[ref.Name]; !ok {
879+
slog.Info("skipping component excluded by bundlers filter", "component", ref.Name)
880+
delete(enabledSet, ref.Name)
881+
continue
882+
}
883+
kept = append(kept, ref)
884+
}
885+
enabledRefs = kept
886+
}
887+
}
888+
848889
if len(enabledRefs) == 0 {
849890
return nil, nil, errors.New(errors.ErrCodeInvalidRequest,
850891
"recipe has no enabled components after filtering")

pkg/bundler/bundler_test.go

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,153 @@ func TestMake_UndeclaredDependencyErrors(t *testing.T) {
426426
}
427427
}
428428

429+
// TestMake_BundlersFilter pins the semantics of the `bundlers` positive
430+
// component-name filter (POST /v1/bundle ?bundlers=…, config.WithBundlers):
431+
// a subset selection bundles only the named components, an unknown or
432+
// disabled name fails with ErrCodeInvalidRequest, and an empty filter
433+
// preserves current behavior (all enabled components). See #1531.
434+
func TestMake_BundlersFilter(t *testing.T) {
435+
t.Parallel()
436+
437+
tests := []struct {
438+
name string
439+
bundlers []string
440+
wantDirs []string
441+
wantAbsent []string
442+
wantErr bool
443+
wantErrText string
444+
}{
445+
{
446+
name: "empty filter bundles all enabled components",
447+
bundlers: nil,
448+
wantDirs: []string{"001-gpu-operator", "002-aws-ebs-csi-driver"},
449+
wantAbsent: []string{"cert-manager", "001-cert-manager", "003-cert-manager"},
450+
},
451+
{
452+
name: "filter selects subset",
453+
bundlers: []string{"gpu-operator"},
454+
wantDirs: []string{"001-gpu-operator"},
455+
wantAbsent: []string{
456+
"aws-ebs-csi-driver", "001-aws-ebs-csi-driver", "002-aws-ebs-csi-driver",
457+
},
458+
},
459+
{
460+
name: "unknown name errors",
461+
bundlers: []string{"gpu-operator", "no-such-component"},
462+
wantErr: true,
463+
wantErrText: "unknown component",
464+
},
465+
{
466+
name: "disabled component request errors",
467+
bundlers: []string{"cert-manager"},
468+
wantErr: true,
469+
wantErrText: "disabled",
470+
},
471+
}
472+
473+
for _, tt := range tests {
474+
t.Run(tt.name, func(t *testing.T) {
475+
t.Parallel()
476+
477+
var bundlerOpts []Option
478+
if tt.bundlers != nil {
479+
bundlerOpts = append(bundlerOpts,
480+
WithConfig(config.NewConfig(config.WithBundlers(tt.bundlers))))
481+
}
482+
bundler, err := New(bundlerOpts...)
483+
if err != nil {
484+
t.Fatalf("New() error = %v", err)
485+
}
486+
487+
recipeResult := &recipe.RecipeResult{
488+
APIVersion: "aicr.run/v1alpha2",
489+
Kind: "Recipe",
490+
Criteria: &recipe.Criteria{Service: "eks", Accelerator: "h100", Intent: "training"},
491+
ComponentRefs: []recipe.ComponentRef{
492+
{Name: "gpu-operator", Version: "v25.3.3", Type: "helm", Source: "https://helm.ngc.nvidia.com/nvidia"},
493+
{Name: "aws-ebs-csi-driver", Version: "2.55.0", Type: "helm", Source: "https://kubernetes-sigs.github.io/aws-ebs-csi-driver"},
494+
{Name: "cert-manager", Version: "v1.20.2", Type: "helm", Source: "https://charts.jetstack.io", Overrides: map[string]any{"enabled": false}},
495+
},
496+
DeploymentOrder: []string{"gpu-operator", "aws-ebs-csi-driver"},
497+
}
498+
499+
ctx := context.Background()
500+
tmpDir := t.TempDir()
501+
_, makeErr := bundler.Make(ctx, recipeResult, tmpDir)
502+
if tt.wantErr {
503+
if makeErr == nil {
504+
t.Fatal("Make() expected error, got nil")
505+
}
506+
if !stderrors.Is(makeErr, errors.New(errors.ErrCodeInvalidRequest, "")) {
507+
t.Errorf("Make() error code = %v, want ErrCodeInvalidRequest", makeErr)
508+
}
509+
if !strings.Contains(makeErr.Error(), tt.wantErrText) {
510+
t.Errorf("Make() error = %q, want substring %q", makeErr.Error(), tt.wantErrText)
511+
}
512+
return
513+
}
514+
if makeErr != nil {
515+
t.Fatalf("Make() error = %v", makeErr)
516+
}
517+
for _, dir := range tt.wantDirs {
518+
if _, statErr := os.Stat(filepath.Join(tmpDir, dir)); os.IsNotExist(statErr) {
519+
t.Errorf("expected %s directory to be created", dir)
520+
}
521+
}
522+
for _, dir := range tt.wantAbsent {
523+
if _, statErr := os.Stat(filepath.Join(tmpDir, dir)); !os.IsNotExist(statErr) {
524+
t.Errorf("expected %s directory to NOT be created", dir)
525+
}
526+
}
527+
})
528+
}
529+
}
530+
531+
// TestMake_BundlersFilterDependencyPruned verifies that a dependency edge
532+
// pointing at an enabled-but-filtered-out component is pruned exactly like a
533+
// disabled one: the helmfile deployer (the only path that recomputes ordering
534+
// from dependency edges) must not fail with a false circular-dependency error
535+
// when the depended-upon component is excluded by the bundlers filter. See #1531.
536+
func TestMake_BundlersFilterDependencyPruned(t *testing.T) {
537+
t.Parallel()
538+
539+
cfg := config.NewConfig(
540+
config.WithDeployer(config.DeployerHelmfile),
541+
config.WithBundlers([]string{"gpu-operator"}),
542+
)
543+
bundler, err := New(WithConfig(cfg))
544+
if err != nil {
545+
t.Fatalf("New() error = %v", err)
546+
}
547+
548+
recipeResult := &recipe.RecipeResult{
549+
APIVersion: "aicr.run/v1alpha2",
550+
Kind: "Recipe",
551+
Criteria: &recipe.Criteria{Service: "eks", Accelerator: "h100", Intent: "training"},
552+
ComponentRefs: []recipe.ComponentRef{
553+
// cert-manager is enabled but excluded by the bundlers filter;
554+
// gpu-operator depends on it — assumed satisfied externally.
555+
{Name: "cert-manager", Version: "v1.20.2", Type: "helm", Source: "https://charts.jetstack.io"},
556+
{Name: "gpu-operator", Version: "v25.3.3", Type: "helm", Source: "https://helm.ngc.nvidia.com/nvidia", DependencyRefs: []string{"cert-manager"}},
557+
},
558+
DeploymentOrder: []string{"cert-manager", "gpu-operator"},
559+
}
560+
561+
ctx := context.Background()
562+
tmpDir := t.TempDir()
563+
if _, makeErr := bundler.Make(ctx, recipeResult, tmpDir); makeErr != nil {
564+
t.Fatalf("Make() with filtered-out depended-upon component error = %v", makeErr)
565+
}
566+
if _, statErr := os.Stat(filepath.Join(tmpDir, "001-gpu-operator")); os.IsNotExist(statErr) {
567+
t.Error("expected 001-gpu-operator to be created")
568+
}
569+
for _, dir := range []string{"cert-manager", "001-cert-manager", "002-cert-manager"} {
570+
if _, statErr := os.Stat(filepath.Join(tmpDir, dir)); !os.IsNotExist(statErr) {
571+
t.Errorf("expected %s directory to NOT be created", dir)
572+
}
573+
}
574+
}
575+
429576
func TestMake_SetEnabledOverridesPrecedence(t *testing.T) {
430577
t.Parallel()
431578

0 commit comments

Comments
 (0)