Skip to content

feat: Auto-initialize the host paths of shares #316

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lordkekz
Copy link

@lordkekz lordkekz commented Dec 28, 2024

Hi, thanks for this amazing project!
I wanted to share a little patch here and hear your opinion if you want to add this kind of feature. I'm looking forward to your feedback!

Motivation

  • Currently microvms will fail if they have a share which doesn't yet exist on the host.
  • If some permissions in the share are not accessible to the microvm:kvm user they will not be accessible for the guest, leading to somewhat surprising errors.
  • I don't want to manually create the shares and ensure they have proper permissions
    • e.g. with this approach I can mount runtime secrets into a VM and have them readable without adding an additional script in my config

Proposed change

  • The install-microvm-... unit creates the directories for shares on the host.
  • The install-microvm-... unit recursively chowns the directories for the configured runner user.
  • Both changes only apply to fully declarative VMs.

Issues / TODO

  • There may be some cases where mkdir is not the desired way to create the share source directories, e.g. if one wants to mount a zfs dataset or btrfs subvolume in the location.
  • There's a risk of permissions of already existing shares being lost. => We could also only initialize the permissions if the directory was newly created by install-microvm

Copy link
Collaborator

@astro astro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a useful improvement. Can anyone think of any host setup that could be broken by this?

@astro
Copy link
Collaborator

astro commented Dec 29, 2024

Oh wait, this PR is only for declarative VMs!

To cover all use-cases, the mkdir must be added to microvm.binScripts.virtiofsd-run in nixos-modules/microvm/virtiofsd/default.nix. I think you won't need any permissions mangling there.

@lordkekz
Copy link
Author

lordkekz commented Feb 11, 2025

Oh wait, this PR is only for declarative VMs!

Ah, sorry for only now getting around to answering. I meant to only create the directories for declarative VMs, but I'm open to also creating them for non-declarative VMs (it's probably even more useful there).

To cover all use-cases, the mkdir must be added to microvm.binScripts.virtiofsd-run in nixos-modules/microvm/virtiofsd/default.nix.

If I move the mkdirs into virtiofsd-run, they'll only run if there is at least one share using virtiofs. If someone uses only 9p shares they'd miss out. Is this acceptable or should I make it a separate script microvm.binScripts.initialize-shares and run that with its own systemd unit before the microvm-virtiofsd@... unit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants