XCP-ng 9 fixes by glehmann · Pull Request #570 · xcp-ng/xcp-ng-tests

glehmann · 2026-06-04T16:42:16Z

Adapt to XCP-ng 9 specificities

rzr · 2026-08-04T15:44:42Z


        self.rescan_block_devices_info()

+        if self.file_exists('/usr/bin/yum'):


woultnt it make sense to prefer dnf over yum ?

I’d say that for now, yum is the default, since we’re on version 8.3 and are adding dnf alongside it for 9.0. And as we don't have dnf on the 8.3 (and before).

If we wanted dnf to be the default over yum for 9.0, we would need to base that on version detection first, before the presence of the file.

There is no yum command on XCP-ng 9, so it doesn't matter much in practice :)

There is actually a yum compatibility wrapper (in package yum). It's surely safer to look for dnf first, just in case.

stormi · 2026-08-04T15:46:39Z

        try:
-            history_str = self.ssh('yum history list --noplugins')
+            history_str = self.ssh(f'{self.package_manager} history list --noplugins')
        except commands.SSHCommandFailed:


It's not directly related to your PR, and is probably code I wrote initially, so I just make it a suggestion: we should probably only tolerate failures that we can verify to be actually caused by an empty history, rather than any command failure that could happen for whatever reason. And let the exception through in the other cases.

stormi · 2026-08-04T15:50:04Z

+        if not history:
+            # we need a transaction to already exist. Install a small package with no deps, and remove it immediately.
+            logging.info(f"[{self}] Install and remove a small package to workaround empty yum history.")
+            small_pkg = 'gpm-libs' if self.xcp_version.major == 8 else 'bubblewrap'


I suggest dummypkg, a package that must be present in our repos because other tests rely on it. If not present in XCP-ng 9, we should add it there too.

It's not in xcp-ng 9 yet
We could use the same package on xcp-ng 9 and xcp-ng 8.3 👍

stormi · 2026-08-04T15:57:53Z

        pytest.skip(f"This test requires an XCP-ng < {version_str} host")

-@pytest.fixture(scope='session')
+@pytest.fixture(scope='function')


Most host_xxx fixtures in this global conftest.py are session-scoped. I think that to change the scope we should change the fixture name too to avoid wrong expectations. And/or bring the fixture closer to where it's used.

stormi · 2026-08-04T16:14:18Z

        ret = fn()
        if not invert and ret:
-            return
+            return ret


nitpick: it took me time to understand this part of the commit message: "Enhance wait_for to return the callback value ", because I thought it meant you returned the callback itself, as a value.

Due to the use of word "callback", I thought there was a function we'd call back once returned, but you just mean you return the value produced by the function executed by wait_for.

Suggestion: "Enhance wait_for to return the polled function's result"

stormi · 2026-08-04T16:18:04Z

        tmp_file = res_host.ssh('mktemp')
        session = f"detached-cat-{self.uuid}"
        ret = False
+        # screen install is broken on RHEL, see https://bugzilla.redhat.com/show_bug.cgi?id=2385964


Okay, another "I got confused by the wording" item: I thought there was something about display issues 😂.

But you're talking about screen, the tool.

Suggestion:

The `screen` tool may be broken on RHEL, see https://bugzilla.redhat.com/show_bug.cgi?id=2385964

stormi · 2026-08-04T16:24:44Z

I forgot to ask: was the PR tested on XCP-ng 8 too? Probably so but it would be best to mention it in the PR description so that we don't ask :)

I tested both for the tests I modified, but I haven't run a complete run on XCP-ng 8.3 yet.

glehmann · 2026-08-27T12:00:48Z

I integrated the requested changes and updated on master

ydirson · 2026-08-27T15:15:55Z


        self.rescan_block_devices_info()

+        if self.file_exists('/usr/bin/yum'):


There is actually a yum compatibility wrapper (in package yum). It's surely safer to look for dnf first, just in case.

ydirson · 2026-08-27T15:29:21Z

        """
        try:
-            history_str = self.ssh('yum history list --noplugins')
-        except commands.SSHCommandFailed:
-            # yum history list fails if the list is empty, and it's also not possible to rollback
-            # to before the first transaction, so "0" would not be appropriate as last transaction.
-            # To workaround this, create transactions: install and remove a small package.
-            logging.info(f"[{self}] Install and remove a small package to workaround empty yum history.")
-            self.yum_install(['gpm-libs'])
-            self.yum_remove(['gpm-libs'])
-            history_str = self.ssh('yum history list --noplugins')
-
-        history = history_str.splitlines()
-        line_index = None
-        for i in range(len(history)):
-            if history[i].startswith('--------'):
-                line_index = i
-                break
+            history_str = self.ssh(f'{self.package_manager} history list --noplugins')
+        except commands.SSHCommandFailed as e:
+            if 'Error: Failed history list' not in e.stdout:
+                # this is not the expected error for an empty history list, re-raise the exception
+                raise e
+            # yum history list fails on xcp-ng 8 if the list is empty, but dnf history list works properly on xcp-ng 9.
+            # Just use a fake empty value to deal with both versions in the same way.
+            if self.package_manager == 'yum':
+                history_str = '''ID     | Command line             | Date and time    | Action(s)      | Altered
+-------------------------------------------------------------------------------'''
+            else:
+                raise

-        if line_index is None:
-            raise Exception('Unable to get yum transactions')
+        def split_history(history_str: str) -> list[str]:
+            history = history_str.splitlines()
+            line_index = None
+            for i in range(len(history)):
+                if history[i].startswith('--------'):
+                    line_index = i
+                    break
+            if line_index is None:
+                raise Exception('Unable to get yum transactions')
+            return history[line_index + 1:]
+
+        history = split_history(history_str)
+        if not history:
+            # we need a transaction to already exist. Install a small package with no deps, and remove it immediately.
+            logging.info(f"[{self}] Install and remove a small package to workaround empty yum history.")
+            self.yum_install(['dummypkg'])
+            self.yum_remove(['dummypkg'])
+            history = split_history(self.ssh(f'{self.package_manager} history list --noplugins'))

        try:
-            return int(history[line_index + 1].split()[0])
+            return int(history[0].split()[0])
        except ValueError:


(nitpick?) This change could have been easier to read by introducing split_history() in a preliminary commit (which involves some reindentation), and then the effective difference specific to v9 would have stood out more clearly in the diff.

ydirson · 2026-08-27T15:36:21Z

+        except commands.SSHCommandFailed as e:
+            if 'Error: Failed history list' not in e.stdout:
+                # this is not the expected error for an empty history list, re-raise the exception
+                raise e


raise without argument would be more correct I think

Agreed, because it would avoid changing the stack trace 👍

ydirson · 2026-08-27T15:40:20Z

+            logging.info(f"[{self}] Install and remove a small package to workaround empty yum history.")
+            self.yum_install(['dummypkg'])
+            self.yum_remove(['dummypkg'])
+            history = split_history(self.ssh(f'{self.package_manager} history list --noplugins'))


A few words in the commit message about that particular behavior change and how it is restructured would be nice (yes the can read the code and actually understand, but a "reading guide" would actually allow the reader to spend less time)

Done. This feels very much not useful to me, but if that's useful to you…

ydirson · 2026-08-27T15:46:14Z

-        self.ssh(
-            f'yum history rollback --enablerepo=xcp-ng-base,xcp-ng-testing,xcp-ng-updates {self.saved_rollback_id} -y'
-        )
+        repositories = ['xcp-ng-base']
+        if self.xcp_version.major == 8:
+            # TODO: activate those repositories in xcp-ng 9. For now they are not available.
+            repositories += ['xcp-ng-testing', 'xcp-ng-updates']
+        self.ssh(f'{self.package_manager} history rollback'
+                 f' --enablerepo={",".join(repositories)} {self.saved_rollback_id} -y')


This also is not implied by "use the right package manager". Sure this is v9 support, and relates to repositories, so likely has a place grouped as such, but would be nice in the commit message

ydirson · 2026-08-27T15:47:23Z

-            self.yum_install(['gpm-libs'])
-            self.yum_remove(['gpm-libs'])


switching to dummypkg because this package is not available in v9?

Are you sure about that? I added it yesterday :)

ydirson · 2026-08-27T15:52:13Z

-    def check_file_type(expected: str) -> None:
-        assert host.ssh(f'file --mime-type -b {filepath}') == expected
+    def check_file_type(expected: list[str]) -> None:
+        assert host.ssh(f'file --mime-type -b {filepath}') in expected

    if compress == 'none':
-        check_file_type('application/x-tar')
+        check_file_type(['application/x-tar'])
    elif compress == 'gzip':
-        check_file_type('application/x-gzip')
+        check_file_type(['application/x-gzip', 'application/gzip'])
    elif compress == 'zstd':
-        check_file_type('application/octet-stream')
+        check_file_type(['application/octet-stream', 'application/zstd'])


I disagree this makes things "more robust". application/octet-stream is "I did not recognize this file", which should be an error when the more specific type is known. I like my "test_export: support modern distro knowing about zstd and better gzip" patch in ydi/9 better 😄

I replace this commit with yours

ydirson · 2026-08-27T15:59:22Z

            raise InstallationFailed(failed)
        return cmd()
-    return wait_for(inner, msg, timeout_secs=timeout_secs)
+    wait_for(inner, msg, timeout_secs=timeout_secs)


It feels awkward to have wait_for gain a real return value, and just not propagate it here.
Any reason not to change this function to behave similarly to ` (possibly by just changing the return type annotation)?

Because the function to call returns a bool (Callable[[], bool]), so, given the behavior of wait_for, the only possible returned value is Literal[True], which is not very useful.
I'm fine with both versions, though

ydirson · 2026-08-27T16:00:26Z

-def wait_for(fn: Callable[[], object], msg: str | None = None, timeout_secs: int = 2 * 60, retry_delay_secs: int = 2,
-             invert: bool = False) -> None:
+def wait_for(fn: Callable[[], T], msg: str | None = None, timeout_secs: int = 2 * 60, retry_delay_secs: int = 2,
+             invert: bool = False) -> T:


This use of the return value should be documented, a function docstring would seem adequate.

Especially the behavior with invert=True may need some notice, that only False-like values will be returned in this case (so not very useful).

ydirson · 2026-08-27T16:08:25Z

        tmp_file = res_host.ssh('mktemp')
        session = f"detached-cat-{self.uuid}"
        ret = False
+        # The screen package installation is broken on RHEL, see https://bugzilla.redhat.com/show_bug.cgi?id=2385964


s/RHEL/el10/ ?

This is required for XCP-ng 9, which uses dnf. Reading guide for get_last_yum_history_tid(): - Behavior change: the empty-history workaround now only runs when the parsed history is genuinely empty, instead of on any failure of "history list". Other errors now propagate (previously they were masked by the workaround). dnf succeeds on an empty history (only the header is output) while yum fails with "Error: Failed history list", so a fake header row is used for yum to handle both the same way. - Restructure: output parsing was extracted into a split_history() helper, and the workaround (install/remove a dummy package to create a transaction) is now triggered by the empty parsed output rather than by the command failing. Also yum_restore_saved_state() doesn't activate xcp-ng-testing and xcp-ng-updates repositories, as they are not yet available. Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>

Enhance wait_for to return the polled function's result and use it to retry file server header retrieval until the server is ready. Change host_with_hsts fixture to function scope for test isolation, and move it in the module where it's used, to make clear it's not a session scope anymore. Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

glehmann force-pushed the gln/9 branch 4 times, most recently from e487535 to f99e917 Compare June 10, 2026 08:57

glehmann mentioned this pull request Jun 10, 2026

Use vm.detect_package_manager() to check if apk is available #540

Merged

glehmann force-pushed the gln/9 branch from f99e917 to 8253f23 Compare June 10, 2026 09:38

glehmann mentioned this pull request Jun 11, 2026

t/g/u/test_xen_guest_agent.py: Use yum on older distros or dnf if available #582

Closed

glehmann force-pushed the gln/9 branch 3 times, most recently from 387d52f to efc2d57 Compare August 4, 2026 13:56

glehmann marked this pull request as ready for review August 4, 2026 13:57

glehmann requested review from a team as code owners August 4, 2026 13:57

glehmann force-pushed the gln/9 branch from efc2d57 to 59a2648 Compare August 4, 2026 14:19

rzr reviewed Aug 4, 2026

View reviewed changes

rzr approved these changes Aug 4, 2026

View reviewed changes

gduperrey approved these changes Aug 4, 2026

View reviewed changes

dinhngtu approved these changes Aug 4, 2026

View reviewed changes

stormi reviewed Aug 4, 2026

View reviewed changes

ydirson requested review from vxgmichel and ydirson August 19, 2026 08:17

glehmann force-pushed the gln/9 branch from 59a2648 to 650b32c Compare August 27, 2026 11:59

gduperrey approved these changes Aug 27, 2026

View reviewed changes

ydirson requested changes Aug 27, 2026

View reviewed changes

glehmann and others added 4 commits August 28, 2026 11:58

test_export: support modern distro knowing about zstd and better gzip

8ca14f7

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>

Work around broken screen package install on XCP-ng 9 hosts

393fec4

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

glehmann force-pushed the gln/9 branch from 650b32c to 393fec4 Compare August 28, 2026 09:58

glehmann requested review from stormi and ydirson August 28, 2026 09:59


		self.rescan_block_devices_info()

		if self.file_exists('/usr/bin/yum'):

Uh oh!

Conversation

glehmann commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glehmann commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

glehmann commented Jun 4, 2026 •

edited

Loading