[PATCH v2 00/10] Improve reliability of VM tests

[PATCH v2 00/10] Improve reliability of VM tests

[John Snow]

This patch series attempts to improve the reliability of several of the
VM test targets. In particular, both CentOS 8 tests are non-functional
because CentOS 8 was EOL at the beginning of this calendar year, with
repositories and mirrors going offline.

Notably, I also remove the ubuntu.i386 test because we no longer support
Ubuntu 18.04 nor do we have explicit need of an i386 build test.

After this series, I am able to successfully run every VM target on an
x86_64 host, except:

- ubuntu.aarch64: Hangs often during testing, see below.
- centos.aarch64: Hangs often during testing, see below.
- haiku.x86_64: Build failures not addressed by this series, see
  https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg02103.html

The unit tests that I see fail most often are:

- qom-test: ENOMEM with default config and many cores
- test-hmp: ENOMEM with default config and many cores
- virtio-net-failover: Seems to like to hang on openbsd
- migration-test: Tends to hang under aarch64 tcg

Increasing the default memory (patch 10) has seemingly helped both
reliability *and* reduced race conditions quite a bit, both on my
12-thread intel laptop and on my 32-thread AMD 5950x desktop.

Due to how long it takes to run these tests, though, I can't claim
perfect reliability. The flakiness of virtio-net-failover and
migration-test is something that probably still needs to be addressed,
but it's outside of my expertise and time budget at present to pursue
it.

Future work (next version? next series?);

- Try to get centos.aarch64 working reliably under TCG
- Upgrade ubuntu.aarch64 to 20.04 after fixing centos.aarch64
- Fix the Haiku build test, if possible.
- Ensure I can reliably run and pass "make vm-build-all".
  (Remove VMs from this recipe if necessary.)

John Snow (10):
  tests/qemu-iotests: hotfix for 307, 223 output
  tests/qemu-iotests: skip 108 when FUSE is not loaded
  qga: treat get-guest-fsinfo as "best effort"
  tests/vm: use 'cp' instead of 'ln' for temporary vm images
  tests/vm: switch CentOS 8 to CentOS 8 Stream
  tests/vm: switch centos.aarch64 to CentOS 8 Stream
  tests/vm: update sha256sum for ubuntu.aarch64
  tests/vm: remove ubuntu.i386 VM test
  tests/vm: remove duplicate 'centos' VM test
  tests/vm: add 512MB extra memory per core

 qga/commands-posix.c       |   8 +-
 tests/qemu-iotests/108     |   5 ++
 tests/qemu-iotests/223.out |   4 +-
 tests/qemu-iotests/307.out |   4 +-
 tests/vm/Makefile.include  |   5 +-
 tests/vm/basevm.py         |   6 ++
 tests/vm/centos            |   8 +-
 tests/vm/centos.aarch64    | 174 +++++--------------------------------
 tests/vm/ubuntu.aarch64    |   8 +-
 tests/vm/ubuntu.i386       |  40 ---------
 10 files changed, 58 insertions(+), 204 deletions(-)
 delete mode 100755 tests/vm/ubuntu.i386

-- 
2.34.3

[PATCH v2 01/10] tests/qemu-iotests: hotfix for 307, 223 output

[John Snow]

Fixes: 58a6fdcc
Signed-off-by: John Snow <jsnow@redhat.com>
Tested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/223.out | 4 ++--
 tests/qemu-iotests/307.out | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 06479415312..26fb347c5da 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -93,7 +93,7 @@ exports available: 3
  export: 'n2'
   description: some text
   size:  4194304
-  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
   min block: 1
   opt block: 4096
   max block: 33554432
@@ -212,7 +212,7 @@ exports available: 3
  export: 'n2'
   description: some text
   size:  4194304
-  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
   min block: 1
   opt block: 4096
   max block: 33554432
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index ec8d2be0e0a..390f05d1b78 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -83,7 +83,7 @@ exports available: 2
  export: 'export1'
   description: This is the writable second export
   size:  67108864
-  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
   min block: XXX
   opt block: XXX
   max block: XXX
@@ -109,7 +109,7 @@ exports available: 1
  export: 'export1'
   description: This is the writable second export
   size:  67108864
-  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+  flags: 0xded ( flush fua trim zeroes df multi cache fast-zero )
   min block: XXX
   opt block: XXX
   max block: XXX
-- 
2.34.3

[PATCH v2 02/10] tests/qemu-iotests: skip 108 when FUSE is not loaded

[John Snow]

In certain container environments we may not have FUSE at all, so skip
the test in this circumstance too.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/108 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
index 9e923d6a59f..54e935acf28 100755
--- a/tests/qemu-iotests/108
+++ b/tests/qemu-iotests/108
@@ -60,6 +60,11 @@ if sudo -n losetup &>/dev/null; then
 else
     loopdev=false
 
+    # Check for usable FUSE in the host environment:
+    if test ! -c "/dev/fuse"; then
+        _notrun 'No passwordless sudo nor usable /dev/fuse'
+    fi
+
     # QSD --export fuse will either yield "Parameter 'id' is missing"
     # or "Invalid parameter 'fuse'", depending on whether there is
     # FUSE support or not.
-- 
2.34.3

[PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[John Snow]

In some container environments, there may be references to block devices
witnessable from a container through /proc/self/mountinfo that reference
devices we simply don't have access to in the container, and could not
provide information about.

Instead of failing the entire fsinfo command, return stub information
for these failed lookups.

This allows test-qga to pass under docker tests, which are in turn used
by the CentOS VM tests.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qga/commands-posix.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0469dc409d4..5989d4dca9d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char const *devpath,
 
     syspath = realpath(devpath, NULL);
     if (!syspath) {
-        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
+        if (errno == ENOENT) {
+            /* This devpath may not exist because of container config, etc. */
+            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n", devpath);
+            fs->name = g_strdup("??\?-ENOENT");
+        } else {
+            error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
+        }
         return;
     }
 
-- 
2.34.3

[PATCH v2 04/10] tests/vm: use 'cp' instead of 'ln' for temporary vm images

[John Snow]

If the initial setup fails, you've permanently altered the state of the
downloaded image in an unknowable way. Use 'cp' like our other test
setup scripts do.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/vm/centos | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/vm/centos b/tests/vm/centos
index 5c7bc1c1a9a..be4f6ff2f14 100755
--- a/tests/vm/centos
+++ b/tests/vm/centos
@@ -34,7 +34,7 @@ class CentosVM(basevm.BaseVM):
     def build_image(self, img):
         cimg = self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2")
         img_tmp = img + ".tmp"
-        subprocess.check_call(["ln", "-f", cimg, img_tmp])
+        subprocess.check_call(['cp', '-f', cimg, img_tmp])
         self.exec_qemu_img("resize", img_tmp, "50G")
         self.boot(img_tmp, extra_args = ["-cdrom", self.gen_cloud_init_iso()])
         self.wait_ssh()
-- 
2.34.3

[PATCH v2 05/10] tests/vm: switch CentOS 8 to CentOS 8 Stream

[John Snow]

The old CentOS image didn't work anymore because it was already EOL at
the beginning of 2022.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/vm/centos | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/vm/centos b/tests/vm/centos
index be4f6ff2f14..3a527c47b3d 100755
--- a/tests/vm/centos
+++ b/tests/vm/centos
@@ -1,8 +1,8 @@
 #!/usr/bin/env python3
 #
-# CentOS image
+# CentOS 8 Stream image
 #
-# Copyright 2018 Red Hat Inc.
+# Copyright 2018, 2022 Red Hat Inc.
 #
 # Authors:
 #  Fam Zheng <famz@redhat.com>
@@ -32,7 +32,7 @@ class CentosVM(basevm.BaseVM):
     """
 
     def build_image(self, img):
-        cimg = self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2")
+        cimg = self._download_with_cache("https://cloud.centos.org/centos/8-stream/x86_64/images/CentOS-Stream-GenericCloud-8-20220125.1.x86_64.qcow2")
         img_tmp = img + ".tmp"
         subprocess.check_call(['cp', '-f', cimg, img_tmp])
         self.exec_qemu_img("resize", img_tmp, "50G")
-- 
2.34.3

[PATCH v2 06/10] tests/vm: switch centos.aarch64 to CentOS 8 Stream

[John Snow]

Switch this test over to using a cloud image like the base CentOS8 VM
test, which helps make this script a bit simpler too.

Note: At time of writing, this test seems pretty flaky when run without
KVM support for aarch64. Certain unit tests like migration-test,
virtio-net-failover, test-hmp and qom-test seem quite prone to fail
under TCG. Still, this is an improvement in that at least pure build
tests are functional.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/vm/centos.aarch64 | 174 ++++++----------------------------------
 1 file changed, 24 insertions(+), 150 deletions(-)

diff --git a/tests/vm/centos.aarch64 b/tests/vm/centos.aarch64
index 96c450f8be5..2de7ef6992c 100755
--- a/tests/vm/centos.aarch64
+++ b/tests/vm/centos.aarch64
@@ -20,150 +20,38 @@ import time
 import traceback
 import aarch64vm
 
+
 DEFAULT_CONFIG = {
     'cpu'          : "max",
     'machine'      : "virt,gic-version=max",
-    'install_cmds' : "yum install -y make ninja-build git python3 gcc gcc-c++ flex bison, "\
-        "yum install -y glib2-devel perl pixman-devel zlib-devel, "\
-        "alternatives --set python /usr/bin/python3, "\
-        "sudo dnf config-manager "\
-        "--add-repo=https://download.docker.com/linux/centos/docker-ce.repo,"\
-        "sudo dnf install --nobest -y docker-ce.aarch64,"\
-        "systemctl enable docker",
+    'install_cmds' : (
+        "dnf config-manager --set-enabled powertools, "
+        "dnf config-manager --add-repo=https://download.docker.com/linux/centos/docker-ce.repo, "
+        "dnf install -y make ninja-build git python38 gcc gcc-c++ flex bison "\
+            "glib2-devel perl pixman-devel zlib-devel docker-ce.aarch64, "
+        "systemctl enable docker, "
+    ),
     # We increase beyond the default time since during boot
     # it can take some time (many seconds) to log into the VM.
     'ssh_timeout'  : 60,
 }
 
+
 class CentosAarch64VM(basevm.BaseVM):
-    name = "centos.aarch64"
+    name = "centos8.aarch64"
     arch = "aarch64"
-    login_prompt = "localhost login:"
-    prompt = '[root@localhost ~]#'
-    image_name = "CentOS-8-aarch64-1905-dvd1.iso"
-    image_link = "http://mirrors.usc.edu/pub/linux/distributions/centos/8.0.1905/isos/aarch64/"
+    image_name = "CentOS-Stream-GenericCloud-8-20220125.1.aarch64.qcow2"
+    image_link = "https://cloud.centos.org/centos/8-stream/aarch64/images/"
     image_link += image_name
     BUILD_SCRIPT = """
         set -e;
         cd $(mktemp -d);
-        sudo chmod a+r /dev/vdb;
-        tar --checkpoint=.10 -xf /dev/vdb;
+        export SRC_ARCHIVE=/dev/vdb;
+        sudo chmod a+r $SRC_ARCHIVE;
+        tar -xf $SRC_ARCHIVE;
         ./configure {configure_opts};
         make --output-sync {target} -j{jobs} {verbose};
     """
-    def set_key_perm(self):
-        """Set permissions properly on certain files to allow
-           ssh access."""
-        self.console_wait_send(self.prompt,
-                               "/usr/sbin/restorecon -R -v /root/.ssh\n")
-        self.console_wait_send(self.prompt,
-                "/usr/sbin/restorecon -R -v "\
-                "/home/{}/.ssh\n".format(self._config["guest_user"]))
-
-    def create_kickstart(self):
-        """Generate the kickstart file used to generate the centos image."""
-        # Start with the template for the kickstart.
-        ks_file = self._source_path + "/tests/vm/centos-8-aarch64.ks"
-        subprocess.check_call("cp {} ./ks.cfg".format(ks_file), shell=True)
-        # Append the ssh keys to the kickstart file
-        # as the post processing phase of installation.
-        with open("ks.cfg", "a") as f:
-            # Add in the root pw and guest user.
-            rootpw = "rootpw --plaintext {}\n"
-            f.write(rootpw.format(self._config["root_pass"]))
-            add_user = "user --groups=wheel --name={} "\
-                       "--password={} --plaintext\n"
-            f.write(add_user.format(self._config["guest_user"],
-                                    self._config["guest_pass"]))
-            # Add the ssh keys.
-            f.write("%post --log=/root/ks-post.log\n")
-            f.write("mkdir -p /root/.ssh\n")
-            addkey = 'echo "{}" >> /root/.ssh/authorized_keys\n'
-            addkey_cmd = addkey.format(self._config["ssh_pub_key"])
-            f.write(addkey_cmd)
-            f.write('mkdir -p /home/{}/.ssh\n'.format(self._config["guest_user"]))
-            addkey = 'echo "{}" >> /home/{}/.ssh/authorized_keys\n'
-            addkey_cmd = addkey.format(self._config["ssh_pub_key"],
-                                       self._config["guest_user"])
-            f.write(addkey_cmd)
-            f.write("%end\n")
-        # Take our kickstart file and create an .iso from it.
-        # The .iso will be provided to qemu as we boot
-        # from the install dvd.
-        # Anaconda will recognize the label "OEMDRV" and will
-        # start the automated installation.
-        gen_iso_img = 'genisoimage -output ks.iso -volid "OEMDRV" ks.cfg'
-        subprocess.check_call(gen_iso_img, shell=True)
-
-    def wait_for_shutdown(self):
-        """We wait for qemu to shutdown the VM and exit.
-           While this happens we display the console view
-           for easier debugging."""
-        # The image creation is essentially done,
-        # so whether or not the wait is successful we want to
-        # wait for qemu to exit (the self.wait()) before we return.
-        try:
-            self.console_wait("reboot: Power down")
-        except Exception as e:
-            sys.stderr.write("Exception hit\n")
-            if isinstance(e, SystemExit) and e.code == 0:
-                return 0
-            traceback.print_exc()
-        finally:
-            self.wait()
-
-    def build_base_image(self, dest_img):
-        """Run through the centos installer to create
-           a base image with name dest_img."""
-        # We create the temp image, and only rename
-        # to destination when we are done.
-        img = dest_img + ".tmp"
-        # Create an empty image.
-        # We will provide this as the install destination.
-        qemu_img_create = "qemu-img create {} 50G".format(img)
-        subprocess.check_call(qemu_img_create, shell=True)
-
-        # Create our kickstart file to be fed to the installer.
-        self.create_kickstart()
-        # Boot the install dvd with the params as our ks.iso
-        os_img = self._download_with_cache(self.image_link)
-        dvd_iso = "centos-8-dvd.iso"
-        subprocess.check_call(["cp", "-f", os_img, dvd_iso])
-        extra_args = "-cdrom ks.iso"
-        extra_args += " -drive file={},if=none,id=drive1,cache=writeback"
-        extra_args += " -device virtio-blk,drive=drive1,bootindex=1"
-        extra_args = extra_args.format(dvd_iso).split(" ")
-        self.boot(img, extra_args=extra_args)
-        self.console_wait_send("change the selection", "\n")
-        # We seem to need to hit esc (chr(27)) twice to abort the
-        # media check, which takes a long time.
-        # Waiting a bit seems to be more reliable before hitting esc.
-        self.console_wait("Checking")
-        time.sleep(5)
-        self.console_wait_send("Checking", chr(27))
-        time.sleep(5)
-        self.console_wait_send("Checking", chr(27))
-        print("Found Checking")
-        # Give sufficient time for the installer to create the image.
-        self.console_init(timeout=7200)
-        self.wait_for_shutdown()
-        os.rename(img, dest_img)
-        print("Done with base image build: {}".format(dest_img))
-
-    def check_create_base_img(self, img_base, img_dest):
-        """Create a base image using the installer.
-           We will use the base image if it exists.
-           This helps cut down on install time in case we
-           need to restart image creation,
-           since the base image creation can take a long time."""
-        if not os.path.exists(img_base):
-            print("Generate new base image: {}".format(img_base))
-            self.build_base_image(img_base);
-        else:
-            print("Use existing base image: {}".format(img_base))
-        # Save a copy of the base image and copy it to dest.
-        # which we will use going forward.
-        subprocess.check_call(["cp", img_base, img_dest])
 
     def boot(self, img, extra_args=None):
         aarch64vm.create_flash_images(self._tmpdir, self._efi_aarch64)
@@ -185,42 +73,28 @@ class CentosAarch64VM(basevm.BaseVM):
         super(CentosAarch64VM, self).boot(img, extra_args=extra_args)
 
     def build_image(self, img):
+        cimg = self._download_with_cache(self.image_link)
         img_tmp = img + ".tmp"
-        self.check_create_base_img(img + ".base", img_tmp)
-
-        # Boot the new image for the first time to finish installation.
-        self.boot(img_tmp)
-        self.console_init()
-        self.console_wait_send(self.login_prompt, "root\n")
-        self.console_wait_send("Password:",
-                               "{}\n".format(self._config["root_pass"]))
-
-        self.set_key_perm()
-        self.console_wait_send(self.prompt, "rpm -q centos-release\n")
-        enable_adapter = "sed -i 's/ONBOOT=no/ONBOOT=yes/g'" \
-                         " /etc/sysconfig/network-scripts/ifcfg-enp0s1\n"
-        self.console_wait_send(self.prompt, enable_adapter)
-        self.console_wait_send(self.prompt, "ifup enp0s1\n")
-        self.console_wait_send(self.prompt,
-                               'echo "qemu  ALL=(ALL) NOPASSWD:ALL" | '\
-                               'sudo tee /etc/sudoers.d/qemu\n')
-        self.console_wait(self.prompt)
-
-        # Rest of the commands we issue through ssh.
+        subprocess.run(['cp', '-f', cimg, img_tmp])
+        self.exec_qemu_img("resize", img_tmp, "50G")
+        self.boot(img_tmp, extra_args = ["-cdrom", self.gen_cloud_init_iso()])
         self.wait_ssh(wait_root=True)
+        self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
 
         # If the user chooses *not* to do the second phase,
         # then we will jump right to the graceful shutdown
         if self._config['install_cmds'] != "":
             install_cmds = self._config['install_cmds'].split(',')
             for cmd in install_cmds:
-                self.ssh_root(cmd)
+                self.ssh_root_check(cmd)
+
         self.ssh_root("poweroff")
-        self.wait_for_shutdown()
+        self.wait()
         os.rename(img_tmp, img)
         print("image creation complete: {}".format(img))
         return 0
 
+
 if __name__ == "__main__":
     defaults = aarch64vm.get_config_defaults(CentosAarch64VM, DEFAULT_CONFIG)
     sys.exit(basevm.main(CentosAarch64VM, defaults))
-- 
2.34.3

[PATCH v2 07/10] tests/vm: update sha256sum for ubuntu.aarch64

[John Snow]

This checksum changes weekly; use a fixed point image and update the
checksum so we don't have to re-download it quite so much.

Note: Just like the centos.aarch64 test, this test currently seems very
flaky when run as a TCG test.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/vm/ubuntu.aarch64 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/vm/ubuntu.aarch64 b/tests/vm/ubuntu.aarch64
index b291945a7e9..fc9c2ce22ff 100755
--- a/tests/vm/ubuntu.aarch64
+++ b/tests/vm/ubuntu.aarch64
@@ -32,9 +32,13 @@ DEFAULT_CONFIG = {
 class UbuntuAarch64VM(ubuntuvm.UbuntuVM):
     name = "ubuntu.aarch64"
     arch = "aarch64"
+    # NOTE: The Ubuntu 18.04 cloud images are updated weekly. The
+    # release below has been chosen as the latest at time of writing.
+    # Using the rolling latest release means the SHA will be wrong
+    # within a week.
     image_name = "ubuntu-18.04-server-cloudimg-arm64.img"
-    image_link = "https://cloud-images.ubuntu.com/releases/18.04/release/" + image_name
-    image_sha256="0fdcba761965735a8a903d8b88df8e47f156f48715c00508e4315c506d7d3cb1"
+    image_link = "https://cloud-images.ubuntu.com/releases/bionic/release-20220610/" + image_name
+    image_sha256="0eacc5142238788365576b15f1d0b6f23dda6d3e545ee22f5306af7bd6ec47bd"
     BUILD_SCRIPT = """
         set -e;
         cd $(mktemp -d);
-- 
2.34.3

[PATCH v2 08/10] tests/vm: remove ubuntu.i386 VM test

[John Snow]

Ubuntu 18.04 is out of our support window, and Ubuntu 20.04 does not
support i386 anymore. The debian project does, but they do not provide
any cloud images for it, a new expect-style script would have to be
written.

Since we have i386 cross-compiler tests hosted on GitLab CI, we don't
need to support this VM test anymore.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/vm/Makefile.include |  3 +--
 tests/vm/ubuntu.i386      | 40 ---------------------------------------
 2 files changed, 1 insertion(+), 42 deletions(-)
 delete mode 100755 tests/vm/ubuntu.i386

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 588bc999cc9..70eee2510c6 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -8,7 +8,7 @@ EFI_AARCH64 = $(wildcard $(BUILD_DIR)/pc-bios/edk2-aarch64-code.fd)
 
 X86_IMAGES := freebsd netbsd openbsd centos fedora haiku.x86_64
 ifneq ($(GENISOIMAGE),)
-X86_IMAGES += ubuntu.i386 centos
+X86_IMAGES += centos
 ifneq ($(EFI_AARCH64),)
 ARM64_IMAGES += ubuntu.aarch64 centos.aarch64
 endif
@@ -39,7 +39,6 @@ vm-help vm-test:
 	@echo "  vm-build-fedora                 - Build QEMU in Fedora VM"
 ifneq ($(GENISOIMAGE),)
 	@echo "  vm-build-centos                 - Build QEMU in CentOS VM, with Docker"
-	@echo "  vm-build-ubuntu.i386            - Build QEMU in ubuntu i386 VM"
 ifneq ($(EFI_AARCH64),)
 	@echo "  vm-build-ubuntu.aarch64         - Build QEMU in ubuntu aarch64 VM"
 	@echo "  vm-build-centos.aarch64         - Build QEMU in CentOS aarch64 VM"
diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
deleted file mode 100755
index 47681b6f87d..00000000000
--- a/tests/vm/ubuntu.i386
+++ /dev/null
@@ -1,40 +0,0 @@
-#!/usr/bin/env python3
-#
-# Ubuntu i386 image
-#
-# Copyright 2017 Red Hat Inc.
-#
-# Authors:
-#  Fam Zheng <famz@redhat.com>
-#
-# This code is licensed under the GPL version 2 or later.  See
-# the COPYING file in the top-level directory.
-#
-
-import sys
-import basevm
-import ubuntuvm
-
-DEFAULT_CONFIG = {
-    'install_cmds' : "apt-get update,"\
-                     "apt-get build-dep -y qemu,"\
-                     "apt-get install -y libfdt-dev language-pack-en ninja-build",
-}
-
-class UbuntuX86VM(ubuntuvm.UbuntuVM):
-    name = "ubuntu.i386"
-    arch = "i386"
-    image_link="https://cloud-images.ubuntu.com/releases/bionic/"\
-               "release-20191114/ubuntu-18.04-server-cloudimg-i386.img"
-    image_sha256="28969840626d1ea80bb249c08eef1a4533e8904aa51a327b40f37ac4b4ff04ef"
-    BUILD_SCRIPT = """
-        set -e;
-        cd $(mktemp -d);
-        sudo chmod a+r /dev/vdb;
-        tar -xf /dev/vdb;
-        ./configure {configure_opts};
-        make --output-sync {target} -j{jobs} {verbose};
-    """
-
-if __name__ == "__main__":
-    sys.exit(basevm.main(UbuntuX86VM, DEFAULT_CONFIG))
-- 
2.34.3

[PATCH v2 09/10] tests/vm: remove duplicate 'centos' VM test

[John Snow]

This is listed twice by accident; we require genisoimage to run the
test, so remove the unconditional entry.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/vm/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 70eee2510c6..f3b7a9d299d 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -6,7 +6,7 @@ HOST_ARCH = $(if $(ARCH),$(ARCH),$(shell uname -m))
 
 EFI_AARCH64 = $(wildcard $(BUILD_DIR)/pc-bios/edk2-aarch64-code.fd)
 
-X86_IMAGES := freebsd netbsd openbsd centos fedora haiku.x86_64
+X86_IMAGES := freebsd netbsd openbsd fedora haiku.x86_64
 ifneq ($(GENISOIMAGE),)
 X86_IMAGES += centos
 ifneq ($(EFI_AARCH64),)
-- 
2.34.3

[PATCH v2 10/10] tests/vm: add 512MB extra memory per core

[John Snow]

If you try to run a 16 or 32 threaded test, you're going to run out of
memory very quickly with qom-test and a few others. Bump the memory
limit to try to scale with larger-core machines.

Add this magic in early to the configuration process so that the
config file, if provided, can still override it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/vm/basevm.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index d7d0413df35..69200ca5f1e 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -99,6 +99,12 @@ def __init__(self, args, config=None):
         self._source_path = args.source_path
         # Allow input config to override defaults.
         self._config = DEFAULT_CONFIG.copy()
+
+        if args.jobs and args.jobs > 4:
+            # Add a gig per every 2 jobs over 4
+            overage = (args.jobs - 4) // 2
+            self._config['memory'] = f"{4 + overage}G"
+
         if config != None:
             self._config.update(config)
         self.validate_ssh_keys()
-- 
2.34.3

Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

Hi

On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:

> In some container environments, there may be references to block devices
> witnessable from a container through /proc/self/mountinfo that reference
> devices we simply don't have access to in the container, and could not
> provide information about.
>
> Instead of failing the entire fsinfo command, return stub information
> for these failed lookups.
>
> This allows test-qga to pass under docker tests, which are in turn used
> by the CentOS VM tests.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qga/commands-posix.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0469dc409d4..5989d4dca9d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char
> const *devpath,
>
>      syspath = realpath(devpath, NULL);
>      if (!syspath) {
> -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +        if (errno == ENOENT) {
> +            /* This devpath may not exist because of container config,
> etc. */
> +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n",
> devpath);
>

qga uses g_critical() (except for some win32 code paths atm)


> +            fs->name = g_strdup("??\?-ENOENT");
>

Hmm, maybe we should make the field optional instead.


> +        } else {
> +            error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +        }
>          return;
>      }
>
> --
> 2.34.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2633 bytes --]

Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[John Snow]

On Thu, Jun 16, 2022 at 10:36 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:
>>
>> In some container environments, there may be references to block devices
>> witnessable from a container through /proc/self/mountinfo that reference
>> devices we simply don't have access to in the container, and could not
>> provide information about.
>>
>> Instead of failing the entire fsinfo command, return stub information
>> for these failed lookups.
>>
>> This allows test-qga to pass under docker tests, which are in turn used
>> by the CentOS VM tests.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qga/commands-posix.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 0469dc409d4..5989d4dca9d 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char const *devpath,
>>
>>      syspath = realpath(devpath, NULL);
>>      if (!syspath) {
>> -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
>> +        if (errno == ENOENT) {
>> +            /* This devpath may not exist because of container config, etc. */
>> +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n", devpath);
>
>
> qga uses g_critical() (except for some win32 code paths atm)

Whoops, this is a debugging thing that I left in by accident. I was
just so excited that after testing overnight, everything worked. :)

>
>>
>> +            fs->name = g_strdup("??\?-ENOENT");
>
>
> Hmm, maybe we should make the field optional instead.

Does that harm compatibility in a meaningful way? I'm happy to do
whatever QGA maintainers want me to do. I just did something quick and
dirty to get it working at all as a conversation starter. O:-)

>
>>
>> +        } else {
>> +            error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
>> +        }
>>          return;
>>      }
>>
>> --
>> 2.34.3
>>
>>
>
>
> --
> Marc-André Lureau

Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[Thomas Huth]

On 16/06/2022 16.43, John Snow wrote:
> On Thu, Jun 16, 2022 at 10:36 AM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>>
>> On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:
>>>
>>> In some container environments, there may be references to block devices
>>> witnessable from a container through /proc/self/mountinfo that reference
>>> devices we simply don't have access to in the container, and could not
>>> provide information about.
>>>
>>> Instead of failing the entire fsinfo command, return stub information
>>> for these failed lookups.
>>>
>>> This allows test-qga to pass under docker tests, which are in turn used
>>> by the CentOS VM tests.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   qga/commands-posix.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 0469dc409d4..5989d4dca9d 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>>> @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char const *devpath,
>>>
>>>       syspath = realpath(devpath, NULL);
>>>       if (!syspath) {
>>> -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
>>> +        if (errno == ENOENT) {
>>> +            /* This devpath may not exist because of container config, etc. */
>>> +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n", devpath);
>>
>>
>> qga uses g_critical() (except for some win32 code paths atm)
> 
> Whoops, this is a debugging thing that I left in by accident. I was
> just so excited that after testing overnight, everything worked. :)
> 
>>
>>>
>>> +            fs->name = g_strdup("??\?-ENOENT");
>>
>>
>> Hmm, maybe we should make the field optional instead.
> 
> Does that harm compatibility in a meaningful way? I'm happy to do
> whatever QGA maintainers want me to do. I just did something quick and
> dirty to get it working at all as a conversation starter. O:-)

Should the device get ignored instead of returning up a dummy device? ... at 
least that's what I'd expect at a quick glance at the problem...

  Thomas

Re: [PATCH v2 02/10] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Thomas Huth]

On 16/06/2022 16.26, John Snow wrote:
> In certain container environments we may not have FUSE at all, so skip
> the test in this circumstance too.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/108 | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> index 9e923d6a59f..54e935acf28 100755
> --- a/tests/qemu-iotests/108
> +++ b/tests/qemu-iotests/108
> @@ -60,6 +60,11 @@ if sudo -n losetup &>/dev/null; then
>   else
>       loopdev=false
>   
> +    # Check for usable FUSE in the host environment:
> +    if test ! -c "/dev/fuse"; then
> +        _notrun 'No passwordless sudo nor usable /dev/fuse'
> +    fi
> +
>       # QSD --export fuse will either yield "Parameter 'id' is missing"
>       # or "Invalid parameter 'fuse'", depending on whether there is
>       # FUSE support or not.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 08/10] tests/vm: remove ubuntu.i386 VM test

[Thomas Huth]

On 16/06/2022 16.26, John Snow wrote:
> Ubuntu 18.04 is out of our support window, and Ubuntu 20.04 does not
> support i386 anymore. The debian project does, but they do not provide
> any cloud images for it, a new expect-style script would have to be
> written.
> 
> Since we have i386 cross-compiler tests hosted on GitLab CI, we don't
> need to support this VM test anymore.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/vm/Makefile.include |  3 +--
>   tests/vm/ubuntu.i386      | 40 ---------------------------------------
>   2 files changed, 1 insertion(+), 42 deletions(-)
>   delete mode 100755 tests/vm/ubuntu.i386

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 09/10] tests/vm: remove duplicate 'centos' VM test

[Thomas Huth]

On 16/06/2022 16.26, John Snow wrote:
> This is listed twice by accident; we require genisoimage to run the
> test, so remove the unconditional entry.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/vm/Makefile.include | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index 70eee2510c6..f3b7a9d299d 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -6,7 +6,7 @@ HOST_ARCH = $(if $(ARCH),$(ARCH),$(shell uname -m))
>   
>   EFI_AARCH64 = $(wildcard $(BUILD_DIR)/pc-bios/edk2-aarch64-code.fd)
>   
> -X86_IMAGES := freebsd netbsd openbsd centos fedora haiku.x86_64
> +X86_IMAGES := freebsd netbsd openbsd fedora haiku.x86_64
>   ifneq ($(GENISOIMAGE),)
>   X86_IMAGES += centos
>   ifneq ($(EFI_AARCH64),)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[Daniel P. Berrangé]

On Thu, Jun 16, 2022 at 06:35:44PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:
> 
> > In some container environments, there may be references to block devices
> > witnessable from a container through /proc/self/mountinfo that reference
> > devices we simply don't have access to in the container, and could not
> > provide information about.
> >
> > Instead of failing the entire fsinfo command, return stub information
> > for these failed lookups.
> >
> > This allows test-qga to pass under docker tests, which are in turn used
> > by the CentOS VM tests.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  qga/commands-posix.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 0469dc409d4..5989d4dca9d 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char
> > const *devpath,
> >
> >      syspath = realpath(devpath, NULL);
> >      if (!syspath) {
> > -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> > +        if (errno == ENOENT) {
> > +            /* This devpath may not exist because of container config,
> > etc. */
> > +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n",
> > devpath);
> >
> 
> qga uses g_critical() (except for some win32 code paths atm)
> 
> 
> > +            fs->name = y
> >
> 
> Hmm, maybe we should make the field optional instead.

In my own testing, this method is called in various scenarios.
Some example:

  devpath==/sys/dev/block/253:0
  syspath==/sys/devices/virtual/block/dm-0

    => fs->name == dm-0

  devpath==/sys/devices/virtual/block/dm-0/slaves/nvme0n1p4
  syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p4

    => fs->name == nvme0n1p4

  devpath==/sys/dev/block/259:2
  syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p2

    => fs->name == nvme0n1p2

We set fs->name from  basename(syspath)

If the realpath call fails, we could use  basename(devpath). That
would sometimes give the correct answer, and in other types it
would at least give the major:minor number, which an admin can
manually correlate if desired via /proc/partitions.

If we want to be really advanced, we could just open /proc/partitions
and resolve the proper name ourselves, but that's probably overkill

  basename(sysfspath)

is better than g_strdup("??\?-ENOENT")  IMHO

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4178 bytes --]

On Fri, Jun 17, 2022, 5:49 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Jun 16, 2022 at 06:35:44PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:
> >
> > > In some container environments, there may be references to block
> devices
> > > witnessable from a container through /proc/self/mountinfo that
> reference
> > > devices we simply don't have access to in the container, and could not
> > > provide information about.
> > >
> > > Instead of failing the entire fsinfo command, return stub information
> > > for these failed lookups.
> > >
> > > This allows test-qga to pass under docker tests, which are in turn used
> > > by the CentOS VM tests.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >  qga/commands-posix.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index 0469dc409d4..5989d4dca9d 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char
> > > const *devpath,
> > >
> > >      syspath = realpath(devpath, NULL);
> > >      if (!syspath) {
> > > -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> > > +        if (errno == ENOENT) {
> > > +            /* This devpath may not exist because of container config,
> > > etc. */
> > > +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n",
> > > devpath);
> > >
> >
> > qga uses g_critical() (except for some win32 code paths atm)
> >
> >
> > > +            fs->name = y
> > >
> >
> > Hmm, maybe we should make the field optional instead.
>
> In my own testing, this method is called in various scenarios.
> Some example:
>
>   devpath==/sys/dev/block/253:0
>   syspath==/sys/devices/virtual/block/dm-0
>
>     => fs->name == dm-0
>
>   devpath==/sys/devices/virtual/block/dm-0/slaves/nvme0n1p4
>
> syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p4
>
>     => fs->name == nvme0n1p4
>
>   devpath==/sys/dev/block/259:2
>
> syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p2
>
>     => fs->name == nvme0n1p2
>
> We set fs->name from  basename(syspath)
>
> If the realpath call fails, we could use  basename(devpath). That
> would sometimes give the correct answer, and in other types it
> would at least give the major:minor number, which an admin can
> manually correlate if desired via /proc/partitions.
>
> If we want to be really advanced, we could just open /proc/partitions
> and resolve the proper name ourselves, but that's probably overkill
>
>   basename(sysfspath)
>
> is better than g_strdup("??\?-ENOENT")  IMHO
>

Sure! I had something like that initially, but chickened out specifically
because I thought major:minor was a nonsense kind of reply, so I opted for
more egregiously obvious nonsense. I figured I'd find strong opinions that
way ;)

I'm just not sure how this data is used in practice so I had no insight as
to what would be best. I can use the basename, sure.

(Should I also add an optional flag field that indicates the path was not
resolvable, do you think? I guess we can always add it later if needed, but
not sure if i need to head that one off at the pass.)

As for Thomas' comment: I wasn't entirely clear on precisely when we'd run
into this scenario and I didn't know if it was a good idea to skip the
entries entirely. Maybe getting platform mount information even if we can't
access it is still important when working with containers? I don't know one
way or the other TBQH. I'm not very well traveled with devices,
filesystems, and permissions where containers are concerned.

/shrug


> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 6128 bytes --]

Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[Daniel P. Berrangé]

On Fri, Jun 17, 2022 at 10:04:14AM -0400, John Snow wrote:
> On Fri, Jun 17, 2022, 5:49 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Thu, Jun 16, 2022 at 06:35:44PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:
> > >
> > > > In some container environments, there may be references to block
> > devices
> > > > witnessable from a container through /proc/self/mountinfo that
> > reference
> > > > devices we simply don't have access to in the container, and could not
> > > > provide information about.
> > > >
> > > > Instead of failing the entire fsinfo command, return stub information
> > > > for these failed lookups.
> > > >
> > > > This allows test-qga to pass under docker tests, which are in turn used
> > > > by the CentOS VM tests.
> > > >
> > > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > > ---
> > > >  qga/commands-posix.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > > index 0469dc409d4..5989d4dca9d 100644
> > > > --- a/qga/commands-posix.c
> > > > +++ b/qga/commands-posix.c
> > > > @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char
> > > > const *devpath,
> > > >
> > > >      syspath = realpath(devpath, NULL);
> > > >      if (!syspath) {
> > > > -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> > > > +        if (errno == ENOENT) {
> > > > +            /* This devpath may not exist because of container config,
> > > > etc. */
> > > > +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n",
> > > > devpath);
> > > >
> > >
> > > qga uses g_critical() (except for some win32 code paths atm)
> > >
> > >
> > > > +            fs->name = y
> > > >
> > >
> > > Hmm, maybe we should make the field optional instead.
> >
> > In my own testing, this method is called in various scenarios.
> > Some example:
> >
> >   devpath==/sys/dev/block/253:0
> >   syspath==/sys/devices/virtual/block/dm-0
> >
> >     => fs->name == dm-0
> >
> >   devpath==/sys/devices/virtual/block/dm-0/slaves/nvme0n1p4
> >
> > syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p4
> >
> >     => fs->name == nvme0n1p4
> >
> >   devpath==/sys/dev/block/259:2
> >
> > syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p2
> >
> >     => fs->name == nvme0n1p2
> >
> > We set fs->name from  basename(syspath)
> >
> > If the realpath call fails, we could use  basename(devpath). That
> > would sometimes give the correct answer, and in other types it
> > would at least give the major:minor number, which an admin can
> > manually correlate if desired via /proc/partitions.
> >
> > If we want to be really advanced, we could just open /proc/partitions
> > and resolve the proper name ourselves, but that's probably overkill
> >
> >   basename(sysfspath)
> >
> > is better than g_strdup("??\?-ENOENT")  IMHO
> >
> 
> Sure! I had something like that initially, but chickened out specifically
> because I thought major:minor was a nonsense kind of reply, so I opted for
> more egregiously obvious nonsense. I figured I'd find strong opinions that
> way ;)

It is a different format but it is semantically giving similar info.

If we want to just leave it empty though that's fine too.

> 
> I'm just not sure how this data is used in practice so I had no insight as
> to what would be best. I can use the basename, sure.
> 
> (Should I also add an optional flag field that indicates the path was not
> resolvable, do you think? I guess we can always add it later if needed, but
> not sure if i need to head that one off at the pass.)
> 
> As for Thomas' comment: I wasn't entirely clear on precisely when we'd run
> into this scenario and I didn't know if it was a good idea to skip the
> entries entirely. Maybe getting platform mount information even if we can't
> access it is still important when working with containers? I don't know one
> way or the other TBQH. I'm not very well traveled with devices,
> filesystems, and permissions where containers are concerned.

I view the primary purpose of this command to be offering a way to
enumerate filesystems. Whether we report what block device the FS
on host is a secondary purpose.  So as long as we can fullfill the
primary purpose, its sufficient IMHO.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 00/10] Improve reliability of VM tests

[John Snow]

On Thu, Jun 16, 2022 at 10:27 AM John Snow <jsnow@redhat.com> wrote:
>
> This patch series attempts to improve the reliability of several of the
> VM test targets. In particular, both CentOS 8 tests are non-functional
> because CentOS 8 was EOL at the beginning of this calendar year, with
> repositories and mirrors going offline.
>
> Notably, I also remove the ubuntu.i386 test because we no longer support
> Ubuntu 18.04 nor do we have explicit need of an i386 build test.
>
> After this series, I am able to successfully run every VM target on an
> x86_64 host, except:
>
> - ubuntu.aarch64: Hangs often during testing, see below.
> - centos.aarch64: Hangs often during testing, see below.

I've since gotten a clean run on centos.aarch64 for the first time in
a long time by upping the memory allocation to be 1G per job; so my 12
thread, 6 core intel laptop used 6GB to complete this test
successfully. With that not having a hard blocker on it anymore, I'll
work on upgrading the ubuntu container.

> - haiku.x86_64: Build failures not addressed by this series, see
>   https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg02103.html
>
> The unit tests that I see fail most often are:
>
> - qom-test: ENOMEM with default config and many cores
> - test-hmp: ENOMEM with default config and many cores
> - virtio-net-failover: Seems to like to hang on openbsd
> - migration-test: Tends to hang under aarch64 tcg
>
> Increasing the default memory (patch 10) has seemingly helped both
> reliability *and* reduced race conditions quite a bit, both on my
> 12-thread intel laptop and on my 32-thread AMD 5950x desktop.
>
> Due to how long it takes to run these tests, though, I can't claim
> perfect reliability. The flakiness of virtio-net-failover and
> migration-test is something that probably still needs to be addressed,
> but it's outside of my expertise and time budget at present to pursue
> it.
>
> Future work (next version? next series?);
>
> - Try to get centos.aarch64 working reliably under TCG
> - Upgrade ubuntu.aarch64 to 20.04 after fixing centos.aarch64
> - Fix the Haiku build test, if possible.
> - Ensure I can reliably run and pass "make vm-build-all".
>   (Remove VMs from this recipe if necessary.)
>
> John Snow (10):
>   tests/qemu-iotests: hotfix for 307, 223 output
>   tests/qemu-iotests: skip 108 when FUSE is not loaded
>   qga: treat get-guest-fsinfo as "best effort"
>   tests/vm: use 'cp' instead of 'ln' for temporary vm images
>   tests/vm: switch CentOS 8 to CentOS 8 Stream
>   tests/vm: switch centos.aarch64 to CentOS 8 Stream
>   tests/vm: update sha256sum for ubuntu.aarch64
>   tests/vm: remove ubuntu.i386 VM test
>   tests/vm: remove duplicate 'centos' VM test
>   tests/vm: add 512MB extra memory per core
>
>  qga/commands-posix.c       |   8 +-
>  tests/qemu-iotests/108     |   5 ++
>  tests/qemu-iotests/223.out |   4 +-
>  tests/qemu-iotests/307.out |   4 +-
>  tests/vm/Makefile.include  |   5 +-
>  tests/vm/basevm.py         |   6 ++
>  tests/vm/centos            |   8 +-
>  tests/vm/centos.aarch64    | 174 +++++--------------------------------
>  tests/vm/ubuntu.aarch64    |   8 +-
>  tests/vm/ubuntu.i386       |  40 ---------
>  10 files changed, 58 insertions(+), 204 deletions(-)
>  delete mode 100755 tests/vm/ubuntu.i386
>
> --
> 2.34.3
>
>

Re: [PATCH v2 01/10] tests/qemu-iotests: hotfix for 307, 223 output

[Hanna Reitz]

On 16.06.22 16:26, John Snow wrote:
> Fixes: 58a6fdcc
> Signed-off-by: John Snow <jsnow@redhat.com>
> Tested-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/223.out | 4 ++--
>   tests/qemu-iotests/307.out | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v2 02/10] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Hanna Reitz]

On 16.06.22 16:26, John Snow wrote:
> In certain container environments we may not have FUSE at all, so skip
> the test in this circumstance too.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/108 | 5 +++++
>   1 file changed, 5 insertions(+)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v2 02/10] tests/qemu-iotests: skip 108 when FUSE is not loaded

[John Snow]

On Fri, Jul 1, 2022 at 4:05 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 16.06.22 16:26, John Snow wrote:
> > In certain container environments we may not have FUSE at all, so skip
> > the test in this circumstance too.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/108 | 5 +++++
> >   1 file changed, 5 insertions(+)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

Hanna, if you want to take just the first two, be my guest. The rest
of this series needs more work still.

--js

Re: [PATCH v2 02/10] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Hanna Reitz]

On 01.07.22 18:15, John Snow wrote:
> On Fri, Jul 1, 2022 at 4:05 AM Hanna Reitz <hreitz@redhat.com> wrote:
>> On 16.06.22 16:26, John Snow wrote:
>>> In certain container environments we may not have FUSE at all, so skip
>>> the test in this circumstance too.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    tests/qemu-iotests/108 | 5 +++++
>>>    1 file changed, 5 insertions(+)
>> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>>
> Hanna, if you want to take just the first two, be my guest.

Thanks for the invite, always happy to be your guest!

(Applied to my block branch:)

https://gitlab.com/hreitz/qemu/-/commits/block

Hanna