request for comments - pacman: speed up most operations when working with a package list

[jraby]

SUMMARY

This code got a bit out of hand...
I started testing ansible a few weeks ago on arch linux and wondered at why checking if a list of package was installed took so long.

I saw #3606 and somehow thought I was already running this code, so I set out to figure out why it was still slow. (I was not, and I realized this after I finished writing this module... 🤦 )

This is the result of that effort.
It is basically a rewrite of the module to use a cache (or inventory) of installed packages and groups, available packages and groups and upgradable packages in order to avoid all the individual calls to pacman that the module was doing.
(I could have plugged a cache in the original code and probably should have, but here we are, I got carried away. I do think this version is a bit easier to follow but I just wrote it, so YMMV)

This version of the module is significantly faster when using state: latest but only marginally faster when using state: present. (it is also a bit slower when listing a single pkg to install / upgrade since the cache/inventory is built anyway)
The performance difference is much more noticeable when a long list of package is used.

I opted to do away with the re module and parsing of pacman's installation/upgrade output which can change depending on the VerbosePkgsList option.
Instead the module will first call --sync/--upgrade --print-format [...], parse that with simple split and strip operations, and then proceed to do the actual installation.
I'm not overly fond of that approach, since it could introduce a time-to-check vs time-of-use problem, but I think this is better than trying to parse the output of pacman which can change depending on the user's settings.

Also, instead of doing each package operation with an individual call to pacman, there's now a single call to pacman with the full list of packages.
pacman is good at reporting errors for individual packages, and those are reported back to the user via stderr.
(or directly in msg if the package cannot be found)

Support for specifying package name in the format <repo>/<packagename> is also a little more robust I think.
(for example, community/elixir is correctly resolved to elixir but would install using pacman -S community/elixir)

Finally, I opted to keep the check mode and the "real mode" together instead of in distinct functions as the original module did. It felt like it would be easier to make sure the check mode was really doing what the real mode would be doing.

Anyways, I thought I might as well show the work now that it is done, even though the speedup in state: present is much less than when I was comparing to ansible without #3606

Oh one more thing, maybe I didn't know where to look since it is my first time looking at ansible code, but I couldn't find any test for the pacman module.
This new version includes as many tests as I could find for the original... :( There are now a bunch of tests for the module.

Performance

For a list of 36 pkgs, doing state: latest:

- name: list of updated pkgs
  become: yes
  pacman:
    state: latest
    name:
      - community/elixir
      - base-devel
      - bc
      - bcc-tools
      - bind
      - bzip2
      - coreutils
      - dstat
      - e2fsprogs
      - file
      - fwupd
      - gdb
      - git
      - gzip
      - htop
      - net-tools
      - openssh
      - perf
      - python
      - python-bcc
      - python-virtualenv
      - rsync
      - sshfs
      - strace
      - sudo
      - sysstat
      - tcpdump
      - throttled
      - tmux
      - unzip
      - util-linux
      - vim
      - wget
      - whois
      - wireguard-tools
      - zip

Old module:

$ time ansible -m include_tasks -a file=test_repo.yml localhost -D  -C
[WARNING]: No inventory was parsed, only implicit localhost is available
localhost | SUCCESS => {
    "changed": false,
    "include": "test_repo.yml",
    "include_args": {}
}
--- before: 
+++ after: installed
@@ -0,0 +1 @@
+community/elixir

localhost | CHANGED => {
    "changed": true,
    "msg": "1 package(s) would be latest"
}

real    0m5.162s
user    0m4.290s
sys     0m1.161s

New module

localhost | SUCCESS => {
    "changed": false,
    "include": "test_repo.yml",
    "include_args": {}
}
localhost | SUCCESS => {
    "changed": false,
    "msg": "package(s) already installed"
}

real    0m1.511s
user    0m1.388s
sys     0m0.184s

When using state: present the difference is much smaller: 0m1.671s vs 0m1.563s

Keep in mind however that the performance gets worse when more pkgs are in the list with the original module, whereas it stays mostly the same with this version (dict lookup vs calling out to pacman)

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

pacman

[ansibullbot]

cc @indrajitr @tchernomax
click here for bot help

[jraby]

hmmm what is the best way to indicate that a variable isn't used if _ isn't allowed?
simply use some name and never refer to it again?

[felixfontein]

Simply use dummy instead of _. (See https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/no-underscore-variable.html for more info.)

[jraby]

thx, I updated the PR and squashed the WIP history.

[felixfontein]

Please note that such rewrites are quite a lot of work to review, so don't be surprised if it takes some time before someone actually starts adding review comments here :)

[ansibullbot]

The test botmeta failed with 1 error:

.github/BOTMETA.yml:0:0: Author jraby not mentioned as active or inactive maintainer for plugins/modules/packaging/os/pacman.py (mentioned are: elasticdog, indrajitr, tchernomax, elasticdog)

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

changelogs/fragments/3907-pacman-speedup.yml:4:5: error: syntax error: could not find expected ':' (syntax)
changelogs/fragments/3907-pacman-speedup.yml:4:5: unparsable-with-libyaml: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/3907-pacman-speedup.yml:0:0: yaml parsing error

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

changelogs/fragments/3907-pacman-speedup.yml:4:5: error: syntax error: could not find expected ':' (syntax)
changelogs/fragments/3907-pacman-speedup.yml:4:5: unparsable-with-libyaml: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/3907-pacman-speedup.yml:0:0: yaml parsing error

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

changelogs/fragments/3907-pacman-speedup.yml:4:5: error: syntax error: could not find expected ':' (syntax)
changelogs/fragments/3907-pacman-speedup.yml:4:5: unparsable-with-libyaml: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/3907-pacman-speedup.yml:0:0: yaml parsing error

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

changelogs/fragments/3907-pacman-speedup.yml:4:5: error: syntax error: could not find expected ':' (syntax)
changelogs/fragments/3907-pacman-speedup.yml:4:5: unparsable-with-libyaml: while scanning a simple key - could not find expected ':'

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/3907-pacman-speedup.yml:0:0: yaml parsing error

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

changelogs/fragments/3907-pacman-speedup.yml:4:5: error: syntax error: could not find expected ':'

click here for bot help

[felixfontein]

You need to add yourself also to .github/BOTMETA.yml's maintainers list for the $modules/packaging/os/pacman.py entry.

[patchback]

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/1580f3c2b4876aeb06a0c785a14c5a4e057e843e/pr-3907

Backported as #4176

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@jraby thanks for implementing this!
@jamincollins thanks for reviewing and testing!

[felixfontein]

In 1580f3c#commitcomment-67899699 someone commented that this PR uses --query --group instead of --query --groups, breaking some AUR helpers (like yay or paru). Both pacman --query --group and pacman --query --groups seem to return the same results (at least for my system), and pacman --query --help only mentions -g and --groups. I guess changing --group to --groups makes sense. Or is there something I'm missing @jraby?

[jraby]

wait, why does it even work with pacman? ....
We can definitely change it to the actual option --groups.

have you started doing it or do you want me to prepare a PR?

[felixfontein]

Interestingly, --gr, --gro, --grou also work for --groups. --g does not though.

[felixfontein]

I haven't started a PR, feel free to create one :)

[jraby]

alright. I'll do that later today.