Skip to content

Commit 7d3bf22

Browse files
committed
fix(recipe): carry the selection through copy, query, and the API schema
Four more review findings, all reproduced before fixing. Each is the same defect in a different place: the recipe acts on a selection it no longer records. RecipeResult.DeepCopy allocated a fresh RecipeConfiguration and cloned only Slurm, dropping RuntimeInventory rather than aliasing it. Client.AdoptRecipe always deep-copies, so an adopted recipe kept the install: false override while losing the configuration explaining it. Every pointer under RecipeConfiguration now has a clause, and the test asserts a real copy rather than a shared pointer. Query hydration projected only Configuration.Slurm, so `aicr query --selector configuration.runtimeInventory.mode` returned NOT_FOUND and hydrated output omitted the decision the recipe records. Verified through the built CLI: the selector now returns the mode. The OpenAPI ConfiguredRecipeConfiguration schema declared `additionalProperties: false` with `required: [slurm]`, so a conforming client could not submit a generated runtime-inventory recipe to POST /v2/bundle -- and a recipe carrying only that selection failed the required check as well. Added the shape to both the strict schema and the permissive base, and replaced `required: [slurm]` with `minProperties: 1`, since either section may now appear alone. The PR body said `Fixes: #2271`, which would have closed the epic on merge. #2310 and #2311 are open and no stock overlay is added here, so it is now `Part of #2271` with the remaining work named. Part of #2271 Signed-off-by: Mark Chmarny <mark@chmarny.com>
1 parent 02130c4 commit 7d3bf22

4 files changed

Lines changed: 98 additions & 1 deletion

File tree

api/aicr/v1/server.yaml

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2622,6 +2622,13 @@ components:
26222622
mode:
26232623
type: string
26242624
enum: [disabled, customer-managed, aicr-provided]
2625+
runtimeInventory:
2626+
type: object
2627+
required: [mode]
2628+
properties:
2629+
mode:
2630+
type: string
2631+
enum: [enabled, disabled]
26252632
componentRefs:
26262633
type: array
26272634
items:
@@ -2812,7 +2819,10 @@ components:
28122819
allOf:
28132820
- $ref: "#/components/schemas/RecipeResponseBase/properties/configuration"
28142821
- type: object
2815-
required: [slurm]
2822+
# At least one section, but not any particular one: a recipe may
2823+
# record a runtime-inventory selection without Slurm accounting, or
2824+
# the reverse. Requiring `slurm` rejected the former outright.
2825+
minProperties: 1
28162826
additionalProperties: false
28172827
properties:
28182828
slurm:
@@ -2828,6 +2838,14 @@ components:
28282838
mode:
28292839
type: string
28302840
enum: [disabled, customer-managed, aicr-provided]
2841+
runtimeInventory:
2842+
type: object
2843+
required: [mode]
2844+
additionalProperties: false
2845+
properties:
2846+
mode:
2847+
type: string
2848+
enum: [enabled, disabled]
28312849

28322850
ProfileRecipeResponse:
28332851
allOf:

pkg/recipe/metadata.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1181,6 +1181,14 @@ func (r *RecipeResult) DeepCopy() *RecipeResult {
11811181
out.Configuration.Slurm.Accounting = &accounting
11821182
}
11831183
}
1184+
// Every pointer under RecipeConfiguration needs a clause here.
1185+
// Omitting one does not alias it, it drops it: the copy keeps the
1186+
// component overrides a selection applied while losing the record
1187+
// explaining them, and Client.AdoptRecipe always deep-copies.
1188+
if r.Configuration.RuntimeInventory != nil {
1189+
runtimeInventory := *r.Configuration.RuntimeInventory
1190+
out.Configuration.RuntimeInventory = &runtimeInventory
1191+
}
11841192
}
11851193

11861194
if r.Constraints != nil {

pkg/recipe/query.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,14 @@ func HydrateResultWithContext(ctx context.Context, result *RecipeResult) (map[st
102102
}
103103
configuration["slurm"] = slurm
104104
}
105+
// Every section of RecipeConfiguration must be projected here or the
106+
// decision is invisible to `aicr query --selector` and absent from
107+
// hydrated output, even though the recipe records it.
108+
if result.Configuration.RuntimeInventory != nil {
109+
configuration["runtimeInventory"] = map[string]any{
110+
"mode": string(result.Configuration.RuntimeInventory.Mode),
111+
}
112+
}
105113
hydrated["configuration"] = configuration
106114
}
107115

pkg/recipe/runtimeinventory_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,3 +312,66 @@ func TestApplyRuntimeInventoryRejectsIncoherentEnable(t *testing.T) {
312312
t.Errorf("error = %v, want a coherence rejection", err)
313313
}
314314
}
315+
316+
// TestDeepCopyPreservesRuntimeInventory covers a defect found in review:
317+
// RecipeResult.DeepCopy allocated a fresh RecipeConfiguration and cloned only
318+
// Slurm, so the runtime inventory record was dropped rather than aliased.
319+
//
320+
// Client.AdoptRecipe always deep-copies, so an adopted recipe kept the
321+
// component override the selection applied while losing the configuration that
322+
// explains it — a recipe acting on a decision it no longer records.
323+
func TestDeepCopyPreservesRuntimeInventory(t *testing.T) {
324+
t.Parallel()
325+
326+
result := runtimeInventoryTestResult()
327+
mode := RuntimeInventoryDisabled
328+
if err := applyBuildConfig(result, &buildConfig{runtimeInventoryMode: &mode}); err != nil {
329+
t.Fatalf("applyBuildConfig() error = %v", err)
330+
}
331+
332+
clone := result.DeepCopy()
333+
got, present := clone.RuntimeInventoryMode()
334+
if !present {
335+
t.Fatal("DeepCopy dropped configuration.runtimeInventory")
336+
}
337+
if got != RuntimeInventoryDisabled {
338+
t.Errorf("cloned mode = %q, want %q", got, RuntimeInventoryDisabled)
339+
}
340+
341+
// A copy, not an alias: mutating the clone must not reach the original.
342+
clone.Configuration.RuntimeInventory.Mode = RuntimeInventoryEnabled
343+
if orig, _ := result.RuntimeInventoryMode(); orig != RuntimeInventoryDisabled {
344+
t.Errorf("original mode = %q after mutating the clone; the pointer is shared", orig)
345+
}
346+
347+
// The sibling section must survive the same copy.
348+
if clone.Configuration.Slurm != nil && result.Configuration.Slurm == nil {
349+
t.Error("clone invented a Slurm section")
350+
}
351+
}
352+
353+
// TestQueryHydrationExposesRuntimeInventory covers the second half of the same
354+
// class: the recipe recorded the selection but `aicr query --selector
355+
// configuration.runtimeInventory.mode` returned NOT_FOUND, because hydration
356+
// projected only Configuration.Slurm.
357+
func TestQueryHydrationExposesRuntimeInventory(t *testing.T) {
358+
t.Parallel()
359+
360+
result := runtimeInventoryTestResult()
361+
mode := RuntimeInventoryDisabled
362+
if err := applyBuildConfig(result, &buildConfig{runtimeInventoryMode: &mode}); err != nil {
363+
t.Fatalf("applyBuildConfig() error = %v", err)
364+
}
365+
366+
hydrated, err := HydrateResult(result)
367+
if err != nil {
368+
t.Fatalf("Hydrate() error = %v", err)
369+
}
370+
got, err := Select(hydrated, "configuration.runtimeInventory.mode")
371+
if err != nil {
372+
t.Fatalf("Select(configuration.runtimeInventory.mode) error = %v", err)
373+
}
374+
if got != string(RuntimeInventoryDisabled) {
375+
t.Errorf("selector returned %v, want %q", got, RuntimeInventoryDisabled)
376+
}
377+
}

0 commit comments

Comments
 (0)