Skip to content

Remove containerd "platform" dependency from client #42623

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.

Sign up for GitHub

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

Merged
thaJeztah merged 2 commits into from
Jul 31, 2021
Merged

Remove containerd "platform" dependency from client #42623

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b33f3c7
client: remove unused Platform field from configWrapper
thaJeztah Jul 12, 2021
5f0703c
client: remove containerd "platform" dependency
thaJeztah Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions client/container_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import (
"context"
"encoding/json"
"net/url"
"path"

"github.com/containerd/containerd/platforms"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/network"
"github.com/docker/docker/api/types/versions"
Expand All @@ -16,7 +16,6 @@ type configWrapper struct {
*container.Config
HostConfig *container.HostConfig
NetworkingConfig *network.NetworkingConfig
Platform *specs.Platform
}

// ContainerCreate creates a new container based on the given configuration.
Expand All @@ -38,8 +37,8 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config
}

query := url.Values{}
if platform != nil {
query.Set("platform", platforms.Format(*platform))
if p := formatPlatform(platform); p != "" {
query.Set("platform", p)
}

if containerName != "" {
Expand All @@ -61,3 +60,15 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config
err = json.NewDecoder(serverResp.body).Decode(&response)
return response, err
}

// formatPlatform returns a formatted string representing platform (e.g. linux/arm/v7).
//
// Similar to containerd's platforms.Format(), but does allow components to be
// omitted (e.g. pass "architecture" only, without "os":
// https://github.com/containerd/containerd/blob/v1.5.2/platforms/platforms.go#L243-L263
func formatPlatform(platform *specs.Platform) string {
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Jul 13, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jul 13, 2021

Choose a reason for hiding this comment

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

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:

  1. pass a string representation of platform (through the --platform flag)
  2. parse it on the CLI to convert it to an oci.Platform{}
  3. pass it to the client as argument
  4. convert it to a string
  5. send it over the API as a string
  6. parse the string and convert it to an oci.Platform{}
  7. 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.

if platform == nil {
return ""
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jul 12, 2021

Choose a reason for hiding this comment

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

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 👀 👀 👀 👀

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Jul 15, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Jul 26, 2021

Choose a reason for hiding this comment

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

Any thoughts on this?

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

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

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
}

}
return path.Join(platform.OS, platform.Architecture, platform.Variant)
}