QEMU backend #195

mtelvers · 2024-10-19T20:13:18Z

This PR adds a QEMU sandbox which allows testing in Windows (and Linux) virtual machines.

MisterDA

Really cool work!
I would refactor the Makefile to put version numbers into variables to ease updating the Makefile, if needed.

MisterDA · 2024-10-20T12:09:03Z

lib/store_spec.ml

@@ -60,7 +66,7 @@ let store_t = Arg.conv (of_string, pp)
 let store ?docs names =
  Arg.opt Arg.(some store_t) None @@
  Arg.info
-    ~doc:"$(docv) must be one of $(b,btrfs:/path), $(b,rsync:/path), $(b,xfs:/path), $(b,overlayfs:/path), $(b,zfs:pool) or $(b,docker:path) for the OBuilder cache."
+    ~doc:"$(docv) must be one of $(b,btrfs:/path), $(b,rsync:/path), $(b,xfs:/path), $(b,overlayfs:/path), $(b,zfs:pool), $(b,qmu:/path) or $(b,docker:path) for the OBuilder cache."


Suggested change

~doc:"$(docv) must be one of $(b,btrfs:/path), $(b,rsync:/path), $(b,xfs:/path), $(b,overlayfs:/path), $(b,zfs:pool), $(b,qmu:/path) or $(b,docker:path) for the OBuilder cache."

~doc:"$(docv) must be one of $(b,btrfs:/path), $(b,rsync:/path), $(b,xfs:/path), $(b,overlayfs:/path), $(b,zfs:pool), $(b,qemu:/path) or $(b,docker:path) for the OBuilder cache."

samoht · 2024-10-23T20:38:10Z

doc/qemu.md

+```
+
+Moving on to the next stage in the build which is the `run` directive.
+First, `qemu-img` creates a snapshot of the current `resuilt` layer into


Suggested change

First, `qemu-img` creates a snapshot of the current `resuilt` layer into

First, `qemu-img` creates a snapshot of the current `result` layer into

samoht · 2024-10-23T20:45:09Z

This is pretty cool! If you install Docker in your base image, you could even have obuilder-in-obuilder working :p

* Added RISCV64 support * Added OpenBSD support

MisterDA · 2024-11-07T16:40:07Z

qemu/Makefile

@@ -1,12 +1,18 @@

 clean:
-	rm -f *.qcow2
+	rm -f *.qcow2 *.iso


Suggested change

rm -f *.qcow2 *.iso

rm -f ./*.qcow2 ./*.iso

MisterDA · 2024-11-07T16:40:50Z

qemu/Makefile

+# busybox
+
+seed.iso: busybox.yaml
+	cp $< user-data.yaml


Suggested change

cp $< user-data.yaml

cp -p $< user-data.yaml

MisterDA · 2024-11-07T16:41:55Z

qemu/Makefile

+	tar -czf mirror/pub/OpenBSD/7.6/amd64/site76.tgz install.site
+	for f in BUILDINFO SHA256.sig base76.tgz bsd bsd.mp bsd.rd comp76.tgz game76.tgz man76.tgz pxeboot xbase76.tgz xfont76.tgz xserv76.tgz xshare76.tgz ; do curl -C - -L https://cdn.openbsd.org/pub/OpenBSD/7.6/amd64/$$f -o mirror/pub/OpenBSD/7.6/amd64/$$f ; done
+	cd mirror/pub/OpenBSD/7.6/amd64 && ls -l > index.txt
+	cp install.conf disklabel mirror


Suggested change

cp install.conf disklabel mirror

cp -p install.conf disklabel mirror

MisterDA · 2024-11-07T17:30:05Z

qemu/Makefile

+	mkdir -p mirror/pub/OpenBSD/7.6/amd64
+	tar -czf mirror/pub/OpenBSD/7.6/amd64/site76.tgz install.site
+	for f in BUILDINFO SHA256.sig base76.tgz bsd bsd.mp bsd.rd comp76.tgz game76.tgz man76.tgz pxeboot xbase76.tgz xfont76.tgz xserv76.tgz xshare76.tgz ; do curl -C - -L https://cdn.openbsd.org/pub/OpenBSD/7.6/amd64/$$f -o mirror/pub/OpenBSD/7.6/amd64/$$f ; done
+	cd mirror/pub/OpenBSD/7.6/amd64 && ls -l > index.txt


What's index.txt used for? ls -l is a bit brittle, maybe find . -print would work?

It is included in the OpenBSD distribution. For example, see https://cdn.openbsd.org/pub/OpenBSD/7.6/amd64/index.txt

The FAQ says it should be updated when a custom site76.tgz is added. Therefore, I think ls -l is the expected format.

Note: If you intend to provide the sets over HTTP(s), place site76.tgz in your source directory and include it in your index.txt. It will then be an option at install time.

shonfeder

Part way thru reviewing! Really fun to read thru, and I'm learning quite a bit about QEMU along the way. I have a few (probably quite naive) questions, not having used QEMU before.

I'll resume reviewing tomorrow!

doc/qemu.md

shonfeder · 2024-11-18T20:16:32Z

doc/qemu.md

+- windows-server-2022-amd64-ocaml-4.14.2.qcow2
+- windows-server-2022-amd64-ocaml-5.2.0.qcow2
+
+The base images build automatically using Cloud Init on Ubuntu,


I'm not quite sure how to parse this, as I assume the base images will only be build on command, and not on their own. But does it mean

Suggested change

The base images build automatically using Cloud Init on Ubuntu,

The build of base images will automatically use Cloud Init on Ubuntu,

?

The base images build are build using make ubuntu, make windows or make openbsd. The builds are unattended builds requiring no manual intervention. Cloud Init is used on Ubuntu, autounattend.xml on Windows and autoinstall on OpenBSD.

doc/qemu.md

lib/qemu_sandbox.ml

shonfeder · 2024-11-19T02:19:15Z

lib/qemu_sandbox.ml

+      | Ok _ -> Lwt_result.ok (Lwt.return ())
+      | _ -> Lwt_unix.sleep 1. >>= fun _ -> loop (n - 1) in
+  Lwt_unix.sleep 5. >>= fun _ ->
+  loop t.qemu_boot_time >>= fun _ ->


iiuc, we ignore the possible error that the loop could return if it runs of out of retries here. Is that intended?

I don't think it matters. A failure here would indicate that qemu_boot_time is too low or the base image is bad. The next ssh commands would also fail and we would then orderly clean up the QEMU machine with the quit command.

I generally have a preference for failing early, as it helps with debugging and can sometimes make it easier to improve fault tolerance later on. But I am convinced it won't cause problems here, so won't press the point.

A comment here to explain that reasoning may help future readers, but I don't insist on that either in case you think it's not needed.

shonfeder · 2024-11-19T02:34:31Z

lib/qemu_sandbox.ml

+    | 0 -> Lwt_result.fail (`Msg "No connection")
+    | n ->
+      Os.exec_result ~pp (ssh @ ["exit"]) >>= function
+      | Ok _ -> Lwt_result.ok (Lwt.return ())


This can be

Suggested change

| Ok _ -> Lwt_result.ok (Lwt.return ())

| Ok _ -> Lwt.return_ok ()

Yes. Updated.

shonfeder · 2024-11-19T02:55:44Z

lib/qemu_sandbox.ml

+      Lwt_unix.sleep 1. >>= fun () ->
+      loop (n - 1)
+    else Lwt.return () in
+  loop t.qemu_boot_time >>= fun _ ->


I'm a bit surprised we use the CLI configured "boot time" also to determine how long to to wait before issuing the quit here. Is it because the boot up time and time before it is responsive to a poweroff times are generally symmetrical?

Yes, in testing, the times when a longer boot time was needed coincided with times when a longer shutdown time was required. Notably, this was needed for RISCV-emulated machines, which are generally slower. Your second sentence is slightly wrong about what is happening here. An orderly shutdown is requested via an ACPI event or an OS shutdown command, but if the machine doesn't shut down in a timely manner, quit is issued to the emulator which terminates the machine akin to powering it off at the wall.

Ah nice. I guess that symmetry may make sense. Given that the variable and flag is called "boot time", I think adding a comment to that effect here could help readability in the future. Feel free to disregard tho.

lib/qemu_sandbox.ml

shonfeder · 2024-11-21T00:34:38Z

qemu/Makefile

+
+# busybox
+
+seed.iso: busybox.yaml


The busybox.yml target seems to be missing, giving make: *** No rule to make target 'busybox.yaml', needed by 'seed.iso'. Stop. is it possible this file needs to be committed?

Yes - I missed it from the commit. It has been added now.

shonfeder · 2024-11-21T00:39:15Z

lib/qemu_store.ml

+
+let strf = Printf.sprintf
+
+let running_as_root = Unix.getuid () = 0


Just curious: I can see that this occurs in several places in the code base. But, it seems like it is a misnomer, since afaict the value here gets decided at compile time. So it seem like it should either be

let running_as_root () = Unix.getuid () = 0

or

let compiled_as_root = Unix.getuid () = 0

Any ideas on the intended meaning?

Yes, it does look to be wrong. It is supposed to be used to decide whether to sudo or not, but since we run as root anyway this has gone unnoticed.

shonfeder

A few suggestions around how the new tar value is exposed

lib/build.ml

shonfeder · 2024-11-21T01:28:13Z

lib/qemu_sandbox.ml

+  | Linux -> None
+  | OpenBSD -> Some ["gtar"; "-xf"; "-"]
+  | Windows -> Some ["/cygdrive/c/Windows/System32/tar.exe"; "-xf"; "-"; "-C"; "/"]


To illustrate my suggestion around cleaning up the API for the new tar value, this would look something like

Suggested change

| Linux -> None

| OpenBSD -> Some ["gtar"; "-xf"; "-"]

| Windows -> Some ["/cygdrive/c/Windows/System32/tar.exe"; "-xf"; "-"; "-C"; "/"]

| Linux -> default_tar

| OpenBSD -> ["gtar"; "-xf"; "-"]

| Windows -> ["/cygdrive/c/Windows/System32/tar.exe"; "-xf"; "-"; "-C"; "/"]

shonfeder · 2024-11-21T01:28:55Z

lib/s.ml

+  val shell : t -> string list option
+  (** [shell] optional value to be used as the default shell. *)
+
+  val tar : t -> string list option


Suggested change

val tar : t -> string list option

val tar : t -> string list

shonfeder · 2024-11-21T01:29:13Z

lib/s.ml

+  (** [shell] optional value to be used as the default shell. *)
+
+  val tar : t -> string list option
+  (** [tar] tar command for this sandbox. *)


Suggested change

(** [tar] tar command for this sandbox. *)

(** [tar t] is the command to use for this sandbox. *)

shonfeder · 2024-11-21T01:29:35Z

lib/sandbox.jail.ml

@@ -168,6 +168,10 @@ let create ~state_dir:_ _c =
 let finished () =
  Lwt.return ()

+let shell _ = None
+
+let tar _ = None


Suggested change

let tar _ = None

let tar _ = default_tar

shonfeder

Finished passing eyes over everything. Awesome work! LMK if you have any questions about my suggestions or questions.

shonfeder · 2024-11-22T00:55:53Z

lib/qemu_store.mli

+(** [create ~path] creates a new overlayfs store where everything will
+    be stored under [path]. *)


Suggested change

(** [create ~path] creates a new overlayfs store where everything will

be stored under [path]. *)

(** [create ~root] creates a new overlayfs store where everything will

be stored under [root]. *)

Yes, but with s/overlayfs/QEMU/g

shonfeder · 2024-11-22T00:58:06Z

lib/sandbox.macos.ml

@@ -114,6 +114,10 @@ let finished () =
  Os.sudo [ "zfs"; "mount"; "obuilder/result" ] >>= fun () ->
  Lwt.return ()

+let shell _ = None


Curious what it means when sell x is None? I haven't been able to figure that out in the course of reviewing. Could you point out how this shell value ends up being used?

The context is that all the sandboxes invoke command x by running /usr/bin/env bash -c x where that default value comes from lib/build.ml by passing no parameter for shell. [Hence why my tar implementation used the same concept]. Shell is defined as:

let shell = Option.value ~default:(if Sys.win32 then ["cmd"; "/S"; "/C"] else ["/usr/bin/env"; "bash"; "-c"]) shell in

This only makes sense when Sys is the machine that we are building on, but in this case we build on a different architecture to the one we target. Also since commands are invoked via SSH, they already have a default shell so adding an extra one isn't needed.

I've reworked the original implementation of the default shell to follow the tar implementation...

Check main.ml for how it is used and of course ocluster-worker

shonfeder · 2024-11-22T01:01:28Z

qemu/Makefile

+	qemu-img create -f qcow2 -b $< -F qcow2 $@ 20G
+	qemu-system-riscv64 -m 16G -smp 8 -machine type=virt -nographic \
+		-bios /usr/lib/riscv64-linux-gnu/opensbi/generic/fw_jump.bin \
+		-kernel /usr/lib/u-boot/qemu-riscv64_smode/uboot.elf \


Solved this by installing a system dep, thanks to Mark's guidance.

shonfeder · 2024-11-22T01:03:13Z

qemu/disklabel

@@ -0,0 +1,10 @@
+/            2G


A readme in the qemu dir that explains what these different files are used for would be really helpful.

shonfeder

This is as far as I've gotten with documenting the requirements I've needed to try to make ubuntu:

# Utilities to support the QEMU backend

See [../doc/qemu.md](../doc/qemu.md) for documentation and usage

## Prerequisites

In order to use the targets defined in the [Makefile](./Makefile), at least the
following must be installed,

- [`qemu`](https://www.qemu.org/download/), including
  - A full installation, e.g.,
    - MacOS: `brew install qemu` 
    - Debian: `apt-get install qemu-system`
    - Arch: `pacman -Syu qemu-full`
  - u-boot-qemu:
    - [Debian](https://packages.debian.org/stable/admin/u-boot-qemu)
    - [Arch](https://aur.archlinux.org/packages/u-boot-qemu-bin)
- `cloud-localds` form [cloud-utils](https://github.com/canonical/cloud-utils)

I suggest putting this (or something like it) in qemu/README.hd, and also including some guidance on what the various different files in this dir are used for. LMK if you'd like me to make a commit into this branch adding the start of the readme.

shonfeder

Looking great! Thanks for the careful explanations in the docs and in reply to my comments.

I have a few minor followup suggestions, but mostly purely optional stuff about where I think comments could help.

The one change I'd still really like to see included is a README with content covering the kinds of stuff I suggested in #195 (review)

I was able to build the ubuntu base images! 🎉 Awesome 👯

doc/qemu.md

lib/os.ml

shonfeder · 2024-11-29T03:40:02Z

lib/qemu_sandbox.ml

+      | Ok _ -> Lwt_result.ok (Lwt.return ())
+      | _ -> Lwt_unix.sleep 1. >>= fun _ -> loop (n - 1) in
+  Lwt_unix.sleep 5. >>= fun _ ->
+  loop t.qemu_boot_time >>= fun _ ->


I generally have a preference for failing early, as it helps with debugging and can sometimes make it easier to improve fault tolerance later on. But I am convinced it won't cause problems here, so won't press the point.

A comment here to explain that reasoning may help future readers, but I don't insist on that either in case you think it's not needed.

shonfeder · 2024-11-29T03:41:35Z

lib/qemu_sandbox.ml

+      Lwt_unix.sleep 1. >>= fun () ->
+      loop (n - 1)
+    else Lwt.return () in
+  loop t.qemu_boot_time >>= fun _ ->


Ah nice. I guess that symmetry may make sense. Given that the variable and flag is called "boot time", I think adding a comment to that effect here could help readability in the future. Feel free to disregard tho.

lib/qemu_sandbox.ml

lib/s.ml

shonfeder

Mark and I agreed that we can add the README in a followup PR. All else looks good to me! Awesome work here. Maybe have a look at my last comments in case there are things there you'd like to incorporate.

QEMU backend

e854a34

mtelvers force-pushed the qemu-backend branch from af41390 to e854a34 Compare October 19, 2024 20:22

MisterDA reviewed Oct 21, 2024

View reviewed changes

samoht reviewed Oct 23, 2024

View reviewed changes

mtelvers added 10 commits October 25, 2024 15:40

Fix typos

f9cd15d

Update base images

de03a88

use gawk

63b2197

use extra disks for cache rather than pscp

8890afb

Add cygsympathy to base images

502c87c

Remove cygsympathy

905c64c

Remove tar_in function

32e6c34

Tidy up

1286c2f

* Speed improvements on Windows platform

c5f2c48

* Added RISCV64 support * Added OpenBSD support

Busybox

a330ec5

MisterDA reviewed Nov 7, 2024

View reviewed changes

mtelvers added 3 commits November 11, 2024 16:56

Set timezone on Windows

9a361d2

Fixes to OpenBSD builds

8fd770d

Create a restricted network when asked

e0a6005

mtelvers marked this pull request as ready for review November 15, 2024 09:59

shonfeder self-requested a review November 18, 2024 20:10

shonfeder reviewed Nov 19, 2024

View reviewed changes

Update base images to OCaml 5.2.1 and opam 2.3

c8f7092

shonfeder reviewed Nov 21, 2024

View reviewed changes

shonfeder reviewed Nov 22, 2024

View reviewed changes

shonfeder requested changes Nov 22, 2024

View reviewed changes

mtelvers added 2 commits November 25, 2024 16:28

Increase stack on OpenBSD and include bash

284ef5c

Updated following review

3334c8e

shonfeder requested changes Nov 29, 2024

View reviewed changes

shonfeder self-requested a review December 10, 2024 13:46

shonfeder approved these changes Dec 10, 2024

View reviewed changes

Final review updates

19a5348

mtelvers merged commit 29cde86 into ocurrent:master Dec 12, 2024
7 of 9 checks passed

	~doc:"$(docv) must be one of $(b,btrfs:/path), $(b,rsync:/path), $(b,xfs:/path), $(b,overlayfs:/path), $(b,zfs:pool), $(b,qmu:/path) or $(b,docker:path) for the OBuilder cache."
	~doc:"$(docv) must be one of $(b,btrfs:/path), $(b,rsync:/path), $(b,xfs:/path), $(b,overlayfs:/path), $(b,zfs:pool), $(b,qemu:/path) or $(b,docker:path) for the OBuilder cache."

	First, `qemu-img` creates a snapshot of the current `resuilt` layer into
	First, `qemu-img` creates a snapshot of the current `result` layer into

	cp install.conf disklabel mirror
	cp -p install.conf disklabel mirror

	The base images build automatically using Cloud Init on Ubuntu,
	The build of base images will automatically use Cloud Init on Ubuntu,

	\| Ok _ -> Lwt_result.ok (Lwt.return ())
	\| Ok _ -> Lwt.return_ok ()


		let strf = Printf.sprintf

		let running_as_root = Unix.getuid () = 0

	(** [tar] tar command for this sandbox. *)
	(** [tar t] is the command to use for this sandbox. *)

		(** [create ~path] creates a new overlayfs store where everything will
		be stored under [path]. *)

QEMU backend #195

QEMU backend #195

Conversation

mtelvers commented Oct 19, 2024 • edited Loading

MisterDA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samoht commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

shonfeder left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder left a comment • edited Loading

Choose a reason for hiding this comment

mtelvers commented Oct 19, 2024 •

edited

Loading

shonfeder Nov 19, 2024 •

edited

Loading

shonfeder Nov 21, 2024 •

edited

Loading

shonfeder Nov 22, 2024 •

edited

Loading

shonfeder left a comment •

edited

Loading

shonfeder left a comment •

edited

Loading