Add docker support #394

ryanbrandenburg · 2017-09-07T20:49:17Z

Add the docker-build command to KoreBuild. Fixes aspnet/Coherence-Signed#647.

dnfclas · 2017-09-07T20:49:21Z

@ryanbrandenburg,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

analogrelay

Looks good, got some comments, mostly about structuring the images so that it's easy to see what they are in docker images (it also opens us up to be able to publish to Docker Hub or a registry at some point)

analogrelay · 2017-09-07T21:36:47Z

files/KoreBuild/KoreBuild.sh

@@ -13,7 +14,7 @@ set_korebuildsettings() {
    local config_file="${4:-}" # optional. Not used yet.

    [ -z "${dot_net_home:-}" ] && dot_net_home="$HOME/.dotnet"
-    [ -z "${tools_source:-}" ] && tools_source='https://aspnetcore.blob.core.windows.net/buildtools'
+    [ -z "${tools_source:-}" ] && tools_source=default_tools_source


Missing a $? tools_source=$default_tools_source

analogrelay · 2017-09-07T21:43:53Z

tools/KoreBuild.Console/Commands/DockerBuildCommand.cs

+            {
+                var buildArgs = new List<string> { "build" };
+
+                var tag = $"{ImagePrefix}:{ContainerName}";


First off: we shouldn't mix the container name into the image name, it can add confusion.

Docker image tag names are generally in the form: [owner]/[name]:[variant]. Here, owner should map to an account on Docker Hub (eventually, doesn't have to right now), I think "aspnetbuild" is a good name. name generally refers to the content of the image (regardless of base OS, etc.), so korebuild or build would be good names. variant is used to distinguish between versions and OSes usually. Take a look at the microsoft/dotnet image for some examples: https://hub.docker.com/r/microsoft/dotnet/

I'd suggest something like aspnetbuild/korebuild:jessie. We may even want to get in to version number tags at some point, but I think we can start with this, since we're always building the container "fresh" (using caches) rather than pulling from Docker Hub.

Combined with the above comment re:"Platform", this string would become something like var tag = $"{ImagePrefix}:{Image} and ImagePrefix becomes aspnetbuild/korebuild

analogrelay · 2017-09-07T21:47:56Z

tools/KoreBuild.Console/Commands/DockerBuildCommand.cs

+
+        public override void Configure(CommandLineApplication application)
+        {
+            Platform = application.Argument("platform", "The docker platform to run on.");


I'd suggest using a term like Image for this (I realize it makes up only part of the image name, see comment below, but I think it's a clearer name regardless). It's a little more aligned with Docker terminology. Platform makes me thing of Docker for Windows vs Docker for Linux.

analogrelay · 2017-09-07T21:56:58Z

tools/KoreBuild.Console/Commands/DockerBuildCommand.cs

+                    return buildResult;
+                }
+
+                var runArgs = new List<string> { "run", "--rm", "-it", "--name", ContainerName, tag };


If we're using --name we should use a uniquely-generated (possibly timestamp-based?) name, perhaps something like aspnetbuild_[timestamp]. If there's already a container with this name, it will conflict.

Also, we should make it possible to disable --rm. If my build fails, it would be nice to have the container stick around.

Finally, make sure the container name is dumped to the console so I know which containers come from which builds.

--name is most useful for long-running containers. In this scenario, I would just use the random name assigned by Docker.

I figured you wouldn't ever run two at the same time due to --rm, but I guess it's still possible, and as below I would like to let you run interactive in the future, so I'll do that.

I had intended to add disabling --rm as an option later on. I was intending to get this running in an actual scenario (one of the CI builds) before complicating things with more options. I just filed #395 for that. Is that good or do you think that option is more important that that?

Good point about the container name.

I think --name becomes relevant when --rm is removable. @mikeharder is definitely right that if the container is just going to be removed after the build, there's no need to know the name.

Perhaps the best solution is to remove --name for now, and when we make --rm removable, we can also add an option to the docker-build command to allow providing a container name.

I'm actually partial to supplying the name ourselves because:

We can easily output the name in case it persists for some reason (including future interactivity).

It makes it easy to tell at a glance which containers belong to KoreBuild.

It doesn't hurt anything (unless there's something I don't know).

I do agree that allowing people to specify the container name in the future would be a nice feature. I created #396 to enumerate all the future improvements we think of while doing this work.

analogrelay · 2017-09-07T21:58:44Z

tools/KoreBuild.Console/Commands/DockerFiles/jessie.dockerfile

+    apt-get install -y git && \
+    apt-get install -y curl && \
+    apt-get install -y unzip && \
+    apt-get install -y apt-transport-https


Add rm -rf /var/lib/apt/lists/* to the end of this chain. See https://github.com/anurse/ConferencePlanner/blob/735d5ef3352d18c7a258267a1f18aad359479ec2/docker/base.Dockerfile#L9

That deletes the files apt-get update downloaded, which take up a fair amount of space and are only needed during installation. You need to chain it in to the RUN command so that it all ends up being baked as one layer in the image. That ensures that the layer includes only the files added by the packages you installed, and no other cruft.

You also only need one call to apt-get install. Example:

https://github.com/aspnet/benchmarks/blob/dev/docker/benchmarks/Dockerfile#L3

👍 more a performance that space saving issue, but definitely a good one.

mikeharder · 2017-09-07T22:11:09Z

tools/KoreBuild.Console/Commands/DockerBuildCommand.cs

+                    return buildResult;
+                }
+
+                var runArgs = new List<string> { "run", "--rm", "-it", "--name", ContainerName, tag };


--name is most useful for long-running containers. In this scenario, I would just use the random name assigned by Docker.

mikeharder · 2017-09-07T22:12:12Z

tools/KoreBuild.Console/Commands/DockerFiles/jessie.dockerfile

+    apt-get install -y git && \
+    apt-get install -y curl && \
+    apt-get install -y unzip && \
+    apt-get install -y apt-transport-https


You also only need one call to apt-get install. Example:

https://github.com/aspnet/benchmarks/blob/dev/docker/benchmarks/Dockerfile#L3

mikeharder · 2017-09-07T22:12:33Z

tools/KoreBuild.Console/Commands/DockerFiles/jessie.dockerfile

+    apt-get install -y unzip && \
+    apt-get install -y apt-transport-https
+
+ADD ./ ./


I've typically see this as ADD . ., though I think the two are equivalent.

mikeharder · 2017-09-07T22:13:00Z

tools/KoreBuild.Console/Commands/DockerFiles/jessie.dockerfile

+
+ADD ./ ./
+
+RUN rm -f ./korebuild-lock.txt


I think it would be better to exclude this via a .dockerignore file, rather than copying and then removing.

ryanbrandenburg · 2017-09-11T23:23:11Z

tools/KoreBuild.Console/Commands/DockerFiles/winservercore.dockerfile

+
+RUN rm /net461dev.exe 
+
+ADD . c:/repo/


I decided not to dump this in c:\ because it's cleaner, but also because I ran into a problem with running KoreBuild from the drive root on windows.

I've typically seen Dockerfiles use the following order of statements instead:

WORKDIR c:\\repo ADD . .

Also, our docs (https://docs.microsoft.com/en-us/virtualization/windowscontainers/manage-docker/manage-windows-dockerfile) recommend escaped backslashes rather than forward slashes, though both may work:

WORKDIR c:\\windows

ryanbrandenburg · 2017-09-11T23:24:55Z

🆙📅

mikeharder · 2017-09-11T23:33:35Z

tools/KoreBuild.Console/Commands/DockerFiles/winservercore.dockerfile

+
+RUN rm /net461dev.exe 
+
+ADD . c:/repo/


I've typically seen Dockerfiles use the following order of statements instead:

WORKDIR c:\\repo ADD . .

mikeharder · 2017-09-11T23:40:43Z

tools/KoreBuild.Console/Commands/DockerFiles/winservercore.dockerfile

@@ -0,0 +1,15 @@
+FROM microsoft/aspnet:4.6.2
+
+ADD https://download.microsoft.com/download/F/1/D/F1DEB8DB-D277-4EF9-9F48-3A65D4D8F965/NDP461-DevPack-KB3105179-ENU.exe /net461dev.exe


You should be able to download, install, and remove in a single command (which is more efficient) like so:

https://github.com/Microsoft/aspnet-docker/blob/master/4.6.2/Dockerfile#L12-L16

This is actually somewhat complicated since NDP461-DevPack-KB3105179-ENU.exe /Passive /NoRestart immediately exits with error code 0, and the install runs in the background. So, the delete will fail unless you wait a bit after starting the install. In your original Dockerfile, I think there's a small delay between the two run commands which usually causes the delete to succeed, but it's not guaranteed. And technically, I don't think it's guaranteed the install will be complete by the time build.cmd tries to use it. The following should work, but there's no way to know the correct length of time to sleep:

RUN Invoke-WebRequest https://download.microsoft.com/download/F/1/D/F1DEB8DB-D277-4EF9-9F48-3A65D4D8F965/NDP461-DevPack-KB3105179-ENU.exe \ -OutFile C:\NDP461-DevPack-KB3105179-ENU.exe; \ C:\NDP461-DevPack-KB3105179-ENU.exe /Passive /NoRestart; \ Start-Sleep -s 10; \ Remove-Item C:\NDP461-DevPack-KB3105179-ENU.exe -Force;

The most reliable way would be to block until we know the install is complete, perhaps using the existence of a file or registry key.

mikeharder · 2017-09-12T01:11:12Z

tools/KoreBuild.Console/Commands/DockerFiles/winservercore.dockerfile

+
+RUN rm /net461dev.exe 
+
+WORKDIR c:\\repo\\


You need to remove the trailing backslash, otherwise it seems Docker treats it as a line continuation:

Step 5/7 : WORKDIR c:\\repo\ADD . .

mikeharder

Approved with one small change to Windows dockerfile.

mikeharder · 2017-09-12T20:00:16Z

tools/KoreBuild.Console/Commands/DockerFiles/winservercore.dockerfile

@@ -0,0 +1,17 @@
+FROM microsoft/aspnet:4.6.2
+
+WORKDIR c:\\repo


Move WORKDIR after RUN to improve caching, so if you decide to change the WORKDIR you don't need to re-execute the RUN command.

natemcmaster · 2017-09-12T20:23:06Z

The tests are failing...

[xUnit.net 00:00:53.9985641]         \x1b[0;36m>>> dotnet /home/travis/.dotnet/buildtools/korebuild/2.1.0-preview1-15493/tools/KoreBuild.Console.dll -s --tools-source https://aspnetcore.blob.core.windows.net/buildtools --dotnet-home /home/travis/.dotnet --repo-path /tmp/korebuild/f5e676bf-b082-40c1-9d80-1558972c49e3 /home/travis/build/aspnet/BuildTools/artifacts -u docker-build jessie -Path /tmp/korebuild/f5e676bf-b082-40c1-9d80-1558972c49e3 /p:BuildNumber=0001 /v:n\x1b[0m
[xUnit.net 00:00:53.9986325]         Specify --help for a list of available options and commands.
[xUnit.net 00:00:53.9987061]         Exception thrown: 'Microsoft.Extensions.CommandLineUtils.CommandParsingException: Unrecognized option '-s'
[xUnit.net 00:00:53.9987362]            at Microsoft.Extensions.CommandLineUtils.CommandLineApplication.HandleUnexpectedArg(CommandLineApplication command, String[] args, Int32 index, String argTypeName) in C:\b\w\9571b3e5f03d051f\shared\Microsoft.Extensions.CommandLineUtils.Sources\CommandLine\CommandLineApplication.cs:line 495
[xUnit.net 00:00:53.9987881]            at Microsoft.Extensions.CommandLineUtils.CommandLineApplication.Execute(String[] args) in C:\b\w\9571b3e5f03d051f\shared\Microsoft.Extensions.CommandLineUtils.Sources\CommandLine\CommandLineApplication.cs:line 207
[xUnit.net 00:00:53.9988528]            at KoreBuild.Console.Program.Main(String[] args) in C:\b\w\9571b3e5f03d051f\tools\KoreBuild.Console\Program.cs:line 23'
[xUnit.net 00:00:53.9988866]         \x1b[0;31merror  : dotnet failed with exit code 1\x1b[0m

natemcmaster · 2017-09-12T20:25:05Z

files/KoreBuild/KoreBuild.sh

    # Call "sync" between "chmod" and execution to prevent "text file busy" error in Docker (aufs)
    chmod +x "$__korebuild_dir/scripts/get-netfx.sh"; sync
-    "$__korebuild_dir/scripts/get-netfx.sh" $verbose_flag $netfx_version "$tools_source" "$ReferenceAssemblyRoot" \
+    # we don't include netfx in the BuildTools artifacts currently, it ends up on the blob store through other means, so we'll only look for it in the default_tools_source
+    "$__korebuild_dir/scripts/get-netfx.sh" $verbose_flag $netfx_version "$default_tools_source" "$ReferenceAssemblyRoot" \


Why this change? This should be reverted as it makes it impossible to override --tools-source for the netfx asset.

Like the comment says, overriding --tools-source for the netfx asset doesn't work right now. Currently the buildtools artifact does not contain netfx, it gets added to the blob store by some other process. If we want to fix that I feel it's better dealt with in another issue/PR.

Ok, we'll track that here: #399

natemcmaster · 2017-09-12T20:26:44Z

test/KoreBuild.FunctionalTests/SimpleRepoTests.cs

+        {
+            var app = _fixture.CreateTestApp("SimpleRepo");
+            var platform = "jessie";
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


You should check which platform the docker host is using, not which platform is running this test. Windows machines can use Linux containers.

Also, I would be good to make this test skip if docker is not installed.

docker version -f "{{ .Server.Os }}"

natemcmaster · 2017-09-12T20:29:21Z

tools/KoreBuild.Console/Commands/DockerBuildCommand.cs

+    {
+        private const string DockerIgnore = ".dockerignore";
+        private const string DockerfileExtension = ".dockerfile";
+        private const string Owner = "aspnetbuild";


Do we already own this docker hub account? Or are you planning on using a private container registry?

We should probably claim it if we can. I think it would be useful to take the simple steps to prepare to use a registry of some kind.

natemcmaster · 2017-09-12T21:03:43Z

tools/KoreBuild.Console/Commands/DockerFiles/winservercore.dockerfile

+# DevPack returns exit 0 immediately, but it's not done, so we wait.
+# A more correct thing would be to block on a registry key existing or similar.
+RUN \
+        Invoke-WebRequest https://download.microsoft.com/download/F/1/D/F1DEB8DB-D277-4EF9-9F48-3A65D4D8F965/NDP461-DevPack-KB3105179-ENU.exe -OutFile .\\net461dev.exe ; \


Hopefully this will be unnecessary once we fix #399

natemcmaster · 2017-09-12T21:06:22Z

tools/KoreBuild.Console/Commands/DockerBuildCommand.cs

+            {
+                dockerToolsSource = "ToolsSource";
+                toolsSourceDestination = Path.Combine(RepoPath, dockerToolsSource);
+                DirectoryCopy(ToolsSource, toolsSourceDestination);


An easier and faster way to do this to volume mount this folder into the container.

natemcmaster · 2017-09-12T21:07:30Z

test/KoreBuild.FunctionalTests/Utilities/TestApp.cs

            }

            arguments.AddRange(new[]
            {
-                "-ToolsSource", _toolsSource,
-                "-Update"
+                "-s", _toolsSource,


Nit: I personally prefer the readability of the long option name. The short option is good for command-line shorthand, but harder to interpret from logs and code.

ryanbrandenburg · 2017-09-13T22:32:32Z

🆙📅

ryanbrandenburg · 2017-09-15T16:47:18Z

ping @natemcmaster.

natemcmaster · 2017-09-15T16:50:40Z

scripts/bootstrapper/run.sh

@@ -218,6 +218,7 @@ fi
 [ -z "$channel" ] && channel='dev'
 [ -z "$tools_source" ] && tools_source='https://aspnetcore.blob.core.windows.net/buildtools'

+__warn "ts: $tools_source"


natemcmaster · 2017-09-15T16:50:59Z

test/KoreBuild.FunctionalTests/SimpleRepoTests.cs

@@ -55,5 +58,85 @@ public SimpleRepoTests(ITestOutputHelper output, RepoTestFixture fixture)
            Assert.Same(task, build);
            Assert.NotEqual(0, build.Result);
        }
+
+        [DockerExistsFact]


natemcmaster

One comment about the __warn addition (I assume it was added to debug stuff), but otherwise looks good.

ryanbrandenburg requested review from analogrelay, natemcmaster and mikeharder September 7, 2017 20:49

dnfclas added the cla-already-signed label Sep 7, 2017

analogrelay reviewed Sep 7, 2017

View reviewed changes

mikeharder requested changes Sep 7, 2017

View reviewed changes

ryanbrandenburg commented Sep 11, 2017

View reviewed changes

mikeharder requested changes Sep 11, 2017

View reviewed changes

ryanbrandenburg force-pushed the rybrande/Docker branch from 37d299c to 516c03d Compare September 11, 2017 23:42

mikeharder requested changes Sep 12, 2017

View reviewed changes

ryanbrandenburg force-pushed the rybrande/Docker branch 2 times, most recently from 5224cca to 778b1a5 Compare September 12, 2017 17:43

mikeharder approved these changes Sep 12, 2017

View reviewed changes

natemcmaster suggested changes Sep 12, 2017

View reviewed changes

natemcmaster reviewed Sep 12, 2017

View reviewed changes

ryanbrandenburg force-pushed the rybrande/Docker branch 2 times, most recently from 34671f4 to 570269a Compare September 13, 2017 21:59

natemcmaster reviewed Sep 15, 2017

View reviewed changes

natemcmaster approved these changes Sep 15, 2017

View reviewed changes

Add docker support

8878da9

ryanbrandenburg force-pushed the rybrande/Docker branch from 8f7c2cc to 8878da9 Compare September 19, 2017 16:52

ryanbrandenburg merged commit 8878da9 into dev Sep 19, 2017

ryanbrandenburg deleted the rybrande/Docker branch September 19, 2017 16:53

		@@ -0,0 +1,15 @@
		FROM microsoft/aspnet:4.6.2

		ADD https://download.microsoft.com/download/F/1/D/F1DEB8DB-D277-4EF9-9F48-3A65D4D8F965/NDP461-DevPack-KB3105179-ENU.exe /net461dev.exe

Add docker support #394

Add docker support #394

Conversation

ryanbrandenburg commented Sep 7, 2017

dnfclas commented Sep 7, 2017

analogrelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharder Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Sep 11, 2017

mikeharder Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Sep 13, 2017

ryanbrandenburg commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster left a comment

Choose a reason for hiding this comment

analogrelay Sep 7, 2017 •

edited

Loading

mikeharder Sep 11, 2017 •

edited

Loading

mikeharder Sep 11, 2017 •

edited

Loading