Support for Windows Docker hosts on Azure by ppadala · Pull Request #3329 · docker-archive-public/docker.machine

ppadala · 2016-04-17T07:44:22Z

This pull request adds support for spinning up Windows 2016 servers on Azure. Brief overview of all the steps

Spin up a Windows 2016 TP4 with containers image.
Enable WinRM using deploy template. WinRM is used to execute commands on the VM, once it is up
Stop docker-engine
Upload certs and keys using WinRM
Start docker-engine

Usage: Three new parameters are added to Azure driver

--azure-os "linux"                  OS for the Azure VM (Windows|Linux)
--azure-winrm-user "docker-user"    Username for WinRM login [$AZURE_WINRM_USER]
--azure-winrm-password              Password for WinRM login [$AZURE_WINRM_PASSWORD]

Sample command

docker-machine -D create -d azure --azure-subscription-id $(cat ~/azure/subid) --azure-os "windows" --azure-image MicrosoftWindowsServer:WindowsServer:2016-Technical-Preview-with-Containers:2016.0.20151118 --azure-winrm-password "mysecretpassword" vhost-win-test

- adds a Windows provisioner that uses WinRM to run commands - adds support to azure driver for instantiating Windows server images on Azure - other plumbing Signed-off-by: Pradeep Padala <pradeep@containerx.io>

ppadala · 2016-04-18T15:38:41Z

cc @ahmetalpbalkan and @nathanleclaire for comments

ahmetb · 2016-04-18T15:49:23Z

 	flAzureLocation        = "azure-location"
 	flAzureSize            = "azure-size"
 	flAzureImage           = "azure-image"
+	flAzureOS              = "azure-os"


I'd say let's group flags for windows together it'll make it easy to refactor to other drivers tomorrow

ahmetb · 2016-04-18T16:01:32Z

I like this! General code quality is really good @ppadala. I left a few comments. Going forward the Linux/Windows distinction is going to grow, therefore I'm concerned with having inline if/else statements to handle those. Our general approach should be better writing different methods/types where we can factor out these if/else statements and have type system do the work for us.

We should also get machine maintainers on board to add --winrm-username, --winrm-password arguments directly to machine rather than individual drivers. For now as long as we document them correctly (by adding (Windows-only) prefix), we can keep ’em in the azure driver.

cc: @enderb-ms

ppadala · 2016-04-18T17:47:11Z

I like this! General code quality is really good @ppadala. I left a few comments. Going forward the Linux/Windows distinction is going to grow, therefore I'm concerned with having inline if/else statements to handle those. Our general approach should be better writing different methods/types where we can factor out these if/else statements and have type system do the work for us.

Agree. Maybe a simple OS interface and implementation for Linux and Windows. As I mentioned above, let's do this as it gets clearer how other drivers will work with Windows.

We should also get machine maintainers on board to add --winrm-username, --winrm-password arguments directly to machine rather than individual drivers. For now as long as we document them correctly (by adding (Windows-only) prefix), we can keep ’em in the azure driver.

I was thinking the same, but it looks like even the -ssh-username is per driver. May be @nathanleclaire can comment.

nathanleclaire · 2016-04-18T17:55:33Z

Wow, epic. It will take me a while to review in detail but thanks for this!

I was thinking the same, but it looks like even the -ssh-username is per driver. May be @nathanleclaire can comment.

Ahmet is right -- they should be global if possible. I think Amazon and other clouds support Windows machines, so it makes sense to me. Making the --ssh-* parameters per-driver happened mostly incidentally, not as a deliberate design decision (and really I'd like to pull them all into one common set of flags), I'd like to get things right from the start on this one if possible.

nathanleclaire · 2016-04-18T18:26:03Z

Just FYI, since it changes the RPC API, it will most likely require a version bump to the API. Otherwise it may try to call methods that don't exist on the other end. We haven't handled such a transition before, but we can discuss how we might handle it in this case and others (#3209, for instance, really should bump the API version as well).

ahmetb · 2016-04-18T18:29:39Z

+		}
+		if err := c.CreateVirtualMachineExtension(d.OS, d.ResourceGroup, d.naming().VM(), d.Location); err != nil {
+			return err
+		}


let's handle else here (and everhywhere else applicable).

also for these stuff, switch statement is more idiomatic.

ahmetb · 2016-04-18T18:31:35Z

+3200 file addition seems a bit weird, did you use godep to vendor the files? normally it should include only the go packages needed for the build (namely only *.go files), but it appears like this just included the entire packer repo

ppadala · 2016-04-18T18:36:11Z

+3200 file addition seems a bit weird, did you use godep to vendor the files? normally it should include only the go packages needed for the build (namely only *.go files), but it appears like this just included the entire packer repo

Yeah, I was not sure how to do this, included packer repo. Are there instructions somewhere on how to do this?

Signed-off-by: Pradeep Padala <pradeep@containerx.io>

ppadala · 2016-04-26T18:53:24Z

@nathanleclaire any comments on ppadala@3cc3785 ?

On a side note, my upstream change for Azure quick-start-template for WinRM is merged, so I removed the bundling of that into this change.

ahmetb · 2016-04-26T19:16:24Z

Azure driver changes LGTM. However when it is time to update the docs, I wish to keep the windows and linux examples separate if we can.

One thing that's still not-LGTM is the dependencies you refer to in CustomScriptExtension should probably be checked in to this repo or another docker repo, otherwise when azure-quickstart-templates repo changes something (which is something it does quite often) this will be immediately broken. Please do not rely that repo and find a more persistent place (preferably outside github) to store these scripts/binaries.

ppadala · 2016-04-26T20:00:11Z

Azure driver changes LGTM. However when it is time to update the docs, I wish to keep the windows and linux examples separate if we can.

Thanks @ahmetalpbalkan. I'll add docs, when we are ready to merge and agree with separating Windows examples to reduce confusion.

One thing that's still not-LGTM is the dependencies you refer to in CustomScriptExtension should probably be checked in to this repo or another docker repo, otherwise when azure-quickstart-templates repo changes something (which is something it does quite often) this will be immediately broken. Please do not rely that repo and find a more persistent place (preferably outside github) to store these scripts/binaries.

Agree, will bring them back in.

nathanleclaire · 2016-04-26T20:09:26Z

Something to be thinking about, how about tests for this? (Unit and integration)

Why not put the empty WinRM-related methods inside of the BaseDriver struct which gets embedded by most drivers?

ppadala · 2016-04-26T20:14:41Z

Something to be thinking about, how about tests for this? (Unit and integration)

I had a unit test that calls create and delete, but I have to manually enter Azure credentials. How are the tests done for other drivers? @ahmetalpbalkan can you point me to existing tests for Azure Linux driver?

Why not put the empty WinRM-related methods inside of the BaseDriver struct which gets embedded by most drivers?

They are already in BaseDriver.

nathanleclaire · 2016-04-26T20:27:09Z

They are already in BaseDriver.

Ah, OK, I was not realizing that the Drivers they have been explicitly set for are mostly the testing-related drivers.

ahmetb · 2016-04-26T22:54:21Z

@ppadala we don't have tests. 😳

ppadala · 2016-04-28T00:21:09Z

@ahmetalpbalkan :-)

@nathanleclaire there's no easy way to do automated tests without Azure credentials. If you have suggestion here, let me know.

StefanScherer · 2016-04-30T14:54:49Z

Has anyone tried this with TP5? I can't find any TP5+Docker image, so probably the provisioning of a vanilla TP5 with --azure-image MicrosoftWindowsServer:WindowsServer:Windows-Server-Technical-Preview:5.0.20160420 won't work.

Can we add provisioning like in "https://get.docker.com/" for bash, but for PowerShell to enable Containers feature if missing, to install Docker if it is missing, to download the base OS image as long as we have to install it w/o docker pull?

ppadala · 2016-04-30T15:08:53Z

@StefanScherer we are looking into it. It looks like Windows Server Core image for TP5 is not yet available. Also, now the scripts (UpdateHost.PS1 for example) are broken on TP4. I am guessing it's because of the URLs pointing to new versions.

cc @enderb-ms

StefanScherer · 2016-04-30T15:19:14Z

@ppadala I followed the updated docs at https://msdn.microsoft.com/virtualization/windowscontainers/deployment/deployment

Install-WindowsFeature containers
Install-PackageProvider ContainerImage -Force
Install-ContainerImage -Name WindowsServerCore
Invoke-WebRequest https://aka.ms/tp5/Update-Container-Host -OutFile update-containerhost.ps1
.\update-containerhost.ps1

I also try to automate all these new steps for my packer build.

ppadala · 2016-04-30T15:21:13Z

@StefanScherer update-containerhost.ps1 is broken for TP4 now. I haven't tried it on TP5 yet.

ppadala · 2016-04-30T22:36:24Z

Another problem we just saw on TP5. https://github.com/Microsoft/Virtualization-Documentation/issues/225. cc @enderb-ms and @taylorb-microsoft

enderjbr · 2016-05-05T17:36:39Z

@ppadala the main reason the new scripts are broken in TP4 is due to how much networking set up has changed between TP4 and TP5.
@StefanScherer in regards to the TP5+Docker image, our current plan is to not publish one, and just have the TP5 image. This is due to some people having issues in TP4 once the version of Docker in the images went stale, and now we could also have an issue with the base images going stale. But if you could give us feedback on whether you think this was a good or bad choice, I'd appreciate it.

StefanScherer · 2016-05-05T18:14:45Z

@enderb-ms I have seen this updated azure template https://github.com/Azure/azure-quickstart-templates/tree/master/windows-server-containers-preview which I want to try out. If that template works it is a starting point to update the steps needed in docker-machine.

If it's a good or bad choice to install docker and the base image depends on the time it takes for a user to have the whole Docker engine up and running. Sure, as long as it's a tech preview it is good to have an up-to-date Docker version.

ppadala · 2016-05-05T19:01:28Z

Agree with @StefanScherer. Installing and setting up docker-engine takes considerably long time, especially getting the core images etc.

@enderb-ms, if you want better user experience have the clean Windows server image, but also have an image with docker-engine installed and configured to run locally.

cc @selvik

StefanScherer · 2016-05-05T20:59:44Z

I would prefer a prebuilt Azure VM with Docker engine and the windowsservercore base image to have a working VM after let's say five minutes.

I've created a VM with the Azure template linked above and it took 59 minutes to have docker installed and to run the first docker commands. And that time would be needed for a docker-machine create. I think that's too long to be useful.

I don't know the process of providing new Azure VMs in the catalog and how long it takes to test/proof/stage/publish it. Having a prebuilt TP5+Docker VM in a regular basis (eg. every 1-2 weeks) would be new enough. And as a user I still can run the update-containerhost to update docker engine to the latest version and so on.

Behind the scenes there should be an automated way to create a new TP5+Docker Azure VM to update the catalog that can be run after a major change (base image, docker engine) or every weekend. Don't know if packer-azure may help here or if you have other automation tools at Azure.

StefanScherer · 2016-05-29T07:44:59Z

Any updates for TP5?

Microsoft switched the installation guide from Install-ContainerHost.ps1 to use dockerd.exe as native Windows service to remove a dependency to NSSM.

But this means that docker-machine should be able to modify the C:\ProgramData\docker\config\daemon.json instead of adding command line arguments to the daemon. -> #3062

StefanScherer · 2016-07-13T06:22:08Z

I just have seen that there is now a "Windows Server 2016 Core with Containers Tech Preview 5" virtual machine in Azure. This might help to update this PR.

StefanScherer · 2016-10-24T05:42:51Z

Any plans to get this running with official Windows Server 2016 VM now available in Azure?

davidarcher · 2017-11-14T20:51:24Z

Just leaving a note here that I am planning to resurrect this PR as I am working on a project where I need to spin up Windows docker hosts via docker-machine in OpenStack and having this WinRM support would be great.

dgageot · 2017-11-17T06:52:11Z

@davidarcher Please do refresh this PR! I'll be here to review it

mitchellmaler · 2018-05-10T20:49:19Z

Is there any status on this PR? I have a use case for windows containers.

Support for spinning up Windows Docker hosts on Azure

23e912f

- adds a Windows provisioner that uses WinRM to run commands - adds support to azure driver for instantiating Windows server images on Azure - other plumbing Signed-off-by: Pradeep Padala <pradeep@containerx.io>

GordonTheTurtle added status/0-triage group/windows dco/no labels Apr 17, 2016

ppadala force-pushed the azurewin branch from 0297e12 to 8f230e5 Compare April 17, 2016 07:55

GordonTheTurtle removed dco/no labels Apr 17, 2016

ppadala mentioned this pull request Apr 17, 2016

Connect to Windows Docker engine #2907

Open

ahmetb reviewed Apr 18, 2016
View reviewed changes

GordonTheTurtle added the dco/no label Apr 18, 2016

ppadala force-pushed the azurewin branch from f673d0e to 950626a Compare April 18, 2016 18:39

GordonTheTurtle removed dco/no labels Apr 18, 2016

Pradeep Padala added 2 commits April 25, 2016 16:35

Vendoring in only the necessary packages and files

6a1adbd

Signed-off-by: Pradeep Padala <pradeep@containerx.io>

Change to upstream Azure WinRM templates

c769959

Signed-off-by: Pradeep Padala <pradeep@containerx.io>

ppadala force-pushed the azurewin branch from a0bf851 to c769959 Compare April 25, 2016 23:35

bazzilic mentioned this pull request Jul 10, 2016

Create a Windows machine in VirtualBox #3576

Open

StefanScherer mentioned this pull request Oct 24, 2016

dockertls: create config.json to fake a docker-machine StefanScherer/dockerfiles-windows#16

Closed

StefanScherer mentioned this pull request Nov 6, 2016

docker-machine cant on windows server? StefanScherer/docker-windows-box#12

Closed

thejohnfreeman mentioned this pull request Apr 26, 2019

Add GitLab CI configuration XRPLF/rippled#2897

Closed

Conversation

ppadala commented Apr 17, 2016

Uh oh!

ppadala commented Apr 18, 2016

Uh oh!

ahmetb Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

ppadala Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

ahmetb commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppadala commented Apr 18, 2016

Uh oh!

nathanleclaire commented Apr 18, 2016

Uh oh!

nathanleclaire commented Apr 18, 2016

Uh oh!

ahmetb Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

ahmetb commented Apr 18, 2016

Uh oh!

ppadala commented Apr 18, 2016

Uh oh!

ppadala commented Apr 26, 2016

Uh oh!

ahmetb commented Apr 26, 2016

Uh oh!

ppadala commented Apr 26, 2016

Uh oh!

nathanleclaire commented Apr 26, 2016

Uh oh!

ppadala commented Apr 26, 2016

Uh oh!

nathanleclaire commented Apr 26, 2016

Uh oh!

ahmetb commented Apr 26, 2016

Uh oh!

ppadala commented Apr 28, 2016

Uh oh!

StefanScherer commented Apr 30, 2016

Uh oh!

ppadala commented Apr 30, 2016

Uh oh!

StefanScherer commented Apr 30, 2016

Uh oh!

ppadala commented Apr 30, 2016

Uh oh!

ppadala commented Apr 30, 2016

Uh oh!

enderjbr commented May 5, 2016

Uh oh!

StefanScherer commented May 5, 2016

Uh oh!

ppadala commented May 5, 2016

Uh oh!

StefanScherer commented May 5, 2016

Uh oh!

StefanScherer commented May 29, 2016

Uh oh!

StefanScherer commented Jul 13, 2016

Uh oh!

StefanScherer commented Oct 24, 2016

Uh oh!

davidarcher commented Nov 14, 2017

Uh oh!

dgageot commented Nov 17, 2017

Uh oh!

mitchellmaler commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ahmetb commented Apr 18, 2016 •

edited

Loading