Refs #32917 - Don't deploy or configure Redis with new tasking system

[wbclark]

This is a more gradual alternative to #206

In particular, it doesn't include the ability to enable or disable management of Redis, nor does it consider the content caching feature.

it simply includes the redis class and configures Pulpcore for Redis, based on the value of use_rq_tasking_system

[wbclark]

rebased and added logic to handle content caching

[ekohl]

Please rebase now that #203 is merged.

[wbclark]

 Failures:

  1) basic installation Command "DJANGO_SETTINGS_MODULE=pulpcore.app.settings PULP_SETTINGS=/etc/pulp/settings.py rq info -c pulpcore.rqconfig" stdout is expected to match /^0 workers, /
     Failure/Error: its(:stdout) { is_expected.to match(/^0 workers, /) }
       expected "Error 99 connecting to localhost:6379. Cannot assign requested address.\n" to match /^0 workers, /
       Diff:
       @@ -1 +1 @@
       -/^0 workers, /
       +Error 99 connecting to localhost:6379. Cannot assign requested address.
       
     # ./spec/acceptance/basic_spec.rb:83:in `block (3 levels) in <top (required)>'

  2) basic installation Command "DJANGO_SETTINGS_MODULE=pulpcore.app.settings PULP_SETTINGS=/etc/pulp/settings.py rq info -c pulpcore.rqconfig" exit_status is expected to eq 0
     Failure/Error: its(:exit_status) { is_expected.to eq 0 }
       
       expected: 0
            got: 1
       
       (compared using ==)
       
     # ./spec/acceptance/basic_spec.rb:85:in `block (3 levels) in <top (required)>'

Finished in 8 minutes 54 seconds (files took 1 minute 7.13 seconds to load)
176 examples, 2 failures

This I can fix, but what I want to point out is, we only don't see similar results in some of the enable/disable rq tasking tests, because of the order in which those tests are running.... once redis gets installed once, the failure won't occur.

[ekohl]

That sounds like Pulp has a hard dependency on Redis and something we should take upstream.

[ekohl]

You can also consider a line in init.pp like $uses_redis = $use_rq_tasking_system or $cache_enabled and use that variable in database.pp and settings.py.

[wbclark]

That sounds like Pulp has a hard dependency on Redis and something we should take upstream.

That isn't how I interpreted it. If you look closely at what happens:

[root@centos8-64-1 ~]# DJANGO_SETTINGS_MODULE=pulpcore.app.settings PULP_SETTINGS=/etc/pulp/settings.py rq info -c pulpcore.rqconfig
Error 111 connecting to localhost:6379. Connection refused.

I'll update that test.

Additionally, I'm going to move the 'enable content caching' test out of basic_spec.rb where we have multiple top-level describe blocks. I'd like to move the reducing worker count test out into its own test as well, but there is already a PR open for it (#209 )

[wbclark]

You can also consider a line in init.pp like $uses_redis = $use_rq_tasking_system or $cache_enabled and use that variable in database.pp and settings.py.

Probably a good idea to consolidate the logic in this way. Thanks

I was actually taking a similar approach with #206 and I'm not sure the reason I didn't follow it here. Probably a simple oversight

[ehelms]

This is already covered by https://github.com/theforeman/puppet-pulpcore/pull/207/files#diff-257fe3136ecfdbfcb123b6445ff2de6c21d1fe08b0d6af54c08dde40637d2594R26

[wbclark]

Nice catch, thank you.

[ehelms]

This are OK here, but they feel better up here https://github.com/theforeman/puppet-pulpcore/pull/207/files#diff-257fe3136ecfdbfcb123b6445ff2de6c21d1fe08b0d6af54c08dde40637d2594R19 for me as it connects all of the configuration file together.

[wbclark]

I started to think that it makes sense to structure the tests around the real world use case rather than the specific object being tested..

So instead of having all tests for the configuration file in the same place, have a test for 'content caching is disabled', 'postgresql tasking system is used', and so on.

I see two primary benefits: 1. This feels more maintainable in the longer term (when we make modifications to a specific feature, the tests for that feature are all located in that block, rather than spread out across different components the feature touches). 2. This should lead to more readable test output, and particularly failures. So instead of getting a failure like default parameters - it configures pulpcore - it is expected to contain a file with (lots of stuff, including all kinds of things that aren't relevant to the specific thing you broke) you would instead see something more like default parameters - it uses new tasking system - it is expected to contain a configuration file with specific things that are relevant to the tasking system

I want to pursue this idea more consistently in my planned refactoring. For this PR I thought, it makes sense to write the net-new tests using this style, to minimize the amount of re-writing I have to do later.

[wbclark]

Thanks for feedback @ehelms !

I pushed an update to address these items. In particular I use %r{} consistently throughout spec/classes/pulpcore_spec.rb (I thought we were already mixing // often but I was mistaken -- there was only a single, very small instance of that before the code I introduced here)

Regarding the overall structure of the tests, I did opt to keep for now the change of moving some tests of the configuration file contents out of the it 'configures pulpcore' block and into feature-specific blocks. Arguably that should be handled in my other test refactoring PR, but I'm balancing that fact vs. the fact that I already need to add these feature-specific blocks here now, and it's less work for me overall to not write it one way now, knowing that I'm planning to re-write it a different way soon.

With that said, if any of those changes are really controversial, I can revert them and tackle them later. The goal in not writing the same code twice is to save time. If it's not actually saving me (rather us, since review time is part of the equation too) time in the end, I'll go ahead and do what is asked of me here, and re-write it the way I think it really should be in the other PR.

[ehelms]

If you are curious where I got the using %r{} everywhere, this comes from some initial work @ekohl did to add Rubocop. You can see what that looks like for example here, https://github.com/theforeman/puppet-qpid/pull/167/files. This uses an inherited Rubocop configuration from voxpupuli-test which has:

https://github.com/voxpupuli/voxpupuli-test/blob/master/rubocop.yml#L467-L470

Thus, they have chosen to enforce the use of %r{} as the way to define regex.

[ekohl]

That sounds like Pulp has a hard dependency on Redis and something we should take upstream.

Looks like we're running into this now: https://community.theforeman.org/t/katello-nightly-rpm-pipeline-1027-failed/24403