Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #37114: Download packages during upgrade before maintenance mode #793

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jan 19, 2024

No description provided.

@ehelms
Copy link
Member Author

ehelms commented Jan 19, 2024

One interesting thing is that the download or update packages step will take (on my test machine) ~30 seconds each even if there are no packages being updated. This appears to be because we clean the cache before hand every time called as here.

I am realizing that implies a few things:

  1. Any cached packages would be discarded by this and defeat the point of pre-downloading.
  2. This can creatable a variable amount of time for the update and download action based upon network conditions and repository sizes.

I would propose:

  1. The cache only be cleaned before the download step
  2. The update step should assume that the cache has everything it needs and not perform any cache cleaning action.

@ehelms
Copy link
Member Author

ehelms commented Jan 19, 2024

When testing a Satellite update with no package updates, the commit I added to only refresh cache on download step shaves 25 seconds off the downtime of the update run.

@ehelms
Copy link
Member Author

ehelms commented Jan 19, 2024

On another note, because we stop services, an update with no packages having changed (so bare minimum downtime) results (on my system) in the installer taking ~3 minutes of the 5 minutes of downtime. The primary culprits:

[28.25 seconds] /Service[foreman]: 
[27.37 seconds] /Service[dynflow-sidekiq@orchestrator]: 
[26.42 seconds] /Service[dynflow-sidekiq@worker-hosts-queue-1]: 
[26.32 seconds] /Service[dynflow-sidekiq@worker-1]: 
[18.67 seconds] /Stage[main]/Foreman::Database/Foreman::Rake[db:migrate]/Exec[foreman-rake-db:migrate]: 

I'll be curious if the change to Ruby 3+ has any direct impact on this.

@evgeni
Copy link
Member

evgeni commented Jan 23, 2024

On another note, because we stop services, an update with no packages having changed (so bare minimum downtime) results (on my system) in the installer taking ~3 minutes of the 5 minutes of downtime. The primary culprits:

[28.25 seconds] /Service[foreman]: 
[27.37 seconds] /Service[dynflow-sidekiq@orchestrator]: 
[26.42 seconds] /Service[dynflow-sidekiq@worker-hosts-queue-1]: 
[26.32 seconds] /Service[dynflow-sidekiq@worker-1]: 
[18.67 seconds] /Stage[main]/Foreman::Database/Foreman::Rake[db:migrate]/Exec[foreman-rake-db:migrate]: 

I'll be curious if the change to Ruby 3+ has any direct impact on this.

Just to spell out what we talkted about: I think we can omit restarting dynflow-sidekiq by Puppet as it is part of foreman.service and already got restarted, but Puppet probably doesn't know that.

@ehelms
Copy link
Member Author

ehelms commented Jan 23, 2024

On another note, because we stop services, an update with no packages having changed (so bare minimum downtime) results (on my system) in the installer taking ~3 minutes of the 5 minutes of downtime. The primary culprits:

[28.25 seconds] /Service[foreman]: 
[27.37 seconds] /Service[dynflow-sidekiq@orchestrator]: 
[26.42 seconds] /Service[dynflow-sidekiq@worker-hosts-queue-1]: 
[26.32 seconds] /Service[dynflow-sidekiq@worker-1]: 
[18.67 seconds] /Stage[main]/Foreman::Database/Foreman::Rake[db:migrate]/Exec[foreman-rake-db:migrate]: 

I'll be curious if the change to Ruby 3+ has any direct impact on this.

Just to spell out what we talkted about: I think we can omit restarting dynflow-sidekiq by Puppet as it is part of foreman.service and already got restarted, but Puppet probably doesn't know that.

@ekohl Do you know if there is a way at the Puppet level to allow foreman to start before the dynflow service definition is evaluated so that a start is not triggered of the dynflow service? Given we connect things at the systemd level (https://github.com/theforeman/foreman/blob/develop/extras/systemd/dynflow-sidekiq%40.service#L5).

Perhaps we just need to manage the file but not the state as we do today (https://github.com/theforeman/puppet-foreman/blob/master/manifests/dynflow/worker.pp#L49-L53) ?

@ekohl
Copy link
Member

ekohl commented Jan 23, 2024

The primary culprits:

Just plain foreman-rake -T (list tasks) is already slow. We execute that at least once (to see if there are any migrations). If we can speed that up, that would help.

It may help to go through the various rake tasks that we define and perhaps make sure they aren't defined in production.

Note that https://guides.rubyonrails.org/active_record_migrations.html#running-migrations suggests to run bin/rails db:migrate. That ends up just being a wrapper around the rake task though, so it doesn't help at all with being faster.

@ehelms
Copy link
Member Author

ehelms commented Jan 23, 2024

Here is an idea I tested to solve the added time due to dynflow restarts:

theforeman/puppet-foreman#1148

@ehelms ehelms marked this pull request as ready for review January 24, 2024 02:48
@ehelms ehelms requested a review from evgeni January 28, 2024 21:47
@ehelms ehelms force-pushed the move-predownload branch 2 times, most recently from 1a26b9e to 0aeb19f Compare January 29, 2024 13:01
@ehelms ehelms mentioned this pull request Feb 6, 2024
@ehelms ehelms changed the title Download packages to dnf cache before enabling maintenance mode Fixes #37114: Download packages during upgrade before maintenance mode Feb 7, 2024
@ehelms
Copy link
Member Author

ehelms commented Feb 7, 2024

@evgeni Your re-review would be appreciated.

@evgeni
Copy link
Member

evgeni commented Feb 9, 2024

LGTM besides the "foreman nightly" file.

Only clean package cache when downloading packages
Only clean metadata when cleaning dnf cache
@ehelms ehelms merged commit f6974d9 into theforeman:master Feb 9, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants