fix: vms now can detect starting of libvirt and start local hypervisor#1356
fix: vms now can detect starting of libvirt and start local hypervisor#1356
Conversation
WalkthroughThis pull request removes deprecated fields and enums in the GraphQL schema and updates related models by adding stricter validations. It refactors the array service to use asynchronous methods for state checking and introduces a new internal enum. The VmsService now implements both initialization and cleanup for file system watching of the PID file. Additionally, redundant utility functions and index files are removed, and the ArrayService test suite is reorganized to enhance error handling and clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant VS as VmsService
participant FW as FileWatcher
participant H as Hypervisor
VS->>VS: onModuleInit()
VS->>FW: attemptHypervisorInitializationAndWatch()
FW->>H: initializeHypervisor()
note over VS,FW: Watcher monitors PID file events
FW->>VS: PID file added/removed event
alt PID file added
VS->>H: initializeHypervisor()
else PID file removed
VS->>H: close hypervisor connection
end
VS->>FW: onModuleDestroy() triggers watcher close
VS->>H: onModuleDestroy() cleans up hypervisor connection
sequenceDiagram
participant C as Client
participant AS as ArrayService
participant S as State Store
C->>AS: updateArrayState(newState)
AS->>AS: async arrayIsRunning()
AS->>S: get current state
S-->>AS: return state
alt Array is running
AS->>C: throw BadRequestException
else Array is not running
AS->>AS: update newPendingState (STARTING/STOPPING)
AS->>C: return updated state
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (2)api/src/unraid-api/graph/resolvers/vms/vms.service.ts (4)
api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (2)
🔇 Additional comments (16)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (2)
75-132: Watcher setup is thorough.
File events are clearly handled, though consider potential load if the file changes frequently.
376-384: Enhanced error differentiation is helpful.
Logging a specific message for connection failures vs. other errors can improve troubleshooting. Consider custom error types for further clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/generated-schema.graphql(1 hunks)api/src/unraid-api/graph/resolvers/array/array.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
api/src/unraid-api/graph/resolvers/array/array.model.ts (1)
web/composables/gql/graphql.ts (1)
ArrayStateInput(252-255)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1)
api/src/store/index.ts (1)
getters(18-29)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
api/generated-schema.graphql (1)
1424-1424:Details
✅ Verification successful
Ensure a proper resolver for this new field.
A resolver must be implemented or updated so clients can actually query thehealthfield.
🏁 Script executed:
#!/bin/bash # Checking where the 'health' field is handled in resolvers rg -A 5 'health'Length of output: 36640
Health Field Resolver Confirmed
The new
health: String!field inapi/generated-schema.graphqlis properly backed by resolvers. We found resolver implementations in both:
packages/unraid-api-plugin-health/index.ts– returns'OK'packages/unraid-api-plugin-connect/index.ts– returns"I am healthy!"No further changes are required.
api/src/unraid-api/graph/resolvers/array/array.model.ts (2)
3-4: Import statement looks good.
This addition properly includes theclass-validatordependency for enum validation with no issues.
185-190: Enum validation ondesiredStateis beneficial.
This guarantees only valid states are accepted, avoiding invalid transitions.api/src/unraid-api/graph/resolvers/vms/vms.service.ts (6)
1-1: Implementation ofOnModuleDestroyis a great addition.
Resource cleanup on module destruction helps prevent leaks.Also applies to: 14-14
7-7: Chokidar import is appropriate.
Usingchokidaris a reliable way to watch for file changes across platforms.
43-44: Graceful teardown inonModuleDestroyis well-handled.
Ensuring the watcher and hypervisor connections are closed avoids dangling resources.Also applies to: 46-58
60-60: Fallback connection logic is good.
Attempting hypervisor init and then setting up the watcher ensures robust startup behavior, even if the initial connection fails.Also applies to: 67-67, 69-71, 73-73
135-138: Hypervisor re-initialization check is effective.
The logic avoids redundant connections by respectingisVmsAvailable.Also applies to: 140-140, 145-145, 148-148
18-20:Details
✅ Verification successful
Validate fallback path usage for
pidPath.
Ifgetters.paths()?.['libvirt-pid']is undefined, confirm the fallback/var/run/libvirt/libvirtd.pidis correct for target environments.Also applies to: 24-25, 34-34
🏁 Script executed:
#!/bin/bash # Verify references to libvirtd.pid fallback rg -A3 'libvirt-pid'Length of output: 858
Validated
pidPathFallback ConfigurationThe fallback path
/var/run/libvirt/libvirtd.pidis consistently used and correctly configured:
- In
api/src/unraid-api/graph/resolvers/vms/vms.service.ts, the fallback is applied via the nullish coalescing operator.- In
api/src/store/modules/paths.ts, the constant value is defined as expected.No further action is required.
elibosley
force-pushed
the
fix/vms-watcher-for-pid
branch
from
afac350 to
851ccb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/generated-schema.graphql(1 hunks)api/src/unraid-api/graph/resolvers/array/array.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/generated-schema.graphql
- api/src/unraid-api/graph/resolvers/array/array.model.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1)
api/src/store/index.ts (1)
getters(18-29)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (13)
1-1: Excellent implementation of the OnModuleDestroy lifecycle hookThe addition of the OnModuleDestroy interface and required watcher property provides proper lifecycle management for the VmsService class, ensuring resources are cleaned up when the module is destroyed.
Also applies to: 14-14, 18-18, 20-20
7-7: Good addition of file system watching capabilitiesAdding the chokidar library for file system watching is appropriate for monitoring the libvirt PID file changes.
24-25: Appropriate path initialization and loggingGood practice to initialize the pidPath in the constructor and log its value for debugging purposes.
34-34: Improved path referenceUsing the class property directly rather than accessing through getters.paths() each time is more efficient.
42-44: Clear initialization sequenceThe modified onModuleInit method provides better logging and delegates to the new specialized initialization method.
46-58: Well-implemented resource cleanupThe onModuleDestroy method properly handles the cleanup of resources, with good error handling for the hypervisor connection close operation.
60-73: Good separation of concernsThis method effectively separates the hypervisor initialization and watcher setup, with appropriate error handling.
92-109: Good PID file detection handlingThe 'add' event handler correctly attempts to initialize the hypervisor when the PID file is detected, with appropriate error handling.
110-126: Robust PID file removal handlingThe 'unlink' event handler properly handles the case when the PID file is removed, indicating that the hypervisor has stopped.
127-131: Comprehensive error handlingThe error event handler ensures that any watcher errors are properly logged.
135-138: Efficient hypervisor initialization checkGood practice to check if the hypervisor is already active before attempting to initialize it again.
140-149: Improved hypervisor creation logicThe refined hypervisor creation logic with better logging improves the code's readability and maintainability.
376-384: Enhanced error differentiationThe improved error handling in getDomains method differentiates between connection issues and other errors, providing more specific error messages.
|
Tested this on my server: |
elibosley
force-pushed
the
fix/vms-watcher-for-pid
branch
from
519f1c1 to
9a63897
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/array/array.model.ts (1)
176-182: Consider test coverage for the newArrayStateInputclass.
Adding tests to verify the enum validation behavior would help you catch unexpected input values and guarantee that the intended states are enforced at runtime.api/src/unraid-api/graph/resolvers/array/array.service.ts (2)
37-41: Question the need for a separategetArrayStatemethod.
SincearrayIsRunningalready fetches and checks the state, consider whether you require this function’s additional abstraction to reduce duplication.
51-51: Refine or clarify the exception message.
Including the pending state in the error is helpful, but consider clarifying it so users clearly understand which state is currently preventing updates vs. which state they want.api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1)
60-73: Graceful fallback if hypervisor initialization fails.
After the first attempt fails, you still set up the watcher. Consider whether a small retry loop might reduce logs or shorten downtime if libvirt starts a bit late.api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (1)
45-52: Well-structured test setupThe beforeEach block properly resets all mocks, initializes them with appropriate return values, and sets up the test environment. The mock data structure aligns with the UnraidArray type.
However, line 89 contains a comment about mocking state if needed, but no actual state is defined.
mockGetState.mockReturnValue({ - /* mock state if needed by getArrayDataUtil */ + // Return mock state structure needed by getArrayDataUtil + array: { + // Add relevant state properties here + } });Also applies to: 60-61, 81-81, 86-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/generated-schema.graphql(1 hunks)api/src/core/utils/array/array-is-running.ts(0 hunks)api/src/core/utils/array/index.ts(0 hunks)api/src/core/utils/index.ts(0 hunks)api/src/unraid-api/graph/resolvers/array/array.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/array/array.resolver.ts(0 hunks)api/src/unraid-api/graph/resolvers/array/array.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/array/array.service.ts(7 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.ts(2 hunks)
💤 Files with no reviewable changes (4)
- api/src/core/utils/array/array-is-running.ts
- api/src/core/utils/index.ts
- api/src/core/utils/array/index.ts
- api/src/unraid-api/graph/resolvers/array/array.resolver.ts
✅ Files skipped from review due to trivial changes (1)
- api/dev/Unraid.net/myservers.cfg
🚧 Files skipped from review as they are similar to previous changes (1)
- api/generated-schema.graphql
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/src/unraid-api/graph/resolvers/array/array.model.ts (1)
web/composables/gql/graphql.ts (1)
ArrayStateInput(252-255)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (2)
api/src/unraid-api/auth/api-key.service.ts (1)
Injectable(27-377)api/src/store/index.ts (1)
getters(18-29)
api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (2)
web/composables/gql/graphql.ts (3)
UnraidArray(1350-1369)ArrayStateInput(252-255)ArrayDiskInput(159-164)api/src/core/utils/clients/emcmd.ts (1)
emcmd(12-41)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (22)
api/src/unraid-api/graph/resolvers/array/array.model.ts (1)
3-4: Use ofIsEnumappears appropriate.
This import correctly enables validation for the enum field. Ensure thatclass-validatoris included in your runtime dependencies to avoid runtime errors.api/src/unraid-api/graph/resolvers/array/array.service.ts (7)
18-21: EnumArrayPendingStateintegration looks consistent.
No specific issues here. This addition cleanly captures the transitional states.
31-35: AsynchronousarrayIsRunningis a neat improvement.
Fetching the array state from the store asynchronously helps avoid potential blocking. Good move.
43-46: EnsuregetArrayDatahandles errors consistently.
Switching to an async method seems appropriate. IfgetArrayDataUtilrejects, ensure you handle it (or let it bubble up) in a predictable manner.
54-55: Local state determination is streamlined.
PreparingstartStateby referencingisRunningis a clean approach. Ensure test coverage includes both scenarios to confirm correct branching.
62-66: Early exit for same-state requests looks good.
Blocking redundant start/stop requests protects from unintended operations. Nicely done.
71-71: SettingpendingStateprior to the operation is wise.
This ensures concurrency conflicts are minimized. Remain mindful of parallel requests that might overwrite this state unexpectedly.
90-90: Consistency in disallowing disk operations while running.
ThrowingArrayRunningErrorif the array is running keeps your domain logic coherent. Just verify that this is indeed the intended restriction in all these methods.Also applies to: 107-107, 124-124, 138-138, 152-152
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (7)
1-1: Adoption ofOnModuleDestroy,FSWatcher, and storingpidPathis well-structured.
The fallback default path forlibvirt-pidensures the service remains robust even if not explicitly configured.Also applies to: 7-7, 14-14, 18-18, 20-20, 24-25
34-34: Ensuring read access to the PID file.
This check makes sense for confirming libvirt is running. Handling partial permission issues may be overkill, so no immediate concerns.
42-44: CallingattemptHypervisorInitializationAndWatchon module init.
A single entry point for initialization plus watch setup is tidy. Relying on subsequent watch triggers if the initial connect fails is a sensible fallback.
46-58: Thorough cleanup inonModuleDestroy.
You properly close the watcher and hypervisor, safely handling potential errors. This ensures resources are released.
75-128:setupWatcherelegantly handles PID addition and removal events.
You correctly toggleisVmsAvailableand attempt to reconnect when needed. The approach looks solid for ephemeral service restarts.
131-161:initializeHypervisormethod is well-structured.
You confirm the PID file presence, create a hypervisor, and attempt to connect. This flow, combined with error-handling, is straightforward.
372-380: Improved error handling for domain lookups.
Logging distinct connection issues vs. general failures simplifies debugging. Good detail here.api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (7)
2-2: Import changes look well-structuredThe added imports introduce necessary error types (BadRequestException, ArrayRunningError) and properly rename the
getArrayDataimport togetArrayDataUtilto avoid naming conflicts with the service method. The UnraidArray type is also correctly imported to provide proper typing for the test data.Also applies to: 7-8, 15-15
23-25: Good mock setup with proper typingThe test file properly mocks all external dependencies including the array data utility and store functions. The variable declarations use proper TypeScript return types instead of
any, which improves type safety throughout the tests.Also applies to: 31-33, 38-42
97-104: Good test for getArrayData methodThis test properly verifies that the service method calls the utility function with the right arguments and returns its result.
106-152: Comprehensive tests for updateArrayState methodThe tests cover all important scenarios: starting a stopped array, stopping a started array, and attempting invalid state transitions. The error handling tests verify both the exception type and that no commands are executed.
154-172: Complete tests for addDiskToArrayTests cover both the success case when the array is stopped and the error case when trying to add a disk to a running array. The verification of the emcmd call parameters ensures the command is constructed correctly.
174-192: Good test coverage for removeDiskFromArraySimilar to the add disk tests, this section correctly verifies both success and failure scenarios with appropriate assertions.
194-214: Well-structured tests for disk operationsThe mountArrayDisk, unmountArrayDisk, and clearArrayDiskStatistics tests all follow a consistent pattern of testing both success and failure cases. Each test verifies the correct command parameters and error handling behavior.
Also applies to: 216-236, 238-258
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.7.0](v4.6.6...v4.7.0) (2025-04-24) ### Features * add basic docker network listing ([#1317](#1317)) ([c4fdff8](c4fdff8)) * add permission documentation by using a custom decorator ([#1355](#1355)) ([45ecab6](45ecab6)) * basic vm controls ([#1293](#1293)) ([bc3ca92](bc3ca92)) * code first graphql ([#1347](#1347)) ([f5724ab](f5724ab)) ### Bug Fixes * container names always null ([#1335](#1335)) ([8a5b238](8a5b238)) * **deps:** update all non-major dependencies ([#1337](#1337)) ([2345732](2345732)) * hide reboot notice for patch releases ([#1341](#1341)) ([4b57439](4b57439)) * move docker mutations to the mutations resolver ([#1333](#1333)) ([1bbe7d2](1bbe7d2)) * PR build issue ([457d338](457d338)) * remove some unused fields from the report object ([#1342](#1342)) ([cd323ac](cd323ac)) * sso unreliable if API outputs more than raw json ([#1353](#1353)) ([e65775f](e65775f)) * vms now can detect starting of libvirt and start local hypervisor ([#1356](#1356)) ([ad0f4c8](ad0f4c8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor