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 nanoserver-1809 container images using multi-stage builds #470

Merged
merged 3 commits into from
May 17, 2021

Conversation

awakecoding
Copy link
Contributor

This pull request adds two simple Dockerfiles to build nanoserver-1809 container images based on the 4.2 and 4.4 servercore-1809 Windows container images. I added only those two for now because I am still using 4.2, and 4.4 is the latest one. I also wanted to ask for a bit of guidance as to how the current project is structured for templating, and how the containers are automatically built.

My original attempt at creating the nanoserver container images was using the powershell nanoserver images, and a modified version of the existing servercore Dockerfiles to pull the MongoDB zip file from the CDN and "install it". I also had to modify Dockerfile to use a different technique to modify the PATH environment variable due to nanoserver limitations. The part that I couldn't get working in a pure nanoserver container image is the installation of the MSVC runtime DLLs, so I opened a ticket to see if this pain point could be improved in the future.

In the end, the simplest has been to do a multi-stage build that does nothing more than copy the files from the servercore container into a clean, minimal nanoserver image. For the CI build environment, all that is required is to build the nanoserver container after the servercore container, because it copies from it. I also used overridable ARGs in the Dockerfile, technically we could have a single nanoserver Dockerfile and call it multiple times with different parameters.

I tried the nanoserver-1809 container image and it works just fine for me, except that instead of 5.62GB, the container weighs 0.92GB for exactly the same end result. MongoDB is the last container image I am using that doesn't have a nanoserver container, and I would rather have it done cleanly upstream instead of publishing my own. In all cases, I'd be glad to give a hand cleaning up and improving the current MongoDB container images! @tianon

@awakecoding
Copy link
Contributor Author

awakecoding commented May 12, 2021

@tianon I'm not familiar with the current tooling, but bashbrew seems to have issues with Dockerfile variables? I can always replace them with their default values and I guess it'll work, but then it would be less reusable without templating:

bashbrew version v0.1.3
failed fetching repo "https://github.com/docker-library/official-images/raw/master/library/${COPY_FROM}

The Dockerfile begins with:

ARG COPY_FROM=library/mongo:4.2-windowsservercore-1809
ARG FROM_IMAGE=mcr.microsoft.com/windows/nanoserver:1809

FROM ${COPY_FROM} AS servercore
FROM ${FROM_IMAGE} AS nanoserver

@tianon
Copy link
Member

tianon commented May 12, 2021

Interesting! (Especially now that Microsoft has managed to resolve #435 - hopefully Nano Server got the same fix. :trollface:)

I've got a few thoughts:

... without templating

Lucky for you, we've already got some pretty basic templating in this repository. 😅

COPY --from=servercore ...

This will need to be a bit more explicit, like what you can see in the Nano Server variants of https://github.com/docker-library/golang (such as https://github.com/docker-library/golang/blob/ad65a7b21b2689de3a15334a7db2917a3b9216ec/1.16/windows/nanoserver-1809/Dockerfile#L25), which will be easier to implement properly with some templating.

ENV PATH="C:\Windows\system32;C:\Windows;C:\mongodb\bin;"

While I trust that the PATH variable for Nano Server is probably this minimal today, I'd rather implement this with setx (as seen in that Go example) so we don't accidentally break it or miss something in the future.

I think we probably should move up the MSVC DLLs too, so they cache better (since they'll theoretically change less often than MongoDB versions, and overlap across versions).

Where I'd suggest to go from here is to look at the Go example and see what we can do to match that structure (for example, renaming your directory from windowsnanoserver-* to nanoserver-* will cause the existing generate-stackbrew-library.sh we already have here to pick it up automatically). I'm also happy to help (or take over) from here if you don't want to continue the minutia. 😅

That Go repository is also using a more advanced templating system that might be worth implementing here (and would probably help out with making this simpler, so I'd be happy to implement that with placeholders for Nano Server work, or roll in the Nano Server bits too).

@awakecoding
Copy link
Contributor Author

@tianon I had a feeling you would mention using "setx" for the PATH, that's what I tried first but there's an issue affecting nanoserver-1809 specifically.

Good point about moving the MSVC DLL copying up, it is much less likely to change. If those DLLs were available as a zip file I would have managed to build the container using nothing more than the nanoserver PowerShell base image + "install" MongoDB by downloading and extracting the zip file. I opened a ticket for the MSVC DLL issue but I think it won't go anywhere, so we'll just have to live with a multi-stage build.

As for renaming "windowsnanoserver" to "nanoserver", that's something I would personally prefer but I didn't want to ask for a chance just because it bothered me. But if you're suggesting it, it would be a whole lot better to use "nanoserver" and "servercore" for the tags, they are more common.

If you have a clear idea in your mind how you'd like to make a few of the structural changes, go ahead, because it would take me longer to figure it out on my side since it's not my project. How exactly do you run the bash script on Windows?

@tianon tianon force-pushed the nanoserver branch 3 times, most recently from a2cc471 to f5f9cd1 Compare May 12, 2021 23:31
@tianon
Copy link
Member

tianon commented May 12, 2021

The naming is honestly a bit historical -- it comes from when the images were microsoft/windowsservercore and microsoft/nanoserver. 😄

This testing is bizarre though -- tests are currently failing to even start mongod, even though I've added explicit icacls to pre-create C:\data bits and adjust permissions for ContainerUser (and I can't reproduce locally, although I'm using --isolation=hyperv not --isolation=process like the tests would be):

{
  "t": {
    "$date": "2021-05-12T23:41:27.569+00:00"
  },
  "s": "E",
  "c": "STORAGE",
  "id": 20557,
  "ctx": "initandlisten",
  "msg": "DBException in initAndListen, terminating",
  "attr": {
    "error": "IllegalOperation: Attempted to create a lock file on a read-only directory: C:\\data\\db\\"
  }
}

(pretty-printed for readability)

@tianon
Copy link
Member

tianon commented May 12, 2021

(Oh, and as for running the bash scripts on Windows, they might or might not work in Git Bash, but I'd really suggest running them inside a Linux container instead -- https://github.com/docker-library/oi-janky-groovy/blob/be908376aae75c2ed452e0f71c5c7bcc4689477a/update.sh/Dockerfile is what @docker-library-bot uses, but you'd probably get pretty far with buildpack-deps:buster-scm by itself. 😅)

tianon added a commit to docker-library/official-images that referenced this pull request May 13, 2021
@tianon
Copy link
Member

tianon commented May 13, 2021

Bah, I swore these tests had been ported to Windows and was wrong 😂

I've excluded nanoserver to match windowsservercore in docker-library/official-images@3a13c01 (and restarted these accordingly).

We can probably ditch the C:\data bits, but they do seem like they might be useful given Nano Server runs as ContainerUser by default (unlike Server Core), but I'm not positive whether Docker volumes on Windows actually copy ACLs properly?

@tianon
Copy link
Member

tianon commented May 13, 2021

We can probably ditch the C:\data bits, but they do seem like they might be useful given Nano Server runs as ContainerUser by default (unlike Server Core), but I'm not positive whether Docker volumes on Windows actually copy ACLs properly?

That's a big noooooope:

C:\>icacls data
data BUILTIN\Users:(F)
     BUILTIN\Administrators:(I)(F)
     BUILTIN\Administrators:(I)(OI)(CI)(IO)(F)
     NT AUTHORITY\SYSTEM:(I)(F)
     NT AUTHORITY\SYSTEM:(I)(OI)(CI)(IO)(F)
     NT AUTHORITY\Authenticated Users:(I)(M)
     NT AUTHORITY\Authenticated Users:(I)(OI)(CI)(IO)(M)
     BUILTIN\Users:(I)(RX)
     BUILTIN\Users:(I)(OI)(CI)(IO)(GR,GE)

Successfully processed 1 files; Failed processing 0 files

C:\>icacls data\db
data\db NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
        BUILTIN\Administrators:(I)(OI)(CI)(F)
        CREATOR OWNER:(I)(OI)(CI)(IO)(F)
        BUILTIN\Users:(I)(OI)(CI)(RX)
        BUILTIN\Users:(I)(CI)(WD,AD,WEA,WA)

Successfully processed 1 files; Failed processing 0 files

@awakecoding
Copy link
Contributor Author

@tianon maybe I don't understand, but how are you supposed to test Windows containers or build Windows containers from inside a Linux container? As for nanoserver and the way it deals with filesystem permissions, yes, I do think it is different from servercore. I did manage to simply swap and existing docker volume that was created with servercore and use the equivalent nanoserver container, but I didn't try creating a new volume.

@tianon
Copy link
Member

tianon commented May 17, 2021

maybe I don't understand, but how are you supposed to test Windows containers or build Windows containers from inside a Linux container?

Sorry for the confusion -- I meant only that the templating/updating shell scripts can be run from a Linux container. The actual builds/tests would still need to happen on Windows.

Comment on lines 73 to 80
COPY --from={{ copy_from }} \
C:\\Windows\\System32\\msvcp140.dll \
C:\\Windows\\System32\\vcruntime140.dll \
C:\\Windows\\System32\\vcruntime140_1.dll \
C:\\Windows\\System32\\
Copy link
Member

Choose a reason for hiding this comment

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

Well, this is fun -- on mongo:4.4 we need vcruntime140_1.dll but mongo:4.2 and 4.0 don't have it. I tried using vcruntime140*.dll (which is how we'd typically resolve something like this on Linux), but that makes Windows sad:

Step 6/12 : COPY --from=mongo:4.4.6-windowsservercore-1809 	C:\\Windows\\System32\\msvcp140.dll 	C:\\Windows\\System32\\vcruntime140*.dll 	C:\\Windows\\System32\\
COPY failed: GetFileInformationByHandle \\?\Volume{9e9e96c1-0320-4c86-ba18-1d91dbf47c0d}: Incorrect function.

I guess we get to use the templating to exclude it for those older versions (and maybe drop that later if/when they update and need the newer DLL too 😅).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering, wouldn't it be a good idea to include the necessary MSVC runtime DLLs inside the MongoDB zip package? Unlike the .msi, the zip doesn't install anything, and there's also no mention that I could find about separately installing the corresponding MSVC runtime when using the zip. Just bundling them inside the zip file would make things a whole lot easier, while also making it fully standalone.

Copy link
Member

Choose a reason for hiding this comment

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

I totally agree, and we tried to use the zip packages previously, but this was the exact reason why we had to stop using them. 😞

@yosifkit yosifkit merged commit dc35ba5 into docker-library:master May 17, 2021
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request May 17, 2021
Changes:

- docker-library/mongo@dc35ba5: Add nanoserver-1809 container images using multi-stage builds (docker-library/mongo#470)
- docker-library/mongo@b3edd5a: Merge pull request docker-library/mongo#472 from infosiftr/4.9-rc
- docker-library/mongo@2e9ea18: Add 4.9-rc
- docker-library/mongo@7ae285a: Merge pull request docker-library/mongo#471 from infosiftr/jq-template
- docker-library/mongo@791d400: Add initial jq-based templating engine
- docker-library/mongo@77e6657: Remove 3.6 (EOL)
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request May 17, 2021
Changes:

- docker-library/mongo@6377a52: Fix Constraints again
- docker-library/mongo@7dadfcc: Add missing Constraints
- docker-library/mongo@dc35ba5: Add nanoserver-1809 container images using multi-stage builds (docker-library/mongo#470)
- docker-library/mongo@b3edd5a: Merge pull request docker-library/mongo#472 from infosiftr/4.9-rc
- docker-library/mongo@2e9ea18: Add 4.9-rc
- docker-library/mongo@7ae285a: Merge pull request docker-library/mongo#471 from infosiftr/jq-template
- docker-library/mongo@791d400: Add initial jq-based templating engine
- docker-library/mongo@77e6657: Remove 3.6 (EOL)
@awakecoding
Copy link
Contributor Author

@tianon woohoo! this PR got merged. Does this mean we'll see official MongoDB nanoserver images soon on Docker Hub?

@tianon
Copy link
Member

tianon commented May 18, 2021

Yep, with the merge of docker-library/official-images#10185 they should be either in-queue or already built. 👍 (and thus available soon)

@awakecoding
Copy link
Contributor Author

@tianon have you been able to use the nanoserver published on docker hub? I tried docker pull mongo:4.2-nanoserver-1809 and I get errors "no matching manifest for windows/amd64 10.0.11763" on Windows Server 2019, and the same error except with version "10.0.19042" on Windows 10. I noticed that while the new nanoserver tags get listed on Docker Hub, there is no associated digest and you can't see the details like for other tags. Something's off with the uploaded nanoserver tags?

@yosifkit
Copy link
Member

yosifkit commented Jun 4, 2021

It seems that the windows server needed an updated Docker to handle the backslash at the end of the path in the COPY. New builds in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants