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

win_service recovery actions support #56445

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@rnsc
Copy link
Contributor

commented May 15, 2019

SUMMARY

This is a new feature for the win_service Ansible module.
I wanted to be able to set actions related to the "Recovery" part of the service management GUI.
What is set in the GUI translates to a REG_BINARY key in the registry.
My code builds a hexadecimal combination of all the value that you passed through Ansible.
I'm comparing the values if it was already set for idempotency, and check mode is supported.

These settings can also be set with the sc.exe program, but I wanted to have something that was idempotent.

As far as variables naming, etc... I'm open to making changes you deem necessary. I commented as much as I saw fit, but if you feel like it's lacking, please do tell.

I've seen two schools when it comes to passing stuff to Powershell modules, either separate variables or a string that represents an Ansible dictionary, I went with the former. (Although I wished we could just pass a dict to a powershell script, it'd be more readable)

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_service.py

ADDITIONAL INFORMATION

I did this for a personal need, so I'm using it in my local Ansible library directory.
I wanted to contribute back to the community in case someone else wanted this.

@ansibot

This comment has been minimized.

@rnsc

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

I'll fix the validation errors.

Renaud Schrobiltgen added some commits May 15, 2019

Renaud Schrobiltgen
Renaud Schrobiltgen
Renaud Schrobiltgen
@rnsc

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

So I think the one piece missing here is the integration test for this specific feature, right?
Any advice for creating those?

@rnsc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

ready_for_review

@rnsc

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Hey @chrishoffman @dagwieers

Can you check this whenever you have time? :)
I'd appreciate feedback on this PR. Although it works for me for my project, I'd very much like to have this feature added for the benefit of the community.

Regards,
rnsc

@nitzmahone
Copy link
Member

left a comment

I like the idea of adding support for this to round out Ansible's Windows service management capability, but I do have concerns (some conceptual, some with the specific implementation here):

  • At a glance, it doesn't appear that this implementation is idempotent, which would be absolutely required
  • No tests
  • From a UI perspective, there are enough args around this that we might want to consider putting them in a sub-dictionary (esp since the new PS/C# arg validation stuff would make it easier to validate that now)
  • TMK, the registry format for this is undocumented, which means manual changes to it are dangerous... There are Win32 APIs/structs available for managing it (see https://docs.microsoft.com/en-us/windows/desktop/api/winsvc/ns-winsvc-service_failure_actionsw), which is probably the way it should go. That said, if the other idempotence/UI/test issues are resolved, we could go forward with the manual registry hacking approach temporarily/experimentally, then replace it with a Win32-backed approach later if needed.

@jborean93 any other thoughts?

@rnsc

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@nitzmahone thank you for reviewing this PR!
And thanks for the feedback!
To answer your concerns, this is idempotent, I wanted something idempotent to avoid using the sc.exe through win_shell.
I do agree that it looks hackish, and it’s not very well documented, but from my testing, the way this reg entry is built is always the same.

Not sure I understand the sub dictionary part. I wanted to do that, but powershell doesn’t seem to be handling a dictionary passed by Ansible too well (maybe I didn’t do it properly), but with a bit more guidance I’d be happy to change that too.

As for the testing, I wasn’t sure I had found the appropriate playbook to update, so again, if you have a tip, I’d be happy to add tests to make it this PR better.

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