[policies] Prefer most specific policy when multiple match

[asadawar]

Problem

When multiple policies within the same module both return True from check(), the policy loader selects whichever class sorts first alphabetically via inspect.getmembers(). This causes RHELPolicy to always win over RedHatCoreOSPolicy inside a toolbox container on RHCOS, because uppercase H sorts before lowercase e in ASCII.

Both policies return True in that context: RHEL matches the container's /etc/redhat-release, and RHCOS matches the host's $HOST/etc/os-release. But RHELPolicy is iterated first and the loader breaks on first match.

This has existed since RedHatCoreOSPolicy was introduced in 2019 (commit fa06bc0) but was never visible because RHCOS had no behavioral differences from RHEL. After the archive naming change in commit 0e919b6, RedHatCoreOSPolicy.get_archive_name() includes the OCP node role and cluster name, but it is never called because the RHEL policy is selected instead.

Reproduction

Inside a toolbox container on an RHCOS node:

from sos.policies import import_policy
for p in import_policy('redhat'):
    print(p.__name__, '->', 'check():', p.check())

Output:

RHELPolicy -> check(): True
RedHatCoreOSPolicy -> check(): True

The loader picks RHELPolicy (first match) and never reaches RedHatCoreOSPolicy.

Fix

Collect all matching policies from a module and sort by MRO depth (descending), so a subclass is always preferred over its parent. A subclass that returns True from check() is by definition a more precise match.

RedHatCoreOSPolicy  MRO depth: 6  (RedHatCoreOSPolicy > RHELPolicy > RedHatPolicy > LinuxPolicy > Policy > object)
RHELPolicy          MRO depth: 5  (RHELPolicy > RedHatPolicy > LinuxPolicy > Policy > object)

This is safe for all existing policy pairs:

• CentOS and Fedora have distinct os_release_file and os_release_id values, so they never both match alongside RHEL
• No existing subclass returns True from check() when its parent should be preferred instead

Alternative considered

Move RedHatCoreOSPolicy to a separate module file (e.g. coreos.py), similar to how Google Container-Optimized OS has cos.py. Module files are sorted alphabetically by ImporterHelper.get_modules(), and coreos sorts before redhat, so RHCOS would be checked first. This avoids changing the loader but requires updating test imports (from sos.policies.distros.redhat import RedHatCoreOSPolicy) and could confuse the naming with the existing cos.py (Google COS).

Test

Verified on OCP 4.22.4, RHCOS 9.8, toolbox with sos-4.11.2-2.el9_8:

• RedHatCoreOSPolicy.check() returns True
• _get_node_role() returns correct role (e.g. worker)
• _get_cluster_name() returns correct cluster name (e.g. sharedocp422)
• Without this fix: archive is sosreport-<hostname>-<date>-<rand>.tar.xz
• With this fix: the RHCOS policy is selected, enabling the role and cluster name in the archive

Related: RHEL-188753

Assisted-by: Claude Code https://claude.ai/code
Signed-off-by: Abhijeet Sadawarte asadawar@redhat.com

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/sosreport-sos-4397
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[pmoravec]

Nice idea, to pick us as-most-childish class as possible (well, written it this way, it sounds silly :) ).

[pmoravec]

For some reason, UbuntuPolicy (as child of DebianPolicy) is not used on Ubuntu test system, while DebianPolicy is picked. That is opposite to expected behaviour of the patch.

The reason is one "distro branch" (ubuntu/debian) is described in two different files. debian module is tested first, offering just DebianPolicy, so it is picked. Later ubuntu module is not considered at all.

Maybe we need to merge ubuntu and debian distros into one file, or populate matches for all modules, and sort+decide at the end?

[pmoravec]

As described in a comment, debian module match skips ubuntu module completely, so we should move this outside for module .. loop. Like:

    import sos.policies.distros
    helper = ImporterHelper(sos.policies.distros)
    matches = []
    for module in helper.get_modules():
        matches.extend([policy for policy in import_policy(module)
                   if policy.check(remote=remote_check)])
    if matches:
        matches.sort(key=lambda p: len(p.__mro__), reverse=True)
        cache['policy'] = matches[0](sysroot=sysroot, init=init,
                                     probe_runtime=probe_runtime,
                                     remote_exec=remote_exec)

This works on Ubuntu, I havent tested it anywhere else.

This code assumes a system is detected to be applicable to a list of *Policy that constitute a chain, but not a tree with different branches. I.e. once the system is below, say, DebianPolicy, it can never ever be detected by RedHatPolicy or its children.

[asadawar]

Thanks @pmoravec, good catch on the Ubuntu/Debian cross-module case. Updated to collect matches across all modules before sorting, as you suggested.

[pmoravec]

This error is not a regression introduced by the patch, but a newly discovered issue that was present in previous code as well.

Learning more on this error, it really can cause issues in future(*) - e.g. in multi-threaded collector that collects sosreports from systems with different policies. Assume (very artificial, but very evident) use case sos collect is invoked for RHEL and Ubuntu system. Then:

• Thread 1 (RHEL node): checks cache, not found, starts loading RHELPolicy
• Thread 2 (Ubuntu node): checks cache, not found, starts loading UbuntuPolicy
• Thread 1: cache['policy'] = RHELPolicy(...)
• Thread 2: cache['policy'] = UbuntuPolicy(...) ← overwrites!
• Thread 1: returns UbuntuPolicy for RHEL node ← wrong!

(*) Currently, we always call the load method either from single-threaded execution, or with explicit cache={}:

host = load(cache={}, sysroot=self.opts.sysroot, ...)    # sosnode.py line 417

So the code is safe now. Still we should fix the (legitimate, though not applicable today) CodeQL error warning.

We can either:

• add some thread locking in place, to be really safe (imho redundantly complex for us)
• remove cache default value and compute it every time from scratch (adds a tiny runtime)
• document the current intentional usage properly - both in load method and in sosnode.py method invocation, and suppress this CodeQL error

Any preference here?