Fixes #38022 - Do not use undefined setting

[adamlazik1]

Sometimes the production.log is filled with a message: "Setting remote_execution_workers_pool_size has no definition, please define it before using"

This message does not appear to be a blocker to any of the existing functionalities, however; it is misleading while troubleshooting the issues.

[adamruzicka]

First, this behaves differently than it used to:

# With this patch
> Foreman::Application.dynflow.world.config.queue
=> {:default=>{:pool_size=>5}}

# Without this patch
> Foreman::Application.dynflow.world.config.queue
=> {:default=>{:pool_size=>5}, :remote_execution=>{:pool_size=>5}}

From what I've seen the initializer is invoked before the settings are initialized, so Settings[key] always logs a warning and returns nil. The nil is then stored deep in dynflow's internals in https://github.com/Dynflow/dynflow/blob/93060070e6169af7a27b3821942880c336a91b87/lib/dynflow/config.rb#L67 before it is overriden to a dynflow global default https://github.com/Dynflow/dynflow/blob/93060070e6169af7a27b3821942880c336a91b87/lib/dynflow/config.rb#L72 , so there's a difference between calling this with a nil value and not calling this at all.

That being said, the pool_size is only used in development. In production we use a multi-process deployment using Sidekiq, where creation of queues is handled externally, however dynflow still needs to be aware of the queues' existence.

Right now I'd say we have three options:

1. Somehow ensure that the queues are configured after the settings are initialized
2. Drop the setting altogether, just register the queue without requesting a particular size for it and let dynflow use whatever default values it deems appropriate
3. Drop the setting altogether and hardcode some value here

@ofedoren opinions?

[ofedoren]

1. I'd love that. But I still don't quite follow our initialization process, it makes me hate both chickens and eggs... Can dynflow be initialized/used after everything is done in Rails and Foreman+Plugins, but before the first request? Must it be initialized before Foreman, but after Rails? When then we should initialize settings if it needs Setting class, which is initialized as a constant somewhere during Rails initialization? Not even sure if initialization of Foreman starts when or after Zeitwerk does it thing. Also, plugin initialization and registration is also... weird...
2. If we can do that, why we did put here a value at first place? (Just asking since (for me) after all these years dynflow is still a black magic box Pandora never dreamed of)
3. If only we didn't show this setting to users before... I mean, no idea if anyone actually changed the value there, thus :/ But if no one used that, it's the best since it's the easiest solution, thus :)

[adamruzicka]

1. ... it makes me hate both chickens and eggs...

Yeah... I must admit I don't really have answers to the things you asked about and overall the effort needed to pull this off just doesn't feel worth it.

2. If we can do that, why we did put here a value at first place? (Just asking since (for me) after all these years dynflow is still a black magic box Pandora never dreamed of)

It used to be honored before we switched to sidekiq in foreman 1.24. Now it is just a leftover and something that could be used in development, but I'm not sure if anyone actually does that.

Now that I've slept on it, I would lean more towards dropping the setting completely, unless anyone objects against it

[ofedoren]

I don't think anyone touches anything related to dynflow in development except maybe for you and some Katello guys? If it's also a leftover and isn't used in production, let's drop that.

[adamlazik1]

Setting removed.

[ofedoren]

Don't we also need a migration to remove it from DB?..

[adamruzicka]

@adamlazik1 ^ it would be better

[adamlazik1]

Added.

[adamruzicka]

Thank you @adamlazik1 !