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

[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 don't understand why a change to apt_update would prevent apt_backports from working, unless there is a hidden dependency problem. Perhaps we need to run apt update after adding backports to sources?

[pillarsdotnet]

Maybe I need to run that scriptlet under sh -c even though provider => shell ?

[pillarsdotnet]

Okay; I'm fresh out of ideas. Help, anyone?

[smortex]

This PR raise a few concerns for me:

1. Now, each run of puppet runs apt update which consume resources on the Debian project side. By default this is every 30mn, so 48 times per day, way more than the default configuration of e.g. unattended-upgrades which does it only twice a day. I do not know if there is some guidelines about how often it is okay to look for updates, but this feels very aggressive;
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. A way to have a similar behavior without corrective changes would be to use puppet to setup e.g. unattended-upgrades to keep the repositories up-to-date and not doing so using puppet.

[kenyon]

Agreed with @smortex, I wouldn't want apt update running on every puppet run, that's indeed what I have https://github.com/voxpupuli/puppet-unattended_upgrades/ configured for.

[kenyon]

But, also, the apt_update exec usually has refreshonly => false, unless frequency => always, and unless commands only get run if the exec is refreshed.

[pillarsdotnet]

@kenyon

I could try to interpret the output of apt update and return nonzero if it actually changes something, then make the main exec something like echo Repository cache has been updated.

[kenyon]

I don't think apt update gives output that would allow that interpretation.

[pillarsdotnet]

@kenyon No, but the content of /var/cache/apt/pkgcache.bin changes if and only if apt update did something.

[pillarsdotnet]

Curiouser and curiouser. For Debian 10 (and Debian 11, replacing "buster" with "bullseye"):

The /etc/apt/sources.list.d/backports.list file has the following contents:

# This file is managed by Puppet. DO NOT EDIT.
# backports
deb http://deb.debian.org/debian buster-backports main contrib non-free

The /etc/apt/preferences.d/backports.pref file has the following contents:

# This file is managed by Puppet. DO NOT EDIT.
Explanation: apt: backports
Package: *
Pin: release a=buster-backports, n=, v=, c=, o=, l=
Pin-Priority: 200

The apt policy command output does not contain a "backports" substring.

[pillarsdotnet]

Okay, so tests pass if I explicitly add a apt update into the backports acceptance test. That means the unless param to Exec['apt_update'] isn't doing its job. Maybe I got the syntax wrong for the DASH shell.

(testing by copying the contents into a test.sh file and running it under dash)

Damnit; had a typo. Fixing...

[pillarsdotnet]

Nope; that didn't fix. I need to see what that scriptlet is doing. Copied it into the failing test, hoping that it will at least output an error message that I can interpret.

[pillarsdotnet]

Okay, I'm stumped again. Help, anyone?

[pillarsdotnet]

Modified code to explicitly run the update scriptlet under /bin/sh -x and output the results. Need to see what is going wrong on Debian 10/11. Can someone please approve tests?

[LTangaF]

I like what you're doing here. I really wish we could get this moving. It's been years.

[david22swan]

Just closing and reopening to re-kick the tests

[puppet-community-rangefinder[bot]]

apt::update is a class

Breaking changes to this file WILL impact these 17 modules (exact match):

* [locp-cassandra](https://github.com/locp/cassandra) * [midonet-midonet](https://github.com/midonet/puppet-midonet) * [puppet-cassandra](https://github.com/voxpupuli/puppet-cassandra) * [midonet-cassandra](https://github.com/midonet/puppet-cassandra) * [hfm-percona](https://github.com/hfm/puppet-percona) * [locp-opscenter](https://github.com/locp/opscenter) * [praekeltfoundation-xylem](https://github.com/praekeltfoundation/puppet-xylem) * [praekeltfoundation-gluster](https://github.com/praekeltfoundation/puppet-gluster) * [praekeltfoundation-consular](https://github.com/praekeltfoundation/puppet-consular) * [poolski-beats](https://github.com/poolski/puppet-beats) * [bltavares-baseline](https://github.com/bltavares/vagrant-baseline) * [wavesoftware-xtreemfs](https://github.com/wavesoftware/puppet-xtreemfs.git) * [narasimhasv-docker](https://forge.puppet.com/narasimhasv/docker) * [narasimhasv-lxc](https://forge.puppet.com/narasimhasv/lxc) * [locp-odoo](https://github.com/locp/puppet-odoo) * [locp-odoo9](https://github.com/locp/puppet-odoo) * [hfm-h2o](https://github.com/hfm/puppet-h2o)

Breaking changes to this file MAY impact these 9 modules (near match):

* [hfm-stns](https://github.com/STNS/puppet-stns) * [puppetlabs-openstack](https://github.com/puppetlabs/puppetlabs-openstack.git) * [meltwater-marathon](https://github.com/meltwater/puppet-marathon) * [hfm-tinyproxy](https://github.com/hfm/puppet-tinyproxy) * [jtopjian-cubbystack](https://github.com/jtopjian/puppet-cubbystack) * [hfm-proxysql](https://github.com/hfm/puppet-proxysql) * [narasimhasv-openstack](https://forge.puppet.com/narasimhasv/openstack) * [factorit-icingaweb2](https://github.com/Icinga/puppet-icingaweb2.git) * [hfm-octopass](https://github.com/hfm/puppet-octopass)

This module is declared in 235 of 580 indexed public Puppetfiles.

---

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

[pillarsdotnet]

Rebased on upstream and added some reasonable defaults for $apt::_update['tries'] and $apt::_update['timeout'].

[david22swan]

Closing and reopening to kick tests

[Geod24]

@pillarsdotnet : Would you like to address the review or are you okay if someone picks this up ? I for one would really like to see this bug fixed.

[pillarsdotnet]

I haven't had time to figure out why the tests are failing.

[pillarsdotnet]

Certainly willing to accept help or to drop this approach in favor of a better one.

[pillarsdotnet]

@Geod24 @smortex @bastelfreak @david22swan

So sorry I've let this languish for so long. I'm not allowed to commit to public projects from my work computer and I haven't checked from my PC in a long time.

Please re-kick tests?

[pillarsdotnet]

Another test run please, @bastelfreak ?

[pillarsdotnet]

Another test run, please, @bastelfreak ?

[Geod24]

BTW, to avoid this problem, you should raise a PR against main that fixes something tiny. Once you are recognized as a committer there won't be a need to approve your work every time.

[pillarsdotnet]

@bastelfreak -- I think the lint-error is a false-positive, but I've rearranged code to make it happy. Another test, please?

[pillarsdotnet]

Well, at least the lint checks pass now.

Made some adjustments that should fix at least the first acceptance test error.

@bastelfreak ?

[pillarsdotnet]

Debian-10 is failing on the eval line. I've re-coded to use source instead of eval.

@bastelfreak ?

[pillarsdotnet]

Missed one; fixed now.

@bastelfreak ?

[pillarsdotnet]

I guess it's possible that mktemp and cmp are missing in the acceptance-test environment; coding to workaround that possibility.

Another test run, @bastelfreak ?

[pillarsdotnet]

Another workaround.

@bastelfreak ?

[pillarsdotnet]

Another workaround. @bastelfreak ?

[pillarsdotnet]

Yet another workaround. It's frustrating that the "acceptance" environment doesn't actually have any working commands. @bastelfreak ?

Thanks so much for your patience on this.

[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.

[bastelfreak]

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).

[pillarsdotnet]

I'm not sure why idempotently_apply() is failing, but I changed the spec test to better match the code in manifests/update.pp

[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?