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

Host-to-guest storage module #656

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

mbssrc
Copy link
Collaborator

@mbssrc mbssrc commented Jun 11, 2024

Description of changes

  • System-wide service to configure shares for guest vms using virtiofs
  • Persistent fingerprint storage configured for gui-vm
  • Preparation for other services using this feature

Checklist for things done

  • Summary of the proposed changes in the PR description
  • More detailed description in the commit message(s)
  • Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
  • Contribution guidelines followed
  • Ghaf documentation updated with the commit - https://tiiuae.github.io/ghaf/
  • PR linked to architecture documentation and requirement(s) (ticket id)
  • Test procedure described (or includes tests). Select one or more:
    • Tested on Lenovo X1 x86_64
    • Tested on Jetson Orin NX or AGX aarch64
    • Tested on Polarfire riscv64
  • Author has run nix flake check --accept-flake-config and it passes
  • All automatic Github Action checks pass - see actions
  • Author has added reviewers and removed PR draft status

Testing

- System-wide service to configure shares for guest vms using virtiofs
- Persistent fingerprint storage configured for gui-vm
- Preparation for other services using this feature

Signed-off-by: Manuel Bluhm <manuel@ssrc.tii.ae>
if [ ! -d "${vmShare.vm-path}" ]; then
mkdir -p ${vmShare.vm-path}
fi
${optionalString (vmShare.tmp-path != null) "cp -r ${vmShare.tmp-path}/* ${vmShare.vm-path}"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way I understand the semantic, I would expect the previous content to be removed.
Maybe using rsync with --delete would be than a better choice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rsync, also would have an option to directly apply the right owner/group without an additional chown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea with this was that the host data remains present. This 'temporary' mountpoint option is intended for the use case where one wants to copy data from the host on VM boot, and apply specific permissions without changing the host permissions or rely on some UID mapping. Deletion could be added as an option as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was also looking into using the impermanence module instead of this script. At the moment, I am waiting for the storage vm implementation to see how to finally implement this

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I know, impermanence doesn't provide copy semantics.

with the default owner 'microvm' and group 'kvm'.
'';
};
tmp-path = mkOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tmp-path = mkOption {
tmp-mountpoint = mkOption {

default = null;
description = ''
(Optional) Temporary directory in the VM to mount the host directory. If this is set,
the folder from host-path will be recursively copied from 'tmp-path' to 'vm-path' and owner, group,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
the folder from host-path will be recursively copied from 'tmp-path' to 'vm-path' and owner, group,
the folder from host-path will be recursively copied from 'tmp-mountpoint' to 'vm-path' and owner, group,

vm-path = mkOption {
type = types.str;
description = ''
Storage directory in the guest where the share is mounted to.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Storage directory in the guest where the share is mounted to.
Storage directory in the guest where the share is mounted or where the data is copied to if `tmp-mountpoint` is set.

type = types.str;
description = ''
Name of the VM the share is mounted into. The name must match the microvm
name in 'config.microvm.vms': e.g., "gui-vm" (note the '-').
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use assertions = [{}]; nixos option and config.microvm.vms ? ${target-vm} condition here to test if the microvm actually exists to check for misconfiguration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes will add assertions later if this is going to be used

Restart = "no";
StandardOutput = "journal";
StandardError = "journal";
ExecStart = "${prepStorage}/bin/storage_prep";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this will be re-runs whenever a new NixOS is deployed. Might be a bit disruptive if a VM gets part of the state wiped without when it's already running.
But I also don't know if life-updates is a requirement here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point - my assumption was that the VM service is restarted, but as we see with netvm now this may not necessary be the case. Adding restartIfChanged = false; should do the trick?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. restartIfChanged = false; would solve this.

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