Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pillarsdotnet
Copy link

@pillarsdotnet pillarsdotnet commented Dec 7, 2022

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.

@pillarsdotnet pillarsdotnet requested a review from a team as a code owner December 7, 2022 21:39
@CLAassistant
Copy link

CLAassistant commented Dec 7, 2022

CLA assistant check
All committers have signed the CLA.

@pillarsdotnet pillarsdotnet changed the title MODULES-10763 Do not report apt-get update as a change MODULES-10763 Do not report apt-get update as a corrective change. Dec 7, 2022
manifests/update.pp Outdated Show resolved Hide resolved
manifests/update.pp Outdated Show resolved Hide resolved
@pillarsdotnet
Copy link
Author

pillarsdotnet commented Dec 8, 2022

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

kenyon
kenyon previously approved these changes Dec 8, 2022
@chelnak
Copy link

chelnak commented Dec 10, 2022

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
Copy link
Author

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
Copy link

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
Copy link
Author

@LukasAud

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

To confirm, I have opened #1077

@pillarsdotnet pillarsdotnet force-pushed the main branch 2 times, most recently from ef6a14c to 43e7a16 Compare December 19, 2022 12:40
@pillarsdotnet pillarsdotnet requested a review from kenyon December 19, 2022 12:42
Copy link

@kenyon kenyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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
Copy link
Author

pillarsdotnet commented Dec 19, 2022

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

@pillarsdotnet pillarsdotnet force-pushed the main branch 2 times, most recently from de12063 to bf97014 Compare December 19, 2022 19:30
@pillarsdotnet
Copy link
Author

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

manifests/update.pp Outdated Show resolved Hide resolved
@pillarsdotnet
Copy link
Author

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
Copy link
Author

@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
Copy link
Collaborator

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

@pillarsdotnet
Copy link
Author

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
Copy link
Author

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 pillarsdotnet force-pushed the main branch 2 times, most recently from dad3813 to 23e22b6 Compare September 27, 2023 18:50
@pillarsdotnet
Copy link
Author

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
Copy link
Author

@kenyon ?
@smortex ?

Copy link

@kenyon kenyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

manifests/update.pp Outdated Show resolved Hide resolved
manifests/update.pp Outdated Show resolved Hide resolved
@pillarsdotnet
Copy link
Author

pillarsdotnet commented Sep 27, 2023

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?

Copy link
Collaborator

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kenyon
Copy link

kenyon commented Sep 28, 2023

@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
Copy link

kenyon commented Sep 28, 2023

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

@smortex
Copy link
Collaborator

smortex commented Sep 29, 2023

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
Copy link

kenyon commented Sep 30, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.