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

[WIP] Add the ability to specify --root. #58017

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Jun 18, 2019

When systemd is not running but changes has to be made such as enabling or disabling services,
using --root allows us to run systemctl without it.
This PR adds the ability to do just that.

(cherry picked from commit 5932c05)
rebase and fix for #44690

Also fix how scope is added, it should ony be needed with commands that use a 'unit'

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

systemd

thedrow and others added 2 commits June 18, 2019 15:54
When systemd is not running but changes has to be made such as enabling or disabling services,
using `--root` allows us to run systemctl without it.
This PR adds the ability to do just that.

(cherry picked from commit 5932c05)

Also fix how scope is added, it should ony be needed with commands that use a 'unit'
@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. system System category labels Jun 18, 2019
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jun 20, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 28, 2019
@bcoca bcoca requested a review from mkrizek June 28, 2019 21:53
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 28, 2019
if module.params['no_block']:
systemctl += " --no-block"

if module.params['force']:
systemctl += " --force"

unit = module.params['name']
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed since the same line has been added recently: https://github.com/ansible/ansible/blob/83bf90e/lib/ansible/modules/system/systemd.py#L343

Copy link
Contributor

@mkrizek mkrizek left a comment

Choose a reason for hiding this comment

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

One minor 'issue', looks good otherwise.

Copy link
Contributor

@tchernomax tchernomax left a comment

Choose a reason for hiding this comment

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

Also (but not related to this PR):

Line 481→485:

                if module.params['scope'] in (None, 'system') and \
                        not module.params['user'] and \
                        is_initd and \
                        not out.strip().endswith('disabled') and \
                        sysv_is_enabled(unit):

This code "fail" on the following configuration ; it detect the service as enabled, even the init.d service is supersede by the systemd service… and disabled:

root@test ~ # l /etc/init.d/toto /etc/rc5.d/S99toto /etc/systemd/system/toto.service
-rwxr-xr-x 1 root root 24 Jul  6 13:38 /etc/init.d/toto
lrwxrwxrwx 1 root root 16 Jul  6 13:42 /etc/rc5.d/S99toto -> /etc/init.d/toto
-rw-r--r-- 1 root root 74 Jul  6 13:41 /etc/systemd/system/toto.service
root@test ~ # systemctl is-enabled toto
disabled

Line 442:

module.warn('The service (%s) is actually an init script but the system is managed by systemd' % unit)

This warning is only triggered when you add an /etc/init.d/… file and don't daemon-reload.
It isn't throw every time/configuration when the service is "an init script but the system is managed by systemd".

Line 447:

masked = ('LoadState' in result['status'] and result['status']['LoadState'] == 'masked')

Just use is_masked (be sure to initialize it).
Also it doesn't work on chroot environment (when list-unit-files is used)

# The other choices match the corresponding switch
if module.params['scope'] not in (None, 'system'):
systemctl += " --%s" % module.params['scope']

Copy link
Contributor

Choose a reason for hiding this comment

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

if scope=user or scope=global, and daemon_reload=true or daemon_rexec=true → ansible will try to daemon-reload or daemon-rexec the system daemon (instead of the user one).

found = False
is_initd = sysv_exists(unit)
is_systemd = False

if module.params['root']:
systemctl_unit += " --root=%s" % module.params['root']
Copy link
Contributor

Choose a reason for hiding this comment

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

fail if scope=user.
(root and scope=user are incompatible)

# check service data, cannot error out on rc as it changes across versions, assume not found
(rc, out, err) = module.run_command("%s show '%s'" % (systemctl, unit))
(rc, out, err) = module.run_command("%s show '%s'" % (systemctl_unit, unit))
Copy link
Contributor

Choose a reason for hiding this comment

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

fail if root is used:
show seams not to take into account the --root parameter, so we are "showing" the wrong unit.

max@mde-oxalide % sudo systemctl --root /var/lib/machines/arch-64-tmp/ show org.cups.cupsd.service | grep LoadState
LoadState=loaded
max@mde-oxalide % sudo find /var/lib/machines/arch-64-tmp/ -name org.cups.cupsd.service
max@mde-oxalide %

@@ -510,7 +524,7 @@ def main():
if action:
result['changed'] = True
if not module.check_mode:
(rc, out, err) = module.run_command("%s %s '%s'" % (systemctl, action, unit))
(rc, out, err) = module.run_command("%s %s '%s'" % (systemctl_unit, action, unit))
Copy link
Contributor

Choose a reason for hiding this comment

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

fail if root is used.

@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 Jul 6, 2019
@bcoca bcoca changed the title Add the ability to specify --root. [WIP] Add the ability to specify --root. Jul 12, 2019
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. 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 12, 2019
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 8, 2020
@ansibot ansibot added collection Related to Ansible Collections work collection:community.general needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 16, 2020
@ansibot ansibot removed collection:community.general needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md labels May 27, 2020
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label May 27, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. 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 Dec 6, 2020
@ansibot ansibot removed the support:community This issue/PR relates to code supported by the Ansible community. label Mar 5, 2021
@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 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.9 This issue/PR affects Ansible v2.9 collection Related to Ansible Collections work feature This issue/PR relates to a feature request. 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. pre_azp This PR was last tested before migration to Azure Pipelines. support:core This issue/PR relates to code supported by the Ansible Engineering Team. system System category 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

5 participants