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

Move the systemd env var check into the systemd_worker? method #20321

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

agrare
Copy link
Member

@agrare agrare commented Jul 1, 2020

When starting a worker we were checking an env var and settings, when stopping a worker we were just checking settings. This meant that if the env var wasn't set we would start the worker with spawn and try to stop it with systemd which will fail because the unit doesn't exist.

@@ -1090,7 +1090,7 @@
:poll_method: :normal
:starting_timeout: 10.minutes
:stopping_timeout: 10.minutes
:systemd_enabled: true
:systemd_enabled: false
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we apparently haven't been testing with systemd enabled on appliances for jansa (probably since #19556) I think disabling this for jansa is safest

Copy link
Member

Choose a reason for hiding this comment

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

For context, we were just looking at an appliance where workers were spawned from evmserverd.service but when we inspected workers, they indicated they were systemd workers and could be the reason that we never were stopping the workers, because they were started from spawn (old way) and we were trying to stop them from systemd (which is not how they started) so nothing happened and it continually tried to stop them in the same way...

irb(main):002:0> MiqWorker.find(13504).systemd_worker?
=> true

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.

I agree with these changes, see comments above... regarding backporting this to jansa... what do you think? @Fryguy @carbonin @bdunne

@carbonin
Copy link
Member

carbonin commented Jul 1, 2020

Yeah, if this just hasn't been tested then we probably shouldn't release Jansa with it in.

@bdunne
Copy link
Member

bdunne commented Jul 1, 2020

I agree that jansa shouldn't be systemd workers (since it hasn't been tested), but I think master should unless there is a different plan going forward.

@agrare
Copy link
Member Author

agrare commented Jul 1, 2020

Agree that master should, I can create a jansa specific PR to disable it and leave the config as is on master @bdunne

When starting a worker we were checking an env var and settings, when
stopping a worker we were just checking settings.  This meant that if
the env var wasn't set we would start the worker with spawn and try to
stop it with systemd which will fail because the unit doesn't exist.
@agrare agrare force-pushed the move_systemd_env_var_check branch from b99e4f8 to e1bbb20 Compare July 1, 2020 15:47
@miq-bot
Copy link
Member

miq-bot commented Jul 1, 2020

Checked commit agrare@e1bbb20 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@jrafanie
Copy link
Member

jrafanie commented Jul 1, 2020

I agree, we can move back to systemd on master after this is backported to jansa although we'll keep the consistent checks for environment/settings 😆

@Fryguy
Copy link
Member

Fryguy commented Jul 2, 2020

Ugh that sucks...if we haven't been using systemd at all, then I agree that it's not safe for jansa.

@jrafanie jrafanie merged commit 3a7421c into ManageIQ:master Jul 2, 2020
@agrare agrare deleted the move_systemd_env_var_check branch July 2, 2020 13:22
agrare pushed a commit to agrare/manageiq that referenced this pull request Jul 2, 2020
Move the systemd env var check into the systemd_worker? method

(cherry picked from commit 3a7421c)
@agrare
Copy link
Member Author

agrare commented Jul 2, 2020

Jansa backport PR #20329

@simaishi
Copy link
Contributor

simaishi commented Jul 2, 2020

Backported to jansa via #20329

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.

8 participants