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: Auto-initialize the host paths of shares #316

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
Owner

@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?

nixos-modules/host/default.nix Outdated Show resolved Hide resolved
@astro
Copy link
Owner

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.

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