dotnet / dotnet-docker

Docker images for .NET and the .NET Tools.
https://hub.docker.com/_/microsoft-dotnet
MIT License
4.47k stars 1.94k forks source link

Consider an icu version of `aot-deps` #4804

Closed richlander closed 1 year ago

richlander commented 1 year ago

This recent issue got me thinking that we should consider providing an icu version of aot-deps and maybe even runtime-deps.

https://github.com/dotnet/core/issues/8663

Today, we don't provide an ICU version of Alpine since we'd need two duplicate branches of images. This policy makes sense. When we think solely of self-contained, which all native aot apps are, then having an ICU image makes a lot more sense and avoids the need for the chisel tool. I see the cost/benefit tradeoff for ICU images being quite different (and more compelling) for aot-deps and consider same for runtime-deps (maybe later).

mthalman commented 1 year ago

[Triage]

jkotas commented 1 year ago

ICU is written in C++ and brings in dependency on C++ runtime.

Is the C++ runtime the only thing that differentiates between aot-deps and runtime-deps? If it is the case, aot-deps w/ ICU is going to be identical to runtime-deps w/ ICU.

richlander commented 1 year ago

Great observation.

See runtime-deps:chiseled -> https://github.com/dotnet/dotnet-docker/blob/main/src/runtime-deps/8.0/jammy-chiseled/amd64/Dockerfile

You can see that we went the "Alpine route" and removed (didn't add) ICU.

I am now thinking that we should do the following:

This will have a couple advantages:

We made this ICU decision before .NET 7 shipped. It was an obvious decision at the time and then we forgot about it and didn't revisit it. W/more information available, this seems like a better plan. If folks want CoreCLR + smaller, then got aot-deps + self-contained + trimming.

Works?

MichaelSimons commented 1 year ago

Works?

What percentage of apps can utilize Native AOT? I has the understanding that there are feature gaps that prevent all apps from using it.

richlander commented 1 year ago

Good question, but that's not what I meant.

Much higher number of users can switch to chiseled (in CoreCLR land) from regular Ubuntu w/no fanfare.

I meant that our goal is to enable a high % of apps that are currently targeting Ubuntu or Debian (with CoreCLR) to switch to our chiseled images, still with CoreCLR. Adding ICU back will help with that. It also means we don't have to document take-backs. We never really discussed that. To be clear, Alpine users can switch to, but that's outside this discussion.

MichaelSimons commented 1 year ago

Your proposal specifically calls out Chiseled images as the goal is to promote migration from "full/regular" Ubuntu/Debian. Does it include adding ICU to the alpine and mariner runtime-deps as well?

richlander commented 1 year ago

I'd propose that we follow the same model for Mariner as I'm proposing for Ubuntu. It enables all the scenarios.

I'd leave Alpine as-is until we fund Alpine distroless and then reconsider.

richlander commented 1 year ago

Sorry, I got this a bit wrong. It's a little more complicated than I thought.

To enable all the scenarios, we need three not two deps images:

The first two can go into runtime-deps and the last into aot-deps.

I assume the sdk-container-builds component can inference all three of these.

/cc @baronfel

MichaelSimons commented 1 year ago

@richlander, your last comments are not clear to me. You enumerated three scenarios and called out the need for three images yet said the first two scenarios can be covered by runtime-deps. Can you please clarify?

mthalman commented 1 year ago

@richlander, your last comments are not clear to me. You enumerated three scenarios and called out the need for three images yet said the first two scenarios can be covered by runtime-deps. Can you please clarify?

I think @richlander left out the "ICU + no stdc++ -- Native AOT + ICU" scenario which would be the third image: aot-icu-deps

richlander commented 1 year ago

What I meant is that the first two scenarios apply to CoreCLR, so make sense in runtime-deps (as a repo). The last scenario doesn't apply to CoreCLR so doesn't below in runtime-deps.

ICU + no stdc++

There is no such scenario.

ICU is written in C++ and brings in dependency on C++ runtime.

That's what Jan said.

mthalman commented 1 year ago

ICU + no stdc++

There is no such scenario.

Ok, I understand that now.

To enable all the scenarios, we need three not two deps images:

I'm still confused. What are the 3 images? Can you refer to actual names rather than scenario descriptions?

richlander commented 1 year ago

For sure.

Same order as above.

There could be some discussion about whether the default chiseled should contain ICU or not. I think we should probably invert it. I started with this scheme to make it easier to talk about. Once we get past this piece (agreeing on the three images), we can talk about whether including ICU should be the default.

richlander commented 1 year ago

I changed my mind. I think the tag scheme above is best. It's simple and accurately descriptive.

The key thing we want is that aspnet and runtime images are built on the image with ICU to enable low-friction adoption of chiseled images.

Our story should be: "Chiseled images give you low size, low CVEs, glibc, and ICU. And if you want to go even smaller (with some more effort), we've got good options for that."

That means that runtime and aspnet tags would be 8.0-jammy-chiseled-icu.

MichaelSimons commented 1 year ago

I see a few potential UX issues with the proposal that I would like to raise and discuss.

  1. Will users interpret non-icu tagged images to mean they do not include icu? This would be true for 8.0-jammy-chiseled but not for jammy or 8.0-bookworm-slim
  2. Alpine users have asked for icu to be included in the past. We would now be providing it for other variants but not alpine.
  3. aot-deps vs runtime-deps - how will users know what to use? If I am using native aot I may think I want aot-deps but if I want icu, I user runtime-deps. Is there a scheme that would be more self-describing?

It all comes back to how incredibly hard tagging can be :)

richlander commented 1 year ago

One option is that we could dual tag (appropriate) images in both aot-deps and runtime-deps. That would help.

We have a couple options:

In my ideal world, we do the following:

Given the popularity of Alpine, I don't think we can do this, at least as acutely as the wording suggests. We'd need a release were we produce both Alpine variants (for fx-dependent). That said, I think this model is the right one. Also, ASP.NET Core doesn't yet the trimming story that this model would demand.

richlander commented 1 year ago

Let's try some options.

ICU not included by default

This aligns closest with status quo.

First and third images are the same (re-tagged).

We'd then also have:

ICU included by default

Note: -min is proposed as a (descriptive) placeholder.

First and third images are the same (re-tagged).

We'd then also have:

richlander commented 1 year ago

He's a concrete proposal.

aot-deps

ICU is not installed by default. All of these images are on the distroless plan, even Alpine.

Note: ICU images are just re-tags between aot-deps and runtime-deps and only differ by repo names (tags are identical).

runtime-deps

ICU is installed by default, except for distroless and Alpine (for historical reasons).

runtime

ICU is installed by default, except for Alpine. ASP.NET layer is the same.

richlander commented 1 year ago

It would be good if we could get a firm size diff between:

and also:

MichaelSimons commented 1 year ago
  1. Is aot-deps necessary? It's feeling more like a variant of runtime-deps now that there is a common image in runtime-deps and aot-deps.
  2. Are we boxing ourselves in by using the term icu rather than a more abstract term. Are there other components we would consider adding in this more full featured variant.

runtime-deps:8.0-alpine runtime-deps:8.0-alpine-aot runtime-deps:8.0-alpine-full runtime-deps:8.0-bookworm-slim runtime-deps:8.0-cbl-mariner2.0-distroless runtime-deps:8.0-cbl-mariner2.0-distroless-aot runtime-deps:8.0-cbl-mariner2.0-distroless-full runtime-deps:8.0-jammy runtime-deps:8.0-jammy-chiseled runtime-deps:8.0-jammy-chiseled-aot runtime-deps:8.0-jammy-chiseled-full

full is a placeholder for a TBD term

richlander commented 1 year ago

I like aot-deps since it will match aot-sdk. However, that's not a big deal. Your approach works just as well.

However, I don't think we want to position AOT as having a runtime in the same way as CoreCLR.

MichaelSimons commented 1 year ago

However, I don't think we want to position AOT as having a runtime in the same way as CoreCLR.

I get what you are saying but to me they are logically the same type of image and that is evident when you inspect the contents and the fact that aots-deps and runtime-deps have a shared image. Just rename runtime-deps if you don't like the implications runtime has.

richlander commented 1 year ago

aot-deps has two images, one of which is in common with runtime-deps. The one unique to aot-deps is the default one. That said, if and when we enable a web pages scenario for ASP.NET Core, then I suspect we'd make the other one the default.

runtime-deps is fully a good name in CoreCLR land.

richlander commented 1 year ago

On further thought, I think just runtime-deps is better. Simpler and less to explain.

That leaves just coming up with the terms.

I think I like extra best since it is the most descriptive. Unless we install ldap and quic, then the images are not full or all. extra is more not full but curated.

We could also add Kerberos back in extra. https://github.com/dotnet/dotnet-docker/issues/4751. Although I'm not sure that's super necessary.

Yes, I wasted 10 mins with a thesaurus.

mthalman commented 1 year ago

We should follow the streaming TV naming and go with plus. I'm only half kidding.

Going back to the name of runtime-deps, what about naming that just dotnet-deps? That may be redundant with the dotnet portion of the repo name path but I don't like just deps either.

I'll weigh in on the runtime-deps and aot-deps being the same image for some tags. We've never shared an image across repos before. Our infrastructure may or may not support that. There could be risk there.

richlander commented 1 year ago

extras or plus are equally good, IMO.

Some thoughts:

Here's a use of "plus", not in tag but product: https://hub.docker.com/r/nginx/nginx-ingress

Here's a use of "extra": https://hub.docker.com/r/pandoc/extra

Separately, if we create such an image, we should consider adding tzdata. It is a drop in the bucket compared to ICU.

https://github.com/dotnet/dotnet-docker/issues/4748#issuecomment-1675288023

We have a much broader story than before with trimming and these fancy deps images. I'd love to see us move more towards a "batteries included" model for our runtime images. However, this still needs more discussion. I might write something up.

There could be risk there.

That's only a risk if we try to use the same Dockerfile. Right?

mthalman commented 1 year ago

That's only a risk if we try to use the same Dockerfile. Right?

Unknown. It will still be one image in separate repos. Not sure if that would have an impact or not.

MichaelSimons commented 1 year ago

On further thought, I think just runtime-deps is better. Simpler and less to explain.

@richlander - can you clarify what you mean? Are you referring to a single "deps" repo versus separate runtime-deps and aot-deps? Or were you referring to liking the name runtime-deps versus renaming it to something else?

MichaelSimons commented 1 year ago

Summary of a discussion on this topic in our dotnet-docker weekly standup.

  1. Single "deps" repo - We will not have separate aot-deps and runtime-deps repos as proposed earlier. They have too much overlap and would be difficult to describe. We will not rename runtime-deps to something more abstract. There is not enough ROI to justify the disruption this would cause.
  2. All non-distroless runtime-deps images will carry icu and tzdata. The exception is Alpine. Alpine will be considered part of our "lightweight" images until the point we add alpine-distroless images.
  3. The distroless runtime-deps variants will be {default}/aot/extra.
  4. The runtime/aspnet distroless images will continue to be opinionated on globalization. They will not include icu and tzdata until there is enough feedback to justify a change. We could add an extra variant at any time or possibly change the base image to be extra (only during a major release).
  5. SDK images will include icu and tzdata by default (current policy)
  6. A new aot sdk variant will be added.
richlander commented 1 year ago

We shipped this, in the nightly branch.