modulesync 5.5.0

[ekohl]

Somehow idempotency is broken with Puppet 7 for one specific case. I'm not too sure how to fix that.

[ekohl]

This is now failing in a way I don't understand and I don't know the module well enough to know what the right way is.

It looks like this condition in instances() follows a different branch than expected: https://github.com/voxpupuli/puppet-augeasproviders_sysctl/blob/d2822732c6cfc07fdf28460390562405110273c4/lib/puppet/provider/sysctl/augeas.rb#L82

However, I don't know what is the correct behavior. It may very well be the cause of the idempotency failures we see in acceptance, so I'm hesitant to just change the stubbing.

[deric]

@ekohl The error does happen in Puppet 6 rspec. It does seem to be connected to changes in augeasproviders_core > 3.1.0.

[deric]

Sorry, the augeasproviders_core changes are unrelated. The breaking change was probably introduced in augeasproviders_sysctl version 2.6.2 (aee8fa659ab376c384ce94a706b832c00f634555).

This is weird:

       #<Puppet::Type::Sysctl::ProviderAugeas (class)> received :sysctl with unexpected arguments
         expected: (["-w", "net.ipv4.ip_forward=1"])
              got: (["-e", "net.ipv4.ip_forward"])

instead of writing the value, it's getting the value. We can allow that for now:

allow(provider_class).to receive(:sysctl).with(['-e', 'net.ipv4.ip_forward']).and_return('net.ipv4.ip_forward = 1')
expect(provider_class).to receive(:sysctl).with(['-w', 'net.ipv4.ip_forward=1'])
expect(provider_class).to receive(:sysctl).with(['-n', 'net.ipv4.ip_forward']).at_least(:once).and_return('1')

then we get:

         expected: (["-w", "net.ipv4.ip_forward=1"])
              got: ("-n", "net.ipv4.ip_forward")

which is even weirder, because we're expecting such call, except this time it's not wrapped in an array. It looks like at least in the tests the method sysctl() is called at least twice:

once as

sysctl(["-w", "net.ipv4.ip_forward=1"])
# and then as 
sysctl("-w", "net.ipv4.ip_forward=1")

[ekohl]

The breaking change was probably introduced in augeasproviders_sysctl version 2.6.2 (aee8fa6).

Do you mean that broke the module or the tests? If the changes are valid, I'd be happy to modify the tests to match.

I also see you're not a member of Vox Pupuli right now, but we can certainly use more people. If you're interested, come join us in #voxpupuli on Libera.Chat or the other ways mentioned at https://voxpupuli.org/ (on the bottom).

[ekohl]

@trevor-vaughan would you mind having a look at the above conversation?

[trevor-vaughan]

@ekohl Took a look and I think that these changes were made during the times when the tests were broken all over the place.

Since they appear to be working now, I think that the tests in sysctl need to be updated which will pave the way to the tests in this module being corrected.

It shouldn't be a heavy lift and I'll try to bang it out today or tomorrow.

@deric If you'd like to make the necessary mods in this PR, let me know and I'll point you at the updates to sysctl.

[trevor-vaughan]

And that comment is what happens when I have too many tabs open.

The code appears to be correct and the tests need to be updated to align with the existing code.

[ekohl]

@trevor-vaughan thanks for confirming that! I won't have time for that soon, but you should have permissions to push commits to this branch.

[trevor-vaughan]

@deric This will get you past the first round of failures

diff --git a/spec/unit/puppet/provider/sysctl/augeas_spec.rb b/spec/unit/puppet/provider/sysctl/augeas_spec.rb
index 32cd5be..2573014 100755
--- a/spec/unit/puppet/provider/sysctl/augeas_spec.rb
+++ b/spec/unit/puppet/provider/sysctl/augeas_spec.rb
@@ -30,6 +30,7 @@ describe provider_class do
     let(:target) { File.join(@tmpdir, "new_file") }

     before :each do
+      expect(provider_class).to receive(:sysctl).with(['-e', 'net.ipv4.ip_forward'])
       expect(provider_class).to receive(:sysctl).with(['-w', 'net.ipv4.ip_forward=1'])
       expect(provider_class).to receive(:sysctl).with(['-n', 'net.ipv4.ip_forward']).at_least(:once).and_return('1')
     end
@@ -53,6 +54,7 @@ describe provider_class do
     let(:target) { tmptarget.path }

     before :each do
+      expect(provider_class).to receive(:sysctl).with(['-e', 'net.ipv4.ip_forward'])
       expect(provider_class).to receive(:sysctl).with(['-w', 'net.ipv4.ip_forward=1'])
       expect(provider_class).to receive(:sysctl).with(['-n', 'net.ipv4.ip_forward']).at_least(:once).and_return('1')
     end

Unfortunately, the next round appears to be deep in the guts of Augeas and I'm not 100% sure how to proceed.

[trevor-vaughan]

Dropped a pry into spec/fixtures/modules/augeasproviders_core/spec/lib/augeas_spec/fixtures.rb and dumped the @logs object and found the following warning:

@logs
=> [#<Puppet::Util::Log:0x0000000003f9dd98
  @level=:err,
  @message="Could not prefetch sysctl provider 'augeas': no implicit conversion of nil into String",
  @source="Puppet",
  @tags=#<Puppet::Util::TagSet: {"err"}>,
  @time=2023-02-20 11:07:21.415325624 -0500>,
 #<Puppet::Util::Log:0x0000000003f9c920
  @level=:warning,
  @message="Skipping because provider prefetch failed",
  @source="/Sysctl[net.ipv4.ip_forward]",
  @tags=#<Puppet::Util::TagSet: {"warning", "sysctl", "net.ipv4.ip_forward"}>,
  @time=2023-02-20 11:07:21.415557624 -0500>]

Looks like we should probably change two things:

1. Dump the logs in the apply method for debugging on failure
2. Stub out prefetch (somehow) so that the provider doesn't fail

[trevor-vaughan]

This works better for the augeas_spec/fixtures.rb in augeasproviders_core:

diff --git a/spec/lib/augeas_spec/fixtures.rb b/spec/lib/augeas_spec/fixtures.rb
index 76cced2..a9577a6 100644
--- a/spec/lib/augeas_spec/fixtures.rb
+++ b/spec/lib/augeas_spec/fixtures.rb
@@ -31,7 +31,7 @@ module AugeasSpec::Fixtures
     # Check for warning+ log messages
     loglevels = Puppet::Util::Log.levels[3, 999]
     firstlogs = @logs.dup
-    @logs.select { |log| loglevels.include?(log.level) && log.message !~ %r{'modulepath' as a setting} }.should be_empty
+    @logs.select { |log| loglevels.include?(log.level) && log.message !~ %r{'modulepath' as a setting} }.should eq([])

     # Check for transaction success after, as it's less informative
     txn.any_failed?.should_not eq(true)

[ekohl]

That reminds me: I had https://github.com/voxpupuli/puppet-augeasproviders_core/pull/45 ready, but didn't submit it as a PR. I've included your suggestion there.

[ekohl]

Stub out prefetch (somehow) so that the provider doesn't fail

I've been wondering if this also causes the idempotency problems om Puppet 7, though I also saw them in _core I think.

[trevor-vaughan]

Stub out prefetch (somehow) so that the provider doesn't fail

I've been wondering if this also causes the idempotency problems om Puppet 7, though I also saw them in _core I think.

Quite probably. Seeing the log output should help figure out what's going on if there are other places to drop it in.

[trevor-vaughan]

@ekohl Updated the bulk of the tests in augeas_spec.

Have to bail, but hopefully this helps move things forward.

[ekohl]

Given everything passes on Puppet 6, I'm leaning to:

• Drop Puppet 7 from metadata.json
• Merge this PR
• Open a new PR to add Puppet 7 support while also fixing it

Any objections?

[trevor-vaughan]

As far as I can tell, it works in puppet 7 but we need to figure out the tests.

On Mon, Feb 20, 2023, 12:41 PM Ewoud Kohl van Wijngaarden < @.***> wrote:

Given everything passes on Puppet 6, I'm leaning to:

• Drop Puppet 7 from metadata.json
• Merge this PR
• Open a new PR to add Puppet 7 support while also fixing it

Any objections?

— Reply to this email directly, view it on GitHub https://github.com/voxpupuli/puppet-augeasproviders_sysctl/pull/65#issuecomment-1437362125, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGAOSJPOMXJBIUFFXP7TWTWYOUFDANCNFSM5JHBB5XQ . You are receiving this because you were mentioned.Message ID: @.***>

[deric]

@ekohl With .fixtures.yml set to (see my comments on https://github.com/voxpupuli/puppet-augeasproviders_core/pull/45):

fixtures:
  repositories:
    augeasproviders_core:
      repo: https://github.com/deric/puppet-augeasproviders_core.git
      ref: 2c487984431bc406cc4c51df307b0dd94a12a798

and apply changed to apply! the tests seems to be passing on Puppet 7.

diff --git spec/unit/puppet/provider/sysctl/augeas_spec.rb spec/unit/puppet/provider/sysctl/augeas_spec.rb
index d351f8e..1c58a44 100755
--- spec/unit/puppet/provider/sysctl/augeas_spec.rb
+++ spec/unit/puppet/provider/sysctl/augeas_spec.rb
@@ -485,7 +485,7 @@ describe provider_class do

     it "should fail to load" do
       expect {
-        apply(Puppet::Type.type(:sysctl).new(
+        apply!(Puppet::Type.type(:sysctl).new(
           :name     => "net.ipv4.ip_forward",
           :value    => "1",
           :target   => target,

@trevor-vaughan Thanks for the quick fix!

[ekohl]

I do wonder why my patch with in _core fixes it. It shouldn't make a difference, but I can observe that it does.

[ekohl]

So that fixes the unit tests, but it still doesn't fix the acceptance tests. They really have a problem with idempotency and I don't know why.

[jay7x]

I was able to reproduce it on my laptop (aarch64) with beaker-docker hypervisor using the command below (replace AARCH64 with 64 if you're on x86_64 arch):

BEAKER_PUPPET_COLLECTION=puppet7 BEAKER_setfile='ubuntu2004-AARCH64{docker_preserve_image=true}' bundle exec rake beaker

NOTE: you need a rootful docker to allow sysctl writes. I'm using a lima VM with the docker-rootful template started as limactl start --name=docker template://docker-rootful for this.

Below are my findings:

1. The main reason is the 20-fs.conf file contents is incomplete on 1st run.

   1st run:

     Info: Loading facts
     Notice: Compiled catalog for ubuntu2004-aarch64-1. in environment production in 0.01 seconds
     Info: Using environment 'production'
     Info: Applying configuration version '1680866097'
     Notice: /Stage[main]/Main/Sysctl[fs.nr_open]/value: changed configuration value from '' to '100001' and live value from '100002' to '100001'
     Notice: /Stage[main]/Main/Sysctl[fs.inotify.max_user_watches]/value: changed configuration value from '' to '8193'
     Notice: Applied catalog in 0.02 seconds

   20-fs.conf file contents is:

   fs.inotify.max_user_watches = 8193

   2nd run:

     Info: Loading facts
     Notice: Compiled catalog for ubuntu2004-aarch64-1. in environment production in 0.01 seconds
     Info: Using environment 'production'
     Info: Applying configuration version '1680866098'
     Notice: /Stage[main]/Main/Sysctl[fs.nr_open]/value: changed configuration value from '' to '100001'
     Notice: Applied catalog in 0.02 seconds

   20-fs.conf file contents is:

   fs.inotify.max_user_watches = 8193
   fs.nr_open = 100001

   3rd run:

     Info: Loading facts
     Notice: Compiled catalog for ubuntu2004-aarch64-1. in environment production in 0.01 seconds
     Info: Using environment 'production'
     Info: Applying configuration version '1680866099'
     Notice: Applied catalog in 0.01 seconds

2. This only happens with Puppet 7. No issues with Puppet 6 (file contents is right on 1st run there).

It looks like either...

• a race condition (things are done in parallel on P7)
• some gem version is different on P7 so it behaves differently

[ekohl]

@jay7x thanks a lot for investigating! To move things forward I'd follow what I said in https://github.com/voxpupuli/puppet-augeasproviders_sysctl/pull/65#issuecomment-1437362125.

[jay7x]

I'd say I agree to release it as it is now to unblock modules release under Voxpupuli account on Forge.. But we should add a ⚠️ warning saying that it might behave unexpectedly on Puppet 7 when targeted on a specific file. Though we should check it works ok on sysctl.conf too.. let me check few more corner cases

[jay7x]

Another IMPORTANT finding.. It's idempotent if the file exists before.. so this works fine:

        context 'when using a target file' do
          let(:manifest) {
            <<-EOM
              file { '/etc/sysctl.d/20-fs.conf':
                ensure => 'file',
              }

              sysctl { 'fs.nr_open':
                value  => '100001',
                target => '/etc/sysctl.d/20-fs.conf'
              }

              sysctl { 'fs.inotify.max_user_watches':
                value  => '8193',
                target => '/etc/sysctl.d/20-fs.conf'
              }
            EOM
          }

Remove the file and it breaks.

[h-haaks]

This module has declared puppet 7 support for 2 years and I think it's sad if that has to be removed. Could a better workaround her be to just add a second apply_manifest_on(host, manifest, :catch_failures => true) at spec/acceptance/sysctl_spec.rb:214, comment it in the code and as @jay7x says make a warning in the readme?

[ekohl]

@h-haaks I think the key point to remember is that there's is an incompatibility with Puppet 7. The tests clearly prove that. That the tests didn't run for ages means it was missed. This isn't something that you should paper over. IMHO you either declare it incompatible until it's fixed (and describe a workaround in the README) or change the API.

This may very well be that the file isn't flushed properly: https://github.com/voxpupuli/puppet-augeasproviders_sysctl/blob/d2822732c6cfc07fdf28460390562405110273c4/lib/puppet/provider/sysctl/augeas.rb#L269-L291

Perhaps there multiple instances call create and overwrite the previously created file?

[h-haaks]

I do agree @ekohl But in this case where 27 out of 28 tests pass with puppet 7 and the end result for the failing test is correct after 2 puppet runs I would rather create another issue, document the findings and temporary fix added to the acceptance test.

A better workaround in the acceptance test could be to add pending('ISSUE REF') if ENV['BEAKER_PUPPET_COLLECTION'] == 'puppet7' to the failing test at https://github.com/voxpupuli/puppet-augeasproviders_sysctl/blob/d2822732c6cfc07fdf28460390562405110273c4/spec/acceptance/sysctl_spec.rb#L217-L219

[ekohl]

@h-haaks Vox Pupuli has never published this module, so dropping support isn't really breaking. And AFAIK no tools actually look at it so it doesn't prevent you from using it. It makes it explicit that Puppet 7 isn't really supported.

[vchepkov]

IMHO, failure to modify non-existing file with augeas is not something specific to sysctl

take this example with pam module:

# cat blah.pp 
pam { "Set cracklib limits in password-auth":
  ensure    => present,
  service   => 'password-auth',
  type      => 'password',
  module    => 'pam_cracklib.so',
  arguments => ['try_first_pass','retry=3', 'minlen=10'],
  target    => '/tmp/pam.conf',
}
# puppet apply blah.pp 
Notice: /Stage[main]/Main/Pam[Set cracklib limits in password-auth]/ensure: created
Error: /Stage[main]/Main/Pam[Set cracklib limits in password-auth]: Could not evaluate: Failed to save Augeas tree to file. See debug logs for details.

Having said that, I tested resource with herculesteam-augeasproviders_sysctl (v2.6.2) on puppet 7 and with modulesync branch of this repo and both create file just fine

# cat sysctl.pp 
sysctl { "net.ipv4.ip_forward":
  ensure => present,
  value  => "1",
  target => "/tmp/forwarding.conf",
}
# puppet apply sysctl.pp 
Notice: /Stage[main]/Main/Sysctl[net.ipv4.ip_forward]/value: changed configuration value from '' to '1' and live value from '0' to '1'
# cat /tmp/forwarding.conf 
net.ipv4.ip_forward = 1

And it is idempotent, second run doesn't make any changes

Ah, missed the point, yes, attempting to add second value to not existing file fail to save first parameter to the file Some sort of flush is missing

[h-haaks]

I did some serious digging and I think the bug is in libaugeas.so.0.25.0 that is bundled with puppet 7.24.0 If I replace libaugeas.so with libaugeas.so.0.24.2 which is bundled with puppet 6.28.0, puppet add both settings at the first run.

[root@centos7-64-1 /]# puppet --version
7.24.0
[root@centos7-64-1 /]# ls -l /opt/puppetlabs/puppet/lib/libaugeas.*
-rw-r--r--. 1 root root 673602 Apr 13 07:47 /opt/puppetlabs/puppet/lib/libaugeas.a
-rwxr-xr-x. 1 root root   1097 Apr 13 07:47 /opt/puppetlabs/puppet/lib/libaugeas.la
lrwxrwxrwx. 1 root root     19 Apr 13 07:47 /opt/puppetlabs/puppet/lib/libaugeas.so -> libaugeas.so.0.24.2
lrwxrwxrwx. 1 root root     19 Apr 13 07:48 /opt/puppetlabs/puppet/lib/libaugeas.so.0 -> libaugeas.so.0.24.2
-rwxr-xr-x. 1 root root 396664 Apr 13 07:47 /opt/puppetlabs/puppet/lib/libaugeas.so.0.24.2
[root@centos7-64-1 /]# rm /etc/sysctl.d/20-fs.conf
rm: remove regular file ‘/etc/sysctl.d/20-fs.conf’? y
[root@centos7-64-1 /]# puppet apply /tmp/apply_manifest_063154630.pp.H1D8UG 
Notice: Compiled catalog for centos7-64-1.spk.no in environment production in 0.01 seconds
Notice: /Stage[main]/Main/Sysctl[fs.nr_open]/value: changed configuration value from '' to '100001'
Notice: /Stage[main]/Main/Sysctl[fs.inotify.max_user_watches]/value: changed configuration value from '' to '8193'
Notice: Applied catalog in 0.04 seconds
[root@centos7-64-1 /]# cat /etc/sysctl.d/20-fs.conf
fs.nr_open = 100001
fs.inotify.max_user_watches = 8193

[ekohl]

@h-haaks thanks! I knew for sure I saw similar issues elsewhere but couldn't find where. Knowing it is a regression in libaugeas.so.0.25 helps a lot.

I don't see any bug report @ https://github.com/hercules-team/augeas but perhaps I'm missing it. It would be great to have a native augtool reproducer for the problem. Has anyone managed that?

[h-haaks]

@ekohl given my last findings do you still think #65(comment) is better than https://github.com/voxpupuli/puppet-augeasproviders_sysctl/pull/65#issuecomment-1503255390 to move this forward?

[vchepkov]

There probably should be a ticket open for Puppet Agent as well, about faulty component. But, this not a show stopper, right? Module will update runtime settings at the first run and will fix itself on the second run. Not ideal, but it's a 'pre-existing' condition.

[ekohl]

It would be good to at least have some issue to refer to in the README, but not a show stopper.

[jay7x]

JFYI I reproduced the same behavior with libaugeas.so.0.24.2 too..

root@ubuntu2004:~# lsb_release -a No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.5 LTS Release: 20.04 Codename: focal root@ubuntu2004:~# cat /etc/sysctl.d/20-fs.conf cat: /etc/sysctl.d/20-fs.conf: No such file or directory root@ubuntu2004:~# puppet apply /home/jay.linux/test/manifests/test.pp Notice: Compiled catalog for ubuntu2004 in environment production in 0.01 seconds Notice: /Stage[main]/Main/Sysctl[fs.nr_open]/value: changed configuration value from '' to '100001' Notice: /Stage[main]/Main/Sysctl[fs.inotify.max_user_watches]/value: changed configuration value from '' to '8193' Notice: Applied catalog in 0.02 seconds root@ubuntu2004:~# cat /etc/sysctl.d/20-fs.conf fs.inotify.max_user_watches = 8193 root@ubuntu2004:~# ldconfig -p | grep libaugeas libaugeas.so.0 (libc6,AArch64) => /lib/aarch64-linux-gnu/libaugeas.so.0 root@ubuntu2004:~# ls -la /lib/aarch64-linux-gnu/libaugeas.so.0* lrwxrwxrwx 1 root root 19 Mar 22 2020 /lib/aarch64-linux-gnu/libaugeas.so.0 -> libaugeas.so.0.24.2 -rw-r--r-- 1 root root 387152 Mar 22 2020 /lib/aarch64-linux-gnu/libaugeas.so.0.24.2 root@ubuntu2004:~# dpkg -L libaugeas0 /. /usr /usr/lib /usr/lib/aarch64-linux-gnu /usr/lib/aarch64-linux-gnu/libaugeas.so.0.24.2 /usr/lib/aarch64-linux-gnu/libfa.so.1.5.3 /usr/share /usr/share/doc /usr/share/doc/libaugeas0 /usr/share/doc/libaugeas0/changelog.Debian.gz /usr/share/doc/libaugeas0/copyright /usr/lib/aarch64-linux-gnu/libaugeas.so.0 /usr/lib/aarch64-linux-gnu/libfa.so.1 root@ubuntu2004:~# dpkg -l libaugeas0 Desired=Unknown/Install/Remove/Purge/Hold | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad) ||/ Name Version Architecture Description +++-================-==============-============-============================================ ii libaugeas0:arm64 1.12.0-1build1 arm64 Augeas configuration editing library and API root@ubuntu2004:~# puppet --version 7.24.0

[h-haaks]

@jay7x puppet agent uses libaugeas in /opt/puppetlabs/puppet/lib/

[jay7x]

@jay7x puppet agent uses libaugeas in /opt/puppetlabs/puppet/lib/

Yeah.. I realized it already.. just confirmed that it works with augeas v1.12.0 but doesn't work with v1.13.0

[jay7x]

Collectively we found the commit which makes things broken in Puppet: https://github.com/hercules-team/augeas/commit/eb04250a05671b2d001444b72b8778328d209d75

[jay7x]

The issue is related to the post_resource_eval somehow.. It works when the line 46 here is changed to false: https://github.com/voxpupuli/puppet-augeasproviders_core/blob/d3fc5676040291aaa3023e7102af4ca297691621/lib/puppet/provider/augeasprovider/default.rb#L46

[jay7x]

Sorry.. closed it accidentally.. 🤦🏻‍♂️

[jay7x]

After following the post_resource_eval trail I found the problem happens when aug.save! is followed by aug.load! (which happens in the augsave method called with reload=true from the flush method). Based on this I was able to craft the minimal reproducer in ruby:

#!/opt/puppetlabs/puppet/bin/ruby

require 'augeas'
file = '/etc/sysctl.d/20-fs.conf'

aug = Augeas.open

def augset(aug, file, name, value)
  aug.set("/files/#{file}/#{name}", value) 
  aug.save!
  aug.load!
end

augset(aug, file, 'fs.nr_open', '100001')
augset(aug, file, 'fs.inotify.max_user_watches', '8193')

aug.close

Reproducing:

root@ubuntu2004:~# cat /etc/sysctl.d/20-fs.conf
cat: /etc/sysctl.d/20-fs.conf: No such file or directory
root@ubuntu2004:~# ./broken_sysctl.rb 
root@ubuntu2004:~# cat /etc/sysctl.d/20-fs.conf
fs.inotify.max_user_watches = 8193

[jay7x]

Little follow-up.. the Ruby example above is confirmed to be broken by https://github.com/hercules-team/augeas/commit/eb04250a05671b2d001444b72b8778328d209d75 too.. Both Puppet and Ruby examples works fine with libaugeas built from previous commit. Both are broken by this commit.

[h-haaks]

Ref https://github.com/hercules-team/augeas/pull/691#issuecomment-1512927107 it seems like a workaround could be implemented in augeasprovider_core by invoking aug.load! twice.

Possibly write some tests that start to fail when a fix become available in the puppet agent.

[ekohl]

I forgot that we need a released version of augeasproviders_core. https://github.com/voxpupuli/puppet-augeasproviders_core/pull/43 should do that.

[h-haaks]

I wonder why the puppet7/centos8 test still pick up augeasproviders_core 3.2.0 ... https://github.com/voxpupuli/puppet-augeasproviders_sysctl/actions/runs/4791754540/jobs/8570214108#step:5:309

[ekohl]

It's green now, perhaps too quick?

[h-haaks]

It's green now, perhaps too quick?

I guess so :)