Remove containerd "platform" dependency from client #42623

thaJeztah · 2021-07-12T10:41:26Z

Note: once / if #42464 is merged/accepted, we should consider replacing github.com/opencontainers/image-spec/specs-go/v1.Platform with our own ImagePlatform, to remove the OCI spec from the API (if possible). If we do so, we can also remove the formatPlatform function here (as it would be provided by ImagePlatform.String()

This removes some of the containerd dependencies from the client:

client: remove unused Platform field from configWrapper
This field was added in 7a9cb29, but appears to be unused, so removing it.
client: remove containerd "platform" dependency

After this, there's no direct dependency on containerd, but the errdefs package still depends on containerd's errdefs. package. I'll have a look if we can refactor so that that doesn't end up in the client package.

thaJeztah · 2021-07-12T10:45:46Z

@cpuguy83 @dims @AkihiroSuda PTAL

dims · 2021-07-12T11:14:32Z

LGTM @thaJeztah thanks!

thaJeztah · 2021-07-12T11:58:33Z

Added the cherry-pick label; I tried to keep this diff small/localised so that we are able to cherry-pick (helps kubernetes / cAdvisor)

thaJeztah · 2021-07-12T12:00:42Z

client/container_create.go

+// instead of "unknown" if no OS is present: https://github.com/containerd/containerd/blob/v1.5.2/platforms/platforms.go#L243-L263
+func formatPlatform(platform *specs.Platform) string {
+ if platform == nil || platform.OS == "" {
+ return ""


Let me call out here that containerd's format would return "unknown" in this case. As far as I can see, we have no use for the "unknown" value, and would treat it equivalent to "" / not set, so I went for "don't set the parameter if we don't have anything useful". but perhaps I overlooked a specific scenario 👀 👀 👀 👀

So, buildkit supports just setting arch... weird that we'd return empty for empty OS.

Any thoughts on this?

Looked up the parsing on the daemon side, and looks indeed that that should be supported; if only arch is specified, the parsing on the daemon side should handle that;

moby/vendor/github.com/containerd/containerd/platforms/platforms.go

Lines 182 to 206 in ada51d6

case 1:

// in this case, we will test that the value might be an OS, then look

// it up. If it is not known, we'll treat it as an architecture. Since

// we have very little information about the platform here, we are

// going to be a little more strict if we don't know about the argument

// value.

p.OS = normalizeOS(parts[0])

if isKnownOS(p.OS) {

// picks a default architecture

p.Architecture = runtime.GOARCH

if p.Architecture == "arm" && cpuVariant() != "v7" {

p.Variant = cpuVariant()

}

return p, nil

}

p.Architecture, p.Variant = normalizeArch(parts[0], "")

if p.Architecture == "arm" && p.Variant == "v7" {

p.Variant = ""

}

if isKnownArch(p.Architecture) {

p.OS = runtime.GOOS

return p, nil

}

tonistiigi · 2021-07-13T18:09:08Z

Why are we duplicating this code? Same for #42464 . If for example windows os-version starts to get encoded in the string value(there is an issue for that somewhere) there are two incompatible implementations. I don't see an issue with depending on the specs repo.

cpuguy83 · 2021-07-13T18:11:11Z

client/container_create.go

+//
+// Similar to containerd's platforms.Format(), but returns an empty string
+// instead of "unknown" if no OS is present: https://github.com/containerd/containerd/blob/v1.5.2/platforms/platforms.go#L243-L263
+func formatPlatform(platform *specs.Platform) string {


Seems like we really want path.Join(platform.OS, platform.Arch....)
This will handle empty values as desired, even for empty values.
https://play.golang.org/p/ztICsyudOrB

Not sure if you want to validate that is always arch is set if variant is also set.

Oh, nice! TIL path.Join skips the empty values (I assumed it would keep those in). Yes, I like that.

Not sure if you want to validate that is always arch is set if variant is also set.

Yeah, perhaps stop at the first empty value (in which case path.Join would not be needed 🤔). We could keep it "simple" (for now), and just let the daemon handle this; wdyt?

I was writing down notes on serializing / deserialising and normalising of oci.Platform (to start a discussion on the OCI spec), because currently it looks like there's no standard approach anywhere.

So add to the "silliness"; I was looking at the CLI code YesterdayI: https://github.com/docker/cli/blob/v20.10.7/cli/command/container/create.go#L244-L249

if opts.platform != "" && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.41") { p, err := platforms.Parse(opts.platform) if err != nil { return nil, errors.Wrap(err, "error parsing specified platform") } platform = &p

So, we:

pass a string representation of platform (through the --platform flag)

parse it on the CLI to convert it to an oci.Platform{}

pass it to the client as argument

convert it to a string

send it over the API as a string

parse the string and convert it to an oci.Platform{}

pass it to the backend as argument

Looking at that, I think we should consider to either have the client (and cli) to not bother with oci.Platform{} at all (pass a string, and let the daemon deal with it), or make the API accept a oci.Platform{} (or equivalent) in the request.

thaJeztah · 2021-07-15T17:48:52Z

@cpuguy83 updated to use path.Join()

thaJeztah · 2021-07-26T16:40:29Z

@cpuguy83 @tianon @tonistiigi ptal

This field was added in 7a9cb29, but appears to be unused, so removing it. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2021-07-29T21:31:23Z

@tianon @cpuguy83 updated, as discussed. PTAL

Signed-off-by: Sebastiaan van Stijn <[email protected]>

cpuguy83

LGTM

dims · 2021-07-29T22:52:39Z

LGTM ... is that an unrelated flake in the failing test?

[2021-07-29T21:45:41.311Z] === FAIL: github.com/docker/docker/libnetwork/networkdb TestNetworkDBNodeJoinLeaveIteration (5.12s)
[2021-07-29T21:45:41.311Z]     networkdb_test.go:511: Network existence verification failed
[2021-07-29T21:45:41.311Z]     networkdb_test.go:513: The networkNodes list has to have be 2 instead of 1 - [cb9502840039]

thaJeztah · 2021-07-29T22:56:25Z

Yes, should be unrelated (but looks like it's become more flaky); #42698

tianon · 2021-07-29T23:25:11Z

Generally seems OK to me, but I am concerned that this particular code path probably doesn't have very much coverage in any of our tests and this formatting is slightly different from what containerd does (so I guess there's some regression potential here?). 😕

Not sure there's a good solution to this, though.

thaJeztah · 2021-07-30T14:45:45Z

Generally seems OK to me, but I am concerned that this particular code path probably doesn't have very much coverage in any of our tests and this formatting is slightly different from what containerd does

So, looking at the differences again, I think the only difference is that containerd returns unknown for an empty OS, and we create an empty string ("no platform passed").

The "unknown" case would produce an error if sent to the daemon (platforms.Parse() will fail;

package main

import (
        "fmt"
        "github.com/containerd/containerd/platforms"
)

func main() {
        p, err := platforms.Parse("unknown")
        if err != nil {
                fmt.Println(err)
        }
        fmt.Println(platforms.Format(p))

        p, err = platforms.Parse("armhf")
        if err != nil {
                fmt.Println(err)
        }
        fmt.Println(platforms.Format(p))
}

go run main.go
"unknown": unknown operating system or architecture: invalid argument
unknown
linux/arm

However (see #42623 (comment)), in the docker cli at least, the string as passed by the user is already parsed with platforms.Parse(), which means that (currently) OS will always be propagated, even if omitted by the user, and invalid values would already error before we reach this code (not taking other consumers into account).

This brings of course question: "should it?" (that's what my comment above was about: #42623 (comment)). Because currently, omitting the OS will result in the OS of the cli to be used as default 😂;

$ docker run --rm --platform=arm64 hello-world
Unable to find image 'hello-world:latest' locally
latest: Pulling from library/hello-world
109db8fad215: Pull complete
Digest: sha256:df5f5184104426b65967e016ff2ac0bfcd44ad7899ca3bbcf8e44e4461491a9e
Status: Downloaded newer image for hello-world:latest
docker: Error response from daemon: image with reference hello-world was found but does not match the specified platform: wanted darwin/arm64, actual: linux/arm64/v8.
See 'docker run --help'.

Something to look at in a follow-up; I think we should consider passing what's provided by the user as-is. The daemon (API) will produce an error if it's invalid, and the daemon can (if needed) fill in defaults.

tianon · 2021-07-30T17:52:32Z

Fair enough; SGTM

dims · 2021-07-31T16:57:46Z

YESSS!!! thanks @thaJeztah

thaJeztah added area/api status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jul 12, 2021

thaJeztah changed the title ~~Remove containerd from client~~ Remove containerd "platform" dependency from client Jul 12, 2021

thaJeztah mentioned this pull request Jul 12, 2021

errdefs: remove dependency on grpc, containerd, and distribution #42624

Closed

thaJeztah added the process/cherry-pick label Jul 12, 2021

thaJeztah commented Jul 12, 2021

View reviewed changes

thaJeztah added this to the 21.xx milestone Jul 13, 2021

cpuguy83 requested changes Jul 13, 2021

View reviewed changes

thaJeztah force-pushed the remove_containerd_from_client branch from db3e1e0 to 33a0513 Compare July 15, 2021 17:42

dims mentioned this pull request Jul 27, 2021

Depend only on containerd API and avoid vendoring unnecessary things from containerd google/cadvisor#2908

Merged

client: remove unused Platform field from configWrapper

b33f3c7

This field was added in 7a9cb29, but appears to be unused, so removing it. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the remove_containerd_from_client branch from 33a0513 to 9a27002 Compare July 29, 2021 21:30

client: remove containerd "platform" dependency

5f0703c

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the remove_containerd_from_client branch from 9a27002 to 5f0703c Compare July 29, 2021 21:32

cpuguy83 approved these changes Jul 29, 2021

View reviewed changes

thaJeztah merged commit 0b39cc2 into moby:master Jul 31, 2021

thaJeztah deleted the remove_containerd_from_client branch July 31, 2021 16:45

thaJeztah mentioned this pull request Mar 21, 2022

errdefs: move GetHTTPErrorStatusCode to api/server/httpstatus #43399

Merged

dims mentioned this pull request Mar 22, 2022

[20.10] Remove containerd "platform" dependency from client #43407

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Mar 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove containerd "platform" dependency from client #42623

Remove containerd "platform" dependency from client #42623

thaJeztah commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

dims commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

thaJeztah Jul 12, 2021

cpuguy83 Jul 15, 2021

cpuguy83 Jul 26, 2021

thaJeztah Jul 29, 2021

tonistiigi commented Jul 13, 2021

cpuguy83 Jul 13, 2021

thaJeztah Jul 13, 2021

thaJeztah commented Jul 15, 2021

thaJeztah commented Jul 26, 2021

thaJeztah commented Jul 29, 2021

cpuguy83 left a comment

dims commented Jul 29, 2021

thaJeztah commented Jul 29, 2021

tianon commented Jul 29, 2021

thaJeztah commented Jul 30, 2021

tianon commented Jul 30, 2021 via email

dims commented Jul 31, 2021

	case 1:
	// in this case, we will test that the value might be an OS, then look
	// it up. If it is not known, we'll treat it as an architecture. Since
	// we have very little information about the platform here, we are
	// going to be a little more strict if we don't know about the argument
	// value.
	p.OS = normalizeOS(parts[0])
	if isKnownOS(p.OS) {
	// picks a default architecture
	p.Architecture = runtime.GOARCH
	if p.Architecture == "arm" && cpuVariant() != "v7" {
	p.Variant = cpuVariant()
	}

	return p, nil
	}

	p.Architecture, p.Variant = normalizeArch(parts[0], "")
	if p.Architecture == "arm" && p.Variant == "v7" {
	p.Variant = ""
	}
	if isKnownArch(p.Architecture) {
	p.OS = runtime.GOOS
	return p, nil
	}

Remove containerd "platform" dependency from client #42623

Remove containerd "platform" dependency from client #42623

Conversation

thaJeztah commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

dims commented Jul 12, 2021

thaJeztah commented Jul 12, 2021

thaJeztah Jul 12, 2021

Choose a reason for hiding this comment

cpuguy83 Jul 15, 2021

Choose a reason for hiding this comment

cpuguy83 Jul 26, 2021

Choose a reason for hiding this comment

thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

tonistiigi commented Jul 13, 2021

cpuguy83 Jul 13, 2021

Choose a reason for hiding this comment

thaJeztah Jul 13, 2021

Choose a reason for hiding this comment

thaJeztah commented Jul 15, 2021

thaJeztah commented Jul 26, 2021

thaJeztah commented Jul 29, 2021

cpuguy83 left a comment

Choose a reason for hiding this comment

dims commented Jul 29, 2021

thaJeztah commented Jul 29, 2021

tianon commented Jul 29, 2021

thaJeztah commented Jul 30, 2021

tianon commented Jul 30, 2021 via email

dims commented Jul 31, 2021