feat: Resume PMSUSPENDED domains by invoking PM wakeup #65
Conversation
📝 WalkthroughWalkthroughDomain.resume now checks runtime state and, if PMSUSPENDED, calls domainPMWakeup instead of domainResume; a new Domain.pmSuspend(target) API was added. TypeScript wrappers, unit tests, and native N-API workers/bindings for domainPMWakeup and domainPMSuspend were introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DomainTS as Domain (TS)
participant HyperTS as Hypervisor (TS)
participant Native as Hypervisor (N-API)
participant Libvirt as libvirt
Note over DomainTS,HyperTS: Domain.resume() checks runtime state
DomainTS->>HyperTS: domainGetInfo(domain)
HyperTS-->>DomainTS: { state: PMSUSPENDED }
alt state == PMSUSPENDED
DomainTS->>HyperTS: domainPMWakeup(domain)
HyperTS->>Native: DomainPMWakeup(domainObject)
activate Native
Native->>Libvirt: virDomainPMWakeup(domainPtr)
Libvirt-->>Native: result
deactivate Native
Native-->>HyperTS: Promise settles
HyperTS-->>DomainTS: Promise settles
else other state
DomainTS->>HyperTS: domainResume(domain)
HyperTS->>Native: DomainResume(domainObject)
activate Native
Native->>Libvirt: virDomainResume(domainPtr)
Libvirt-->>Native: result
deactivate Native
Native-->>HyperTS: Promise settles
HyperTS-->>DomainTS: Promise settles
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)lib/types.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/domain.ts (1)
64-68: Logic correctly handles PMSUSPENDED state.The implementation properly checks the domain state and dispatches to
domainPMWakeupfor PMSUSPENDED domains while preserving existing behavior for other states. This aligns with the PR objective.Optional consideration: The
getInfo()call adds one extra round-trip to the hypervisor for every resume operation, even when the domain is not in PMSUSPENDED state. For VM management operations (typically not a hot path), this overhead is likely acceptable and the straightforward implementation is preferred over alternatives like try/catch error handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/domain.spec.tslib/domain.tslib/hypervisor.tssrc/hypervisor-domain.ccsrc/hypervisor.ccsrc/hypervisor.h
🧰 Additional context used
🧬 Code graph analysis (4)
lib/domain.spec.ts (1)
lib/hypervisor.ts (2)
domainGetInfo(249-254)domainPMWakeup(212-217)
src/hypervisor-domain.cc (3)
src/hypervisor-node.cc (3)
ret(26-29)dummyCallback(12-16)dummyCallback(12-12)src/worker.cc (2)
SetVirError(12-17)SetVirError(12-12)src/hypervisor-connect.cc (2)
dummyCallback(15-19)dummyCallback(15-15)
src/hypervisor.h (1)
src/hypervisor-domain.cc (3)
DomainPMWakeupWorker(950-955)DomainPMWakeup(966-986)DomainPMWakeup(966-966)
src/hypervisor.cc (1)
src/hypervisor-domain.cc (2)
DomainPMWakeup(966-986)DomainPMWakeup(966-966)
🪛 GitHub Actions: Docker Integration Tests
src/hypervisor-domain.cc
[error] 958-958: virDomainPMWakeup(domain->domainPtr, 0) uses private member 'Domain::domainPtr'. Access to private member is forbidden.
🪛 GitHub Actions: Test Libvirt
src/hypervisor-domain.cc
[error] 958-958: virDomainPMWakeup(domain->domainPtr, 0) uses private member Domain::domainPtr. The member 'domainPtr' is declared private in src/domain.h, causing access violation during build.
🔇 Additional comments (7)
src/hypervisor.cc (1)
44-44: LGTM!The binding is correctly added following the established pattern.
src/hypervisor.h (1)
44-44: LGTM!The forward declaration, method signature, and friendship declaration are correctly placed and follow the established pattern in this file. Note that
DomainPMWakeupWorkeralso requires a friend declaration insrc/domain.hto accessdomain->domainPtr(see comment on src/hypervisor-domain.cc).Also applies to: 79-79, 112-112
lib/hypervisor.ts (1)
207-217: LGTM!The method implementation follows the established pattern, and the JSDoc clearly documents the purpose and behavior.
lib/domain.spec.ts (3)
9-10: LGTM!The mock setup correctly initializes both
domainGetInfoanddomainPMWakeup, and properly adds them to the hypervisor mock object.Also applies to: 20-26, 32-32, 47-48
105-105: LGTM!Good addition to verify that
domainGetInfois called as part of the new resume flow.
109-123: LGTM!Excellent test coverage for the PMSUSPENDED path. The test properly verifies that:
domainGetInfois called to check the statedomainPMWakeupis invoked when the state is PMSUSPENDEDdomainResumeis NOT called in this caselib/domain.ts (1)
1-1: LGTM: Import correctly adds DomainState.The addition of
DomainStateto the imports is necessary for the new state check in theresume()method.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @lib/hypervisor.spec.ts:
- Around line 7-12: The test currently stubs nativeHypervisor directly which
bypasses the wrapMethod middleware; update the test to exercise the real
domainPMWakeup through Hypervisor.wrapMethod by instantiating a Hypervisor (or
setting up its native binding) so wrapMethod is invoked, or if that’s too heavy,
explicitly document in the test that it only verifies delegation to
nativeHypervisor and does not exercise wrapMethod; target symbols: use
Hypervisor (constructor/instance), wrapMethod, domainPMWakeup, and
nativeHypervisor to either wire the real binding or add the documented note.
🧹 Nitpick comments (2)
lib/hypervisor.spec.ts (2)
6-6: Test name doesn't match scope.The test name "should wake a PMSUSPENDED domain" suggests it verifies PMSUSPENDED-specific behavior, but it only tests that
domainPMWakeupdelegates to the native binding. The PMSUSPENDED state logic resides inDomain.resume(), not here.Consider renaming to clarify scope, e.g., "should delegate domainPMWakeup to native binding".
18-21: Consider adding error case coverage.The test verifies the happy path, but could benefit from additional test cases:
- Error propagation when
nativeHypervisor.domainPMWakeuprejects- Handling of edge cases (though
Domainvalidation may occur upstream)These additions would improve robustness but are not critical for the current implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/hypervisor.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
lib/hypervisor.spec.ts (1)
lib/hypervisor.ts (1)
domainPMWakeup(212-217)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and (Possibly) Publish Pre-built Binaries (18, arm64, ubuntu-latest)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (1)
lib/hypervisor.spec.ts (1)
1-5: LGTM: Clean test setup.The imports and test suite structure are appropriate for testing the Hypervisor class with Vitest.
| const hypervisor = Object.create(Hypervisor.prototype) as Hypervisor; | ||
| const domainPMWakeup = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| (hypervisor as any).nativeHypervisor = { | ||
| domainPMWakeup | ||
| }; |
There was a problem hiding this comment.
Test bypasses wrapMethod integration.
The test mocks nativeHypervisor directly, which skips the wrapMethod wrapper used in the actual implementation (see lib/hypervisor.ts:212-215). This means error handling, promise transformation, or other middleware behavior in wrapMethod is not tested.
Consider testing the actual domainPMWakeup method without mocking nativeHypervisor, or at minimum document that this test only verifies delegation to the native binding.
🔎 Alternative approach to test full integration
- it('should wake a PMSUSPENDED domain', async () => {
- const hypervisor = Object.create(Hypervisor.prototype) as Hypervisor;
- const domainPMWakeup = vi.fn().mockResolvedValue(undefined);
-
- (hypervisor as any).nativeHypervisor = {
- domainPMWakeup
- };
+ it('should delegate to native domainPMWakeup', async () => {
+ // Mock the native binding module that provides the C++ binding
+ const mockNativeBinding = {
+ domainPMWakeup: vi.fn().mockResolvedValue(undefined)
+ };
+
+ // Create hypervisor with the mocked native binding
+ const hypervisor = new Hypervisor(mockNativeBinding);Note: This approach tests through wrapMethod but requires understanding how Hypervisor is constructed with the native binding.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @lib/hypervisor.spec.ts around lines 7-12, The test currently stubs
nativeHypervisor directly which bypasses the wrapMethod middleware; update the
test to exercise the real domainPMWakeup through Hypervisor.wrapMethod by
instantiating a Hypervisor (or setting up its native binding) so wrapMethod is
invoked, or if that’s too heavy, explicitly document in the test that it only
verifies delegation to nativeHypervisor and does not exercise wrapMethod; target
symbols: use Hypervisor (constructor/instance), wrapMethod, domainPMWakeup, and
nativeHypervisor to either wire the real binding or add the documented note.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 785 811 +26
Branches 154 157 +3
=========================================
+ Hits 785 811 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/hypervisor-domain.cc:
- Around line 1002-1006: The call to virDomainPMSuspendForDuration is using a
non-existent constant; replace VIR_DOMAIN_PMSUSPEND_MEM with the correct libvirt
suspend target enum value VIR_NODE_SUSPEND_TARGET_MEM (suspend-to-RAM) in the
virDomainPMSuspendForDuration call (e.g., in the code around function/method
using domain->domainPtr and virDomainPMSuspendForDuration) so the function
receives a virNodeSuspendTarget value; ensure any headers required for that enum
are included if not already.
♻️ Duplicate comments (1)
lib/hypervisor.spec.ts (1)
6-21: Same concern as previous review: tests bypass wrapMethod integration.Both test cases mock
nativeHypervisordirectly, which skips thewrapMethodwrapper and its error handling. The previous review comment on lines 7-12 already identified this issue for the first test, and the same concern applies to the second test (lines 23-38).Also applies to: 23-38
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/domain.spec.tslib/domain.tslib/hypervisor.spec.tslib/hypervisor.tssrc/domain.hsrc/hypervisor-domain.ccsrc/hypervisor.ccsrc/hypervisor.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/domain.h
- src/hypervisor.cc
- lib/domain.ts
🧰 Additional context used
🧬 Code graph analysis (4)
lib/domain.spec.ts (1)
lib/hypervisor.ts (3)
domainGetInfo(261-266)domainPMWakeup(212-217)domainPMSuspend(224-229)
src/hypervisor-domain.cc (3)
src/hypervisor-node.cc (3)
ret(26-29)dummyCallback(12-16)dummyCallback(12-12)src/worker.cc (2)
SetVirError(12-17)SetVirError(12-12)src/hypervisor-connect.cc (2)
dummyCallback(15-19)dummyCallback(15-15)
src/hypervisor.h (1)
src/hypervisor-domain.cc (6)
DomainPMWakeupWorker(950-955)DomainPMSuspendWorker(994-999)DomainPMWakeup(966-986)DomainPMWakeup(966-966)DomainPMSuspend(1014-1034)DomainPMSuspend(1014-1014)
lib/hypervisor.ts (2)
lib/domain.ts (1)
Domain(8-151)lib/error.ts (1)
wrapMethod(55-61)
🪛 GitHub Actions: Test Libvirt
src/hypervisor-domain.cc
[error] 1004-1004: Build error in libvirt-domain.cc:1004: use of undeclared identifier 'VIR_DOMAIN_PMSUSPEND_MEM'; did you mean 'VIR_DOMAIN_PMSUSPENDED'?
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (7)
src/hypervisor.h (1)
44-45: LGTM! Declarations follow existing patterns.The forward declarations, public method declarations, and friend declarations for
DomainPMWakeupWorkerandDomainPMSuspendWorkerare consistent with the existing code structure and properly mirror the pattern used by other domain worker classes.Also applies to: 80-81, 114-115
lib/domain.spec.ts (4)
9-10: LGTM! Test setup properly integrates new mocks.The mock setup for
domainGetInfo,domainPMWakeup, anddomainPMSuspendfollows the existing test patterns and properly initializes all required mocks in thebeforeEachhook.Also applies to: 20-33, 48-50
107-109: LGTM! Correctly verifies domain state check.The assertion confirms that
domainGetInfois called before resuming, which aligns with the updated resume flow that inspects domain state before deciding which operation to perform.
111-125: LGTM! Test correctly verifies PMSUSPENDED wakeup path.The test properly validates that when a domain is in the
PMSUSPENDEDstate,resume()callsdomainPMWakeupinstead ofdomainResume. Good use ofmockResolvedValueOnceand the negative assertion.
128-133: LGTM! Test adequately covers pmSuspend delegation.The test follows the established pattern for verifying that
Domain.pmSuspend()correctly delegates tohypervisor.domainPMSuspend().lib/hypervisor.ts (1)
207-229: LGTM! Methods follow existing patterns and are properly documented.Both
domainPMWakeupanddomainPMSuspendmethods:
- Use
wrapMethodcorrectly for error handling- Include clear JSDoc documentation
- Follow the same pattern as other domain operation methods in the class
- Properly delegate to the native hypervisor with
domain.getNativeDomain()src/hypervisor-domain.cc (1)
944-986: LGTM! DomainPMWakeup implementation is correct.The
DomainPMWakeupWorkerclass andHypervisor::DomainPMWakeupmethod follow the established worker pattern consistently. The call tovirDomainPMWakeup(domain->domainPtr, 0)is correct, with flags set to 0.
elibosley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/types.ts (1)
160-162: Logic error in type error message construction.The code uses
typeof str1to show the received type, but sincestr1is already typed asstring | undefined,typeof str1will always evaluate to"string"or"undefined", not the actual type of the original argument that caused the error. This means the error message won't accurately reflect what type was received.🔎 Consider capturing the actual argument type
The current approach can't recover the original argument's type because by the time this constructor runs, the argument has already been processed. If you need to report the actual type that was passed incorrectly, you should either:
- Catch the type error at the call site where the actual argument type is known, or
- Pass the stringified value of the problematic argument (not its type) in
str1For example, if the intent is to show the actual value:
- if (message === 'Expected a number.') { - this.message = `Type error: Expected a number but received ${typeof str1 || 'undefined'}. This error typically occurs when calling libvirt methods that require numeric parameters.`; - } + if (message === 'Expected a number.' && str1) { + this.message = `Type error: Expected a number but received '${str1}'. This error typically occurs when calling libvirt methods that require numeric parameters.`; + }
♻️ Duplicate comments (1)
lib/hypervisor.spec.ts (1)
7-56: Existing concern about wrapMethod bypass applies to all tests.All three tests in this file share the same pattern of mocking
nativeHypervisordirectly, which bypasses thewrapMethodintegration. This issue was previously flagged in the existing review comment (lines 8-13) for the first test. The same concern applies to all tests here: error handling, promise transformation, and other middleware behavior inwrapMethodis not exercised.
🧹 Nitpick comments (1)
lib/domain.ts (1)
63-69: Consider updating JSDoc to reflect PM wakeup handling.The method now handles both regular resume (for paused domains) and PM wakeup (for PMSUSPENDED domains), but the JSDoc only mentions "Resumes a paused domain." Consider updating the documentation to reflect both behaviors.
🔎 Suggested JSDoc improvement
/** - * Resumes a paused domain. - * This operation restarts execution of a domain that was previously paused. + * Resumes a paused or PM-suspended domain. + * If the domain is in PMSUSPENDED state, wakes it using PM wakeup. + * Otherwise, resumes execution of a domain that was previously paused. * @throws {LibvirtError} If resuming the domain fails */ async resume(): Promise<void> {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/domain.spec.tslib/domain.tslib/hypervisor.spec.tslib/hypervisor.tslib/types.tssrc/hypervisor-domain.cc
🧰 Additional context used
🧬 Code graph analysis (4)
lib/hypervisor.ts (3)
lib/domain.ts (1)
Domain(8-151)lib/types.ts (1)
Domain(139-139)lib/error.ts (1)
wrapMethod(55-61)
lib/domain.spec.ts (2)
lib/domain.ts (1)
Domain(8-151)lib/hypervisor.ts (3)
Hypervisor(13-365)domainPMWakeup(212-217)domainPMSuspend(224-230)
lib/hypervisor.spec.ts (3)
lib/hypervisor.ts (3)
Hypervisor(13-365)domainPMWakeup(212-217)domainPMSuspend(224-230)lib/domain.ts (1)
Domain(8-151)lib/types.ts (1)
Domain(139-139)
src/hypervisor-domain.cc (2)
src/worker.cc (2)
SetVirError(12-17)SetVirError(12-12)src/hypervisor-connect.cc (2)
dummyCallback(15-19)dummyCallback(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker Integration Tests (amd64)
🔇 Additional comments (7)
lib/types.ts (1)
94-101: LGTM!The enum is well-defined and aligns with libvirt's power-management suspend targets.
lib/domain.ts (1)
71-77: LGTM!The method correctly delegates to the hypervisor and provides a sensible default for the suspend target.
lib/domain.spec.ts (1)
9-10: LGTM!The test extensions properly cover the new PM suspend/wakeup functionality:
- Resume path correctly tests both normal resume and PMSUSPENDED wakeup scenarios
- pmSuspend tests validate default MEM target and custom target override
- Mock structure allows flexible per-test behavior overrides
Also applies to: 20-26, 32-33, 48-50, 107-109, 111-138
lib/hypervisor.ts (2)
207-217: LGTM!The method correctly delegates to the native PM wakeup binding and follows the established pattern for domain operations.
219-230: LGTM!The method correctly delegates to the native PM suspend binding with an appropriate default target value.
src/hypervisor-domain.cc (2)
948-986: LGTM! DomainPMWakeup implementation follows established patterns.The worker and N-API binding correctly implement the PM wakeup functionality using
virDomainPMWakeup. The implementation is consistent with other domain lifecycle operations in this file, with proper input validation and error handling.
992-1044: LGTM! DomainPMSuspend implementation with configurable target.The worker and N-API binding correctly implement PM suspend with an optional target parameter. The default
VIR_NODE_SUSPEND_TARGET_MEMis appropriate, and the optional override allows flexibility for different suspend modes (MEM/DISK/HYBRID). Input validation and error handling are consistent with the codebase patterns.
elibosley
deleted the
codex/fix-resume-functionality-for-pmsuspended-vm
branch
Motivation
PMSUSPENDEDstate cannot be resumed via the normal resume path.Description
DomainPMWakeupbinding and worker that callsvirDomainPMWakeupand expose it fromsrc/hypervisor-domain.cc,src/hypervisor.cc, andsrc/hypervisor.h.domainPMWakeupwrapper tolib/hypervisor.tsthat calls the native binding viawrapMethod.Domain.resume()inlib/domain.tsto callgetInfo()and invokedomainPMWakeupwheninfo.state === DomainState.PMSUSPENDED, otherwise call the existingdomainResume.lib/domain.spec.tsto mockdomainGetInfoanddomainPMWakeupand verify both the normal resume path and thePMSUSPENDEDwakeup path.Testing
lib/domain.spec.tsto cover thePMSUSPENDEDwakeup behavior and the normal resume path.Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.