[Fixed] - Add Dockerfiles for Linux .NET Composite Images #4594

ivdiazsa · 2023-05-05T00:04:43Z

This PR follows up #4590, which got seriously messed up when rebasing.

ivdiazsa · 2023-05-05T00:05:09Z

MichaelSimons

Please work with @lbussell to define the appropriate tests. Also the size baseline will need updating to include these new image variants.

MichaelSimons · 2023-05-05T15:37:37Z

README.aspnet.md

 -----------| -------------| -------------
 8.0.0-preview.4-bookworm-slim-amd64, 8.0-preview-bookworm-slim-amd64, 8.0.0-preview.4-bookworm-slim, 8.0-preview-bookworm-slim, 8.0.0-preview.4, 8.0-preview, latest | [Dockerfile](https://github.com/dotnet/dotnet-docker/blob/nightly/src/aspnet/8.0/bookworm-slim/amd64/Dockerfile) | Debian 12
 8.0.0-preview.4-alpine3.17-amd64, 8.0-preview-alpine3.17-amd64, 8.0-preview-alpine-amd64, 8.0.0-preview.4-alpine3.17, 8.0-preview-alpine3.17, 8.0-preview-alpine | [Dockerfile](https://github.com/dotnet/dotnet-docker/blob/nightly/src/aspnet/8.0/alpine3.17/amd64/Dockerfile) | Alpine 3.17
+8.0.0-preview.4-alpine3.17-amd64-composite, 8.0-preview-alpine3.17-amd64-composite, 8.0-preview-alpine-amd64-composite, 8.0.0-preview.4-alpine3.17, 8.0-preview-alpine3.17, 8.0-preview-alpine | [Dockerfile](https://github.com/dotnet/dotnet-docker/blob/nightly/src/aspnet-composite/8.0/alpine3.17/amd64/Dockerfile) | Alpine 3.17


I think composite should appear immediately after the distro - 8.0.0-preview.4-alpine3.17-composite-amd64

This is consistent with the other image variants like mariner2.0-distroless, jammy-chiseled, and debian-slim

MichaelSimons · 2023-05-05T15:40:57Z

src/aspnet-composite/8.0/alpine3.17/amd64/Dockerfile

The folder structure should match the tagging pattern. To follow my earlier comment regarding the tagging, this should be in a src/aspnet/8.0/alpine3.17-composite/amd64/ folder structure.

MichaelSimons · 2023-05-05T15:44:07Z

src/aspnet-composite/8.0/alpine3.17/amd64/Dockerfile

+# by the base runtime-deps image. See https://aka.ms/dotnet-globalization-alpine-containers for more information.
+
+# ASP.NET Core version
+ENV ASPNET_VERSION=8.0.0-preview.4.23251.12


In order to provide parity with the non-composite images, I wonder if this should also define DOTNET_VERSION. This is necessary because of being based on runtime-deps versus runtime.

MichaelSimons · 2023-05-05T15:46:34Z

samples/aspnetapp/aspnetapp/NuGet.config

What precipitates the need for this?

I think this is leftover from my PRs where I was using this sample to test the ASP.NET composite images. If we want to test an 8.0 sample on the composite runtime then we'd need to have this somewhere. I think there would be ways to test without that either though.

…n and paths.

…erfiles, since those are built from runtime-deps, rather than runtime.

mthalman · 2023-05-08T17:41:48Z

eng/dockerfile-templates/runtime/Dockerfile.linux.install-runtime

+    set filePlatform to when(ARGS["is-rpm-install"], "", when(isAlpine, "-linux-musl", "-linux")) ^
+    set baseUrl to VARIABLES[cat("base-url|", dotnetVersion, "|", VARIABLES["branch"])] ^
+    set runtimeBaseUrl to cat(baseUrl, "/Runtime/", runtimeVersionDir, "/") ^
+    set aspnetUrl to cat(baseUrl, "/aspnetcore/Runtime/", aspnetVersionDir, "/aspnetcore-runtime-composite-", aspnetVersionFile,


Name this aspnetCompositeUrl instead of aspnetUrl to avoid confusion.

mthalman · 2023-05-08T17:46:33Z

eng/update-dependencies/DockerfileShaUpdater.cs

                { "runtime-deps-cm.2", new string[] { $"$DOTNET_BASE_URL/Runtime/$VERSION_DIR/dotnet-runtime-deps-$VERSION_FILE-cm.2-{GetRpmArchFormat()}.$ARCHIVE_EXT" } },

                { "aspnet", new string[] { $"$DOTNET_BASE_URL/aspnetcore/Runtime/$VERSION_DIR/aspnetcore-runtime-$VERSION_FILE$OPTIONAL_OS-{GetRuntimeSdkArchFormat()}.$ARCHIVE_EXT" } },
+                { "aspnet", new string[] { $"$DOTNET_BASE_URL/aspnetcore/Runtime/$VERSION_DIR/aspnetcore-runtime-$COMPOSITE_SUFFIX$VERSION_FILE$OPTIONAL_OS-{GetRuntimeSdkArchFormat()}.$ARCHIVE_EXT" } },


This isn't going to run. It's introducing a duplicate key, aspnet, into the dictionary. I would suggest naming it aspnet-composite instead. This variable name change then needs to be reflected in the spot where it's referenced at https://github.com/dotnet/dotnet-docker/pull/4594/files#diff-9ae9ccb196a27239e9c7f5c7cb97edec392356c2ac3451cb6e6582cee2455b94R73.

There also needs to be corresponding entries in the manifest.versions.json file at the root of the repo to define these SHA variables and their values. That's the file which defines the SHAs and are then injected into the Dockerfile.

This is the reason the build is failing with checksum mismatches. The Dockerfile template is referencing the aspnet prefix variable from manifest.versions.json file which is giving you the SHA of the non-composite aspnet tarball.

You should be able to run eng\Set-DotnetVersions.ps1 which will regenerate all Dockerfiles to reflect the checksums that are defined in manifest.versions.json.

mthalman · 2023-05-08T17:52:34Z

samples/aspnetapp/aspnetapp/aspnetapp.csproj


  <PropertyGroup>
-    <TargetFramework>net7.0</TargetFramework>
+    <TargetFramework>net8.0</TargetFramework>


We don't want samples updated to 8.0 yet.

ivdiazsa · 2023-05-08T20:32:09Z

@mthalman Thanks for your feedback Matt. I will get to work on it now.

…s because the Set-DotnetVersions script keeps failing because I have uncommitted changes. Will try again after this.

…ll finally stop complaining.

…ghtly.linux.json file.

… May be more than a template...

mthalman · 2023-05-10T12:58:04Z

tests/Microsoft.DotNet.Docker.Tests/AspnetCompositeImageTests.cs

+    public static IEnumerable<object[]> GetImageData() => GetImageData(DotNetImageType.Aspnet_Composite);
+
+    [DotNetTheory]
+    [MemberData(memberof(GetImageData))]


All these memberof references need to be renamed to nameof.

mthalman · 2023-05-10T13:09:50Z

tests/Microsoft.DotNet.Docker.Tests/AspnetCompositeImageTests.cs

+namespace Microsoft.DotNet.Docker.Tests;
+
+[Trait("Category", "aspnet-composite")]
+public class AspnetCompositeImageTests : CommonRuntimeImageTests


It looks like, in general, this class has a very similar implementation to the existing AspnetImageTests class. I would suggest refactoring such that there is a common base class between them that implements the majority of these tests. It looks like much of the differences here are just conditional checks for whether the test should be skipped. That can be implemented via an abstract method in the base which is overridden appropriately in the derived classes.

mthalman · 2023-05-10T13:18:22Z

README.aspnet.md

We should really have some sort of documentation that describes the purpose of these composite images (i.e. what are they? how do they differ from the regular aspnet images? how do I determine which one to use?). If necessary, this can link to another doc page that describes things in more detail.

Note that the Docker repository readmes are generated from templates. For example, the About section of README.aspnet.md comes from https://github.com/dotnet/dotnet-docker/blob/nightly/eng/readme-templates/About.aspnet.md. Readmes can be regenerated from templates by running eng\readme-templates\Get-GeneratedReadmes.ps1

@mthalman - we have a technical description of the composite format here:

https://github.com/dotnet/runtime/blob/main/docs/design/features/readytorun-composite-format-design.md

In terms of our current shipping story the composite container is a tool a customer can use to reduce container size on disk while keeping performance of default R2R publishing with the caveat that the composite container has tighter version coupling - in particular, the final app is not allowed to use handpicked custom versions of framework / ASP.NET assemblies that are baked into the composite image. @ivdiazsa - please let me know if you need further help with the actual wording.

…ormal and composite.

ivdiazsa · 2023-05-11T00:28:37Z

In my latest commit, I added a base class for the common functionality between normal ASP.NET and composite ASP.NET tests, as suggested by @mthalman. However, I can't get it to work. I keep getting an error saying that those methods are not supported. Does anyone know why that's happening?

I pushed the changes to "save my progress" and show easily what I added and modified.

mthalman · 2023-05-12T18:33:01Z

In my latest commit, I added a base class for the common functionality between normal ASP.NET and composite ASP.NET tests, as suggested by @mthalman. However, I can't get it to work. I keep getting an error saying that those methods are not supported. Does anyone know why that's happening?

I pushed the changes to "save my progress" and show easily what I added and modified.

We have no visibility into these errors because it's not even getting to that point in the pipeline. It's failing on the prebuild validation which ensures that both the Dockerfiles and readmes are up-to-date with respect to the templates. It says the Dockerfiles are not in sync. So you'll need to regenerate them with .\eng\dockerfile-templates\Get-GeneratedDockerfiles.ps1

ivdiazsa · 2023-05-12T22:28:52Z

We have no visibility into these errors because it's not even getting to that point in the pipeline. It's failing on the prebuild validation which ensures that both the Dockerfiles and readmes are up-to-date with respect to the templates. It says the Dockerfiles are not in sync. So you'll need to regenerate them with .\eng\dockerfile-templates\Get-GeneratedDockerfiles.ps1

I don't think that's the problem. I just ran Get-GeneratedDockerfiles.ps1 and Get-GeneratedReadmes.ps1 again, and there are no changes to commit. Tomas and I did some looking into and it's some problem with Xunit but we haven't devised a definitive fix yet.

I don't know why the CI is failing with that. Running those scripts doesn't change anything in my machine.

MichaelSimons · 2023-05-12T22:36:19Z

I don't know why the CI is failing with that. Running those scripts doesn't change anything in my machine.

It is failing because CI always merges in the latest changes from the target branch before running. The tip of nightly is on a newer .NET version therefore the new Dockerfiles you are adding are out of date. Sync your local branch with the tip of nightly and regen the Dockerfiles.

mthalman · 2023-05-15T21:21:50Z

The issue with the abstract class and xUnit can be solved without reverting the refactoring. The problem was that the test methods in CommonAspnetImageTests were referencing a method named GetImageData. Before the refactoring, that method was defined here:

dotnet-docker/tests/Microsoft.DotNet.Docker.Tests/AspnetImageTests.cs

Line 23 in df92078

    
           public static IEnumerable<object[]> GetImageData() => GetImageData(DotNetImageType.Aspnet);

But as a result of moving the test methods into a base class, they're now referencing an inherited static method with the same name but takes a parameter:

dotnet-docker/tests/Microsoft.DotNet.Docker.Tests/CommonRuntimeImageTests.cs

Line 20 in df92078

public static IEnumerable<object[]> GetImageData(DotNetImageType imageType)

xUnit can't bind to that properly because no argument is provided from the MemberData attribute and it throws a very unhelpful exception.

This can be fixed by changing those implemented methods in CommonAspnetImageTests to be abstract and implementing them with the adorned attributes in the two derived classes. This will then bind to the locally defined GetImageData method.

In addition, my expectation was that the CommonAspnetImageTests class would have a common implementation of the test methods wherever possible. For example: AspnetImageTests.cs/VerifyAppScenario and AspnetCompositeImageTests.cs/VerifyAppScenario only differ in the if block. The common implementation of that method should be moved to the base class and the if check factored out into an abstract method. The same applies to the other test methods between AspnetImageTests and AspnetCompositeImageTests.

trylek · 2023-05-15T21:34:17Z

@mthalman - I have originally advised Ivan to get rid of the class hierarchy exactly because xUnit can bind neither GetImageData, nor ImageType, to a non-static method and all the tests are tagged as Theory with MemberData. Are you suggesting that all these tests be duplicated in the final classes (AspnetImageTests vs. AspnetCompositeImageTests) as just wrappers calling into CommonAspnetImageTests in the vast majority of cases? My original impression was that it would be easier to just add the composite image as one more test case produced by GetImageData - after all, most of the test stack should be oblivious of the distinction.

mthalman · 2023-05-16T13:21:07Z

@mthalman - I have originally advised Ivan to get rid of the class hierarchy exactly because xUnit can bind neither GetImageData, nor ImageType, to a non-static method and all the tests are tagged as Theory with MemberData. Are you suggesting that all these tests be duplicated in the final classes (AspnetImageTests vs. AspnetCompositeImageTests) as just wrappers calling into CommonAspnetImageTests in the vast majority of cases? My original impression was that it would be easier to just add the composite image as one more test case produced by GetImageData - after all, most of the test stack should be oblivious of the distinction.

I just want there to be an elimination of duplication between AspnetImageTests and AspnetCompositeImageTests. For example, these two test methods have very similar implementations:

My suggestion for accomplishing that was to have a base class with a common implementation. This pattern is already used for common test logic between the aspnet and runtime image types. For example, the derived test class implements a wrapper method necessary to satisfy xUnit:

https://github.com/dotnet/dotnet-docker/blob/df920782fdedd2d4b3bc74c3c7a3fe9a2ba76a07/tests/Microsoft.DotNet.Docker.Tests/AspnetImageTests.cs#L87C1-L92

While the base class provides the implementation:

dotnet-docker/tests/Microsoft.DotNet.Docker.Tests/ProductImageTests.cs

Lines 29 to 35 in df92078

    
           protected void VerifyCommonNoSasToken(ProductImageData imageData) 
        
           { 
        
               string imageTag = imageData.GetImage(ImageType, DockerHelper); 
        
               string historyContents = DockerHelper.GetHistory(imageTag); 
        
               Match match = Regex.Match(historyContents, @"=\?(sig|\S+&sig)=\S+"); 
        
               Assert.False(match.Success, $"A SAS token was detected in the Docker history of '{imageTag}'."); 
        
           }

Making use of inheritance to provide this common implementation is especially useful if there is any slight variation needed between derived classes as this can be handled via an abstract method implementation.

MichaelSimons · 2023-05-19T18:35:37Z

Where should I add such documentation? dotnet-docker/documentation?

To the aspnet readme. This is the readme that gets syndicated to Docker Hub and MAR.

MichaelSimons · 2023-05-19T18:43:26Z

Sorry for accidentally closing.

ivdiazsa · 2023-05-19T21:05:48Z

Composite doc is up

trylek

Thanks Ivan for driving this to the finish line!

eng/readme-templates/Use.aspnet.md

src/aspnet/8.0/alpine3.17-composite/amd64/Dockerfile

README.aspnet.md

MichaelSimons · 2023-05-22T17:56:17Z

README.aspnet.md


 This image contains the ASP.NET Core and .NET runtimes and libraries and is optimized for running ASP.NET Core apps in production.

+# Composite container images


Suggested change

# Composite container images

## Composite container images

.mar/portal/README.aspnet.portal.md

eng/readme-templates/About.aspnet.md

mthalman

Looks good. Thanks for all the work you put into this.

lbussell · 2023-05-23T20:03:19Z

eng/dockerfile-templates/aspnet/Dockerfile.linux-composite

+}}ARG REPO=mcr.microsoft.com/dotnet/runtime-deps{{ if isSingleStage:
+{{
+
+    _ SINGLE STAGE


I would just like to note that there are unused options in this file but we should leave them for if we decide to ship non-musl variants of composite images in the future.

mangod9 · 2023-05-23T20:44:08Z

Thanks everyone for getting this change merged! Assume this will now push the new images, or is there more work required for these composite images to showup on dockerhub?

mthalman · 2023-05-23T20:54:54Z

This will kick off a build (takes about an hour to finish) that will publish the new images to MCR. The tags will show up at https://hub.docker.com/_/microsoft-dotnet-nightly-aspnet/.

mangod9 · 2023-05-23T20:57:00Z

Ah nice! Thanks for the info.

lbussell · 2023-05-23T22:28:49Z

The images are now available. I filed #4634 for a minor issue with the manifest.json/readme.

mangod9 · 2023-05-23T22:33:43Z

Yeah I see them. Do the tags look ok? Seems like we need to add composite to the last two tags?

8.0.0-preview.5-alpine3.17-arm64v8, 8.0-preview-alpine3.17-arm64v8, 8.0-preview-alpine-arm64v8, 8.0.0-preview.5-alpine3.17, 8.0-preview-alpine3.17, 8.0-preview-alpine	Dockerfile	Alpine 3.17	05/18/2023
8.0.0-preview.5-alpine3.17-composite-arm64v8, 8.0-preview-alpine3.17-composite-arm64v8, 8.0-preview-alpine-composite-arm64v8, 8.0.0-preview.5-alpine3.17, 8.0-preview-alpine3.17, 8.0-preview-alpine	Dockerfile	Alpine 3.17	05/23/2023

lbussell · 2023-05-23T23:01:59Z

Opened #4635

Co-authored-by: Logan Bussell <[email protected]>

Added and enabled ASP.NET Composite Images, following the messed up PR …

fbfa384

…#4590.

ivdiazsa requested review from MichaelSimons, lbussell and mthalman as code owners May 5, 2023 00:04

ghost added area-dockerfiles Concerns the official .NET Dockerfiles or Dockerfile templates untriaged labels May 5, 2023

MichaelSimons reviewed May 5, 2023

View reviewed changes

ivdiazsa added 2 commits May 5, 2023 13:57

Changed the composite suffix to match the other tags naming conventio…

fa81417

…n and paths.

Added the environment variable 'DOTNET_VERSION' to the composite Dock…

de07614

…erfiles, since those are built from runtime-deps, rather than runtime.

mthalman reviewed May 8, 2023

View reviewed changes

ivdiazsa added 6 commits May 8, 2023 14:49

Addressed some comments. Haven't regenerated the composite Dockerfile…

9147f59

…s because the Set-DotnetVersions script keeps failing because I have uncommitted changes. Will try again after this.

Fake composite Dockerfiles to see if the Set-DotnetVersions script wi…

1feaa43

…ll finally stop complaining.

Generated and added the SHA's for the Composite Dockerfiles.

0c1ca68

Added placeholders for the composite images sizes in the ImageSize.ni…

a431f15

…ghtly.linux.json file.

Merge branch 'nightly' into containers-base

3e39eef

Added at least the template for the Aspnet Composite Container tests.…

7309915

… May be more than a template...

mthalman reviewed May 10, 2023

View reviewed changes

Does not work yet: Added base abstract class for Aspnet tests, both n…

bf383ad

…ormal and composite.

ivdiazsa added 3 commits May 15, 2023 12:01

Merge branch 'nightly' into containers-base

183b642

Updated Composite Dockerfiles with the most recent Nightly Branch

8b6d6f1

Undid the abstract class change because Xunit doesn't like it

a981e74

MichaelSimons closed this May 19, 2023

mthalman reopened this May 19, 2023

Added composite containers documentation.

99ee0ed

trylek approved these changes May 19, 2023

View reviewed changes

mthalman reviewed May 22, 2023

View reviewed changes

eng/readme-templates/Use.aspnet.md Outdated Show resolved Hide resolved

eng/readme-templates/Use.aspnet.md Outdated Show resolved Hide resolved

src/aspnet/8.0/alpine3.17-composite/amd64/Dockerfile Show resolved Hide resolved

MichaelSimons reviewed May 22, 2023

View reviewed changes

README.aspnet.md Outdated Show resolved Hide resolved

README.aspnet.md Outdated Show resolved Hide resolved

Rephrased and moved composites doc

51f7e31

MichaelSimons reviewed May 22, 2023

View reviewed changes

Made composites a subsection of About

5eabb6c

mthalman reviewed May 22, 2023

View reviewed changes

eng/readme-templates/About.aspnet.md Outdated Show resolved Hide resolved

Flow top-header to the composite sub-template

321434a

mthalman reviewed May 23, 2023

View reviewed changes

eng/readme-templates/About.aspnet.md Show resolved Hide resolved

Templates were wrong

d385abd

mthalman approved these changes May 23, 2023

View reviewed changes

MichaelSimons approved these changes May 23, 2023

View reviewed changes

lbussell approved these changes May 23, 2023

View reviewed changes

mthalman merged commit 4819bd1 into dotnet:nightly May 23, 2023

mthalman mentioned this pull request May 26, 2023

Duplicate matrix leg name from introduction of aspnet composite Dockerfiles dotnet/docker-tools#1141

Closed

ivdiazsa mentioned this pull request May 26, 2023

Publish Docker containers using optimized composite R2R images dotnet/runtime#85358

Closed

4 tasks

lbussell added a commit to lbussell/dotnet-docker that referenced this pull request Jun 9, 2023

Add Dockerfiles for Linux .NET Composite Images (dotnet#4594)

dc939e1

Co-authored-by: Logan Bussell <[email protected]>

ivdiazsa mentioned this pull request Jun 29, 2023

Enable Composite Docker Images for Jammy Chiseled and Mariner Distroless #4710

Merged


		This image contains the ASP.NET Core and .NET runtimes and libraries and is optimized for running ASP.NET Core apps in production.

		# Composite container images

[Fixed] - Add Dockerfiles for Linux .NET Composite Images #4594

[Fixed] - Add Dockerfiles for Linux .NET Composite Images #4594

Uh oh!

Conversation

ivdiazsa commented May 5, 2023

Uh oh!

ivdiazsa commented May 5, 2023

Uh oh!

MichaelSimons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivdiazsa commented May 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivdiazsa commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mthalman commented May 12, 2023

Uh oh!

ivdiazsa commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelSimons commented May 12, 2023

Uh oh!

mthalman commented May 15, 2023

Uh oh!

trylek commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mthalman commented May 16, 2023

Uh oh!

MichaelSimons commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelSimons commented May 19, 2023

Uh oh!

ivdiazsa commented May 19, 2023

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mthalman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mangod9 commented May 23, 2023

ivdiazsa commented May 11, 2023 •

edited

Loading

ivdiazsa commented May 12, 2023 •

edited

Loading

trylek commented May 15, 2023 •

edited

Loading

MichaelSimons commented May 19, 2023 •

edited

Loading