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

systemd module will now wait on deactivating state (#59471) #60939

Merged
merged 2 commits into from Sep 4, 2019

Conversation

@kustodian
Copy link
Contributor

commented Aug 20, 2019

If a service is in the 'deactivating' state running systemctl stop foo,
would wait for the foo service to actually stop before it exits. The
module didn't behave like that and it considered the deactivating state
as if the service wasn't running. This change will align the module with
the systemctl behaviour.

(cherry picked from commit 54d9d78)

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

systemd

systemd module will now wait on deactivating state (#59471)
If a service is in the 'deactivating' state running systemctl stop foo,
would wait for the foo service to actually stop before it exits. The
module didn't behave like that and it considered the deactivating state
as if the service wasn't running. This change will align the module with
the systemctl behaviour.

(cherry picked from commit 54d9d78)
@@ -269,7 +269,11 @@


def is_running_service(service_status):
return service_status['ActiveState'] in set(['active', 'activating'])
return service_status['ActiveState'] in set(['active', 'activating', 'deactivating'])

This comment has been minimized.

Copy link
@abadger

abadger Sep 3, 2019

Member

This doesn't look right. If the user requests that the service is started then the service should be running after the task ends. But with this change, if the service is deactivating when the task runs, then the service could be stopped by the time the task ends (or at some point after the task ends and before a task which needs the service to be running starts).

This comment has been minimized.

Copy link
@kustodian

kustodian Sep 3, 2019

Author Contributor

@abadger you're absolutely right. I have no idea how I left deactivating in there, when I created a separate function below just to check if it's deactivating. Could you remove deactivating from the list, or should I do something about it? I could make a separate PR to fix this in devel.

This comment has been minimized.

Copy link
@abadger

abadger Sep 4, 2019

Member

@kustodian Make a separate PR in devel. Ping me and I'll merge that. Then you can cherry-pick that commit onto this PR and I'll merge this as well. Thanks!

This comment has been minimized.

Copy link
@abadger

abadger Sep 4, 2019

Member

Oh, and the new PR will need a separate backport to the stable-2.9 branch too since we branched that on Thursday last week.

This comment has been minimized.

Copy link
@samdoran

samdoran Sep 4, 2019

Member

I opened the backport PR against stable-2.9 (#61786) and added the commit to this PR. Sorry I did not catch this during review. Thank you, as always, @abadger.

@abadger

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Left a comment that I think this breaks user expectations around behaviour of started. Let me know what you think.

@ansibot ansibot removed the stale_ci label Sep 4, 2019

@abadger abadger merged commit aec6dc3 into ansible:stable-2.8 Sep 4, 2019

1 check passed

Shippable Run 141789 status is SUCCESS.
Details
@abadger

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Merged for 2.8.5. Thanks @kustodian and @sdoran!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.