Using Yum's setopt with Salt fails only for dom0

ben-grande commented 3 weeks ago

How to file a helpful issue

Qubes OS release

R4.2

Brief summary

On qubes-dom0-update, can't pass value of an option separated by space as it is treated differently.

This is especially problematic when the format of the command can't be controlled, such as when using Salt.

Setopt value separated by space fails:

$ sudo qubes-dom0-update --setopt install_weak_deps=False --action=install scrot
Using sys-pihole as UpdateVM for Dom0
Downloading packages. This may take a while...
script --quiet --return --command '/usr/lib/qubes/qubes-download-dom0-updates.sh --doit --nogui '\''--exclude=qubes-template-*'\'' '\''--setopt'\'' '\''install_weak_deps=False'\'' '\''--action=install'\'' '\''scrot'\''' /dev/null

Unable to detect release version (use '--releasever' to specify release version)
fakeroot dnf --best --allowerasing --noplugins -y install --downloadonly --installroot /var/lib/qubes/dom0-updates --config=/var/lib/qubes/dom0-updates/etc/dnf/dnf.conf --setopt=reposdir=/var/lib/qubes/dom0-updates/etc/yum.repos.d --exclude=qubes-template-* --setopt install_weak_deps=False scrot
...
Complete!
The downloaded packages were saved in cache until the next successful transaction.
You can remove cached packages by executing 'dnf clean packages'.
Setopt argument has no value: install
No such command: install_weak_deps=False. Please use /usr/bin/dnf --help
It could be a DNF plugin command, try: "dnf install 'dnf-command(install_weak_deps=False)'"

Setopt separated by = works:

$ sudo qubes-dom0-update --setopt=install_weak_deps=False --action=install scrot
Using sys-pihole as UpdateVM for Dom0
Downloading packages. This may take a while...
script --quiet --return --command '/usr/lib/qubes/qubes-download-dom0-updates.sh --doit --nogui '\''--exclude=qubes-template-*'\'' '\''--setopt=install_weak_deps=False'\'' '\''--action=install'\'' '\''scrot'\''' /dev/null
Unable to detect release version (use '--releasever' to specify release version)
fakeroot dnf --best --allowerasing --noplugins -y install --downloadonly --installroot /var/lib/qubes/dom0-updates --config=/var/lib/qubes/dom0-updates/etc/dnf/dnf.conf --setopt=reposdir=/var/lib/qubes/dom0-updates/etc/yum.repos.d --exclude=qubes-template-* --setopt=install_weak_deps=False scrot
...
Complete!
The downloaded packages were saved in cache until the next successful transaction.
You can remove cached packages by executing 'dnf clean packages'.

Steps to reproduce

With the salt state:

install:
  pkg.installed:
    - install_recommends: False
    - skip_suggestions: True
    - setopt: "install_weak_deps=False"
    - pkgs:
      - scrot

Targetting dom0:

DEBUG Loaded plugins: builddep, changelog, config-manager, copr, debug, debuginfo-install, download, generate_completion_cache, groups-manager, needs-restarting, playground, repoclosure, repodiff, repograph, repomanage, reposync, system-upgrade
DEBUG DNF version: 4.18.0
DDEBUG Command: dnf --exclude=qubes-template-* -y --best --allowerasing --setopt install install_weak_deps=False scrot
DDEBUG Installroot: /
DDEBUG Releasever: 4.2
DEBUG cachedir: /var/cache/dnf
CRITICAL No such command: install_weak_deps=False. Please use /usr/bin/dnf --help
CRITICAL It could be a DNF plugin command, try: "dnf install 'dnf-command(install_weak_deps=False)'"

Targetting a fedora-40 qube:

DEBUG Loaded plugins: builddep, changelog, config-manager, copr, debug, debuginfo-install, download, generate_completion_cache, groups-manager, needs-restarting, playground, qubes-hooks, repoclosure, repodiff, repograph, repomanage, reposync, system-upgrade
DEBUG DNF version: 4.21.1
DDEBUG Command: dnf -y --best --allowerasing --setopt install_weak_deps=False install scrot
DDEBUG Installroot: /
DDEBUG Releasever: 40
DEBUG cachedir: /var/cache/dnf
DDEBUG Base command: install
DDEBUG Extra commands: ['-y', '--best', '--allowerasing', '--setopt', 'install_weak_deps=False', 'install', 'scrot']
DEBUG User-Agent: constructed: 'libdnf (Fedora Linux 40; generic; Linux.x86_64)'

Expected behavior

Qubes Salt module builds command line to qubes-dom0-update that doesn't fail when it is a valid dnf syntax.

Actual behavior

Same state that works on Fedora qubes fails on Dom0 due to how qubes-dom0-update interpret arguments.

marmarek commented 3 weeks ago

I know it wouldn't be very consistent, but maybe a better approach is to adjust qubes-dom0-update to understand that --setopt has an argument? I hope it applies to only very few options used by salt (for example --enablerepo= works just fine), maybe just --setopt.

ben-grande commented 3 weeks ago

I know it wouldn't be very consistent, but maybe a better approach is to adjust qubes-dom0-update to understand that --setopt has an argument?

I can check if setopt item does not contain = and them replace the space between it and the next item with =, which I guess is less prone to mistakes as maintainer might forget to patch the setopt line when upgrading yumpkg from upstream.

I hope it applies to only very few options used by salt (for example --enablerepo= works just fine), maybe just --setopt.

I don't have the percentage of how many Yum options Salt covers, but not all for sure, which can cause inconsistencies. In this case, as it covers --enablerepo= with the correct separator, it doesn't cause problems.

It is actually an inconsistency on the salt code to not have the = separator, see how setopt differs:

https://github.com/QubesOS/qubes-mgmt-salt-dom0-update/blob/fd81a5a7d2847fb4ed7afa6d573721e21cc49481/_modules/qubes_dom0_update.py#L320-L351

    if fromrepo:
        ...
        ret.extend(["--disablerepo=*", f"--enablerepo={fromrepo}"])
    else:
            ...
            ret.extend([f"--disablerepo={x}" for x in targets])
        if enablerepo:
            ...
            ret.extend([f"--enablerepo={x}" for x in targets])

    if disableexcludes:
        ...
        ret.append(f"--disableexcludes={disableexcludes}")

    if branch:
        ...
        ret.append(f"--branch={branch}")

    for item in setopt:
        ret.extend(["--setopt", str(item)])

    if get_extra_options:
                ...
                ret.append(f"--{key}={value}")

ben-grande commented 3 weeks ago

Well, as setopt was irregular compared to the other options, I believe that give us some leverage to fix it upstream as it would benefit them also to have a standard format:

But in case it is not accepted or delayed to be in a release packaged in Qubes:

https://github.com/QubesOS/qubes-core-admin-linux/pull/171

ben-grande commented 3 weeks ago

PR was merged to upstream's 3006.x branch. Next release 3006.10 might take some time as there are 38 open issues to complete that milestone. Now it is your choice to wait for the 3006.10 release or merging one of these PRs:

This issue can be closed if just waiting for upstream.

QubesOS / qubes-issues