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

add skip_verify for service.enabled and .disabled #50009

Merged
merged 4 commits into from
Dec 10, 2018

Conversation

jigish
Copy link

@jigish jigish commented Oct 11, 2018

What does this PR do?

Adds the skip_verify option for service.enabled and service.disabled.

When running service.enabled or service.disabled, salt verifies that the service is 'available'. This option allows the user to skip that verification for cases that it doesn't make sense. For example, while running salt in a chroot environment (like packer's salt-masterless provisioner while using the amazon-chroot builder), systemctl status fails as it is invalid. Without skip_verify, this would cause the service.enabled state to fail as well.

What issues does this PR fix or reference?

#38751 -- Doesn't fix, just supplies a workaround.

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

I'm curious how this would help, if the service name is invalid, wouldn't enabling/disabling it also fail?

% sudo systemctl enable foo
Failed to enable unit: Unit file foo.service does not exist.
% echo $?
1
% sudo systemctl disable foo
Failed to disable unit: Unit file foo.service does not exist.
% echo $?
1

It seems to me like this just kicks the can to a further point where it will also fail.

@jigish
Copy link
Author

jigish commented Oct 16, 2018

You're absolutely right for cases in which the service name is invalid, which is why the option is defaulted to False.

The skip_verify option I've added is intended to be used in cases where the available check itself is not applicable and/or invalid but the service name is valid. In chroot environments, systemctl status calls fail even for valid service names as systemd seems to consider it an invalid use case. In those cases you could use skip_verify to skip that and directly systemctl enable which will work just fine. This is especially important while using salt to provision an AMI via amazon-chroot.

That being said, I've implemented this as a stopgap. It may be worth modifying the systemd module itself to not run status in chroot environments. I just have a lot less experience with the other service modules so I'm not sure if the same change is needed in them.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

I don't like that you've inserted this skip_verify argument such that it is required to be passed every time the function is invoked. If we expect the vast majority of the invocations to be False, then the skip_verify argument should be moved until after result and should have a default value of False. Then we don't have to pass a False in most of the calls to _enable() and _disable(), and in the few cases where we are passing through the value from the state, we can simply pass skip_verify=skip_verify.

@jigish
Copy link
Author

jigish commented Nov 26, 2018

@terminalmage - Just made the requested changes. Thanks for taking a look!

Make skip_verify optional and default it to False.
@cachedout cachedout merged commit addced9 into saltstack:develop Dec 10, 2018
@dwoz dwoz mentioned this pull request Apr 21, 2020
3 tasks
@sagetherage sagetherage added the has master-port port to master has been created label Apr 24, 2020
waynew pushed a commit to dwoz/salt that referenced this pull request Feb 9, 2021
This is a port of PR saltstack#50009. This change is slightly updated - since
_enable and _disable took skip_verify as a default of False, no need to
update the numerous calls to pass in the default.

Also adds versionadded to the docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-jam has master-port port to master has been created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants