Skip to content
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

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

ForsakenHarmony
Copy link
Contributor

@ForsakenHarmony ForsakenHarmony commented Jun 29, 2023

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated templates in /examples for any new or updated resources / data sources.
  • I have ran make examples to verify that the change works as expected.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Related to #256

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Jun 29, 2023

🤖 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

- `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
Copy link
Contributor Author

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

@ForsakenHarmony ForsakenHarmony force-pushed the hrmny/lxc-mount-points branch 2 times, most recently from 0f695f7 to a1df544 Compare July 5, 2023 23:48
@bpg
Copy link
Owner

bpg commented Jul 6, 2023

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:

  • directory: for example /mnt/bindmounts/shared
  • datastore_id: for example local-lvm
  • file_id: for example local:iso/bionic-server-cloudimg-amd64.img
  • block_device: for example /dev/sde1

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 directory (bind mount) in this PR and save the other cases for future updates.

Oh, and if you could share some examples of the container templates you've been using in your tests, that would be really awesome! :)

Copy link
Owner

@bpg bpg left a 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! 🚀

@bpg bpg merged commit beef9b1 into bpg:main Jul 17, 2023
@ghost ghost mentioned this pull request Jul 17, 2023
@bpg bpg mentioned this pull request Jul 17, 2023
@ForsakenHarmony ForsakenHarmony deleted the hrmny/lxc-mount-points branch July 17, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants