Skip to content

Commit

Permalink
[antlir2] Fix nspawn subtarget mount handling
Browse files Browse the repository at this point in the history
Summary:
This was a tricky failure to track down, and I don't love the solution so am open to other ideas, but TL;DR:

antlir2_isolate takes a collection of "inputs" and converts them to escpaed `--bind-ro` CLI args (where `:` is significant). We thus can't escape the mount arg from this wrapper binary as we're doing today.

Some options I'd considered:
- Make antlir2_isolate also take "already-escaped" inputs - could work, but would require a fair amount of extra boilerplate for all the `Into<_>` conversions, and complicates the API
- Make the wrapper binary parse its mount args in the same way that systemd-nspawn does (i.e. using optional `:` as delimiter, allowing escaped colons) - requires a fair amount of implementation complexity
- Do what was done here and just explicitly allow space-separated SRC -> DST bind mount mapping arg

I opted to do the latter as it's by far the simplest and matches all known input types to the lib today, but open to ideas.

Test Plan:
For regressions:
```
$ buck2 run "//os_foundation/images/impl/v2/centos8:base.os[nspawn]"
...
[root@subvol-compilecompile-448a91e009bf978a ~]# ls /
bin  boot  data  dev  etc  home  lib  lib64  media  mnt  opt  proc  root  run  sbin  srv  sys  tmp  usr  var
```

also tested in above diffs in stack using src -> dst mounts.

Reviewed By: vmagro

Differential Revision: D47533620

fbshipit-source-id: 4be3f4887f05a10f8b1664a24ef147933bfad886
  • Loading branch information
justintrudell authored and facebook-github-bot committed Jul 20, 2023
1 parent 24103f7 commit d28539c
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 13 deletions.
2 changes: 1 addition & 1 deletion antlir/antlir2/bzl/image/layer.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def _nspawn_sub_target(nspawn_binary: "dependency", subvol: "artifact", mounts:
if REPO_CFG.artifacts_require_repo:
dev_mode_args = cmd_args(
"--artifacts-require-repo",
cmd_args([["--bind-ro", x] for x in REPO_CFG.host_mounts_for_repo_artifacts]),
cmd_args([cmd_args("--bind-mount-ro", p, p) for p in REPO_CFG.host_mounts_for_repo_artifacts]),
)
return [
DefaultInfo(),
Expand Down
10 changes: 2 additions & 8 deletions antlir/antlir2/bzl/image/mounts.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,8 @@ def all_mounts(

def nspawn_mount_args(mount: mount_record.type) -> cmd_args.type:
if mount.layer:
return cmd_args(
"--bind-ro",
cmd_args(mount.layer.src.subvol_symlink, format = "{{}}:{}".format(mount.layer.mountpoint.replace(":", "\\:"))),
)
return cmd_args("--bind-mount-ro", mount.layer.src.subvol_symlink, mount.layer.mountpoint)
elif mount.host:
return cmd_args(
"--bind-ro",
mount.host.src + ":" + mount.host.mountpoint.replace(":", "\\:"),
)
return cmd_args("--bind-mount-ro", mount.host.src, mount.host.mountpoint)
else:
fail("neither host nor layer mount, what is it?!")
21 changes: 17 additions & 4 deletions antlir/antlir2/nspawn_in_subvol/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
* LICENSE file in the root directory of this source tree.
*/

use std::collections::HashSet;
use std::collections::HashMap;
use std::os::unix::process::CommandExt;
use std::path::PathBuf;

use antlir2_isolate::isolate;
use antlir2_isolate::InvocationType;
use antlir2_isolate::IsolationContext;
use anyhow::anyhow;
use anyhow::Context;
use clap::Parser;
use tracing_subscriber::filter;
Expand All @@ -22,8 +23,9 @@ use tracing_subscriber::prelude::*;
struct Args {
#[clap(long)]
subvol: PathBuf,
#[clap(long)]
bind_ro: Vec<PathBuf>,
/// `--bind-mount-ro src dst` creates an RO bind-mount of src to dst in the subvol
#[clap(long, num_args = 2)]
bind_mount_ro: Vec<PathBuf>,
#[clap(long)]
artifacts_require_repo: bool,
}
Expand All @@ -50,9 +52,20 @@ fn main() -> anyhow::Result<()> {
)
.context("while looking for repo root")?;

// antlir2_isolate re-parses these into --bind-ro args and escapes any colons, so we
// instead take an explicit pair to not have to deal with the added complexity of
// de-and-re-serializing.
let bind_ro_inputs = args
.bind_mount_ro
.chunks(2)
.map(|pair| match pair {
[src, dst] => Ok((dst.clone(), src.clone())),
_ => Err(anyhow!("Unrecognized --mount arg: {:?}", pair)),
})
.collect::<anyhow::Result<HashMap<_, _>>>()?;
let mut cmd_builder = IsolationContext::builder(args.subvol);
cmd_builder
.inputs(args.bind_ro.into_iter().collect::<HashSet<_>>())
.inputs(bind_ro_inputs)
.ephemeral(true)
.invocation_type(InvocationType::Pid2Interactive);
if args.artifacts_require_repo {
Expand Down

0 comments on commit d28539c

Please sign in to comment.