Refs #30057 - Configure Pulpcore Worker Count

[wbclark]

No description provided.

[wbclark]

@ehelms I checked with several of the largest enterprise Katello users and found that some do indeed use more than 8 workers for Pulp 2. The largest count I'm aware of in production is one organization that uses 16 workers in their deployment. That is an environment with extensive hardware resources and I believe around 65,000 Content Hosts.

For such use cases, I felt it was necessary to not set a hard limit of 8 workers but instead set an upper limit which is beyond what anyone could reasonably use.

So my intended behavior at the time that I wrote this is that default parameters will enable one worker per logical CPU up to the soft limit of 8, but users who understand the performance trade-offs should still have the option to increase that value up to a hard limit.

The hard limit wouldn't be strictly necessary at all, except that it provides a convenient way to disable and stop excess workers when the count is decreased by the user. With that said, perhaps there is a better pattern in Puppet for that but I'm not aware of it. My current implementation is admittedly pretty kludgy but - pulp{2,3} differences notwithstanding for now - should satisfy my 3 requirements of 1. sane default behavior for the majority of users, 2. ability to tune higher if really desired, and 3. prevent excess workers from remaining enabled in cases where the count is lowered.

[ehelms]

Sounds good, once tests are passing I'm good with this change.

[ekohl]

In theforeman/puppet-foreman#843 I chose a custom fact. In this case you don't have to worry about the migration so it's easier. You also don't have to deal with packaging placing a file that you need to clean up.

I don't mind merging this now (provided the lint comment is addressed), but it'd be nice to lift the artificial limit of 25.

[ekohl]

Why do you ignore this? It is documented now.

[wbclark]

I was getting a lint failure that $facts['processors']['count'] is not documented. (Interesting to me that we didn't see the same issue with $facts['networking']['fqdn'] three lines above)

[ekohl]

voxpupuli/puppet-lint-param-docs#12 should address that.

[wbclark]

Thanks @ekohl for the suggestions. I've pushed some changes and I'll explain these below:

1. I removed the #lint_ignore but I'm expecting it to complain due to a bug with the linter (we'll probably add that back for now while investigating a fix for the bug)

2. I'm taking the suggested approach of using a custom fact, which allows me to lift the arbitrary limit of 24 workers. While I think it's unlikely anyone would ever need more than 24 workers, I think the code is a bit cleaner this way.

3. TODO: I want to implement a test case of decreasing the worker count to have confidence about the above implementation

[ekohl]

For this to be true, you need to set the fact:

let(:facts) { super().merge(pulpcore_workers: ['pulpcore-worker@6.service']) }

I'd also make this a separate it block since it's something different you want to ensure from the previous one. The test with worker 5 ensures worker_count sets the right number of running workers. Worker 6 tests that the purging happens.

If you want to test more edge cases, you'd test with:

• worker count 5
  • fact missing
  • fact present, but empty
  • fact with 6 workers
  • fact with 6 workers

[wbclark]

@ekohl how could the fact be missing? my understanding is that the fact should always be present, and only if the current worker count is zero should it be empty.

[ekohl]

The choice I made in puppet-foreman was not to generate a fact if the directory didn't exist. Similar, you could check for the existence of the pulp-worker@.service file. One benefit of not presenting it is that you don't generate the fact for every host in your Puppet environment. That helps with fact storage, though I don't know how much it really matters.

[wbclark]

Thanks for the explanation @ekohl . In that case I think it does matter, c.f. https://access.redhat.com/solutions/3764171

I'll check for pulpcore-worker@service in the fact definition

[ekohl]

Rather than repeating it every time I'd suggest to nest:

context 'with worker_count set to 5' do
  let(:params) { { worker_count: 5} }

  context 'with 6 workers from fact' do
    let(:facts) { super().merge(pulpcore_workers: (1..6).map { |n| "pulpcore-worker@#{n}.service" } }

    it 'is expected to reduce the workers to 5' do
      # ...
    end
  end

  context 'with 4 workers from fact' do
    # ...
  end

  context 'with 0 workers from fact' do
    # ...
  end
end

That's how rspec is supposed to be used and you can see the same structure if you run the tests (in verbose mode).

[wbclark]

@ekohl could you recommend syntax to actually loop over workers 1 through 5 here rather than just checking the fifth worker?

[ekohl]

In Ruby you can do it:

it 'is expected to reduce the workers to 5' do
  (1..5).each do |n|
    is_expected.to contain_service("pulpcore-worker@#{n}")
  end
end

[wbclark]

Thanks, I wasn't sure about nesting it within it...do

[ekohl]

https://www.rubyguides.com/2016/02/ruby-procs-and-lambdas/ might be interesting to read. The signature is roughly it(message = nil, &block). Once you know that, it feels more familiar.