Merging foreman-tasks into the core

Marek_Hulan · August 24, 2020, 9:27am

Hello,

we briefly discussed this in a smaller group, but we’d like to revisit possibility of merging foreman-tasks into the core. It was discussed in the past and got support, but we ended up reverting the change, due to packaging issues. So this thread is created to get the feedback and find any concerns early, before the any effort is invested.

Summary of the reasons for the merge:

many plugins depend on it, katello, REX, ansible, salt, chef, scc_manager, wreckingball, foreman_template_tasks
it’s quite stable, without breaking changes, the main dependency (dynflow) is already a dependency of core
the list of runtime dependencies is not big - dynflow (already a dependency), foreman-tasks-core (depends on dynflow only), get_process_mem (requires ffi, already in), parse_cron (no deps), sinatra (this one can raise eyebrow, it’s used by dynflow console, we already have it pacakged in SCL for proxy)
we’d have mgmt console for active job tasks we already spawn on background, more cron based tasks could be converted and monitored/managed (audits expiration, config reports cleanup etc)

I think it shouldn’t be much work if we decide to merge it in as is. Meaning we’d keep the namespace for example. We should only modify the plugin registration from engine.rb to use native Foreman definitions.

Is there something else people would like to see changed before the merge? E.g. changes in API, like renaming recurring logics? Is there any concern with the merge in general?

Thanks for any feedback!

lzap · August 24, 2020, 11:02am

As a discovery maintainer, I would love to merge discovery as well long-term. But the plans are to base discovery on SSH, which actually requires tasks/ReX in core too. I think (bare-metal) provisioning with discovery should be vastly improved as one of the key features of Foreman going forward.

+1 from me.

ekohl · August 24, 2020, 5:40pm

In the installer there’s also code to deploy a cronjob to clean up.

github.com

theforeman/puppet-foreman/blob/master/manifests/plugin/tasks.pp

# @summary Install the foreman-tasks plugin
#
# @param automatic_cleanup
#   Enable automatic task cleanup using a cron job
#
# @param cron_line
#   Cron line defining when the cleanup cron job should run
#
class foreman::plugin::tasks(
  Boolean $automatic_cleanup = false,
  String $cron_line = '45 19 * * *',
) {
  foreman::plugin { 'tasks':
    package => $foreman::plugin_prefix.regsubst(/foreman[_-]/, 'foreman-tasks'),
  }
  $cron_state = $automatic_cleanup ? {
    true    => 'file',
    default => 'absent',
  }
  file { '/etc/cron.d/foreman-tasks':

This file has been truncated. show original

Should that be moved to core or can the cleanup be executed via a task? It has the benefit that deployment is easier due to fewer requirements, which also makes it easier run in HA setups. On the other hand, cleanin up a system from inside the system has its own challenges.

In packaging we also have a logrotate config that we deploy. That should also be moved into the Foreman package itself.

For sinatra we can probably make a separate foreman-tasks-console subpackage in our RPMs/debs to keep it optional if we want to. This requires a separate bundler group and of course the code to gracefully handle its absence.

However, for my understanding: what spawns the dynflow console process? AFAIK it’s listening on a separate TCP port. Does it have a systemd service? I can’t find it.

Also note that we currently have foreman-tasks-core which is also required by some smart_proxy parts. You can’t just merge that into foreman itself.

aruzicka · August 25, 2020, 7:20am

I don’t really like the idea of spawning tasks to remove tasks.

Is it worth it?

It just gets mounted under /foreman_tasks/dynflow route in the rails process and that’s it.

github.com

theforeman/foreman-tasks/blob/master/config/routes.rb#L70


      
                    get :summary
                    get '/:parent_task_id/sub_tasks', action: 'index'
                    get '/summary/:id/sub_tasks/', action: 'summary_sub_tasks'
                    post :callback
                  end
                end
              end
          
              if ForemanTasks.dynflow.required?
                require 'dynflow/web'
                mount ForemanTasks.dynflow.web_console => '/dynflow'
                if defined? ::Sidekiq
                  require 'sidekiq/web'
                  redis_url = ENV['DYNFLOW_REDIS_URL'] || SETTINGS.dig(:dynflow, :redis_url)
                  Sidekiq.redis = { url: redis_url }
                  mount Sidekiq::Web => '/sidekiq', :constraints => ForemanTasks::Dynflow::SidekiqConsoleConstraint.new
                end
              end
            end
          end

I’d say foreman-tasks-core part should stay a separate gem. foreman-tasks uses only a small subset of foreman-tasks-core and having it as a gem would mean we could load it into both foreman and into proxy, the same way we do it now. It also hardly ever changes nowadays so it would be a set it and forget it kind of thing.

ehelms · August 27, 2020, 6:49pm

This feels right given it’s place in the ecosystem and the simplification it provides to deployment and coding.

I can see the hesitation there. Given tasks growth is a predictable and well known issue for users, having the tasks code self clean itself without a user having to deploy anything separate from the application itself has big advantages for maintenance. Is there another way we could do this from within the application? Or that tasks can safely clean themselves up?

aruzicka · August 28, 2020, 8:29am

I can see the hesitation there.

I’m not saying it couldn’t be done, we’d just have to make extra sure the task-cleaning-task doesn’t try to clean itself and so on.

Is there another way we could do this from within the application?

There’s the execution plan cleaner we use in dynflow on smart proxy to wipe old (older than a day) execution plans. This could be extended and used, but its operation is a bit hidden from the users.

Or that tasks can safely clean themselves up?

Something like ephemeral tasks which delete themselves when they succeed and stick around unless there’s an error?

ehelms · August 28, 2020, 12:41pm

I was assuming here, by task cleanup, this would only affect completed task runs and not destroy any running or scheduled tasks.

The more I think on that one the more I realize this would help the problem, but you can still end up with a growing table of tasks in the end and still need a cleaner.

ekohl · August 28, 2020, 12:57pm

IMHO the cleanup doesn’t have to be a blocker to merging, but it’s good to think about this. Also when we keep the containerization case in mind. Prior to merging this, it’s not something that you need to worry about. After, it probably is. For me it would be sufficient if the Foreman manual at least has a recommendation about best practices.