Fixes #34824 - properly restart foreman when puma config changed

[evgeni]

we need to restart foreman.service before a (possible) restart of
foreman.socket, as the later also does restart foreman.service which
leads to foreman.socket being started instead of restarted

/Service[foreman.socket]: Starting to evaluate the resource (2265 of 2522)
Executing: '/bin/systemctl is-active -- foreman.socket'
Executing: '/bin/systemctl restart -- foreman.socket'
/Service[foreman]: Starting to evaluate the resource (2266 of 2522)
Executing: '/bin/systemctl is-active -- foreman'
Executing: '/bin/systemctl show --property=NeedDaemonReload -- foreman'
Executing: '/bin/systemctl daemon-reload'
Executing: '/bin/systemctl unmask -- foreman'
Executing: '/bin/systemctl start -- foreman'
Executing: '/bin/systemctl is-enabled -- foreman'
/Stage[main]/Foreman::Service/Service[foreman]/ensure: ensure changed 'stopped' to 'running'

But in this case the now running foreman.service didn't see the changes
to the service file that daemon-reload would have loaded.

[wbclark]

that's a lot simpler than what I thought it would be

how about a test to prevent us from breaking it in the future?

[evgeni]

how about a test to prevent us from breaking it in the future?

a test of what? the before relationship? That I can add. What I don't really know is how to test the actual "foreman is running with the expected puma settings".

[wbclark]

It's a little rough maybe but you could do something like

require 'spec_helper_acceptance'

describe 'configures puma worker count', :order => :defined do
  context 'initial configuration with 2 puma workers' do
    it_behaves_like 'an idempotent resource' do
      let(:manifest) do
        <<-PUPPET
        class { 'foreman':
          foreman_service_puma_workers => 2,
        }
        PUPPET
      end
    end

    describe service("foreman") do
      it { is_expected.to be_enabled }
      it { is_expected.to be_running }
    end

    [0,1].each do |i|
      describe command("ps aux | grep -v grep | grep \"puma: cluster worker #{i}\"") do
        its(:exit_status) { is_expected.to eq 0 }
      end
    end

    describe command('ps aux | grep -v grep | grep "puma: cluster worker 2"') do
      its(:exit_status) { is_expected.to eq 1 }
    end
  end

  context 'reconfigure to use 1 puma worker and restart foreman.service' do
    it_behaves_like 'an idempotent resource' do
      let(:manifest) do
        <<-PUPPET
        class { 'foreman':
          foreman_service_puma_workers => 1,
        }
        PUPPET
      end
    end

    describe service("foreman") do
      it { is_expected.to be_enabled }
      it { is_expected.to be_running }
    end

    describe command('ps aux | grep -v grep | grep "puma: cluster worker 0"') do
      its(:exit_status) { is_expected.to eq 0 }
    end

    [1,2].each do |i|
      describe command("ps aux | grep -v grep | grep \"puma: cluster worker #{i}\"") do
        its(:exit_status) { is_expected.to eq 1 }
      end
    end
  end
end

[evgeni]

It's a little rough maybe but you could do something like

I'll try tomorrow, thanks

[wbclark]

It's a little rough maybe but you could do something like

I'll try tomorrow, thanks

OK, cool. And I suppose a cleaner implementation might be possible with https://github.com/mizzy/serverspec/blob/master/lib/serverspec/type/process.rb

[evgeni]

        expected: "2"
            got: "2\n"

really rspec?!

[ehelms]

Lots of options, there is also journalctl -t foreman | grep Workers and systemctl status foreman.service | grep worker

[evgeni]

@ehelms @wbclark what do you find more readable?

    describe command('ps aux | grep -v grep | grep -c "puma: cluster worker"') do
      its('stdout.to_i') { is_expected.to eq 1 }
    end

or

    describe process('puma: cluster worker') do
      its(:count) { is_expected.to eq 1 }
    end

Myself, I prefer process (its count does roughly the same as our grep above: ps aux | grep -w -- #{escape(process)} | grep -v grep | wc -l https://github.com/mizzy/specinfra/blob/1da649b2934c7b8e8872f5622c10cb00df1b656b/lib/specinfra/command/base/process.rb#L8), but has the downside that while a process in serverspec has other features (like be_running), they don't work here as for other process details it uses ps -C and that fails catastrophically with puma overriding the cmdline with fancy data.

What should probably work is process('rails') and then counting N+1 - N workers and 1 for the scheduler process, like this:

   describe process('rails') do
     it { is_expected.to be_running }
     # 1 worker + 1 scheduler = 2 processes
     its(:count) { is_expected.to eq 2 }
   end

Edit: turns out it does not work, as now the ps -C matches rails correctly, but the grep in count does not.

[ehelms]

The use of process is very readable and understandable. I am not seeing downsides to that approach so I lean there.

[evgeni]

The use of process is very readable and understandable. I am not seeing downsides to that approach so I lean there.

Updated then!

[evgeni]

Ubuntu 🙄

[wbclark]

\o/

[evgeni]

Thanks @wbclark for the test suggestions! That worked perfectly.