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-11113) Allow present and latest as package version #565

Merged
merged 4 commits into from
Jul 23, 2021
Merged

(MODULES-11113) Allow present and latest as package version #565

merged 4 commits into from
Jul 23, 2021

Conversation

relearnshuffle
Copy link
Contributor

This pull requests intends to implement the package versions present and latest as valid package versions. According to #333, this was the case prior to commit c2206a7. The issue with #333 is that it is an outdated pull request that does not apply to the current state of main. This, along with the fact that the author of #333 did not agree to the CLA in over 2 years now, is why I am raising this as a separate pull request.

Since there is no issues section in this project, I would also like to use this pull request as a discussion for this feature. The issue with the implementation as it is proposed in this pull request is that every puppet run will end with:

Notice: Stopping run after puppet-agent upgrade. Run puppet agent -t or apply your manifest again to finish the transaction.

when the version is set to present or latest. This is because the needs_upgrade? function in the puppet_agent_end_run type uses the Puppet::Util::Package.versioncmp function to compare the literal string present and latest with the actual version of the Puppet agent installed, e.g. 6.22.1, which will never be equal.

I am not sure of a suitable fix for this and was hoping someone else could help.

I stumbled upon this issue when I realized the pc_repo.repo file was no longer being managed by this module after removing setting puppet_agent::package_version to auto which in itself led to an issue where the Puppet agent would also be downgraded (despite the server being the same version as the client) anytime the Puppet agent upgraded through the package manager. This is with puppet_agent::collection set to puppet6 for both the main server and the agent, the puppetserver version being 6.15.3 and the puppet-agent version, prior to a catalogue pull, being 6.22.1 on both the main server and the agent.

@puppet-community-rangefinder
Copy link

puppet_agent is a class

that may have no external impact to Forge modules.

puppet_agent::install is a class

that may have no external impact to Forge modules.

This module is declared in 3 of 576 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.

@GabrielNagy GabrielNagy requested a review from a team June 4, 2021 15:15
@GabrielNagy
Copy link
Contributor

Thanks for your contribution @relearnshuffle!

This is because the needs_upgrade? function in the puppet_agent_end_run type uses the Puppet::Util::Package.versioncmp function to compare the literal string present and latest with the actual version of the Puppet agent installed, e.g. 6.22.1, which will never be equal.

After digging a bit into this, I found that we can search the catalog for the version we just upgraded to (in case latest is set in the manifest):

[73] pry(#<Puppet::Type::Puppet_agent_end_run::ProviderPuppet_agent_end_run>)> @resource.catalog.resource('package', 'puppet-agent').parameters[:ensure].latest                                                                    
=> "0:7.8.0.64.g6670bf40b-1.el8"

Since the version format is specific to yum-based packages, we have to normalize it so it looks like what Facter.value(:aio_agent_version) outputs. This regex appears to work for me:

[26] pry(#<Puppet::Type::Puppet_agent_end_run::ProviderPuppet_agent_end_run>)> "0:7.8.0.64.g6670bf40b-1.el8".match(/^(?:[0-9]:)?(?<aio_version>\d+\.\d+\.\d+(?:\.\d+))?/)[:aio_version]
=> "7.8.0.64"

So the needs_upgrade? logic would become something like this:

  def needs_upgrade?
    current_version = Facter.value('aio_agent_version')
    desired_version = @resource.name

    return false if desired_version == 'present'

    if desired_version == 'latest'
      latest_version = @resource.catalog.resource('package', 'puppet-agent').parameters[:ensure].latest
      desired_version = latest_version.match(/^(?:[0-9]:)?(?<aio_version>\d+\.\d+\.\d+(?:\.\d+))?/)[:aio_version]
    end

    Puppet::Util::Package.versioncmp(desired_version, current_version) != 0
  end

I assume present will be a noop everytime since the module requires puppet-agent to run, so the condition will always be satisfied.

Let me know if this works for you!

@GabrielNagy
Copy link
Contributor

@relearnshuffle I created the following JIRA ticket to track this issue: https://tickets.puppetlabs.com/browse/MODULES-11113

To get the PR commit check to pass, please prefix (MODULES-11113) to your commit message, something like: (MODULES-11113) Allow present and latest as package version

@GabrielNagy GabrielNagy changed the title Allow present and latest as package version (MODULES-11113) Allow present and latest as package version Jun 22, 2021
@GabrielNagy GabrielNagy marked this pull request as ready for review July 14, 2021 12:05
@GabrielNagy
Copy link
Contributor

I took it upon myself to work on this PR, since I believe this is an useful improvement.

This was also a good opportunity to add some test coverage, and bump rspec-puppet-facts which lowers CI duration with about ~5 minutes per run (due to the memoization of on_supported_os).

Setting package_version to 'latest' should now be supported on all platforms for which we ship repos (EL, Fedora, Ubuntu, Debian, SLES), other platforms will raise an error.

Copy link
Contributor

@luchihoratiu luchihoratiu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍
Manually tested these changes on Ubuntu 20.04 and everything seems to be working as expected. Manifests used:

class {'::puppet_agent':
  package_version => latest,
  collection => 'puppet6'
}

and

class {'::puppet_agent':
  package_version => latest,
  apt_source => "http://nightlies.puppet.com/apt",
  collection => 'puppet6-nightly'
}

Output example:

puppet apply manifest.pp
Notice: Compiled catalog for example.puppetlabs.net in environment production in 0.45 seconds
Notice: /Stage[main]/Puppet_agent::Osfamily::Debian/Apt::Source[pc_repo]/Apt::Setting[list-pc_repo]/File[/etc/apt/sources.list.d/pc_repo.list]/content: content changed '{md5}88e1295cfdbda94bebe58c92215d90b7' to '{md5}935d4fe674b74276a929259e113d3e65'
Notice: /Stage[main]/Apt::Update/Exec[apt_update]: Triggered 'refresh' from 1 event
Notice: /Stage[main]/Puppet_agent::Osfamily::Debian/Exec[pc_repo_force]/returns: forcing apt update for pc_repo puppet6-nightly
Notice: /Stage[main]/Puppet_agent::Osfamily::Debian/Exec[pc_repo_force]: Triggered 'refresh' from 2 events
Notice: /Stage[main]/Puppet_agent::Install/Package[puppet-agent]/ensure: ensure changed '6.23.0-1bionic' to '6.24.0.28.g6b5947a6a-1bionic'
Notice: Stopping run after puppet-agent upgrade. Run puppet agent -t or apply your manifest again to finish the transaction.
Notice: Applied catalog in 12.81 secondspuppet apply manifest.pp
Notice: Compiled catalog for example.puppetlabs.net in environment production in 0.59 seconds
Notice: Applied catalog in 0.99 seconds

@ciprianbadescu ciprianbadescu requested a review from a team July 21, 2021 07:29
relearnshuffle and others added 4 commits July 22, 2021 16:51
Treat latest and present as valid package versions. Add tests for the
puppet_agent_end_run provider and switch to rspec style mocks.
Newer versions of rspec-puppet-facts memoize the values of
`on_supported_os` which we call multiple times in our spec tests. This
makes loading the facts about 5 times faster.

Unpin rspec-puppet as it had multiple releases since we pinned it to
2.7.9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants