MODULES-10763 Only report apt-get update as a corrective change if /var/cache/apt/pkgcache.bin changes.

[pillarsdotnet]

Instead of running apt-get update as an exec directly, we run it as the unless attribute of an exec.

After running apt-get update, if the content of /var/cache/apt/pkgcache.bin is unchanged, the exec does not run, and reports no change.

If the content of /var/cache/apt/pkgcache.bin changes, the exec runs and reports a corrective change.

This resolves MODULES-10763.

[CLAassistant]

All committers have signed the CLA.

[pillarsdotnet]

Thanks, @kenyon -- Fixed along with another missed dollar-sign escape.

[chelnak]

Hey @pillarsdotnet - appreciate this contribution.

However, I wonder if the changes introduce are slightly hard to follow.

Is there a more idiomatic approach we could take?

[pillarsdotnet]

I'm open to suggestions, but this is how I've been resolving the problem locally while this bug stood unresolved for the last two years.

[LukasAud]

Hi @pillarsdotnet, it seems like this change is producing a few failures in our automated testing. These need to be addressed before we can think about merging your PR.

[pillarsdotnet]

@LukasAud

I believe the automated testing errors are unrelated to my PR.

To confirm, I have opened #1077

[kenyon]

Test failures are certainly due to these changes. Maybe need to set the provider to shell? Not sure why it's giving an empty string as the command in the error.

[pillarsdotnet]

The weird thing is that it's giving an empty string as the command in errors for other execs besides the one I modified.

[pillarsdotnet]

Okay, setting provider => shell helped, but there was also a spec test that needed to be modified.

[pillarsdotnet]

Re-coded in case the testing box lacks the seq binary.

[pillarsdotnet]

I also get an email for each push, you don't need to mention me. That will send an additional email :D (also I don't work for puppet and would appreciate the modules team taking care of this).

Sorry; I don't know how to ping the modules team. Should I post in slack every time I push a commit?

[pillarsdotnet]

@Geod24

@pillarsdotnet : I reiterate my suggestion that you made a minor tweak to main in another PR so that you don't have to ping @bastelfreak for every time you want to run the CI.

Sorry; I was assuming (based on previous experience) that unless I ping someone, the tests won't run.

What is the recommended process for getting tests to run in a timely manner?

[bastelfreak]

the tests will run when someone with merge permissions presses the button :)
that's a restriction for every new contributor to the repo.

[pillarsdotnet]

WOO-HOO!

All acceptance tests that are in any way related to the changes in this PR are now passing.

I've squashed all the commits in order to reduce noise.

Can I get a review, please?

[pillarsdotnet]

Some of the other spec tests check whether the apt::update exec accepts timeout and tries params; since it's now a simple echo, those tests are no longer applicable.

[pillarsdotnet]

On re-reading the docs for the exec resource, it seems that the timeout and tries params are honored for the unless and onlyif stanzas as well, so I reverted their removal and accompanying test changes.

[pillarsdotnet]

@kenyon ?
@smortex ?

[kenyon]

I don't really like this because it makes the apt_update exec much more complicated. As mentioned in previous comments #1073 (comment), the right way to set up a periodic apt update is with unattended-upgrades. So then the only time this exec command gets run is when there really is a corrective change, because it has been refreshed by another resource.

[pillarsdotnet]

Thanks, @kenyon

I've implemented your suggestions.

Unfortunately, re-tooling to run unattended-upgrades instead of invoking the package resource is not an option for us.

Since there is not a mechanism that will trigger Exec['apt_update'] before any package resources, we'd like to set apt::update_frequency to always without causing an intentional change on every Puppet run.

The yum package provider updates the yum cache on every Puppet run without logging the change;
Why shouldn't the apt provider be configurable to do likewise?

[bastelfreak]

looks good to me, but I'm not an apt expert.

[kenyon]

@pillarsdotnet I don't think that's true about the yum/dnf package provider. dnf works differently from apt: it automatically does the equivalent of apt update when metadata is out of date. So there is no need to explicitly update the cache, and it's definitely not done on every puppet run.

[kenyon]

Also, this PR is for the apt module. Package providers are here: https://github.com/puppetlabs/puppet/tree/main/lib/puppet/provider/package

[smortex]

Hey 👋! I don't want to be the guy blocking things but this change does not look correct from my window 😞:

1. My base concerns still apply: for me the current behavior is more correct than the proposed one (ref MODULES-10763 Only report apt-get update as a corrective change if /var/cache/apt/pkgcache.bin changes. #1073 (comment): point 1 is I think no more (refreshonly used to be removed IRRC), but point 2 still apply):

   2. The current behavior seems perfectly valid: running apt update is a corrective change (from a state where we do not know if the repository is up-to-date to a state where we know that the repository is currently up-to-date). So it makes sense that a configuration that enforce "we have up-to-date repositories" appears as having corrective changes all the time.

2. The new implementation look complicated (I think this is mostly due to the way we have 50 lines of code as a single command: writing this as a regular shell script, with vertical space to separate sections, real comments (without epp burden), etc. can improve this);
3. The new behavior seems fragile, for example it seems to return 0 (so no change reported to the user and no issue reported by Puppet) when apt configuration is completely broken, for example if a module setup a non-functional repo (call sequence: apt-get update fail and return 1 on each run; maximum attempts reached; EXIT_CODE is set to 1 (good); cache file has not changed on disk; EXIT_CODE is set to 0 (bad)).

[kenyon]

@pillarsdotnet can you elaborate on why enabling the apt-daily systemd service and timer that come with apt are not possible for you? There is no "re-tooling" to be done, you already have it because it comes with apt. That is the standard Debian way of keeping the package cache up to date: https://debian-handbook.info/browse/stable/sect.regular-upgrades.html (which the unattended_upgrades module can configure, and it is not required to also enable automatic package upgrades)

If you wanted to run apt-daily.service every 30 minutes like what puppet would do with this PR, you would just use a systemd drop-in (using the systemd module) to edit the [Timer] section of /lib/systemd/system/apt-daily-upgrade.timer.