NAS-137987 / 26.04 / fix: call vm.resume on suspended VMs

[mattwyatt-ix]

Changes:
for suspended VMs, we properly displayed that they were suspended and displayed 'Resume' instead of 'Start' on the UI. the problem is that we still called vm.start instead of vm.resume.

• rename doStart to doStartResume. it now calls vm.resume on a suspended VM instead of vm.start. the original behavior is unaffected and the correct parameters are passed to both methods.
• update usage of doStart to doStartResume throughout the codebase.
• add unit tests to cover the new resume/start functionality.
• improve test coverage for vm.service.ts.

Testing:
see ticket for the original bug behavior.

1. create a VM and start it.
2. use midclt call vm.suspend <VM_ID> to suspend your VM.
3. use the 'Resume' button and observe that it successfully resumes the VM instead of creating a middleware error.

Notes:
we could have avoided modifying the doStart method and instead creating a doResume method and using it inside the components. this is an alternate implementation that we could consider.

Downstream

Affects	Reasoning
Documentation
Testing

[bugclerk]

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

[claude]

Claude finished @mattwyatt-ix's task —— View job

---

Code Review Complete ✅

Great implementation! The fix correctly addresses the bug where suspended VMs were calling vm.start instead of vm.resume, and the test coverage is solid 🎉

Strong Points

The implementation is well thought out with proper type safety, comprehensive JSDoc, excellent test coverage including edge cases, and the conditional logic correctly handles the overcommit parameter only for start operations.

Issues Found

1. Missing Error Test for Resume Operation ❗

The test suite covers error scenarios for vm.start but not for vm.resume. Since both code paths can fail, there should be a test for resume failures.

Suggested test (add to vm.service.spec.ts:180):

it('should handle error when resuming suspended VM fails', async () => {
  const vm = mockVm(VmState.Suspended);
  const apiService = spectator.inject(ApiService);
  const errorHandlerService = spectator.inject(ErrorHandlerService);
  const callSpy = jest.spyOn(apiService, 'call');
  const mockImpl = callSpy.getMockImplementation();

  callSpy.mockImplementation((method) => {
    if (method === 'vm.resume') {
      return throwError(() => new ApiCallError({ 
        code: JsonRpcErrorCode.CallError, 
        message: 'Failed to resume VM' 
      }));
    }
    return mockImpl(method);
  });

  await firstValueFrom(spectator.service.doStartResume(vm));
  expect(apiService.call).toHaveBeenCalledWith('vm.resume', [1]);
  expect(errorHandlerService.showErrorModal).toHaveBeenCalled();
});

2. Type Assertion Could Be Safer 💡

In vm.service.ts:89, the type union for StartResumeParams with as casting could be more type-safe.

Current code (vm.service.ts:87-89):

type StartResumeParams = ApiCallParams<typeof this.wsMethods.start> | ApiCallParams<typeof this.wsMethods.resume>;

return this.api.call(method, params as StartResumeParams)

Suggested improvement:
Since TypeScript can't narrow the union properly, consider using conditional types or a type guard. However, the current approach is acceptable given TypeScript's limitations with this pattern. Consider adding a comment explaining why the assertion is necessary:

// Type assertion needed: TypeScript can't narrow the union of method/params pairs
type StartResumeParams = ApiCallParams<typeof this.wsMethods.start> | ApiCallParams<typeof this.wsMethods.resume>;
return this.api.call(method, params as StartResumeParams)

Minor Observations

• The codecov report shows 2 missing lines, likely from the new conditional branches. The missing test for resume error handling would likely improve this coverage.
• The mockVm helper function in the test file is a nice addition that improves test readability 👍

Architecture Note

The PR description mentions an alternate implementation using a separate doResume method. The current approach (single doStartResume method) is cleaner and maintains better encapsulation since resume is semantically a type of "starting" a VM that happens to be suspended. Good choice! 🚀

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.71%. Comparing base (1b4fd67) to head (cb0b26a).
⚠️ Report is 4 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/app/pages/vm/vm-list.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13039      +/-   ##
==========================================
+ Coverage   86.69%   86.71%   +0.01%     
==========================================
  Files        1846     1846              
  Lines       69371    69373       +2     
  Branches     8574     8575       +1     
==========================================
+ Hits        60143    60156      +13     
+ Misses       9228     9217      -11     

☔ 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.