Fixes #32891 - Add feature to enable new tasking system and enable it by default #203

wbclark · 2021-06-27T23:29:36Z

This introduces a new parameter use_legacy_tasking_system which
defaults to true and configures the application the same as before.
When set to false, pulpcore is configured with the new tasking system
instead. Acceptance tests are included to ensure that users can switch
between the two tasking systems. This is backwards compatible with
Pulpcore versions older than 3.13 ONLY when configured to use the
legacy tasking system. The newer tasking system requires Pulpcore
version >= 3.13.

wbclark · 2021-06-27T23:32:06Z

NOTE: While the default (use_legacy_tasking_system = true) should be fully backwards compatible and consistent with all behavior prior to this commit, this also introduces tests to enable and disable the new tasking system, and those will FAIL until pulpcore >= 3.13 is available in foreman-packaging.

wbclark · 2021-06-27T23:36:44Z

spec/acceptance/disable_new_tasking_system_spec.rb

+    its(:exit_status) { is_expected.to eq 0 }
+  end
+
+  describe command("PULP_SETTINGS=/etc/pulp/settings.py pulpcore-manager diffsettings") do


This is somewhat of a hack. diffsettings will show the difference relative to default settings for Django, so pulpcore's USE_NEW_WORKER_TYPE setting (introduced with v3.13) should be visible here.

Is there a reason not to just assert the contents of settings.py?

I think you can also use dynaconf to dump the settings. I think that won't break when Pulp switches its default.

Is there a reason not to just assert the contents of settings.py?

To me, reading the running configuration of the application seems closer to testing the true, desired outcome vs. reading the contents of a file. For that reason I see it as a better approach in an "acceptance" test.

With that said, we could easily test both and treat one as an in memory test for the currently running process and the other as a test of the persistent configuration.

I think you can also use dynaconf to dump the settings. I think that won't break when Pulp switches its default.

I'll have to look into how to do this

This test looks good enough for now. Or just yank it since we test this in the unit tests and we are asserting the behaviors we expect anyway. That's my TLDR way of saying, let's not hold this up on that sorta detail.

I'll have to look into how to do this

FYI: sosreport has this:
https://github.com/sosreport/sos/blob/c7d3644c0c64e9e5439806250592a55c8e2de26f/sos/report/plugins/pulpcore.py#L99-L102

spec/acceptance/disable_new_tasking_system_spec.rb

templates/settings.py.erb

ekohl · 2021-06-28T08:52:14Z

Should this be rebased on #202?

ehelms · 2021-06-28T12:22:47Z

manifests/init.pp

@@ -197,6 +202,7 @@
  Pulpcore::ChecksumTypes $allowed_content_checksums = ['sha224', 'sha256', 'sha384', 'sha512'],
  String[1] $remote_user_environ_name = 'HTTP_REMOTE_USER',
  Integer[0] $worker_count = min(8, $facts['processors']['count']),
+  Boolean $use_legacy_tasking_system = true,


To avoid the ole legacy wording, perhaps we name it use_postgresql_tasking_system or `use_rq_tasking_system.

or an Enum of ['rq', 'postgresql'] or ['rq', 'new']?

If I understand pulp/pulpcore@0e1c629 correctly, there are two changes in that commit:

add a script ("entrypoint" in Python lingo) pulpcore-worker which should be used instead of rq

allow for two different tasking systems: old RQ and new PostgreSQL based

So switching to the old one doesn't require us to switch to calling rq directly (like this PR does right now) and I think this is actually officially unsupported by Pulpcore (or should be unsupported)

And if you look at the commit in pulp_installer (pulp/pulp_installer@39a8869) they switch to the new binary for all 3.13+ installs, unrelated to the used tasking system

I went with use_rq_tasking_system for now but I'm open to other ideas.

Some thoughts: at a base level it would be nice for this module to be consistent with the setting name in pulpcore, but USE_NEW_WORKER_TYPE has a problem in that the postgresql tasking system won't be "new" forever, and something that is described as "new" could be perceived by users as being unstable, unsafe, etc.

I like being specific and descriptive about what technology is actually used, so Enum['rq', 'postgresql'] is appealing for that reason, but that could be more difficult for foreman-installer users ("what were the options again? I need to find that in the docs somewhere") vs. a simple boolean which can be set true or false.

So I went with use_rq_tasking_system, added a note in the parameter documentation that this refers to the older rq workers tasking system and the alternative is the postgresql tasking system introduced in pulpcore 3.13.

When we later reach a point where use_rq_tasking_system is false by default, this seems particularly intuitive as an interface to allow users to opt-in to use the older tasking system instead.

spec/acceptance/disable_new_tasking_system_spec.rb

templates/pulpcore-resource-manager.service.erb

ehelms · 2021-06-28T15:28:12Z

To recap, the first big question is whether we take a progressive enhancement, keep supporting Pulp 3.11 and 3.13+ or just support 3.13+ only with the new wrapper. #202 takes the latter approach.

I lean towards the 3.13+ only approach, there are enough changes coming that I think its safe to cut off 3.11 from the main branch, and create a stable release branch if we need to backport to supporting 3.11 for any reason. The big changes, supporting new tasking system, content caching are only applicable to 3.13+ and there is no plan to backport any of those so I think its safe for our module to look forward only.

wbclark · 2021-06-28T16:39:16Z

@ehelms @ekohl @evgeni thanks for all comments and feedback!

I've updated this with the following changes:

rebased on Support Pulp 3.13, drop earlier versions #202 (support only pulpcore >= 3.13, always use new worker entrypoint)
renamed the parameter to use_rq_tasking_system (kept this a boolean instead of moving to an enum)
removed a redundant test of ps -e -o command output from the new acceptance tests (since always using new worker entrypoint)
removed the new inaccurate comment from settings.py
always include USE_NEW_WORKER_TYPE in settings.py (don't rely on default) and set its value based on use_rq_tasking_system

ekohl · 2021-06-28T16:50:14Z

To recap, the first big question is whether we take a progressive enhancement, keep supporting Pulp 3.11 and 3.13+ or just support 3.13+ only with the new wrapper. #202 takes the latter approach.

Just to throw it out there, we could introduce the same wrapper in 3.11 if we really cared about this.

evgeni · 2021-06-28T18:31:39Z

To recap, the first big question is whether we take a progressive enhancement, keep supporting Pulp 3.11 and 3.13+ or just support 3.13+ only with the new wrapper. #202 takes the latter approach.

Just to throw it out there, we could introduce the same wrapper in 3.11 if we really cared about this.

Given the current plans around pulpcore upgrades, I don't think we need to invest in 3.11 compatibility too long anymore?

manifests/init.pp

evgeni · 2021-06-29T09:45:33Z

A question on how this will play out when we decide to flip the switch and use the new (PostgreSQL) system:

Right now this is a direct parameter to the class (pulpcore::use_rq_tasking_system) and thus will be recorded in the answers file as true.
Once we decide to flip the switch, we'll edit init.pp and set use_rq_tasking_system to false.
Given the class parameters are overridden by answers, users that had it set to true before, will remain on the RQ system.
When we now introduce a migration, it would migrate all users from true to false, regardless whether they have set this manually or because it's the default. Wouldn't this migration thus make it impossible to flip back to RQ, as every time the installer is run, the value gets migrated?

ekohl · 2021-06-29T09:45:36Z

To recap, the first big question is whether we take a progressive enhancement, keep supporting Pulp 3.11 and 3.13+ or just support 3.13+ only with the new wrapper. #202 takes the latter approach.

Just to throw it out there, we could introduce the same wrapper in 3.11 if we really cared about this.

Given the current plans around pulpcore upgrades, I don't think we need to invest in 3.11 compatibility too long anymore?

The case I can imagine is that we have update the pulpcore repo definition in katello-repos.rpm. So the user runs:

yum update
foreman-installer

Now what the user should have done:

yum update katello-repos
yum update
foreman-installer

By shipping the wrapper in 3.11, at least the service will continue to start up.

ekohl · 2021-06-29T09:47:56Z

Wouldn't this migration thus make it impossible to flip back to RQ, as every time the installer is run, the value gets migrated?

Migrations only run once. There's a file (IIRC .applied in $scenario.migrations) that lists names of migrations that already ran.

evgeni · 2021-06-29T10:13:05Z

The case I can imagine is that we have update the pulpcore repo definition in katello-repos.rpm. So the user runs:
* yum update
* foreman-installer

Ooooh, you mean for users already on 4.1 that is using 3.11? Fair. But at the same time, this will only happen once?

ekohl · 2021-06-29T10:15:13Z

Ooooh, you mean for users already on 4.1 that is using 3.11? Fair. But at the same time, this will only happen once?

Yes, but it's certainly a case I worry about. We should very carefully approach it to make it a smooth and painless migration.

ehelms · 2021-06-29T14:36:30Z

You can now rebase this on master

wbclark · 2021-06-29T14:49:30Z

You can now rebase this on master

Rebased

Requires theforeman/puppet-pulpcore#203

templates/settings.py.erb

wbclark · 2021-07-06T18:37:37Z

Current implementation is dealing with this issue when enabling the new tasking system:

# systemctl status pulpcore-resource-manager.service
● pulpcore-resource-manager.service
   Loaded: not-found (Reason: Unit pulpcore-resource-manager.service not found.)
   Active: active (running) since Tue 2021-07-06 18:32:08 UTC; 3min 56s ago
 Main PID: 26471 (pulpcore-worker)
    Tasks: 2 (limit: 5970)
   Memory: 46.2M
   CGroup: /system.slice/pulpcore-resource-manager.service
           └─26471 /usr/libexec/platform-python /usr/bin/pulpcore-worker --resource-manager

Jul 06 18:32:08 centos8-64-1 systemd[1]: Started Pulp Resource Manager.
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: pulpcore.tasking.entrypoint:INFO: Starting rq type worker
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: pulpcore.tasking.worker_watcher:INFO: Worker 'resource-manager' shutdown
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: pulpcore.tasking.worker_watcher:INFO: Cleaning up shutdown worker 'resource-manager'.
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: rq.worker:INFO: Worker rq:worker:resource-manager: started, version 1.8.1
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: rq.worker:INFO: Subscribing to channel rq:pubsub:resource-manager
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: rq.worker:INFO: *** Listening on resource-manager...
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: pulpcore.tasking.worker_watcher:INFO: New worker 'resource-manager' discovered
Jul 06 18:32:12 centos8-64-1 pulpcore-resource-manager[26471]: pulp [None]: rq.worker:INFO: Cleaning registries for queue: resource-manager
Jul 06 18:32:54 centos8-64-1 systemd[1]: pulpcore-resource-manager.service: Current command vanished from the unit file, execution of the command list won't be resumed.

manifests/service.pp

wbclark · 2021-07-06T20:32:29Z

To my knowledge, this is now ready to go, as it provides the agreed upon design and has complete test coverage.

Requesting a fresh round of reviews. Thanks for all the feedback and testing so far.

templates/settings.py.erb

spec/acceptance/enable_new_tasking_system_spec.rb

templates/settings.py.erb

evgeni

i'll let the others do the nitpicks, looks good to me!

manifests/init.pp

This introduces a new parameter `use_rq_tasking_system` with default value false that configures pulpcore to use the same RQ worker tasking system as before. When false, it instead configures pulpcore to use the newer PostgreSQL tasking system introduced in pulpcore version 3.14. Acceptance tests are included to ensure users can switch between either worker type.

ehelms

Updates look good, thanks @wbclark for addressing

Requires theforeman/puppet-pulpcore#203

Requires theforeman/puppet-pulpcore#203 (cherry picked from commit baa0e9c)

theforeman-bot added Needs testing Not yet reviewed labels Jun 27, 2021

wbclark force-pushed the 32891_new_tasking_system branch from 8e3dd6a to e444885 Compare June 27, 2021 23:34

wbclark commented Jun 27, 2021

View reviewed changes

spec/acceptance/disable_new_tasking_system_spec.rb Outdated Show resolved Hide resolved

wbclark commented Jun 27, 2021

View reviewed changes

templates/settings.py.erb Outdated Show resolved Hide resolved

ehelms reviewed Jun 28, 2021

View reviewed changes

ekohl reviewed Jun 28, 2021

View reviewed changes

spec/acceptance/disable_new_tasking_system_spec.rb Outdated Show resolved Hide resolved

ekohl reviewed Jun 28, 2021

View reviewed changes

templates/pulpcore-resource-manager.service.erb Outdated Show resolved Hide resolved

wbclark force-pushed the 32891_new_tasking_system branch from e444885 to 1027dbf Compare June 28, 2021 16:33

evgeni reviewed Jun 29, 2021

View reviewed changes

manifests/init.pp Outdated Show resolved Hide resolved

wbclark force-pushed the 32891_new_tasking_system branch from 1027dbf to a2a9d61 Compare June 29, 2021 14:47

wbclark force-pushed the 32891_new_tasking_system branch from a2a9d61 to 2cb68a8 Compare June 29, 2021 14:50

wbclark added a commit to wbclark/puppet-foreman_proxy_content that referenced this pull request Jun 29, 2021

Refs #32891 - Optionally enable new Pulpcore tasking system

52e9cf0

Requires theforeman/puppet-pulpcore#203

wbclark mentioned this pull request Jun 29, 2021

Refs #32891 - Optionally enable new Pulpcore tasking system theforeman/puppet-foreman_proxy_content#371

Merged

wbclark added a commit to wbclark/puppet-foreman_proxy_content that referenced this pull request Jun 29, 2021

Refs #32891 - Optionally enable new Pulpcore tasking system

abdc9cf

Requires theforeman/puppet-pulpcore#203

ehelms reviewed Jun 30, 2021

View reviewed changes

templates/settings.py.erb Outdated Show resolved Hide resolved

ekohl requested changes Jul 6, 2021

View reviewed changes

manifests/service.pp Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 6, 2021

wbclark force-pushed the 32891_new_tasking_system branch from 445fb38 to e9424f9 Compare July 6, 2021 19:20

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 6, 2021

wbclark requested a review from ekohl July 6, 2021 20:27

wbclark requested review from evgeni and ehelms July 6, 2021 20:32

ehelms reviewed Jul 7, 2021

View reviewed changes

templates/settings.py.erb Outdated Show resolved Hide resolved

ekohl requested changes Jul 7, 2021

View reviewed changes

spec/acceptance/enable_new_tasking_system_spec.rb Outdated Show resolved Hide resolved

templates/settings.py.erb Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 7, 2021

evgeni reviewed Jul 7, 2021

View reviewed changes

manifests/init.pp Outdated Show resolved Hide resolved

wbclark force-pushed the 32891_new_tasking_system branch from e9424f9 to df41d0f Compare July 7, 2021 16:08

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 7, 2021

wbclark force-pushed the 32891_new_tasking_system branch from df41d0f to 4ccdb94 Compare July 7, 2021 16:16

ehelms approved these changes Jul 7, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Jul 7, 2021

ekohl approved these changes Jul 8, 2021

View reviewed changes

ehelms merged commit 9ad14f5 into theforeman:master Jul 8, 2021

ekohl mentioned this pull request Jul 8, 2021

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

Merged

ehelms pushed a commit to theforeman/puppet-foreman_proxy_content that referenced this pull request Jul 8, 2021

Refs #32891 - Optionally enable new Pulpcore tasking system

baa0e9c

Requires theforeman/puppet-pulpcore#203

ehelms added the Enhancement New feature or request label Jul 12, 2021

ehelms pushed a commit to ehelms/puppet-foreman_proxy_content that referenced this pull request Jul 12, 2021

Refs #32891 - Optionally enable new Pulpcore tasking system

bacb624

Requires theforeman/puppet-pulpcore#203 (cherry picked from commit baa0e9c)

ehelms pushed a commit to theforeman/puppet-foreman_proxy_content that referenced this pull request Jul 12, 2021

Refs #32891 - Optionally enable new Pulpcore tasking system

3567bed

Requires theforeman/puppet-pulpcore#203 (cherry picked from commit baa0e9c)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #32891 - Add feature to enable new tasking system and enable it by default #203

Fixes #32891 - Add feature to enable new tasking system and enable it by default #203

wbclark commented Jun 27, 2021

wbclark commented Jun 27, 2021

wbclark Jun 27, 2021 •

edited

Loading

evgeni Jul 5, 2021

ekohl Jul 5, 2021

wbclark Jul 6, 2021

wbclark Jul 6, 2021

ehelms Jul 7, 2021

ekohl Jul 7, 2021

ekohl commented Jun 28, 2021

ehelms Jun 28, 2021

evgeni Jun 28, 2021

wbclark Jun 28, 2021

ehelms commented Jun 28, 2021

wbclark commented Jun 28, 2021

ekohl commented Jun 28, 2021

evgeni commented Jun 28, 2021

evgeni commented Jun 29, 2021

ekohl commented Jun 29, 2021

ekohl commented Jun 29, 2021

evgeni commented Jun 29, 2021

ekohl commented Jun 29, 2021

ehelms commented Jun 29, 2021

wbclark commented Jun 29, 2021

wbclark commented Jul 6, 2021

wbclark commented Jul 6, 2021

evgeni left a comment

ehelms left a comment

Fixes #32891 - Add feature to enable new tasking system and enable it by default #203

Fixes #32891 - Add feature to enable new tasking system and enable it by default #203

Conversation

wbclark commented Jun 27, 2021

wbclark commented Jun 27, 2021

wbclark Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Jun 28, 2021

wbclark commented Jun 28, 2021

ekohl commented Jun 28, 2021

evgeni commented Jun 28, 2021

evgeni commented Jun 29, 2021

ekohl commented Jun 29, 2021

ekohl commented Jun 29, 2021

evgeni commented Jun 29, 2021

ekohl commented Jun 29, 2021

ehelms commented Jun 29, 2021

wbclark commented Jun 29, 2021

wbclark commented Jul 6, 2021

wbclark commented Jul 6, 2021

evgeni left a comment

Choose a reason for hiding this comment

ehelms left a comment

Choose a reason for hiding this comment

wbclark Jun 27, 2021 •

edited

Loading