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

Remove Vmdb::Settings last_loaded and simplify server settings reload #19758

Merged
merged 4 commits into from
Jan 24, 2020

Conversation

carbonin
Copy link
Member

This PR moves the work done when a server process needs to reload settings from a two phase action to one. Specifically, all of the work that needs to be done when settings change for a server is now done in reload_settings.

A call to this method is enqueued by a worker when it changes the
settings for a server. Previously, this method would reload the
server process's settings and this would set the last_changed
attribute on Vmdb::Settings. This attribute would later indicate to
the server to do some more work because its settings had changed
recently. This condition was checked and the work was done in
the #sync_monitor method.

This commit changes this flow so the work that was previously done
in #sync_monitor if the settings had been changed is now just done
directly when the server is told that it should reload its settings.

This simplifies the #sync_monitor method and also allows us to
remove Vmdb::Settings.last_loaded attribute entirely.

A call to this method is enqueued by a worker when it changes the
settings for a server. Previously, this method would reload the
server process's settings and this would set the last_changed
attribute on Vmdb::Settings. This attribute would later indicate to
the server to do some more work because its settings had changed
recently. This condition was checked and the work was done in
the #sync_monitor method.

This commit changes this flow so the work that was previously done
in #sync_monitor if the settings had been changed is now just done
directly when the server is told that it should reload its settings.

This simplifies the #sync_monitor method and also allows us to
remove Vmdb::Settings.last_loaded attribute entirely.
@carbonin
Copy link
Member Author

Additionally this solves an issue I was running into in #19734 because last_loaded is not scoped to the server. So whenever any server updated its settings all the servers and workers would run through the code path to reload everything.

@Fryguy
Copy link
Member

Fryguy commented Jan 24, 2020

@carbonin Since this is affecting Settings itself, can you do a cross_repo test with at least ui and api?

@carbonin
Copy link
Member Author

Cross repo test for ui and api: ManageIQ/manageiq-cross_repo-tests#57

@carbonin
Copy link
Member Author

Cross repo tests passed.

@jrafanie
Copy link
Member

This is nice. I ❤️ this simplification.

We might have to do some manual testing of the queue pre-fetch to ensure role changes trigger already pre-fetched messages to be re-evaluated for pre-fetch. We might have to do a synthetic test for that since it might be hard to fill the queue with enough work to have a pre-fetch that's not drained quickly.

@Fryguy
Copy link
Member

Fryguy commented Jan 24, 2020

@jrafanie You can probably just queue up a ton of {:class_name => "Kernel", :method_name => "sleep", :args => [5]}

This class attribute was previously used to indicate the last time
a server reloaded its settings. We can remove it now because the
server doesn't need to know if it reloaded its settings recently
anymore.
As the comments said, this is done by the Vmdb::Settings::Activator
so we don't need to do it here right after reloading the settings
@miq-bot
Copy link
Member

miq-bot commented Jan 24, 2020

Checked commits carbonin/manageiq@9798755~...81810e2 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 1 offense detected

app/models/miq_server/worker_management/monitor.rb

@Fryguy Fryguy merged commit c067197 into ManageIQ:master Jan 24, 2020
@Fryguy Fryguy added this to the Sprint 129 Ending Feb 3, 2020 milestone Jan 24, 2020
@carbonin carbonin deleted the remove_settings_last_loaded branch January 24, 2020 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants