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 try to handle missing sessions #75097

Draft
wants to merge 17 commits into
base: devel
Choose a base branch
from

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Jun 23, 2021

hopefully mitigates #72674

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

systemd

@ansibot ansibot added affects_2.12 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 23, 2021
@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 23, 2021
@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Jun 23, 2021
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed ci_verified Changes made in this PR are causing tests to fail. labels Jun 23, 2021
@ansibot

This comment has been minimized.

@ansibot ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jun 23, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 24, 2021
@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 24, 2021
bcoca and others added 2 commits June 24, 2021 10:12
Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 24, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 25, 2021
Co-authored-by: Tadej Borovšak <70951+tadeboro@users.noreply.github.com>
Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

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

This probably needs to be an explicit module parameter (or maybe a loginctl module?) allowing linger to be explicitly disabled/enabled for a specific user. And the argument spec needs to account for this option only making sense when scope=user. This probably also means we need a user parameter as well.

Right now we are inferring/guessing the user based on the effective UID or the login user account. I can see that not always being correct, say if someone is trying to configure a user unit for a user account other than the one being used by Ansible to manage the system.


from ansible.module_utils.basic import AnsibleModule
from ansible.module_utils.facts.system.chroot import is_chroot
from ansible.module_utils.service import sysv_exists, sysv_is_enabled, fail_if_missing
from ansible.module_utils._text import to_native
from ansible.module_utils.common.text.converters import to_bytes, to_native, to_text
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: this import should be on line 276 so it's alphabetical.

lib/ansible/modules/systemd.py Outdated Show resolved Hide resolved
lib/ansible/modules/systemd.py Outdated Show resolved Hide resolved
lib/ansible/modules/systemd.py Outdated Show resolved Hide resolved
@@ -551,6 +600,9 @@ def main():
# this should not happen?
module.fail_json(msg="Service is in unknown state", status=result['status'])

if lingered:
disable_linger(module)
Copy link
Contributor

Choose a reason for hiding this comment

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

Disabling linger here will most likely result in the user unit not working as intended. In order for user units to start on boot and/or not be killed when a login session for the user account no longer exists, the user account must have linger permanently enabled.

''' allows for lingering dbus sessions '''

enabled = False
user = getpass.getuser()
Copy link
Contributor

Choose a reason for hiding this comment

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

This may not always be the correct user account and may differ from the value in XDG_RUNTIME_DIR.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 30, 2021
bcoca and others added 3 commits June 30, 2021 12:18
Co-authored-by: Sam Doran <sdoran@redhat.com>
Co-authored-by: Sam Doran <sdoran@redhat.com>
Co-authored-by: Sam Doran <sdoran@redhat.com>
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 30, 2021
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jul 8, 2021
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels May 21, 2022
@bcoca bcoca marked this pull request as draft August 24, 2022 20:08
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Aug 24, 2022
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 5, 2023
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. and removed has_issue labels Jul 12, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.12 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants