Skip to content
This repository was archived by the owner on Jun 2, 2026. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/pkg/api/handler/rack.go
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ func (furh UpdateRackFirmwareHandler) Handle(c echo.Context) error {
}

flowResp, err := common.ExecuteFirmwareUpdateWorkflow(ctx, c, logger, stc, targetSpec, apiRequest.Version,
fmt.Sprintf("rack-firmware-update-%s", rackStrID), "Rack")
nil, fmt.Sprintf("rack-firmware-update-%s", rackStrID), "Rack")
if err != nil {
return err
}
Expand Down Expand Up @@ -1268,7 +1268,7 @@ func (furbh BatchUpdateRackFirmwareHandler) Handle(c echo.Context) error {
targetSpec := request.Filter.ToTargetSpec()

flowResp, err := common.ExecuteFirmwareUpdateWorkflow(ctx, c, logger, stc, targetSpec, request.Version,
fmt.Sprintf("rack-firmware-batch-update-%s", common.RequestHash(request.Filter)), "Rack")
nil, fmt.Sprintf("rack-firmware-batch-update-%s", common.RequestHash(request.Filter)), "Rack")
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions api/pkg/api/handler/tray.go
Original file line number Diff line number Diff line change
Expand Up @@ -1144,7 +1144,7 @@ func (futh UpdateTrayFirmwareHandler) Handle(c echo.Context) error {
}

flowResp, err := common.ExecuteFirmwareUpdateWorkflow(ctx, c, logger, stc, targetSpec, apiRequest.Version,
fmt.Sprintf("tray-firmware-update-%s", trayStrID), "Tray")
apiRequest.Targets, fmt.Sprintf("tray-firmware-update-%s", trayStrID), "Tray")
if err != nil {
return err
}
Expand Down Expand Up @@ -1259,7 +1259,7 @@ func (futbh BatchUpdateTrayFirmwareHandler) Handle(c echo.Context) error {
targetSpec := request.Filter.ToTargetSpec()

flowResp, err := common.ExecuteFirmwareUpdateWorkflow(ctx, c, logger, stc, targetSpec, request.Version,
fmt.Sprintf("tray-firmware-batch-update-%s", common.RequestHash(request.Filter)), "Tray")
request.Targets, fmt.Sprintf("tray-firmware-batch-update-%s", common.RequestHash(request.Filter)), "Tray")
if err != nil {
return err
}
Expand Down
9 changes: 9 additions & 0 deletions api/pkg/api/handler/util/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -1762,19 +1762,28 @@ func ExecuteBringUpRackWorkflow(

// ExecuteFirmwareUpdateWorkflow builds an UpgradeFirmwareRequest, executes the UpgradeFirmware
// workflow via Temporal, and returns the raw SubmitTaskResponse.
//
// targets, when non-empty, restricts the upgrade to the listed firmware
// sub-parts within each targeted tray (e.g. ["bmc", "nvos"] for switch
// trays). An empty/nil slice keeps the historical "update everything in
// the bundle" behavior. Names are passed through verbatim to Flow as
// `sub_targets`, which resolves them against the tray-type-specific
// component-manager enums (see flow/pkg/common/firmwarecomponents).
func ExecuteFirmwareUpdateWorkflow(
ctx context.Context,
c echo.Context,
logger zerolog.Logger,
stc tclient.Client,
targetSpec *flowv1.OperationTargetSpec,
version *string,
targets []string,
workflowID string,
entityName string,
) (*flowv1.SubmitTaskResponse, error) {
flowRequest := &flowv1.UpgradeFirmwareRequest{
TargetSpec: targetSpec,
TargetVersion: version,
SubTargets: targets,
Description: fmt.Sprintf("API firmware update %s", entityName),
}

Expand Down
45 changes: 43 additions & 2 deletions api/pkg/api/model/firmware.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,29 @@ import (
type APIUpdateFirmwareRequest struct {
SiteID string `json:"siteId"`
Version *string `json:"version,omitempty"`
// Targets, when non-empty, restricts the update to a subset of
// firmware sub-parts within the targeted tray (e.g. ["bmc", "nvos"]
// for switch trays). Names are lowercase. The authoritative supported
// set per tray type is derived from the Flow service's NICo proto
// bindings (mirroring Core's per-tray-type enums in
// carbide-core/crates/rpc/proto/forge.proto); see
// flow/pkg/common/firmwarecomponents for the resolution logic and
// helpers like SupportedNICoNVSwitchNames.
// Empty/nil means "update everything in the bundle". Requires Version.
//
// REST surface intentionally calls these "targets" to avoid confusion
// with carbide's tray-level "Component" vocabulary; the downstream
// Flow proto field is still named `components` and represents the
// same enum subset.
Targets []string `json:"targets,omitempty"`
}

// Validate validates the firmware update request
func (r *APIUpdateFirmwareRequest) Validate() error {
if r.SiteID == "" {
return fmt.Errorf("siteId is required")
}
return nil
return validateFirmwareTargets(r.Targets, r.Version)
}

// ========== Firmware Update Response ==========
Expand Down Expand Up @@ -89,12 +104,38 @@ type APIBatchTrayFirmwareUpdateRequest struct {
SiteID string `json:"siteId"`
Filter *TrayFilter `json:"filter,omitempty"`
Version *string `json:"version,omitempty"`
// Targets, when non-empty, restricts the update to a subset of
// firmware sub-parts within each matched tray. Same semantics as the
// single-tray variant. Requires Version.
Targets []string `json:"targets,omitempty"`
}

// Validate checks required fields and filter constraints.
func (r *APIBatchTrayFirmwareUpdateRequest) Validate() error {
if r.SiteID == "" {
return fmt.Errorf("siteId is required")
}
return r.Filter.Validate()
if err := r.Filter.Validate(); err != nil {
return err
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
return validateFirmwareTargets(r.Targets, r.Version)
}

// validateFirmwareTargets enforces the cross-field constraint that a
// firmware-target subset selection is only meaningful when a target version
// is also supplied. Per-tray-type name validation is delegated to Flow,
// where the mapping from string to component-manager enum lives.
func validateFirmwareTargets(targets []string, version *string) error {
if len(targets) == 0 {
return nil
}
for _, t := range targets {
if t == "" {
return fmt.Errorf("targets must not contain empty strings")
}
}
if version == nil || *version == "" {
return fmt.Errorf("targets requires version to be set")
}
return nil
}
82 changes: 82 additions & 0 deletions api/pkg/api/model/firmware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,88 @@ func TestAPIUpdateFirmwareRequest_Validate(t *testing.T) {
request: APIUpdateFirmwareRequest{Version: strPtr("24.11.0")},
wantErr: true,
},
{
name: "valid - targets with version",
request: APIUpdateFirmwareRequest{
SiteID: "site-1",
Version: strPtr("24.11.0"),
Targets: []string{"bmc", "nvos"},
},
wantErr: false,
},
{
name: "invalid - targets without version",
request: APIUpdateFirmwareRequest{
SiteID: "site-1",
Targets: []string{"bmc"},
},
wantErr: true,
},
{
name: "invalid - targets with empty version string",
request: APIUpdateFirmwareRequest{
SiteID: "site-1",
Version: strPtr(""),
Targets: []string{"bmc"},
},
wantErr: true,
},
{
name: "invalid - targets contains empty string",
request: APIUpdateFirmwareRequest{
SiteID: "site-1",
Version: strPtr("24.11.0"),
Targets: []string{"bmc", ""},
},
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.request.Validate()
if tt.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}

func TestAPIBatchTrayFirmwareUpdateRequest_Validate(t *testing.T) {
tests := []struct {
name string
request APIBatchTrayFirmwareUpdateRequest
wantErr bool
}{
{
name: "valid - siteId only",
request: APIBatchTrayFirmwareUpdateRequest{SiteID: "site-1"},
wantErr: false,
},
{
name: "valid - targets with version",
request: APIBatchTrayFirmwareUpdateRequest{
SiteID: "site-1",
Version: strPtr("24.11.0"),
Targets: []string{"bmc"},
},
wantErr: false,
},
{
name: "invalid - targets without version",
request: APIBatchTrayFirmwareUpdateRequest{
SiteID: "site-1",
Targets: []string{"bmc"},
},
wantErr: true,
},
Comment on lines +104 to +133

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Test cases will panic due to nil Filter dereference.

Three test cases ("valid - siteId only" at lines 104-107, "valid - targets with version" at lines 109-116, and "invalid - targets without version" at lines 118-124) create APIBatchTrayFirmwareUpdateRequest without setting the Filter field, leaving it nil. When Validate() is invoked, the code at firmware.go:118 unconditionally calls r.Filter.Validate(), causing a nil pointer panic.

These tests cannot pass until the nil guard is added to APIBatchTrayFirmwareUpdateRequest.Validate().

🛡️ Proposed fix: update tests to set Filter or wait for validation fix

Option 1: Set a valid (or empty) Filter in each test case to avoid nil:

 		{
 			name:    "valid - siteId only",
-			request: APIBatchTrayFirmwareUpdateRequest{SiteID: "site-1"},
+			request: APIBatchTrayFirmwareUpdateRequest{SiteID: "site-1", Filter: &TrayFilter{}},
 			wantErr: false,
 		},
 		{
 			name: "valid - targets with version",
 			request: APIBatchTrayFirmwareUpdateRequest{
 				SiteID:  "site-1",
+				Filter:  &TrayFilter{},
 				Version: strPtr("24.11.0"),
 				Targets: []string{"bmc"},
 			},
 			wantErr: false,
 		},
 		{
 			name: "invalid - targets without version",
 			request: APIBatchTrayFirmwareUpdateRequest{
 				SiteID:  "site-1",
+				Filter:  &TrayFilter{},
 				Targets: []string{"bmc"},
 			},
 			wantErr: true,
 		},

Option 2 (recommended): Fix the nil guard in firmware.go first (as flagged in the separate comment), then these tests will pass as-is.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
name: "valid - siteId only",
request: APIBatchTrayFirmwareUpdateRequest{SiteID: "site-1"},
wantErr: false,
},
{
name: "valid - targets with version",
request: APIBatchTrayFirmwareUpdateRequest{
SiteID: "site-1",
Version: strPtr("24.11.0"),
Targets: []string{"bmc"},
},
wantErr: false,
},
{
name: "invalid - targets without version",
request: APIBatchTrayFirmwareUpdateRequest{
SiteID: "site-1",
Targets: []string{"bmc"},
},
wantErr: true,
},
{
name: "valid - siteId only",
request: APIBatchTrayFirmwareUpdateRequest{SiteID: "site-1", Filter: &TrayFilter{}},
wantErr: false,
},
{
name: "valid - targets with version",
request: APIBatchTrayFirmwareUpdateRequest{
SiteID: "site-1",
Filter: &TrayFilter{},
Version: strPtr("24.11.0"),
Targets: []string{"bmc"},
},
wantErr: false,
},
{
name: "invalid - targets without version",
request: APIBatchTrayFirmwareUpdateRequest{
SiteID: "site-1",
Filter: &TrayFilter{},
Targets: []string{"bmc"},
},
wantErr: true,
},
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/pkg/api/model/firmware_test.go` around lines 104 - 124, The tests panic
because APIBatchTrayFirmwareUpdateRequest.Validate() calls r.Filter.Validate()
without checking for nil; add a nil guard in
APIBatchTrayFirmwareUpdateRequest.Validate (or ensure r.Filter is non-nil before
calling) so it only calls r.Filter.Validate() when r.Filter != nil, or
initialize r.Filter to an empty/zero-value Filter inside Validate() before
delegation; update the logic around the
APIBatchTrayFirmwareUpdateRequest.Validate method to safely handle nil Filter
and re-run tests.

{
name: "invalid - missing siteId",
request: APIBatchTrayFirmwareUpdateRequest{},
wantErr: true,
},
}

for _, tt := range tests {
Expand Down
1 change: 1 addition & 0 deletions flow/internal/converter/protobuf/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -1239,6 +1239,7 @@ func ScheduledOperationFrom(
info := &operations.FirmwareControlTaskInfo{
Operation: operations.FirmwareOperationUpgrade,
TargetVersion: r.UpgradeFirmware.GetTargetVersion(),
SubTargets: r.UpgradeFirmware.GetSubTargets(),
}

if r.UpgradeFirmware.GetStartTime() != nil {
Expand Down
1 change: 1 addition & 0 deletions flow/internal/service/server_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -1241,6 +1241,7 @@ func (rs *FlowServerImpl) UpgradeFirmware(
Operation: operations.FirmwareOperationUpgrade,
TargetVersion: req.GetTargetVersion(),
RuleID: protobuf.UUIDStringFrom(req.GetRuleId()),
SubTargets: req.GetSubTargets(),
}

// Parse optional time parameters for scheduled upgrade
Expand Down
14 changes: 14 additions & 0 deletions flow/internal/task/componentmanager/compute/nico/nico.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ func (m *Manager) FirmwareControl(ctx context.Context, target common.Target, inf
log.Debug().
Str("components", target.String()).
Str("target_version", info.TargetVersion).
Strs("sub_targets", info.SubTargets).
Msg("Scheduling firmware update for compute via NICo")

if m.nicoClient == nil {
Expand All @@ -325,6 +326,19 @@ func (m *Manager) FirmwareControl(ctx context.Context, target common.Target, inf
return fmt.Errorf("target is invalid: %w", err)
}

if len(info.SubTargets) > 0 {
// SetFirmwareUpdateTimeWindow + SetMachineAutoUpdate (the path used
// here for compute trays) do not expose per-sub-target selection;
// auto-update will install whatever is in the desired bundle. Log
// the requested subset so the limitation is visible until we can
// route this through UpdateComponentFirmware (planned alongside the
// version → FW Object identifier migration).
log.Warn().
Str("components", target.String()).
Strs("sub_targets", info.SubTargets).
Msg("compute firmware sub-target selection is not yet honored; whole bundle will be applied")
}

desiredEntries, err := m.nicoClient.GetDesiredFirmwareVersions(ctx)
if err != nil {
return fmt.Errorf("failed to query desired firmware versions: %w", err)
Expand Down
33 changes: 33 additions & 0 deletions flow/internal/task/componentmanager/compute/nico/nico_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,39 @@ func mustMarshal(t *testing.T, v any) json.RawMessage {
return data
}

// TestFirmwareControl_SubTargetsAccepted verifies that the compute/nico
// FirmwareControl path tolerates info.SubTargets without erroring. This
// path goes through SetMachineAutoUpdate + SetFirmwareUpdateTimeWindow,
// which has no per-sub-target selection in NICo, so the manager only logs
// a warning and proceeds; we exercise that branch here. The actual
// per-sub-target dispatch will be added when compute moves to NICo's
// UpdateComponentFirmware (see comment in nico.go).
func TestFirmwareControl_SubTargetsAccepted(t *testing.T) {
tests := map[string]struct {
subTargets []string
}{
"nil sub_targets (legacy path)": {subTargets: nil},
"empty sub_targets (legacy path)": {subTargets: []string{}},
"non-empty sub_targets (warn path)": {subTargets: []string{"bmc", "bios"}},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
m := New(nicoapi.NewMockClient(), 0)
target := common.Target{
Type: devicetypes.ComponentTypeCompute,
ComponentIDs: []string{"machine-1"},
}

err := m.FirmwareControl(context.Background(), target, operations.FirmwareControlTaskInfo{
Operation: operations.FirmwareOperationUpgrade,
SubTargets: tc.subTargets,
})
require.NoError(t, err)
})
}
}

// --- Tests for firmware version helper functions ---

func desiredEntry(versions map[string]string) *pb.DesiredFirmwareVersionEntry {
Expand Down
10 changes: 9 additions & 1 deletion flow/internal/task/componentmanager/nvlswitch/nico/nico.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/NVIDIA/infra-controller-rest/flow/internal/task/executor/temporalworkflow/common"
"github.com/NVIDIA/infra-controller-rest/flow/internal/task/operations"
"github.com/NVIDIA/infra-controller-rest/flow/pkg/common/devicetypes"
"github.com/NVIDIA/infra-controller-rest/flow/pkg/common/firmwarecomponents"
)

const (
Expand Down Expand Up @@ -242,12 +243,18 @@ func (m *Manager) FirmwareControl(ctx context.Context, target common.Target, inf
log.Debug().
Str("components", target.String()).
Str("target_version", info.TargetVersion).
Strs("sub_targets", info.SubTargets).
Msg("Starting firmware update for NVLSwitch via NICo")

if err := target.Validate(); err != nil {
return fmt.Errorf("target is invalid: %w", err)
}

subComponents, err := firmwarecomponents.ParseNICoNVSwitch(info.SubTargets)
if err != nil {
return err
}

if info.TargetVersion == "" {
upToDate, err := m.checkFirmwareUpToDate(ctx, target)
if err != nil {
Expand All @@ -263,7 +270,8 @@ func (m *Manager) FirmwareControl(ctx context.Context, target common.Target, inf
req := &pb.UpdateComponentFirmwareRequest{
Target: &pb.UpdateComponentFirmwareRequest_Switches{
Switches: &pb.UpdateSwitchFirmwareTarget{
SwitchIds: switchIDsProto(target.ComponentIDs),
SwitchIds: switchIDsProto(target.ComponentIDs),
Components: subComponents,
},
},
TargetVersion: info.TargetVersion,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/NVIDIA/infra-controller-rest/flow/internal/task/executor/temporalworkflow/common"
"github.com/NVIDIA/infra-controller-rest/flow/internal/task/operations"
"github.com/NVIDIA/infra-controller-rest/flow/pkg/common/devicetypes"
"github.com/NVIDIA/infra-controller-rest/flow/pkg/common/firmwarecomponents"
)

const (
Expand Down Expand Up @@ -166,11 +167,16 @@ func mapPowerOperation(op operations.PowerOperation) (nsmapi.PowerAction, error)

// FirmwareControl initiates firmware update without waiting for completion.
// Returns immediately after the update request is accepted.
//
// info.SubTargets, when non-empty, restricts the update to the selected
// firmware sub-parts (e.g. ["bmc", "nvos"]). Empty means "all components
// in the bundle" (NSM's historical default).
func (m *Manager) FirmwareControl(ctx context.Context, target common.Target, info operations.FirmwareControlTaskInfo) error {
log.Debug().
Str("components", target.String()).
Str("operation", fmt.Sprintf("%v", info.Operation)).
Str("target_version", info.TargetVersion).
Strs("sub_targets", info.SubTargets).
Msg("Starting firmware update")

if m.nsmClient == nil {
Expand All @@ -181,7 +187,12 @@ func (m *Manager) FirmwareControl(ctx context.Context, target common.Target, inf
return fmt.Errorf("target is invalid: %w", err)
}

updates, err := m.nsmClient.QueueUpdates(ctx, target.ComponentIDs, info.TargetVersion, nil)
subComponents, err := firmwarecomponents.ParseNSMNVSwitch(info.SubTargets)
if err != nil {
return err
}

updates, err := m.nsmClient.QueueUpdates(ctx, target.ComponentIDs, info.TargetVersion, subComponents)
if err != nil {
return fmt.Errorf("firmware update request failed: %w", err)
}
Expand Down
Loading
Loading