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

[Rearch] Combine worker messages 'sync_config' and sync_active_role' into a single 'sync_config' message. #15597

Merged

Conversation

gtanzillo
Copy link
Member

Preparing for running workers in pods, we won't be able to pass messages to workers over a Drb connection. Combining
the messages into one that does both every time will simplify the implementation when we use a different mechanism.

In this case, we can simplify the logic in #15471 where memcached is new mechanism

/cc @Fryguy

@Fryguy Fryguy changed the title [Rearch] Combine worker messages 'sync_config' and sync_active_role' into a single 'sync_config' message. [WIP] [Rearch] Combine worker messages 'sync_config' and sync_active_role' into a single 'sync_config' message. Jul 19, 2017
@Fryguy
Copy link
Member

Fryguy commented Jul 19, 2017

Marking as WIP because of the TODOs and because of the discussion we had

_log.info("#{log_prefix} Synchronizing active roles...")
opts = _args.extract_options!
sync_active_roles(opts[:roles])
_log.info("#{log_prefix} Synchronizing active roles complete...")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, let's just move this into the sync_config method itself (or perhaps let's just merge the two methods into message_sync_config and get rid of sync_config itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please verify the order of when sync_active_roles should be called with respect to reloading the config

@miq-bot miq-bot added the wip label Jul 19, 2017
@gtanzillo gtanzillo force-pushed the rearch-combine-config-and-roles-messages branch from 6d8b07d to 84696b0 Compare July 28, 2017 20:37
@gtanzillo gtanzillo changed the title [WIP] [Rearch] Combine worker messages 'sync_config' and sync_active_role' into a single 'sync_config' message. [Rearch] Combine worker messages 'sync_config' and sync_active_role' into a single 'sync_config' message. Jul 28, 2017
…ngle 'sync_config' message.

Preparing for running workers in pods, we won't be able to pass messages to workers over a Drb connection. Combining
the messages into one that does both every time will simplify the implementation when we use a different mechanism.
…to `sync_config` method

Removed all callers of `sync_active_roles`
@miq-bot
Copy link
Member

miq-bot commented Jul 28, 2017

This pull request is not mergeable. Please rebase and repush.

@gtanzillo gtanzillo force-pushed the rearch-combine-config-and-roles-messages branch from 84696b0 to c801bdb Compare July 28, 2017 20:42
@gtanzillo
Copy link
Member Author

@Fryguy This one is ready to go. I did a lot of testing with this and determined that it doesn't really matter what order the roles and config are synchronized. I left it with the roles going first followed by the config because that's the order it was originally done when both things needed synchronization.

@miq-bot
Copy link
Member

miq-bot commented Jul 28, 2017

Checked commits gtanzillo/manageiq@580b0ab~...c801bdb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy Fryguy merged commit 9dd6d3b into ManageIQ:master Aug 1, 2017
@Fryguy Fryguy modified the milestones: Roadmap, Sprint 66 Ending Aug 7, 2017 Aug 1, 2017
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.

3 participants