-
-
Notifications
You must be signed in to change notification settings - Fork 139
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
feat(lxc): add support for lxc mount points #394
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
- `replicate` (Boolean) Will include this volume to a storage replica job | ||
- `shared` (Boolean) Mark this non-volume mount point as available on all nodes | ||
- `size` (String) Volume size (read only value) | ||
- `volume` (Required) Volume, device or directory to mount into the container |
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.
TODO: document that directory mounts require that you log in with root@pam
and a password, nothing else works
0f695f7
to
a1df544
Compare
a1df544
to
9eadb48
Compare
Hey @ForsakenHarmony, still going through this pull request... I was checking out Section 11.4.4 of the docs, which covers the different types of mount points supported by PVE. From what I gather, the mount point implemented in this PR seems to be a bind mount point, as mentioned in the original ticket #256. Can you confirm if I got that right? Now, here's the thing with the new attributes: they might confuse or mislead users when they try to define their mounts. The value for "volume" could be a path on a host filesystem, a datastore ID, a reference to an image, a block device on a host. To avoid this confusion, what do you think about having separate attributes for each case? Something like:
With separate attributes, we can add some validation and handling logic specific to each use case. For instance, a datastore_id could require a non-zero size, and the resulting volume should follow the format "datastore_id:size", and so on. Also configure TF validation to allow only one of those attributes per mount. Does this approach make sense to you? And I'm totally fine to focus on supporting only the Oh, and if you could share some examples of the container templates you've been using in your tests, that would be really awesome! :) |
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.
@ForsakenHarmony thanks a lot for the PR!
I think we can leave the mount_point
attributes as-is, and just wait for community feedback :)
I tested bind mounts and including some failure use cases, and made a couple of improvements on top of your code in container creation / startup error handling.
LGTM! 🚀
Contributor's Note
/docs
for any user-facing features or additions./examples
for any new or updated resources / data sources.make examples
to verify that the change works as expected.Community Note
Related to #256