Closed mefellows closed 9 years ago
This is very creative. My first instinct was to think, "this is not a literal 'provisioner,' so why do this?" But you're dead on it's a problem that is awkward to solve and this cleans it up quite nicely.
I could argue that we could make this a combined ssh/winrm "restart" provisioner, taking the opportunity to craft a clean provisioner that knows what to do for a given communicator. (I wasn't fond of the guest_os_type
builder setting I'd seen elsewhere.) I feel like we need a good pattern for this at some point if we're going to consider doing puppet/chef/ansible provisioners.
I understand your reticence. I went through much of the above thinking also, and then settled on the fact that it could be extended down the track to work for other OS types (although it is rare that you need to restart *nix machines).
I'll look into if there is a neater way to detect OS type. In the mean time, is there value in releasing this Provisioner as is (possibly with a rename to simply 'restart'?).
Do we need to "detect OS type" or understand which communicator we're using or...?
Back to the PR, an alternative would be to add "restart_after", "restart_command", and "restart_timeout" behavior to the shell provisioners.
To properly restart a guest machine I think you need to understand the guest OS type, not just the communicator that's controlling it. I'm not that familiar with the types of OS's that QEMU might be able to host, but I suspect they are likely to be Linux flavours anyway. Perhaps whilst in theory there could be an infinite amount of ways to restart the machine it will come down to supporting just Windows and *nix anyway so maybe I'm over thinking it.
If we go down the above path then adding it as an option to exisiting shell provisioners could provide to be difficult.
Alternatively we stick with our Windows-centric view and modify the shell provisioners to give them a nifty little capability. I actually am leaning toward the latter personally.
The existing shell provisioner has start_retry_timeout
, which assumes you've done a reboot from the previous provisioner step (shell, chef, puppet, whatever).
So this new start_retry_timeout
means the same thing, referring to a previous step, or is it more like done_retry_timeout
referring to this restart-windows
step?
I'm okay not conflating this to non-windows stuff. :)
Thought i might do some research into any Packer issues relating to restarts - this is the top Google result when i searched for "packer issues": https://github.com/mitchellh/packer/issues/354. Perhaps there is a genuine need. I'll keep researching...
It should be given a better name. restart_timeout
might be more appropriate as it's how long after issuing a restart that the provisioner will wait until it fails, returning an error.
I agree, we should rename it.
I'm more of a fan of adding it directly onto the shell/powershell provisioners (implemented in a DRY way) but that's imposing quite a change on something we want to gracefully merge into core packer later. Given that, I'm happy to see it released as is and see what the world makes of it.
It is often inevitable when installing software and configuring Windows environments that a restart will be required at some stage. Whilst it is possible to manually add to a
powershell
orwindows-shell
script, it can be a little bit tedious and certain race conditions can make it flakey.For example, you need to kill WinRM reliably and issue a shutdown and restart of the computer - the former can be difficult to do reliably over WinRM for obvious reasons, and the latter can take some time before all processes are shutdown. Meanwhile, Packer can move on to the next provisioner and very occasionally it may move on to the next provisioner whilst a restart is in progress and this is where things get messy.
This Provisioner simplifies the user experience by creating a simple plugin that will both perform this restart and properly awaits the machine to restart before returning control to the following Provisioner.
Supported configuration parameters:
start_retry_timeout
which defaults to5m
restart_command
which defaults toshutdown /r /c \"packer restart\" /t 5 && net stop winrm
. It shouldn't be necessary to modify this but is reasonable to do so.Example Provisioner setup:
Example Output: