[PATCH 0/5] Update CentOS VM tests

[PATCH 0/5] Update CentOS VM tests

[John Snow]

This patch series attempts to revive the CentOS VM test targets, because
both appear to be presently non-functional.

I didn't quite get it working 100%, but I'm sending anyway to ask for
help in solving straggler problems.

My experience testing these is that CentOS 8 (x86_64) fails on test-qga:

――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stderr: ** ERROR:../src/tests/unit/test-qga.c:321:test_qga_get_fsinfo:
assertion failed ret: GenericError realpath(\ "/sys/dev/block/252:1"):
No such file or directory

(test program exited with status code -6)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

... I'm assuming this is something to do with the docker environment in
which the test is being run, but I don't know exactly what right now.

See the tail of this cover letter for more failures found when running
the docker tests manually, outside of the VM.

Meanwhile, the aarch64 VM appears to fail on a few different cases:

  3/586 qemu:qtest+qtest-arm / qtest-arm/qom-test
  ERROR          1022.98s   killed by signal 6 SIGABRT

  1/586 qemu:qtest+qtest-aarch64 / qtest-aarch64/qom-test
  ERROR          1023.86s   killed by signal 6 SIGABRT

 39/586 qemu:qtest+qtest-aarch64 / qtest-aarch64/test-hmp
 ERROR          184.02s   killed by signal 6 SIGABRT

 41/586 qemu:qtest+qtest-arm / qtest-arm/test-hmp
 ERROR          178.55s   killed by signal 6 SIGABRT

... Unfortunately, I wasn't able to rescue the log for this one in time,
so I don't have record of the failure. I guess I'll run again overnight
and see if I can capture it this time.

Following up from the CentOS8 VM failure up top; running "make
docker-test-block@centos8" on my development machine (not in any VM)
*also* seems to fail, but in a manner differently than the same test
step appears to from within the CentOS 8 Stream VM:

Failures: 086 150 221 253
Failed 4 of 61 iotests
Test failed: iotests raw

--- /tmp/qemu-test/src/tests/qemu-iotests/086.out
+++ /tmp/qemu-test/086.out.bad
@@ -9,9 +9,69 @@
 wrote 1048576/1048576 bytes at offset 33554432
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
     (0.00/100%)
+    (1.56/100%)

(And so on. This looks like a race.)

--- /tmp/qemu-test/src/tests/qemu-iotests/150.out.raw
+++ /tmp/qemu-test/150.out.bad
@@ -3,7 +3,7 @@
 === Mapping sparse conversion ===

 Offset          Length          File
-0               0x1000          TEST_DIR/t.IMGFMT
+0               0x100000        TEST_DIR/t.IMGFMT

 === Mapping non-sparse conversion ===

--- /tmp/qemu-test/src/tests/qemu-iotests/221.out
+++ /tmp/qemu-test/221.out.bad
@@ -5,14 +5,13 @@
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65537
 discard 65537/65537 bytes at offset 0
 64.001 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 66048, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
-[{ "start": 0, "length": 66048, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 66048, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 65537, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 65537, "length": 511, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
 wrote 1/1 bytes at offset 65536
 1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET},
-{ "start": 65536, "length": 1, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
+[{ "start": 0, "length": 65537, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
 { "start": 65537, "length": 511, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
-[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET},
-{ "start": 65536, "length": 1, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
+[{ "start": 0, "length": 65537, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
 { "start": 65537, "length": 511, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
 *** done

--- /tmp/qemu-test/src/tests/qemu-iotests/253.out
+++ /tmp/qemu-test/253.out.bad
@@ -3,16 +3,10 @@
 === Check mapping of unaligned raw image ===

 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048575
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 4096, "length": 1044480, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 4096, "length": 1044480, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
 wrote 65535/65535 bytes at offset 983040
 63.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 4096, "length": 978944, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET},
-{ "start": 983040, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
-{ "start": 4096, "length": 978944, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET},
-{ "start": 983040, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
 *** done

... I'll work my way through trying to understand all of these failures,
but any help would be appreciated to get these tests humming again.

Meanwhile, VM tests that I have observed to be in non-working condition
on latest upstream:

- centos (x86_64) - CentOS 8 is EOL
- centos.aarch64 - CentOS 8 is EOL, image is MIA
- haiku.x86_64 - build issues
- openbsd - virtio-net-failover tests hang indefinitely
- ubuntu.aarch64 - Honestly, I don't recall. I'm re-running overnight to
  find out.

I'll continue to try and sort out the issues with all of the tests I am
seeing fail and gather better diagnostics and intel for each.

:(

--js

John Snow (5):
  tests/qemu-iotests: hotfix for 307, 223 output
  tests/qemu-iotests: skip 108 when FUSE is not loaded
  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/qemu-iotests/108     |   6 ++
 tests/qemu-iotests/223.out |   4 +-
 tests/qemu-iotests/307.out |   4 +-
 tests/vm/centos            |  10 +--
 tests/vm/centos.aarch64    | 174 ++++++-------------------------------
 5 files changed, 41 insertions(+), 157 deletions(-)

-- 
2.34.3

[PATCH 1/5] tests/qemu-iotests: hotfix for 307, 223 output

[John Snow]

Fixes: 58a6fdcc
Signed-off-by: John Snow <jsnow@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 2/5] 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
index 9e923d6a59f..e401c5e9933 100755
--- a/tests/qemu-iotests/108
+++ b/tests/qemu-iotests/108
@@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
 else
     loopdev=false
 
+    # Check for fuse support in the host environment:
+    lsmod | grep fuse &>/dev/null;
+    if [[ $? -ne 0 ]]; then
+        _notrun 'No Passwordless sudo nor FUSE kernel module'
+    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 3/5] 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>
---
 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 4/5] 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>
---
 tests/vm/centos | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/vm/centos b/tests/vm/centos
index be4f6ff2f14..f5bbdecf62d 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>
@@ -18,7 +18,7 @@ import basevm
 import time
 
 class CentosVM(basevm.BaseVM):
-    name = "centos"
+    name = "centos8s"
     arch = "x86_64"
     BUILD_SCRIPT = """
         set -e;
@@ -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 5/5] 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.

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

diff --git a/tests/vm/centos.aarch64 b/tests/vm/centos.aarch64
index 96c450f8be5..022bf17f7bb 100755
--- a/tests/vm/centos.aarch64
+++ b/tests/vm/centos.aarch64
@@ -20,150 +20,42 @@ 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, "
+    ),
+    # "--nobest" for dnf install of docker
+    # "alternatives --set python /usr/bin/python3, "
     # 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 = "centos8s.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 +77,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

Re: [PATCH 3/5] tests/vm: use 'cp' instead of 'ln' for temporary vm images

[Thomas Huth]

On 14/06/2022 03.50, John Snow wrote:
> 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>
> ---
>   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])

I wonder whether it would make sense to use "qemu-img create -b" instead to 
save some disk space?

Anyway, your patch is certainly already an improvement, so:

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

Re: [PATCH 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream

[Thomas Huth]

On 14/06/2022 03.50, John Snow wrote:
> 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>
> ---
>   tests/vm/centos | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

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

Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Thomas Huth]

On 14/06/2022 03.50, 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> index 9e923d6a59f..e401c5e9933 100755
> --- a/tests/qemu-iotests/108
> +++ b/tests/qemu-iotests/108
> @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
>   else
>       loopdev=false
>   
> +    # Check for fuse support in the host environment:
> +    lsmod | grep fuse &>/dev/null;

That doesn't work if fuse has been linked statically into the kernel. Would 
it make sense to test for /sys/fs/fuse instead?

(OTOH, we likely hardly won't run this on statically linked kernels anyway, 
so it might not matter too much)

> +    if [[ $? -ne 0 ]]; then

I'd prefer single "[" instead of "[[" ... but since we're requiring bash 
anyway, it likely doesn't matter.

> +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> +    fi
> +
>       # QSD --export fuse will either yield "Parameter 'id' is missing"
>       # or "Invalid parameter 'fuse'", depending on whether there is
>       # FUSE support or not.

  Thomas

Re: [PATCH 1/5] tests/qemu-iotests: hotfix for 307, 223 output

[Daniel P. Berrangé]

On Mon, Jun 13, 2022 at 09:50:40PM -0400, John Snow wrote:
> Fixes: 58a6fdcc

CC'ing Eric given the above commit hash

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/223.out | 4 ++--
>  tests/qemu-iotests/307.out | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Tested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> 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
> 

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 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Daniel P. Berrangé]

On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> On 14/06/2022 03.50, 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 | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > index 9e923d6a59f..e401c5e9933 100755
> > --- a/tests/qemu-iotests/108
> > +++ b/tests/qemu-iotests/108
> > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> >   else
> >       loopdev=false
> > +    # Check for fuse support in the host environment:
> > +    lsmod | grep fuse &>/dev/null;
> 
> That doesn't work if fuse has been linked statically into the kernel. Would
> it make sense to test for /sys/fs/fuse instead?
> 
> (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> so it might not matter too much)

But more importantly 'lsmod' may not be installed in our container
images. So checking /sys/fs/fuse avoids introducing a dep on the
'kmod' package.

> 
> > +    if [[ $? -ne 0 ]]; then
> 
> I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> anyway, it likely doesn't matter.

Or

    if  test $? != 0 ; then

> 
> > +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> > +    fi
> > +
> >       # QSD --export fuse will either yield "Parameter 'id' is missing"
> >       # or "Invalid parameter 'fuse'", depending on whether there is
> >       # FUSE support or not.
> 
>  Thomas
> 

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 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream

[Daniel P. Berrangé]

On Mon, Jun 13, 2022 at 09:50:43PM -0400, John Snow wrote:
> 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>
> ---
>  tests/vm/centos | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/vm/centos b/tests/vm/centos
> index be4f6ff2f14..f5bbdecf62d 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>
> @@ -18,7 +18,7 @@ import basevm
>  import time
>  
>  class CentosVM(basevm.BaseVM):
> -    name = "centos"
> +    name = "centos8s"


What's the effect of this ?  It feels a little odd to set name to 'centos8s'
here but have this file still called just 'centos' - I assume the 'name'
variable was intended to always match the filename

>      arch = "x86_64"
>      BUILD_SCRIPT = """
>          set -e;
> @@ -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")


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 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[John Snow]

On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> > On 14/06/2022 03.50, 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 | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > > index 9e923d6a59f..e401c5e9933 100755
> > > --- a/tests/qemu-iotests/108
> > > +++ b/tests/qemu-iotests/108
> > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> > >   else
> > >       loopdev=false
> > > +    # Check for fuse support in the host environment:
> > > +    lsmod | grep fuse &>/dev/null;
> >
> > That doesn't work if fuse has been linked statically into the kernel. Would
> > it make sense to test for /sys/fs/fuse instead?
> >
> > (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> > so it might not matter too much)
>
> But more importantly 'lsmod' may not be installed in our container
> images. So checking /sys/fs/fuse avoids introducing a dep on the
> 'kmod' package.
>
> >
> > > +    if [[ $? -ne 0 ]]; then
> >
> > I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> > anyway, it likely doesn't matter.
>
> Or
>
>     if  test $? != 0 ; then
>
> >
> > > +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> > > +    fi
> > > +
> > >       # QSD --export fuse will either yield "Parameter 'id' is missing"
> > >       # or "Invalid parameter 'fuse'", depending on whether there is
> > >       # FUSE support or not.
> >

Good suggestions, thanks!

--js

Re: [PATCH 3/5] tests/vm: use 'cp' instead of 'ln' for temporary vm images

[John Snow]

On Tue, Jun 14, 2022 at 12:40 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 14/06/2022 03.50, John Snow wrote:
> > 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>
> > ---
> >   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])
>
> I wonder whether it would make sense to use "qemu-img create -b" instead to
> save some disk space?
>
> Anyway, your patch is certainly already an improvement, so:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I wondered the same, but decided to keep a smaller series this time
around. VM tests already use a lot of space, so I doubt this is adding
new constraints that didn't exist before. A more rigorous overhaul may
be in order, but not right now. (It looks like the config file stuff
to override defaults is not necessarily rigorously respected by the
different installer recipes.)

I think the caching of the fully set-up image needs work, too. In
practice we leave the image sitting around, but we seem to always
rebuild it no matter what, so it's not that useful. There's a few
things that can be done here to drastically speed up some things,
but... later.

--js

Re: [PATCH 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream

[John Snow]

On Tue, Jun 14, 2022 at 5:09 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jun 13, 2022 at 09:50:43PM -0400, John Snow wrote:
> > 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>
> > ---
> >  tests/vm/centos | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/vm/centos b/tests/vm/centos
> > index be4f6ff2f14..f5bbdecf62d 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>
> > @@ -18,7 +18,7 @@ import basevm
> >  import time
> >
> >  class CentosVM(basevm.BaseVM):
> > -    name = "centos"
> > +    name = "centos8s"
>
>
> What's the effect of this ?  It feels a little odd to set name to 'centos8s'
> here but have this file still called just 'centos' - I assume the 'name'
> variable was intended to always match the filename
>

Changes the logfile names in ~/.cache/qemu-vm, changes the hostname
config in gen_cloud_init_iso(), not much else.

You're right, though, I shouldn't change it in one place but not the
other ... I'll just leave it as "centos". I felt compelled briefly to
indicate it was "the newer, different CentOS" but with the old one
being EOL I suppose it's easy enough to infer.

--js

Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[John Snow]

On Tue, Jun 14, 2022 at 10:30 AM John Snow <jsnow@redhat.com> wrote:
>
> On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> > > On 14/06/2022 03.50, 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 | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > > > index 9e923d6a59f..e401c5e9933 100755
> > > > --- a/tests/qemu-iotests/108
> > > > +++ b/tests/qemu-iotests/108
> > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> > > >   else
> > > >       loopdev=false
> > > > +    # Check for fuse support in the host environment:
> > > > +    lsmod | grep fuse &>/dev/null;
> > >
> > > That doesn't work if fuse has been linked statically into the kernel. Would
> > > it make sense to test for /sys/fs/fuse instead?
> > >
> > > (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> > > so it might not matter too much)
> >
> > But more importantly 'lsmod' may not be installed in our container
> > images. So checking /sys/fs/fuse avoids introducing a dep on the
> > 'kmod' package.
> >
> > >
> > > > +    if [[ $? -ne 0 ]]; then
> > >
> > > I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> > > anyway, it likely doesn't matter.
> >
> > Or
> >
> >     if  test $? != 0 ; then
> >
> > >
> > > > +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> > > > +    fi
> > > > +
> > > >       # QSD --export fuse will either yield "Parameter 'id' is missing"
> > > >       # or "Invalid parameter 'fuse'", depending on whether there is
> > > >       # FUSE support or not.
> > >
>
> Good suggestions, thanks!
>

I think I need to test against /dev/fuse instead, because /sys/fs/fuse
actually exists, but because of docker permissions (etc), FUSE isn't
actually usable from the child container.

I wound up with this:

# Check for usable FUSE in the host environment:
if test ! -c "/dev/fuse"; then
    _notrun 'No passwordless sudo nor usable /dev/fuse'
fi

Seems to work for my case here, at least, but I don't have a good
sense for how broadly flexible it might be. It might be nicer to
concoct some kind of NOP fuse mount instead, but I wasn't able to
figure out such a command quickly.

The next problem I have is actually related; test-qga (for the
Centos.x86_64 run) is failing because the guest agent is reading
/proc/self/mountinfo -- which contains entries for block devices that
are not visible in the current container scope. I think when QGA goes
to read info about these devices to populate a response, it chokes.
This might be a genuine bug in QGA if we want it to tolerate existing
inside of a container.

--js

Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Daniel P. Berrangé]

On Wed, Jun 15, 2022 at 09:41:32AM -0400, John Snow wrote:
> On Tue, Jun 14, 2022 at 10:30 AM John Snow <jsnow@redhat.com> wrote:
> >
> > On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> > > > On 14/06/2022 03.50, 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 | 6 ++++++
> > > > >   1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > > > > index 9e923d6a59f..e401c5e9933 100755
> > > > > --- a/tests/qemu-iotests/108
> > > > > +++ b/tests/qemu-iotests/108
> > > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> > > > >   else
> > > > >       loopdev=false
> > > > > +    # Check for fuse support in the host environment:
> > > > > +    lsmod | grep fuse &>/dev/null;
> > > >
> > > > That doesn't work if fuse has been linked statically into the kernel. Would
> > > > it make sense to test for /sys/fs/fuse instead?
> > > >
> > > > (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> > > > so it might not matter too much)
> > >
> > > But more importantly 'lsmod' may not be installed in our container
> > > images. So checking /sys/fs/fuse avoids introducing a dep on the
> > > 'kmod' package.
> > >
> > > >
> > > > > +    if [[ $? -ne 0 ]]; then
> > > >
> > > > I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> > > > anyway, it likely doesn't matter.
> > >
> > > Or
> > >
> > >     if  test $? != 0 ; then
> > >
> > > >
> > > > > +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> > > > > +    fi
> > > > > +
> > > > >       # QSD --export fuse will either yield "Parameter 'id' is missing"
> > > > >       # or "Invalid parameter 'fuse'", depending on whether there is
> > > > >       # FUSE support or not.
> > > >
> >
> > Good suggestions, thanks!
> >
> 
> I think I need to test against /dev/fuse instead, because /sys/fs/fuse
> actually exists, but because of docker permissions (etc), FUSE isn't
> actually usable from the child container.
> 
> I wound up with this:
> 
> # Check for usable FUSE in the host environment:
> if test ! -c "/dev/fuse"; then
>     _notrun 'No passwordless sudo nor usable /dev/fuse'
> fi
> 
> Seems to work for my case here, at least, but I don't have a good
> sense for how broadly flexible it might be. It might be nicer to
> concoct some kind of NOP fuse mount instead, but I wasn't able to
> figure out such a command quickly.
> 
> The next problem I have is actually related; test-qga (for the
> Centos.x86_64 run) is failing because the guest agent is reading
> /proc/self/mountinfo -- which contains entries for block devices that
> are not visible in the current container scope. I think when QGA goes
> to read info about these devices to populate a response, it chokes.
> This might be a genuine bug in QGA if we want it to tolerate existing
> inside of a container.

Yes, we should fix this. Even if you don't run QGA in a container,
someone might configure the systemd service to harden it, by
restricting what /dev it is able to see and thus trigger the
same issue.


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 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[John Snow]

On Wed, Jun 15, 2022 at 11:33 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jun 15, 2022 at 09:41:32AM -0400, John Snow wrote:
> > On Tue, Jun 14, 2022 at 10:30 AM John Snow <jsnow@redhat.com> wrote:
> > >
> > > On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> > > > > On 14/06/2022 03.50, 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 | 6 ++++++
> > > > > >   1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > > > > > index 9e923d6a59f..e401c5e9933 100755
> > > > > > --- a/tests/qemu-iotests/108
> > > > > > +++ b/tests/qemu-iotests/108
> > > > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> > > > > >   else
> > > > > >       loopdev=false
> > > > > > +    # Check for fuse support in the host environment:
> > > > > > +    lsmod | grep fuse &>/dev/null;
> > > > >
> > > > > That doesn't work if fuse has been linked statically into the kernel. Would
> > > > > it make sense to test for /sys/fs/fuse instead?
> > > > >
> > > > > (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> > > > > so it might not matter too much)
> > > >
> > > > But more importantly 'lsmod' may not be installed in our container
> > > > images. So checking /sys/fs/fuse avoids introducing a dep on the
> > > > 'kmod' package.
> > > >
> > > > >
> > > > > > +    if [[ $? -ne 0 ]]; then
> > > > >
> > > > > I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> > > > > anyway, it likely doesn't matter.
> > > >
> > > > Or
> > > >
> > > >     if  test $? != 0 ; then
> > > >
> > > > >
> > > > > > +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> > > > > > +    fi
> > > > > > +
> > > > > >       # QSD --export fuse will either yield "Parameter 'id' is missing"
> > > > > >       # or "Invalid parameter 'fuse'", depending on whether there is
> > > > > >       # FUSE support or not.
> > > > >
> > >
> > > Good suggestions, thanks!
> > >
> >
> > I think I need to test against /dev/fuse instead, because /sys/fs/fuse
> > actually exists, but because of docker permissions (etc), FUSE isn't
> > actually usable from the child container.
> >
> > I wound up with this:
> >
> > # Check for usable FUSE in the host environment:
> > if test ! -c "/dev/fuse"; then
> >     _notrun 'No passwordless sudo nor usable /dev/fuse'
> > fi
> >
> > Seems to work for my case here, at least, but I don't have a good
> > sense for how broadly flexible it might be. It might be nicer to
> > concoct some kind of NOP fuse mount instead, but I wasn't able to
> > figure out such a command quickly.
> >
> > The next problem I have is actually related; test-qga (for the
> > Centos.x86_64 run) is failing because the guest agent is reading
> > /proc/self/mountinfo -- which contains entries for block devices that
> > are not visible in the current container scope. I think when QGA goes
> > to read info about these devices to populate a response, it chokes.
> > This might be a genuine bug in QGA if we want it to tolerate existing
> > inside of a container.
>
> Yes, we should fix this. Even if you don't run QGA in a container,
> someone might configure the systemd service to harden it, by
> restricting what /dev it is able to see and thus trigger the
> same issue.

Naive solution: if we try to look in /sys/dev/block/%u:%u and find
that we are unable to do so for whatever reason (ENOENT et al), just
skip that entry for the fsinfo returned to the caller.

Does it need to be fancier than that?

--js

Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded

[Daniel P. Berrangé]

On Wed, Jun 15, 2022 at 11:48:02AM -0400, John Snow wrote:
> On Wed, Jun 15, 2022 at 11:33 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Jun 15, 2022 at 09:41:32AM -0400, John Snow wrote:
> > > On Tue, Jun 14, 2022 at 10:30 AM John Snow <jsnow@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > >
> > > > > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote:
> > > > > > On 14/06/2022 03.50, 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 | 6 ++++++
> > > > > > >   1 file changed, 6 insertions(+)
> > > > > > >
> > > > > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> > > > > > > index 9e923d6a59f..e401c5e9933 100755
> > > > > > > --- a/tests/qemu-iotests/108
> > > > > > > +++ b/tests/qemu-iotests/108
> > > > > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then
> > > > > > >   else
> > > > > > >       loopdev=false
> > > > > > > +    # Check for fuse support in the host environment:
> > > > > > > +    lsmod | grep fuse &>/dev/null;
> > > > > >
> > > > > > That doesn't work if fuse has been linked statically into the kernel. Would
> > > > > > it make sense to test for /sys/fs/fuse instead?
> > > > > >
> > > > > > (OTOH, we likely hardly won't run this on statically linked kernels anyway,
> > > > > > so it might not matter too much)
> > > > >
> > > > > But more importantly 'lsmod' may not be installed in our container
> > > > > images. So checking /sys/fs/fuse avoids introducing a dep on the
> > > > > 'kmod' package.
> > > > >
> > > > > >
> > > > > > > +    if [[ $? -ne 0 ]]; then
> > > > > >
> > > > > > I'd prefer single "[" instead of "[[" ... but since we're requiring bash
> > > > > > anyway, it likely doesn't matter.
> > > > >
> > > > > Or
> > > > >
> > > > >     if  test $? != 0 ; then
> > > > >
> > > > > >
> > > > > > > +        _notrun 'No Passwordless sudo nor FUSE kernel module'
> > > > > > > +    fi
> > > > > > > +
> > > > > > >       # QSD --export fuse will either yield "Parameter 'id' is missing"
> > > > > > >       # or "Invalid parameter 'fuse'", depending on whether there is
> > > > > > >       # FUSE support or not.
> > > > > >
> > > >
> > > > Good suggestions, thanks!
> > > >
> > >
> > > I think I need to test against /dev/fuse instead, because /sys/fs/fuse
> > > actually exists, but because of docker permissions (etc), FUSE isn't
> > > actually usable from the child container.
> > >
> > > I wound up with this:
> > >
> > > # Check for usable FUSE in the host environment:
> > > if test ! -c "/dev/fuse"; then
> > >     _notrun 'No passwordless sudo nor usable /dev/fuse'
> > > fi
> > >
> > > Seems to work for my case here, at least, but I don't have a good
> > > sense for how broadly flexible it might be. It might be nicer to
> > > concoct some kind of NOP fuse mount instead, but I wasn't able to
> > > figure out such a command quickly.
> > >
> > > The next problem I have is actually related; test-qga (for the
> > > Centos.x86_64 run) is failing because the guest agent is reading
> > > /proc/self/mountinfo -- which contains entries for block devices that
> > > are not visible in the current container scope. I think when QGA goes
> > > to read info about these devices to populate a response, it chokes.
> > > This might be a genuine bug in QGA if we want it to tolerate existing
> > > inside of a container.
> >
> > Yes, we should fix this. Even if you don't run QGA in a container,
> > someone might configure the systemd service to harden it, by
> > restricting what /dev it is able to see and thus trigger the
> > same issue.
> 
> Naive solution: if we try to look in /sys/dev/block/%u:%u and find
> that we are unable to do so for whatever reason (ENOENT et al), just
> skip that entry for the fsinfo returned to the caller.
> 
> Does it need to be fancier than that?

/sys stuff maybe unfiltered, while /dev is restricted.

I've not looked at the QGA code for this, but conceptually I think
I would just identify where in the code errors hit, and ignore the
appropriate error conditions. The goal is to return as much info
as we reasonably can offer, given our execution environment
constraints.


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 :|