Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jan 20, 2024

Migrate to github.com/containerd/platforms module

Containerd's "platforms" package is being moved to a separate module. This
allows updating the platforms parsing independent of the containerd module
itself.

For existing containerd versions (1.6, 1.7), the package in containerd will
be an alias for the new module.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jan 20, 2024
@thaJeztah thaJeztah added this to the 26.0.0 milestone Jan 20, 2024
@thaJeztah thaJeztah self-assigned this Jan 20, 2024
@thaJeztah
Copy link
Member Author

Ah, looks like some updates are needed;

# github.com/docker/docker/libcontainerd/local
libcontainerd/local/local_windows.go:944:34: cannot use p[i].CreateTimestamp (variable of type time.Time) as *timestamppb.Timestamp value in struct literal

@thaJeztah
Copy link
Member Author

@thaJeztah
Copy link
Member Author

Two tests failing that look related, looks like there's some change in behavior in the package;

=== FAIL: github.com/docker/docker/integration/build TestBuildPlatformInvalid (0.06s)
    build_test.go:685: assertion failed: expression is false: errdefs.IsInvalidParameter(err)

Platform: "foobar",
})
assert.Assert(t, err != nil)
assert.ErrorContains(t, err, "unknown operating system or architecture")
assert.Assert(t, errdefs.IsInvalidParameter(err))

=== FAIL: amd64.integration.image TestImagePullPlatformInvalid (0.01s)
    pull_test.go:37: assertion failed: expression is false: errdefs.IsInvalidParameter(err)

_, err := client.ImagePull(ctx, "docker.io/library/hello-world:latest", types.ImagePullOptions{Platform: "foobar"})
assert.Assert(t, err != nil)
assert.ErrorContains(t, err, "unknown operating system or architecture")
assert.Assert(t, errdefs.IsInvalidParameter(err))

@thaJeztah
Copy link
Member Author

Improved the test-assert a bit, and it looks like it's returning a 500 / internal server error;

=== RUN   TestImagePullPlatformInvalid
    pull_test.go:37: assertion failed: invalid type for expected: func(error) error: errdefs.errSystem: Error response from daemon: "foobar": unknown operating system or architecture: invalid argument
--- FAIL: TestImagePullPlatformInvalid (0.01s)

@thaJeztah
Copy link
Member Author

Daemon logs show that it returns an untyped error, hence using the default (500 / Internal Server error);

time="2024-01-21T00:18:31.260074294Z" level=debug msg="Calling POST /v1.45/images/create?fromImage=hello-world&platform=foobar&tag=latest"
time="2024-01-21T00:18:31.260220544Z" level=debug msg="FIXME: Got an API for which error does not match any expected type!!!" error="\"foobar\": unknown operating system or architecture: invalid argument" error_type="*fmt.wrapError" module=api
time="2024-01-21T00:18:31.260320336Z" level=error msg="Handler for POST /v1.45/images/create returned error: \"foobar\": unknown operating system or architecture: invalid argument"
time="2024-01-21T00:18:31.260407169Z" level=debug msg="FIXME: Got an API for which error does not match any expected type!!!" error="\"foobar\": unknown operating system or architecture: invalid argument" error_type="*fmt.wrapError" module=api

@thaJeztah
Copy link
Member Author

Found the likely cause; the new module doesn't use containerd errdefs;

Screenshot 2024-01-21 at 01 25 19

@thaJeztah thaJeztah force-pushed the migrate_to_platforms_module branch 3 times, most recently from 0273a3c to 448d592 Compare January 23, 2024 09:29
@vvoland vvoland modified the milestones: 26.0.0, v-future Feb 26, 2024
@thaJeztah thaJeztah force-pushed the migrate_to_platforms_module branch from 448d592 to 4da9067 Compare May 26, 2024 11:46
@thaJeztah thaJeztah modified the milestones: v-future, 28.0.0 May 26, 2024
@thaJeztah thaJeztah force-pushed the migrate_to_platforms_module branch from 4da9067 to 42a1bd3 Compare May 26, 2024 14:39
@thaJeztah
Copy link
Member Author

Some error messages need to be updated in tests;

=== Failed
=== FAIL: amd64.integration.image TestImportWithCustomPlatformReject/_______ (0.00s)
    import_test.go:182: assertion failed: expected error to contain "is an invalid component", got "Error response from daemon: \"       \" is an invalid OS component of \"       \": OSAndVersion specifier component must match \"^([A-Za-z0-9_-]+)(?:\\\\(([A-Za-z0-9_.-]*)\\\\))?$\": invalid argument"
        Error response from daemon: "       " is an invalid OS component of "       ": OSAndVersion specifier component must match "^([A-Za-z0-9_-]+)(?:\\(([A-Za-z0-9_.-]*)\\))?$": invalid argument
    --- FAIL: TestImportWithCustomPlatformReject/_______ (0.00s)

=== FAIL: amd64.integration.image TestImportWithCustomPlatformReject// (0.00s)
    import_test.go:182: assertion failed: expected error to contain "is an invalid component", got "Error response from daemon: \"\" is an invalid OS component of \"/\": OSAndVersion specifier component must match \"^([A-Za-z0-9_-]+)(?:\\\\(([A-Za-z0-9_.-]*)\\\\))?$\": invalid argument"
        Error response from daemon: "" is an invalid OS component of "/": OSAndVersion specifier component must match "^([A-Za-z0-9_-]+)(?:\\(([A-Za-z0-9_.-]*)\\))?$": invalid argument
    --- FAIL: TestImportWithCustomPlatformReject// (0.00s)

=== FAIL: amd64.integration.image TestImportWithCustomPlatformReject (0.01s)

@thaJeztah thaJeztah force-pushed the migrate_to_platforms_module branch 2 times, most recently from 4ad5d69 to 649c049 Compare June 13, 2024 07:35
@thaJeztah
Copy link
Member Author

Yay! CodeCov action is having a bad day on Windows;

Screenshot 2024-06-13 at 12 34 25

thaJeztah added 2 commits July 2, 2024 21:19
Highlights

- Fix support for OTLP config
- Add API go module
- Remove overlayfs volatile option on temp mounts
- Update runc binary to v1.1.13
- Migrate platforms package to github.com/containerd/platforms
- Migrate reference/docker package to github.com/distribution/reference

Container Runtime Interface (CRI)

- Fix panic in NRI from nil CRI reference
- Fix Windows HPC working directory

full diff: containerd/containerd@v1.7.18...v1.7.19

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Switch to use github.com/containerd/platforms module, because containerd's
platforms package has moved to a separate module. This allows updating the
platforms parsing independent of the containerd module itself.

The package in containerd is deprecated, but kept as an alias to provide
compatibility between codebases.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the migrate_to_platforms_module branch from 6f9f5b9 to d0aa3ea Compare July 2, 2024 19:20
@thaJeztah thaJeztah changed the title [WIP] Migrate to github.com/containerd/platforms module vendor: github.com/containerd/containerd v1.7.19 ,migrate to github.com/containerd/platforms module Jul 2, 2024
@thaJeztah thaJeztah changed the title vendor: github.com/containerd/containerd v1.7.19 ,migrate to github.com/containerd/platforms module vendor: github.com/containerd/containerd v1.7.19, migrate to github.com/containerd/platforms module Jul 2, 2024
@thaJeztah thaJeztah marked this pull request as ready for review July 2, 2024 19:21
@thaJeztah thaJeztah requested a review from dmcgowan July 2, 2024 19:21
@thaJeztah
Copy link
Member Author

@vvoland @dmcgowan PTAL

@AkihiroSuda AkihiroSuda merged commit 1205a90 into moby:master Jul 4, 2024
@thaJeztah thaJeztah deleted the migrate_to_platforms_module branch July 4, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

3 participants