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 MiqWorker#supports_container? as every worker but one supports podified #20583

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Sep 23, 2020

Every worker except the Cockpit Worker supports running containerized, having an opt-in supports_container? that defaults to false is inefficient and prone to bugs (e.g. #20497)

>> MiqWorker.leaf_subclasses.reject(&:supports_container?).map(&:name)
=> ["MiqCockpitWsWorker"]

The cockpit web services worker already has logic to control if it can start or not so we can utilize this and check for is_podified? as well

Every worker save the Cockpit Worker supports running containerized,
having an opt-in supports_contianer? that defaults to false is
inefficient.
@agrare
Copy link
Member Author

agrare commented Sep 23, 2020

cc @jrafanie @bdunne @Fryguy

@miq-bot
Copy link
Member

miq-bot commented Sep 23, 2020

Checked commit agrare@f0a7083 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
14 files checked, 0 offenses detected
Everything looks fine. 👍

@@ -16,6 +16,7 @@ class WS

def self.can_start_cockpit_ws?
MiqEnvironment::Command.is_linux? &&
!MiqEnvironment::Command.is_podified? &&
File.exist?(COCKPIT_WS_PATH) &&
File.exist?(COCKPIT_SSH_PATH)
Copy link
Member Author

Choose a reason for hiding this comment

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

def self.containerized_worker?
MiqEnvironment::Command.is_podified? && supports_container?
MiqEnvironment::Command.is_podified?
Copy link
Member

Choose a reason for hiding this comment

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

10 lines below there is also a supports_systemd? Should we remove that one as well for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

That one checks Settings so you can disable it if something isn't working right and also make sure the systemd gem is installed so its not just a static true/false like supports_container? is.

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM. I believe this was done because we slowly enabled workers and now that only 1 type doesn't support it, it makes sense to flip the logic and make it the default.

@jrafanie jrafanie merged commit 28447d3 into ManageIQ:master Sep 23, 2020
@jrafanie jrafanie self-assigned this Sep 23, 2020
@agrare agrare deleted the simplify_miq_worker_supports_container branch September 23, 2020 14:28
@jrafanie
Copy link
Member

@agrare do you anticipate needing this for backport as a prerequisite for other PRs?

@agrare
Copy link
Member Author

agrare commented Sep 23, 2020

No I don't think we need to backport this @jrafanie I just ran into this while looking at something else and wanted to clean it up

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