NAS-137252 / 26.04 / Allow MPIO for Fibre Channel

[aervin]

Changes:

Testing:

Downstream

Affects	Reasoning
Documentation
Testing

[bugclerk]

Jira URL: https://ixsystems.atlassian.net/browse/NAS-137252

[claude]

Claude finished @aervin's task —— View job

---

Code Review Complete! 🎉

This is a well-architected PR that adds MPIO support for Fibre Channel! The implementation demonstrates excellent separation of concerns, comprehensive test coverage, and follows Angular best practices.

Strengths

Excellent architecture and code organization - The new helpers (createFormArraySnapshot, configurePortControlsForMode) solve real problems elegantly with clear documentation and comprehensive tests. The component decomposition is spot-on.

Comprehensive test coverage - Great use of harnesses throughout (particularly in fc-port-item-controls.component.spec.ts), with tests covering edge cases like port filtering, mode switching, and reactivity to input changes.

Clean reactive patterns - Smart use of computed signals and the snapshot helper to prevent infinite change detection loops. The separation of the modeControl from the parent form is a thoughtful design decision.

---

Issues to Address

🔴 Critical: Potential Race Condition in FibreChannelService.linkFiberChannelPortsToTarget

Location: src/app/services/fibre-channel.service.ts:101-107

The service creates virtual ports and deletes existing ports in parallel via forkJoin, which could cause race conditions or ordering issues. If a delete operation conflicts with a create, or if the backend expects sequential operations, this could fail.

// Current implementation runs all operations in parallel
return forkJoin(operations);

Recommendation: Consider if operations need sequencing (deletes before creates) or if the backend handles this safely. If ordering matters, chain the operations:

// Delete first, then create
const deletes$ = forkJoin(deleteOperations.length > 0 ? deleteOperations : [of(null)]);
const creates$ = forkJoin(createOperations.length > 0 ? createOperations : [of(null)]);
return deletes$.pipe(switchMap(() => creates$));

🟡 Error Handling: Missing Error Handling in createNewPort

Location: src/app/services/fibre-channel.service.ts:146-154

The createNewPort method doesn't handle the case where the host query returns no results:

private createNewPort(hostId: number): Observable<string> {
  return this.api.call('fc.fc_host.query', [[['id', '=', hostId]]]).pipe(
    switchMap((hosts) => {
      const host = hosts[0]; // ⚠️ What if hosts is empty?
      return this.api.call('fc.fc_host.update', [host.id, { npiv: host.npiv + 1 }]).pipe(
        map(() => `${host.alias}/${host.npiv + 1}`),
      );
    }),
  );
}

Recommendation: Add a check:

switchMap((hosts) => {
  if (!hosts.length) {
    return throwError(() => new Error(`FC host with id ${hostId} not found`));
  }
  const host = hosts[0];
  // ... rest of logic
})

🟡 Code Quality: Effect Should Use takeUntilDestroyed

Location: src/app/pages/sharing/iscsi/fibre-channel-ports/fc-port-item-controls/fc-port-item-controls.component.ts:82-90

The effect() in the constructor doesn't have explicit cleanup. While Angular 17+ effects auto-cleanup, it's best practice to use takeUntilDestroyed for observables inside effects:

effect(() => {
  const currentPortValue = this.currentPort();
  if (this.isEdit() && currentPortValue) {
    this.modeControl.setValue('existing');
    // ...
  }
});

Recommendation: Consider if this effect is even needed - it runs on every change detection when inputs change. You might be able to initialize the form state in ngOnInit instead, which would be clearer.

🟡 Accessibility: ARIA Label Missing

Location: src/app/pages/sharing/iscsi/fibre-channel-ports/fc-mpio-info-banner/fc-mpio-info-banner.component.html:2

The info banner uses role="status" but should include an aria-label for better screen reader support:

<div class="info-box" role="status">

Recommendation:

<div class="info-box" role="status" [attr.aria-label]="'MPIO Configuration Information' | translate">

---

Minor Observations

1. Test Improvement - fc-mpio-info-banner.component.spec.ts:19-22: Tests directly access DOM attributes instead of using harnesses. While acceptable for simple components, consider if an IxIconHarness exists for consistency.

2. Type Safety - form-array-snapshot.helper.ts:43: The // eslint-disable-next-line @typescript-eslint/no-explicit-any is justified here, but consider if FormArray<FormControl<T>> or similar could work.

3. Code Organization - The getUsedPhysicalPortsFn wrapper function in iscsi-wizard.component.ts:326-328 is only needed because the child component expects a function. Consider if the child could accept the computed signal directly to simplify the API.

---

Questions

1. Physical Port Validation - When a user edits a target with existing FC ports, the current port is always included in the dropdown (line 365-367 in fc-port-item-controls.component.ts). Is this the correct UX if that port's physical adapter is now being used elsewhere? Should we warn the user?

2. Mode Switching UX - When switching between "existing" and "new" modes, values are cleared (via configurePortControlsForMode). Is this intentional? Users might expect the form to remember their previous selection if they toggle back.

---

Overall, this is high-quality work! The main concern is the potential race condition in the FC service. The other issues are minor and easily addressable.

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.63%. Comparing base (be0e24e) to head (ab5df5a).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../iscsi/target/target-form/target-form.component.ts	57.14%	21 Missing ⚠️
...aring/iscsi/iscsi-wizard/iscsi-wizard.component.ts	71.69%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12958      +/-   ##
==========================================
+ Coverage   86.61%   86.63%   +0.02%     
==========================================
  Files        1844     1847       +3     
  Lines       69246    69383     +137     
  Branches     8536     8548      +12     
==========================================
+ Hits        59980    60113     +133     
- Misses       9266     9270       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.