feat(volumes): Add volume 'access_mode' flag#341
Conversation
Also adapt fields to new formats. Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
This allows for mounting volumes in multiple instances. The access mode can't be edited after creation, as such there is no support for patching. Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
There was a problem hiding this comment.
Pull request overview
Adds support for configuring a volume’s access mode at creation time (to enable multi-instance mounting scenarios) and updates CLI code to match recent SDK signature/type changes.
Changes:
- Introduce
types.AccessMode(validated wrapper overplatform.VolumeAccessMode) and wire--access-modeintounikraft volume create. - Include
access-modein volume field mirroring/output/help fixtures and add integration coverage forrwo/rox. - Update several SDK callsites for breaking SDK changes (e.g.,
DestinationPortand logtailnow pointers) and bumpunikraft.com/cloud/sdk.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/volimport/volimport.go | Update service port mapping to use pointer DestinationPort per SDK change. |
| internal/types/states.go | Add types.AccessMode with validation for allowed volume access modes. |
| internal/cmd/volumes.go | Add --access-mode shortcut flag and map it into CreateVolumeRequest.AccessMode; update examples. |
| internal/cmd/testdata/TestOutput/volumes | Update expected output/field metadata to include access-mode. |
| internal/cmd/services.go | Update DestinationPort usage to pointer per SDK change. |
| internal/cmd/run.go | Update instance log reader call to pass tail as a pointer per SDK change. |
| internal/cmd/instances.go | Update DestinationPort and change instance logs --tail flag type to *int for optionality. |
| internal/cmd/instances_tui.go | Update TUI log reader call for new tail pointer signature (currently contains a compile issue). |
| go.mod / go.sum | Bump unikraft.com/cloud/sdk dependency. |
| cmd/unikraft/testdata/TestHelp/volumes | Update help golden file to include access-mode flag/field. |
| cmd/unikraft/integration/volume_test.go | Add integration tests validating access-mode behavior (rwo, rox). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
Looks mostly good, left some comments. Once fixed, we can merge after the feature freeze is over.
| filesystem: ext4 | ||
| quota-policy: hard | ||
| persistent: true | ||
| access-mode: |
There was a problem hiding this comment.
Hm, why empty? Shouldn't the struct be updated so the gen of this is correct?
There was a problem hiding this comment.
Good question, need to recheck, I think I'm setting it to empty in the request to make the platform decide
There was a problem hiding this comment.
nit: just noticed that this golden file (and maybe others) don't have the right linguist-generated gitattributes.
There was a problem hiding this comment.
what the hyok is that? 👀
| --size 10 \ | ||
| --metro fra`, | ||
| --metro fra \ | ||
| --access-mode rwo`, |
There was a problem hiding this comment.
No need to add the access-mode to this specific example, IMO, it should just be a very basic example.
This allows for mounting volumes in multiple instances.
The access mode can't be edited after creation, as such
there is no support for patching.
Closes: TOOL-826