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

nixos/qemu-vm: Option to use squashfs Nix store closure instead of virtfs access to host's store #72354

Closed
wants to merge 3 commits into from

Conversation

chkno
Copy link
Member

@chkno chkno commented Oct 31, 2019

Motivation for this change

Allow VMs built with nixos-rebuild build-vm to work without any virtfs mounts into the host. In particular, allow them to use a nix store closure in a squashfs like the installer uses.

This allows better isolation of guests. A host that runs multiple VMs may not wish the guests to be able to see each others' derivations.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review rev --branch staging HEAD" -- It's zero packages, but this refactoring touches all the NixOS tests. I ran a handful of existing ones.
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after) -- N/A?
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@chkno
Copy link
Member Author

chkno commented Nov 1, 2019

(Rebased on top of #72342 so the NixOS tests can run)

@chkno
Copy link
Member Author

chkno commented Nov 1, 2019

@GrahamcOfBorg test qemu-private-store

@matthewbauer
Copy link
Member

@GrahamcOfBorg test qemu-private-store

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

This all looks like a good idea, though I don't have enough familiarity to really judge whether this breaks anything. Requested a few more people to review.

@chkno
Copy link
Member Author

chkno commented Jan 29, 2020

Updated to rebase on top of #55645. I confirm that login, upnp, installer, virtualbox, and the new test pass on x86_64-linux.

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

Looks good!

@chkno
Copy link
Member Author

chkno commented Mar 19, 2020

Updated test from perl test framework to python test framework.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/157

Comment on lines 311 to 364
virtualisation.shareExchangeDir =
mkOption {
default = true;
type = types.bool;
description =
''
Share /tmp/xchg with the host.
'';
};

virtualisation.shareSharedDir =
mkOption {
default = true;
type = types.bool;
description =
''
Share /tmp/shared with the host.
'';
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference of these?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know.

By default they point at the same directory in the host. The test framework points them at different directories by setting SHARED_DIR.

History:

  • Aug 2011 d75efe4 edolstra: Introduced /tmp/xchg: "For security, don't mount the entire host filesystem."
  • Aug 2012 a025e7e edolstra: Introduced /tmp/shared: "Provide a common share between VMs to allow easy communication"
  • May 2020 d85dc4f erikarvstedt: Mentions "xchg is advertised as a bidirectional exchange dir."

Current uses I could find:

Mount Purpose Location
xchg coverage-data nixos/modules/testing/test-instrumentation.nix nixos/lib/test-driver/test-driver.pl
xchg runInMachine nixos/lib/testing.nix
shared copy_from_host copy_from_vm nixos/lib/test-driver/test-driver.py

I just want a lever to turn them off. I guess it makes more sense to be one lever rather than two.

I combined these two options into one and added a note about what these directories are currently used for in the option description.

Copy link
Contributor

Choose a reason for hiding this comment

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

@erikarvstedt @edolstra can you provide some insight here?

Copy link
Member

Choose a reason for hiding this comment

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

We can probably get rid of coverage-data. AFAIK we're not doing coverage runs in VMs anymore (and IIRC it was only used in some toy examples like the Quake 3 test anyway).

Copy link
Member

Choose a reason for hiding this comment

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

We can probably also eliminate copy_from_* since it's only used in 3 places, which can be replaced by machine.execute("cat ...").

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably get rid of coverage-data. AFAIK we're not doing coverage runs in VMs anymore (and IIRC it was only used in some toy examples like the Quake 3 test anyway).

I updated the list in #72828, and filed #97231 removing the remaining coverage-data references - PTAL.

@flokli
Copy link
Contributor

flokli commented Jul 6, 2020

This also needs a rebase.

@flokli flokli requested a review from nh2 July 6, 2020 18:50
chkno added 3 commits July 6, 2020 13:36
When the store is not shared, the closure used in the VM it is packaged
up in a squashfs, just like the ISO installer does.
@chkno chkno changed the base branch from staging to master July 6, 2020 20:37
@chkno
Copy link
Member Author

chkno commented Jul 6, 2020

Rebased on top of #92205, #92122, and #90280.

Comment on lines +361 to +362
Share /tmp/shared and /tmp/xchg with the host. These are used
for runInMachine, copy_from_{host,vm}, and coverage-data.
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to use <literal> markup for easier reading, e.g.

Suggested change
Share /tmp/shared and /tmp/xchg with the host. These are used
for runInMachine, copy_from_{host,vm}, and coverage-data.
Share <literal>/tmp/shared</literal> and <literal>/tmp/xchg</literal> with the host.
These are used for <literal>runInMachine</literal>, <literal>copy_from_{host,vm}</literal>,
and <literal>coverage-data</literal>.

(Please check the manual builds after this, one can accidentlaly screw it up like I did recently.)

Copy link
Member

Choose a reason for hiding this comment

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

@nh2: ... or go even further and use <filename/> and <function/>:

Suggested change
Share /tmp/shared and /tmp/xchg with the host. These are used
for runInMachine, copy_from_{host,vm}, and coverage-data.
Share <filename class="directory">/tmp/shared</filename> and
<filename class="directory">/tmp/xchg</filename> with the host.
These are used for <function>runInMachine</function>,
<literal>copy_from_{host,vm}</literal>, and
<literal>coverage-data</literal>.

this makes building the VM much faster requires less disk
space. Disable to give the guest a separate, fresh store
in a squashfs that contains only the closure of the guest's
dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording that contains only the closure of the guest's dependencies is a bit misleading, because it may be understood as that if you shareNixStore = true, then all of the hosts nix store is shared with the guest:

Even if you share the nix store with the host, only the closure of the guest's dependencies are "available" in the nix store; all the store paths are present as files in /nix/store, but most of those are not valid (registered in the sqlite DB).

This recently tricked me: https://discourse.nixos.org/t/how-to-build-a-nixos-vm-with-nix-in-which-nixos-rebuild-is-a-no-op/7937

Because of this ambiguity, I think we should add an explicit:

Suggested change
dependencies.
dependencies.
Note that even with <literal>virtualisation.shareNixStore = true;</literal>,
not the entire host Nix store is available in the guest Nix store:
While all <literal>/nix/store</literal> files will be present, only the ones
pulled in by <option>virtualisation.pathsInNixDB</option>
will be registered as valid in the Nix store database.

appear in the guest Nix store as well, but are considered
garbage (because they are not registered in the Nix
database in the guest).
database in the VM. If virtualisation.shareNixStore is true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
database in the VM. If virtualisation.shareNixStore is true,
database in the VM. If <option>virtualisation.shareNixStore</option> is true,

shared.succeed("[[ $(mount | grep -c virt) -gt 0 ]]")
private.succeed("[[ $(mount | grep -c virt) -eq 0 ]]")
'';
})
Copy link
Contributor

Choose a reason for hiding this comment

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

New tests are great!

Optionally, maybe also add a copy of this test that exercises the default virtualisation.shareNixStore = false;?

flokli added a commit to flokli/nixpkgs that referenced this pull request Sep 5, 2020
@stale
Copy link

stale bot commented Mar 31, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 31, 2021
r2r-dev added a commit to r2r-dev/nixos-shell that referenced this pull request May 12, 2021
@chkno
Copy link
Member Author

chkno commented Oct 25, 2021

Since ec6c604 from #127933 , this can be accomplished via configuration; it doesn't need to be explicitly supported in the qemu-vm module. Thanks, @rnhmjoj!

The necessary configuration is now available at https://github.com/chkno/nixos-qemu-vm-isolation/blob/main/modules/qemu-vm-isolation.nix , which can be used as an example or directly as a library for folks that want this feature.

@chkno chkno closed this Oct 25, 2021
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Oct 25, 2021

Nice! It's good to know my cleanup has been useful.

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

Successfully merging this pull request may close these issues.

None yet

8 participants