From 659c611d3f330486245af4f0a071992ea4ab2b7d Mon Sep 17 00:00:00 2001 From: Patrice Breton Date: Thu, 6 Aug 2026 17:04:55 -0700 Subject: [PATCH 1/2] fix(flow): preserve expected component descriptions Signed-off-by: Patrice Breton --- .../expected_mirror_component.go | 36 +++++++++- .../expected_mirror_component_test.go | 70 ++++++++++++++++++- .../inventorysync/expected_mirror_db_test.go | 59 ++++++++++++++++ 3 files changed, 163 insertions(+), 2 deletions(-) diff --git a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go index b2d1ded5e2..2efc43a106 100644 --- a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go +++ b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go @@ -36,6 +36,7 @@ const ( labelComponentSlotID = "slot_id" labelComponentTrayIdx = "tray_idx" labelComponentHostID = "host_id" + expectedDescriptionKey = "expected_description" ) // expectedComponentSpec is the normalised view of one Core expected_* row. @@ -53,6 +54,7 @@ type expectedComponentSpec struct { SerialNumber string Model string Name string + Description string SlotID int TrayIndex int HostID int @@ -97,6 +99,7 @@ func machineDetailToSpec(d nicoapi.ExpectedMachineDetail) expectedComponentSpec Type: devicetypes.ComponentTypeToString(devicetypes.ComponentTypeCompute), SerialNumber: d.ChassisSerialNumber, Name: d.Name, + Description: d.Description, RackExternalID: d.RackID, BMC: expectedBMCSpec{ MACAddress: d.BMCMACAddress, @@ -112,6 +115,7 @@ func switchDetailToSpec(d nicoapi.ExpectedSwitchDetail) expectedComponentSpec { Type: devicetypes.ComponentTypeToString(devicetypes.ComponentTypeNVSwitch), SerialNumber: d.SwitchSerialNumber, Name: d.Name, + Description: d.Description, RackExternalID: d.RackID, BMC: expectedBMCSpec{ MACAddress: d.BMCMACAddress, @@ -127,6 +131,7 @@ func powerShelfDetailToSpec(d nicoapi.ExpectedPowerShelfDetail) expectedComponen Type: devicetypes.ComponentTypeToString(devicetypes.ComponentTypePowerShelf), SerialNumber: d.ShelfSerialNumber, Name: d.Name, + Description: d.Description, RackExternalID: d.RackID, BMC: expectedBMCSpec{ MACAddress: d.BMCMACAddress, @@ -438,7 +443,7 @@ func mirrorExpectedComponents( p.toUpdate[i].UpdatedAt = now if _, err := tx.NewUpdate(). Model(&p.toUpdate[i]). - Column("name", "model", "slot_id", "tray_index", "host_id", "rack_id", "deleted_at", "updated_at"). + Column("name", "model", "description", "slot_id", "tray_index", "host_id", "rack_id", "deleted_at", "updated_at"). WhereAllWithDeleted(). Where("id = ?", p.toUpdate[i].ID). Exec(ctx); err != nil { @@ -534,6 +539,7 @@ func componentFromSpec(s expectedComponentSpec, rackID uuid.UUID) model.Componen Manufacturer: s.Manufacturer, SerialNumber: s.SerialNumber, Model: s.Model, + Description: componentDescriptionWithExpected(nil, s.Description), SlotID: s.SlotID, TrayIndex: s.TrayIndex, HostID: s.HostID, @@ -541,6 +547,25 @@ func componentFromSpec(s expectedComponentSpec, rackID uuid.UUID) model.Componen } } +// componentDescriptionWithExpected returns a copy with only Core's reserved +// description key changed. An empty expected description removes that key, +// while values owned by runtime sync or operators remain untouched. +func componentDescriptionWithExpected(existing map[string]any, expected string) map[string]any { + description := make(map[string]any, len(existing)+1) + for key, value := range existing { + description[key] = value + } + if expected == "" { + delete(description, expectedDescriptionKey) + } else { + description[expectedDescriptionKey] = expected + } + if len(description) == 0 { + return nil + } + return description +} + // applyComponentChanges copies mirror-managed fields from desired into // existing. Identity (Manufacturer/SerialNumber/Type), runtime (ComponentID, // PowerState, FirmwareVersion), lifecycle (Status, IngestedAt) and audit @@ -551,6 +576,7 @@ func componentFromSpec(s expectedComponentSpec, rackID uuid.UUID) model.Componen func applyComponentChanges(existing, desired *model.Component, spec expectedComponentSpec) { existing.Name = desired.Name existing.Model = desired.Model + existing.Description = componentDescriptionWithExpected(existing.Description, spec.Description) existing.RackID = desired.RackID if !spec.preserveFields["slot_id"] { existing.SlotID = desired.SlotID @@ -579,6 +605,14 @@ func diffComponentFields(existing, desired *model.Component, spec expectedCompon if existing.Model != desired.Model { diffs = append(diffs, fieldChange{"model", existing.Model, desired.Model}) } + existingDescription, hasExpectedDescription := existing.Description[expectedDescriptionKey] + if spec.Description == "" { + if hasExpectedDescription { + diffs = append(diffs, fieldChange{expectedDescriptionKey, fmt.Sprint(existingDescription), ""}) + } + } else if current, ok := existingDescription.(string); !ok || current != spec.Description { + diffs = append(diffs, fieldChange{expectedDescriptionKey, fmt.Sprint(existingDescription), spec.Description}) + } if !spec.preserveFields["slot_id"] && existing.SlotID != desired.SlotID { diffs = append(diffs, fieldChange{"slot_id", strconv.Itoa(existing.SlotID), strconv.Itoa(desired.SlotID)}) } diff --git a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component_test.go b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component_test.go index f45ec46105..d60cde2929 100644 --- a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component_test.go +++ b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component_test.go @@ -85,6 +85,7 @@ func TestMachineDetailToSpec(t *testing.T) { assert.Equal(t, "SN-001", s.SerialNumber) assert.Equal(t, "Foxconn", s.Manufacturer) assert.Equal(t, "MGX-Compute-Gen2", s.Model) + assert.Equal(t, "compute node", s.Description) assert.Equal(t, 5, s.SlotID) assert.Equal(t, 1, s.TrayIndex) assert.Equal(t, 3, s.HostID) @@ -97,20 +98,24 @@ func TestSwitchDetailToSpec_TypeIsNVSwitch(t *testing.T) { s := switchDetailToSpec(nicoapi.ExpectedSwitchDetail{ SwitchSerialNumber: "SW-1", BMCMACAddress: "00:00:00:00:00:01", + Description: "fabric switch", Labels: map[string]string{labelComponentManufacturer: "NVIDIA"}, }) assert.Equal(t, devicetypes.ComponentTypeToString(devicetypes.ComponentTypeNVSwitch), s.Type) assert.Equal(t, "SW-1", s.SerialNumber) + assert.Equal(t, "fabric switch", s.Description) } func TestPowerShelfDetailToSpec_TypeIsPowerShelf(t *testing.T) { s := powerShelfDetailToSpec(nicoapi.ExpectedPowerShelfDetail{ ShelfSerialNumber: "PS-1", BMCMACAddress: "00:00:00:00:00:02", + Description: "power shelf", Labels: map[string]string{labelComponentManufacturer: "NVIDIA"}, }) assert.Equal(t, devicetypes.ComponentTypeToString(devicetypes.ComponentTypePowerShelf), s.Type) assert.Equal(t, "PS-1", s.SerialNumber) + assert.Equal(t, "power shelf", s.Description) } func TestSpecValid(t *testing.T) { @@ -162,6 +167,7 @@ func TestComponentFromSpec(t *testing.T) { SerialNumber: "SN-1", Model: "MGX", Name: "node-1", + Description: "compute node", SlotID: 5, TrayIndex: 1, HostID: 3, @@ -172,6 +178,7 @@ func TestComponentFromSpec(t *testing.T) { assert.Equal(t, "Foxconn", c.Manufacturer) assert.Equal(t, "SN-1", c.SerialNumber) assert.Equal(t, "MGX", c.Model) + assert.Equal(t, map[string]any{expectedDescriptionKey: "compute node"}, c.Description) assert.Empty(t, c.FirmwareVersion, "firmware_version is owned by runtime sync, mirror must leave it unset") assert.Equal(t, 5, c.SlotID) assert.Equal(t, 1, c.TrayIndex) @@ -209,6 +216,45 @@ func TestDiffComponentFields(t *testing.T) { assert.Empty(t, diffComponentFields(base(), desired, expectedComponentSpec{})) }) + for _, tc := range []struct { + name string + existing map[string]any + expected string + wantChanged bool + }{ + { + name: "new expected description is detected", + expected: "new description", + wantChanged: true, + }, + { + name: "unchanged expected description produces no diff", + existing: map[string]any{expectedDescriptionKey: "same", "operator": "keep"}, + expected: "same", + }, + { + name: "cleared expected description is detected", + existing: map[string]any{expectedDescriptionKey: "old", "operator": "keep"}, + wantChanged: true, + }, + { + name: "unrelated description entries do not produce a diff", + existing: map[string]any{"operator": "keep"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + existing := base() + existing.Description = tc.existing + diffs := diffComponentFields(existing, base(), expectedComponentSpec{Description: tc.expected}) + if tc.wantChanged { + require.Len(t, diffs, 1) + assert.Equal(t, expectedDescriptionKey, diffs[0].field) + } else { + assert.Empty(t, diffs) + } + }) + } + for name, mutate := range map[string]func(*model.Component){ "name": func(c *model.Component) { c.Name = "n2" }, "model": func(c *model.Component) { c.Model = "m2" }, @@ -253,6 +299,10 @@ func TestApplyComponentChanges_DoesNotTouchIdentityOrRuntimeFields(t *testing.T) Model: "old-model", RackID: rackA, ComponentID: &extID, // runtime-owned, must not be touched + Description: map[string]any{ + "nvos_ip": "10.0.0.2", + "operator": "keep", + }, } desired := &model.Component{ Name: "new", @@ -260,7 +310,7 @@ func TestApplyComponentChanges_DoesNotTouchIdentityOrRuntimeFields(t *testing.T) RackID: rackB, } - applyComponentChanges(existing, desired, expectedComponentSpec{}) + applyComponentChanges(existing, desired, expectedComponentSpec{Description: "Core description"}) assert.Equal(t, "new", existing.Name) assert.Equal(t, "new-model", existing.Model) @@ -270,6 +320,24 @@ func TestApplyComponentChanges_DoesNotTouchIdentityOrRuntimeFields(t *testing.T) assert.Equal(t, "SN-1", existing.SerialNumber, "SerialNumber is identity") require.NotNil(t, existing.ComponentID) assert.Equal(t, "runtime-id", *existing.ComponentID, "external_id is runtime-owned") + assert.Equal(t, "10.0.0.2", existing.Description["nvos_ip"], "runtime-owned description entry must survive") + assert.Equal(t, "keep", existing.Description["operator"], "operator-owned description entry must survive") + assert.Equal(t, "Core description", existing.Description[expectedDescriptionKey]) +} + +func TestComponentDescriptionWithExpected(t *testing.T) { + existing := map[string]any{ + expectedDescriptionKey: "old", + "operator": "keep", + } + + updated := componentDescriptionWithExpected(existing, "new") + assert.Equal(t, map[string]any{expectedDescriptionKey: "new", "operator": "keep"}, updated) + assert.Equal(t, "old", existing[expectedDescriptionKey], "helper must not mutate the input map") + + cleared := componentDescriptionWithExpected(updated, "") + assert.Equal(t, map[string]any{"operator": "keep"}, cleared) + assert.Nil(t, componentDescriptionWithExpected(map[string]any{expectedDescriptionKey: "old"}, "")) } func TestApplyComponentChanges_PreservedFieldsKeepFlowValue(t *testing.T) { diff --git a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go index 793895a6d9..c111471b28 100644 --- a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go +++ b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go @@ -224,6 +224,65 @@ func compType() string { return devicetypes.ComponentTypeToString(devicetypes.ComponentTypeCompute) } +func TestMirrorComponents_DescriptionLifecycle(t *testing.T) { + for _, tc := range []struct { + name string + componentType devicetypes.ComponentType + serial string + mac string + }{ + {"ExpectedMachine", devicetypes.ComponentTypeCompute, "DESC-COMPUTE", "aa:bb:cc:dd:ef:01"}, + {"ExpectedSwitch", devicetypes.ComponentTypeNVSwitch, "DESC-SWITCH", "aa:bb:cc:dd:ef:02"}, + {"ExpectedPowerShelf", devicetypes.ComponentTypePowerShelf, "DESC-POWER", "aa:bb:cc:dd:ef:03"}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, pool := mirrorTestPool(t) + componentType := devicetypes.ComponentTypeToString(tc.componentType) + spec := expectedComponentSpec{ + Type: componentType, + Manufacturer: "Mfg", + SerialNumber: tc.serial, + Name: tc.serial, + Description: "initial description", + BMC: expectedBMCSpec{MACAddress: tc.mac}, + } + + mirrorExpectedComponents(ctx, pool, componentType, []expectedComponentSpec{spec}, map[string]uuid.UUID{}) + + loadComponent := func() model.Component { + var component model.Component + err := pool.DB.NewSelect().Model(&component). + Where("manufacturer = ? AND serial_number = ?", spec.Manufacturer, spec.SerialNumber). + Scan(ctx) + require.NoError(t, err) + return component + } + + component := loadComponent() + assert.Equal(t, "initial description", component.Description[expectedDescriptionKey]) + + component.Description["operator"] = "keep" + component.Description["nvos_ip"] = "10.0.0.2" + _, err := pool.DB.NewUpdate().Model(&component).Column("description").WherePK().Exec(ctx) + require.NoError(t, err) + + spec.Description = "updated description" + mirrorExpectedComponents(ctx, pool, componentType, []expectedComponentSpec{spec}, map[string]uuid.UUID{}) + component = loadComponent() + assert.Equal(t, "updated description", component.Description[expectedDescriptionKey]) + assert.Equal(t, "keep", component.Description["operator"]) + assert.Equal(t, "10.0.0.2", component.Description["nvos_ip"]) + + spec.Description = "" + mirrorExpectedComponents(ctx, pool, componentType, []expectedComponentSpec{spec}, map[string]uuid.UUID{}) + component = loadComponent() + assert.NotContains(t, component.Description, expectedDescriptionKey) + assert.Equal(t, "keep", component.Description["operator"]) + assert.Equal(t, "10.0.0.2", component.Description["nvos_ip"]) + }) + } +} + // #11: a successful but empty Core response soft-deletes all Flow components // of the type. func TestMirrorComponents_EmptyCoreSoftDeletesAll(t *testing.T) { From 1643cc963e7275a2dde08711bb917d5d636526be Mon Sep 17 00:00:00 2001 From: Patrice Breton Date: Fri, 7 Aug 2026 15:40:49 -0700 Subject: [PATCH 2/2] fix(flow): update expected descriptions atomically Signed-off-by: Patrice Breton --- .../expected_mirror_component.go | 42 ++++++++++++-- .../inventorysync/expected_mirror_db_test.go | 57 +++++++++++++++++++ 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go index 2efc43a106..f448ffba56 100644 --- a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go +++ b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go @@ -441,12 +441,8 @@ func mirrorExpectedComponents( // tombstone row — bun otherwise appends "deleted_at IS NULL" to // the UPDATE and the resurrect would silently match zero rows. p.toUpdate[i].UpdatedAt = now - if _, err := tx.NewUpdate(). - Model(&p.toUpdate[i]). - Column("name", "model", "description", "slot_id", "tray_index", "host_id", "rack_id", "deleted_at", "updated_at"). - WhereAllWithDeleted(). - Where("id = ?", p.toUpdate[i].ID). - Exec(ctx); err != nil { + expectedDescription, _ := p.toUpdate[i].Description[expectedDescriptionKey].(string) + if err := updateMirroredComponent(ctx, tx, &p.toUpdate[i], expectedDescription); err != nil { return fmt.Errorf("update component %q: %w", p.toUpdate[i].SerialNumber, err) } ops := p.toUpdateBMCs[i] @@ -497,6 +493,40 @@ func mirrorExpectedComponents( return result } +// updateMirroredComponent persists mirror-owned scalar fields and changes only +// Core's reserved description key against the current database value. Keeping +// the JSONB mutation in SQL prevents a stale reconciliation snapshot from +// replacing runtime or operator entries written after the snapshot was read. +func updateMirroredComponent(ctx context.Context, idb bun.IDB, component *model.Component, expectedDescription string) error { + var rackID any + if component.RackID != uuid.Nil { + rackID = component.RackID + } + query := idb.NewUpdate(). + Model((*model.Component)(nil)). + Set("name = ?", component.Name). + Set("model = ?", component.Model). + Set("slot_id = ?", component.SlotID). + Set("tray_index = ?", component.TrayIndex). + Set("host_id = ?", component.HostID). + Set("rack_id = ?", rackID). + Set("deleted_at = ?", component.DeletedAt). + Set("updated_at = ?", component.UpdatedAt). + WhereAllWithDeleted(). + Where("id = ?", component.ID) + if expectedDescription == "" { + query.Set("description = NULLIF((CASE WHEN jsonb_typeof(description) = 'object' THEN description ELSE '{}'::jsonb END) - ?::text, '{}'::jsonb)", expectedDescriptionKey) + } else { + query.Set( + "description = jsonb_set(CASE WHEN jsonb_typeof(description) = 'object' THEN description ELSE '{}'::jsonb END, ARRAY[?::text], to_jsonb(?::text), true)", + expectedDescriptionKey, + expectedDescription, + ) + } + _, err := query.Exec(ctx) + return err +} + // specValid rejects rows missing fields the mirror needs to construct a row // that both inserts cleanly (Component.Manufacturer / SerialNumber are // NOT NULL and form a unique index) and reconciles BMC (MAC is BMC PK). diff --git a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go index c111471b28..cd0706ef9d 100644 --- a/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go +++ b/rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go @@ -283,6 +283,63 @@ func TestMirrorComponents_DescriptionLifecycle(t *testing.T) { } } +func TestUpdateMirroredComponent_PreservesRuntimeWriteAfterReconciliationRead(t *testing.T) { + for _, tc := range []struct { + name string + expectedDescription string + wantExpected bool + }{ + {"set expected description", "updated description", true}, + {"clear expected description", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, pool := mirrorTestPool(t) + component := model.Component{ + Type: compType(), + Manufacturer: "Mfg", + SerialNumber: "INTERLEAVE-" + tc.name, + Name: "before", + Description: map[string]any{ + expectedDescriptionKey: "initial description", + "operator": "keep", + }, + } + require.NoError(t, component.Create(ctx, pool.DB)) + + var reconciliationSnapshot model.Component + require.NoError(t, pool.DB.NewSelect().Model(&reconciliationSnapshot).Where("id = ?", component.ID).Scan(ctx)) + + // Simulate runtime sync committing after reconciliation read the row + // but before the mirror writes its planned update. + _, err := pool.DB.NewUpdate(). + Model((*model.Component)(nil)). + Set( + "description = jsonb_set(COALESCE(description, '{}'::jsonb), ARRAY[?::text], to_jsonb(?::text), true)", + nvosIPDescriptionKey, + "10.0.0.2", + ). + Where("id = ?", component.ID). + Exec(ctx) + require.NoError(t, err) + + reconciliationSnapshot.Name = "after" + reconciliationSnapshot.UpdatedAt = time.Now() + require.NoError(t, updateMirroredComponent(ctx, pool.DB, &reconciliationSnapshot, tc.expectedDescription)) + + var updated model.Component + require.NoError(t, pool.DB.NewSelect().Model(&updated).Where("id = ?", component.ID).Scan(ctx)) + assert.Equal(t, "after", updated.Name) + assert.Equal(t, "10.0.0.2", updated.Description[nvosIPDescriptionKey]) + assert.Equal(t, "keep", updated.Description["operator"]) + if tc.wantExpected { + assert.Equal(t, tc.expectedDescription, updated.Description[expectedDescriptionKey]) + } else { + assert.NotContains(t, updated.Description, expectedDescriptionKey) + } + }) + } +} + // #11: a successful but empty Core response soft-deletes all Flow components // of the type. func TestMirrorComponents_EmptyCoreSoftDeletesAll(t *testing.T) {