-
Notifications
You must be signed in to change notification settings - Fork 0
Move stage/publish volumes state to a dedicated local file #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors state management by separating node-local volume state (staging/publishing information) from the global volume state. The LocalVolume struct now contains Staged, Published, Attached, and ReadOnlyAttach fields, while the main Volume struct embeds it. A new local state file is introduced to persist this node-specific data separately.
Key Changes:
- Introduced
LocalVolumestruct and refactoredVolumeto embed it - Added dedicated local state file management with separate persistence
- Updated
state.New()to acceptlocalStatefilePathandnodeIDparameters
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/state/state.go | Implements LocalVolume struct, local state file persistence/restoration, and updates state management to handle two separate state files |
| pkg/state/state_test.go | Updates all test calls to state.New() with new signature including localStatefilePath and nodeID parameters |
| pkg/hostpath/hostpath.go | Adds logic to determine local state file path based on environment variables and passes it to state.New() |
Comments suppressed due to low confidence (1)
pkg/state/state_test.go:51
- The test doesn't verify that the NodeID is correctly set on volumes after reconstruction. Since line 289 in state.go sets NodeID on all volumes during restore, consider adding an assertion to verify that the volume's NodeID matches the expected value.
s, err = New(statefileName, localStatefileName, "node1")
require.NoError(t, err, "reconstruct state")
_, err = s.GetVolumeByID("foo")
require.NoError(t, err, "get existing volume by ID")
_, err = s.GetVolumeByName("bar")
require.NoError(t, err, "get existing volume by name")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.