Fixes #18757 - Move foreman-tasks service to Foreman #530

dLobatog · 2017-03-01T18:11:05Z

As per #18618, the dynflow executor is moving over to Foreman, in order
to support ActiveJob. Tasks will provide an UI and persistence layer for
Dynflow, but Foreman should be able to run jobs without it too.

Part of this effort includes moving the Dynflow executor over to
Foreman, which at the moment is managed by the foreman-tasks service.

theforeman/foreman#4316 has to be merged before this

ekohl · 2017-03-03T19:39:11Z

Any reason it has to live in tasks_service rather than service? I'm guessing that the service needs a restart if any settings change which it currently won't.

Other than that we generally keep compatibility with older Foreman versions (current and current - 1 is what we at least aim for) or bump the major version. The current implementation breaks on 1.14 without foreman-tasks so it needs to deal with this.

timogoebel · 2017-03-05T20:01:20Z

manifests/tasks_service.pp

+  $service = $::foreman::params::tasks_service,
+) inherits foreman::params {
+  service { 'foreman-tasks':
+    ensure => running,


I'd suggest pulling the variables out so you can overwrite them if you want.
Just move them to the "signature" of the class, similar to service.

dLobatog · 2017-03-09T15:52:23Z

@ekohl Actually no need to restart the tasks service AFAIK - that's not a problem. I see a small problem though if foreman 1.14 has to be supported. The situation is the following:

On 1.14:

foreman-tasks ships the foreman-tasks service. This service starts the dynflow executor.
foreman 'core' does not ship the foreman-tasks service. it does not have anything to do with dynflow

On 1.15:

foreman-tasks does not ship the foreman-tasks service. It needs foreman to run the dynflow executor
foreman 'core' ships 'foreman-tasks' to start the dynflow executor.

To keep this module compatible with both versions it would require:

having a generic 'foreman-tasks' that can be triggered from either the foreman-tasks or the foreman class https://github.com/theforeman/puppet-foreman/pull/530/files#diff-bb391d4e28641499e1a968d496bde646R2
on 1.15: trigger foreman-tasks service from foreman
on 1.14: trigger foreman-tasks service from foreman-tasks
So it'd require some kind of `if version then'

We thought about changing the foreman-tasks service to a "fake" one that simply calls 'foreman-jobs' (a renamed foreman-tasks task in Foreman core) for everything. That would mean:

Keeping the foreman-tasks module as-is
On 1.15: foreman-jobs is started automatically with the foreman service.

dLobatog · 2017-03-10T16:13:05Z

@mmoll What you suggested was having a parameter dynflow_in_core = true that triggers the foreman-tasks service from foreman or foreman-tasks depending on its value, correct?

mmoll · 2017-03-10T16:25:08Z

@dLobatog exactly.

dLobatog · 2017-03-10T17:09:52Z

Updated to reflect the changes asked for @ekohl @mmoll & @timogoebel - I have to add tests for this dynflow_in_core option to ensure it works properly. I suppose I'd have to add dynflow_in_core: true to the default options in the installer if theforeman/foreman#4316 gets merged on time

ohadlevy · 2017-03-14T08:59:16Z

manifests/plugin/tasks.pp

 ) inherits foreman::plugin::tasks::params {
+  if $dynflow_in_core == false {


nitpick, cant you just use

unless $dynflow_in_core {

ohadlevy · 2017-03-14T08:59:32Z

manifests/params.pp

@@ -99,6 +99,7 @@
      $init_config_tmpl = 'foreman.sysconfig'
      $puppet_etcdir = '/etc/puppet'
      $puppet_home = '/var/lib/puppet'
+      $tasks_service = 'foreman-tasks'


IMHO we should not call it foreman-tasks

ohadlevy · 2017-03-14T09:01:01Z

manifests/tasks_service.pp

@@ -0,0 +1,28 @@
+# = foreman-tasks service


nitpick about the name, imho dynflow or background-jobs makes sense not to confuse with task plugin.

I think I'd prefer to name it foreman::service::tasks (or jobs).

ohadlevy · 2017-03-14T09:01:21Z

manifests/tasks_service.pp

+  $ensure  = $::foreman::params::tasks_service_ensure,
+  $enable  = $::foreman::params::tasks_service_enable,
+) inherits foreman::params {
+  service { 'foreman-tasks':


nitpick: use the parameter?

mmoll · 2017-03-17T09:35:57Z

please check the test errors

ekohl

There's a section in the README with compatibility notes. We typically describe parameters like dynflow_in_core there with a note how 1.14 users should continue using this module.

Given these changes, we should also bump the major version after merging this. That should be a different commit and can be done in a separate PR.

ekohl · 2017-03-17T14:32:22Z

manifests/plugin/tasks.pp

-    enable => true,
-    name   => $service,
+  }
+  unless $dynflow_in_core {


I'm not sure this parameter is actually needed. Given there's an include in foreman::service if it's true that means it's going to be included anyway.

You're also losing the notify on the service if the plugin changes.

This one is precisely for the opposite thing - if it's false, I don't want to include the tasks service as it's not necessary to restart it when foreman-tasks was installed.

My point is: if it's true, it's included anyway (through foreman::service). Here you also ensure it's included if it's false. Doesn't that mean you can unconditionally include it?

ekohl · 2017-03-17T14:34:23Z

spec/classes/foreman_tasks_service_spec.rb

+
+describe 'foreman::tasks_service' do
+  let :facts do
+    on_supported_os['redhat-7-x86_64']


Given this code is relevant for every distro where we run Foreman, I'd feel more comfortable if this was tested on more platforms. We've already seen that there are subtle problems with dependencies if we take these shortcuts.

dLobatog · 2017-03-21T18:26:51Z

@ekohl @mmoll mind to double check? I think I've implemented the changes you asked for. Thanks!

mmoll · 2017-03-21T20:22:31Z

manifests/plugin/remote_execution.pp


  foreman::plugin {'remote_execution':
-    notify => Service['foreman-tasks'],
+    notify => Service[$::foreman::params::tasks_service],


what happens if that parameter is overridden?

mmoll · 2017-03-21T20:24:30Z

what's the name of service? it'll come with the RPM/DEB package, correct?

ekohl

I think this is correct now. @mmoll could you have a look again?

mmoll · 2017-08-31T13:28:32Z

README.md

@@ -51,6 +51,7 @@ previous stable release.

 ### Foreman version compatibility notes

+For Foreman 1.15 or older, please set the 'dynflow_in_core' parameter to false.


mmoll · 2017-08-31T13:29:45Z

manifests/service/jobs.pp

@@ -0,0 +1,35 @@
+# = Jobs service
+#
+# Service to start a Dynflow executorfor background job


excutor for

mmoll · 2017-08-31T13:30:09Z

manifests/service/jobs.pp

+# On foreman 1.16+ this runs the dynflow-executor. This class could be
+# renamed to dynflow-executor when this module has to be compatible with
+# 1.16 and 1.17 only
+#


version updates needed also here

mmoll · 2017-08-31T13:30:30Z

spec/classes/plugin/remote_execution_spec.rb

+      it { should contain_foreman__plugin('remote_execution').that_notifies("Class[foreman::service::jobs]") }
+    end
+
+    context "in 1.15 or lower, tasks is included" do


mmoll · 2017-08-31T13:31:17Z

looking good, but I first want to have the change done in nightly packages (after 1.16 RC1 would be good, I think).

ekohl · 2017-09-15T10:27:56Z

What's the status of the packaging? Can this be merged?

dLobatog

@ekohl @mmoll Rebased and updated the versions to say 1.16 where it was 1.15

ekohl · 2017-09-19T23:06:07Z

So now it's only pending theforeman/foreman-packaging#1549?

ares · 2017-10-16T07:52:14Z

@dLobatog sadly this needs another rebase

As per #18618, the dynflow executor is moving over to Foreman, in order to support ActiveJob. Tasks will provide an UI and persistence layer for Dynflow, but Foreman should be able to run jobs without it too. Part of this effort includes moving the Dynflow executor over to Foreman, which at the moment is managed by the foreman-tasks service. Change contain to notify Conflicts: manifests/plugin/tasks.pp

dLobatog

Rebased with the latest master

dLobatog · 2017-10-23T17:40:28Z

Tests 🍏 cc @ekohl @mmoll

ekohl

I'll send you a patch with suggestions.

ekohl · 2017-10-24T08:57:09Z

README.md

@@ -51,6 +51,7 @@ previous stable release.

 ### Foreman version compatibility notes

+For Foreman 1.16 or older, please set the 'dynflow_in_core' parameter to false.


If we want to get this in 1.16, this should be 1.15 or older

ekohl · 2017-10-24T08:59:08Z

manifests/service.pp

@@ -5,6 +5,7 @@
  $ssl       = $::foreman::ssl,
 ) {
  anchor { ['foreman::service_begin', 'foreman::service_end']: }
+  contain ::foreman::service::jobs


I think this should be surrounded by if $dynflow_in_core { and added added to the class parameters.

ekohl · 2017-10-24T09:02:40Z

manifests/params.pp

@@ -102,6 +104,8 @@
      $init_config_tmpl = 'foreman.sysconfig'
      $puppet_etcdir = '/etc/puppet'
      $puppet_home = '/var/lib/puppet'
+      $jobs_service = $dynflow_in_core ? { true => 'dynflow-executor',


Since here $dynflow_in_core is always true, you can just write $jobs_service = 'dynflow-executor. If you want to dynamically determine this it should be in the foreman::service::jobs class. I'd recommend $jobs_service = undef here and then determine it later.

ekohl · 2018-01-15T10:05:02Z

Closing since the replacement PR was merged.

dLobatog force-pushed the 18757 branch 2 times, most recently from f6171f4 to acfdc6e Compare March 1, 2017 20:15

dLobatog mentioned this pull request Mar 2, 2017

Fixes #18618 - Use Dynflow as an ActiveJob backend theforeman/foreman#4316

Merged

timogoebel suggested changes Mar 5, 2017

View reviewed changes

dLobatog force-pushed the 18757 branch from acfdc6e to 7bc317b Compare March 10, 2017 17:08

ohadlevy reviewed Mar 14, 2017

View reviewed changes

dLobatog force-pushed the 18757 branch 3 times, most recently from 55cdc15 to 2cd459d Compare March 16, 2017 13:06

dLobatog mentioned this pull request Mar 17, 2017

Add dynflow_in_core to deploy dynflow executor theforeman/foreman-installer#222

Closed

ekohl requested changes Mar 17, 2017

View reviewed changes

dLobatog force-pushed the 18757 branch 9 times, most recently from 5ca06c8 to abe98eb Compare March 21, 2017 17:34

mmoll suggested changes Mar 21, 2017

View reviewed changes

dLobatog force-pushed the 18757 branch from abe98eb to 16d3935 Compare March 22, 2017 06:34

ekohl mentioned this pull request Aug 15, 2017

Deploy dynflow-executor service via Foreman package theforeman/foreman-packaging#1549

Merged

1 task

ekohl added the Waiting on contributor label Aug 24, 2017

dLobatog force-pushed the 18757 branch from eb3a659 to 5e5b8fb Compare August 25, 2017 11:59

theforeman-bot removed the Waiting on contributor label Aug 25, 2017

dLobatog force-pushed the 18757 branch from 5e5b8fb to 8cc5359 Compare August 25, 2017 13:44

ekohl approved these changes Aug 31, 2017

View reviewed changes

ekohl mentioned this pull request Aug 31, 2017

allow foreman tasks plugin to be configured #484

Closed

mmoll reviewed Aug 31, 2017

View reviewed changes

mmoll approved these changes Aug 31, 2017

View reviewed changes

ekohl added the Waiting on contributor label Aug 31, 2017

dLobatog commented Sep 19, 2017

View reviewed changes

dLobatog force-pushed the 18757 branch from 8cc5359 to b35fd02 Compare September 19, 2017 18:35

theforeman-bot removed the Waiting on contributor label Sep 19, 2017

This was referenced Oct 4, 2017

candlepin: use own certs for qpid theforeman/puppet-katello#215

Merged

Refs #18422 - Allow configuring Dynflow pool size #596

Merged

dLobatog force-pushed the 18757 branch from b35fd02 to 07ef71f Compare October 23, 2017 16:47

dLobatog commented Oct 23, 2017

View reviewed changes

ekohl reviewed Oct 24, 2017

View reviewed changes

ekohl mentioned this pull request Oct 24, 2017

Fixes #18757 - Handle dynflow service in foreman core #602

Merged

ekohl closed this Jan 15, 2018

		) inherits foreman::plugin::tasks::params {
		if $dynflow_in_core == false {

		@@ -51,6 +51,7 @@ previous stable release.

		### Foreman version compatibility notes

		For Foreman 1.15 or older, please set the 'dynflow_in_core' parameter to false.

		@@ -51,6 +51,7 @@ previous stable release.

		### Foreman version compatibility notes

		For Foreman 1.16 or older, please set the 'dynflow_in_core' parameter to false.

Fixes #18757 - Move foreman-tasks service to Foreman #530

Fixes #18757 - Move foreman-tasks service to Foreman #530

Conversation

dLobatog commented Mar 1, 2017

ekohl commented Mar 3, 2017

Choose a reason for hiding this comment

dLobatog commented Mar 9, 2017

dLobatog commented Mar 10, 2017

mmoll commented Mar 10, 2017

dLobatog commented Mar 10, 2017

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

mmoll commented Mar 17, 2017

ekohl left a 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

dLobatog commented Mar 21, 2017

Choose a reason for hiding this comment

mmoll commented Mar 21, 2017

ekohl left a 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

mmoll commented Aug 31, 2017

ekohl commented Sep 15, 2017

dLobatog left a comment

Choose a reason for hiding this comment

ekohl commented Sep 19, 2017

ares commented Oct 16, 2017

dLobatog left a comment

Choose a reason for hiding this comment

dLobatog commented Oct 23, 2017

ekohl left a 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 Jan 15, 2018