Add api Go module and move all protos under api #10151
Conversation
|
Skipping CI for Draft Pull Request. |
dmcgowan
force-pushed
the
move-proto-to-api
branch
from
7eac706
to
0095cb8
Compare
|
|
dmcgowan
changed the title
|
/retest |
Could you document this in |
dmcgowan
force-pushed
the
move-proto-to-api
branch
from
0095cb8
to
bb5759c
Compare
|
@AkihiroSuda good point , updated the release doc and think it is much clearer this way. |
There was a problem hiding this comment.
LGTM
Have one question though. In commit 3a03f02, where the runc/options are moved to api/types, we no longer have the information that the options are for v2 runtime, since the package name is different (core/runtime/v2/runc/options -> api/types/runc/options). Is that ok?
There was a problem hiding this comment.
From a discussion on Slack there was a mention of removing the api code when creating release branches (to prevent confusion; as code in those directories is not used); is that something we need to document / decide on (and to include in the release checklist)?
| `github.com/containerd/containerd/api` Go package and imported separately from the | ||
| rest of containerd. | ||
|
|
||
| The API minor version will continue to be incremented for each major and minor |
There was a problem hiding this comment.
I guess this depends on whether there were changes in the package / module? (currently it reads as it will always be incremented, which may not always be needed)
There was a problem hiding this comment.
(basically; versioning should depend on the changes in the api module itself to follow SemVer, which could mean that only a patch release is needed between containerd releases, and versions can be updated separate from containerd releases; order of releasing may still require the API to be tagged before doing a containerd release of course)
There was a problem hiding this comment.
I'm sure there will always be changes, we can change the doc if a situation comes up later that doesn't make sense.
It was an idea to remove, but I don't think that will be necessary. There are a few different ways we could handle the versioning, I think the cleanest will be just to not do patch releases for prior minor versions though. The logic is that the highest level API needs to be supported anyway and should not break any compatibility. The whole issue that made this change necessary is related to multiple Go packages in use and in that same scenario the API version will go up. I feel like we have a few different ways we could approach the versioning though and this change gives us more options than we have without it.
| github.com/opencontainers/image-spec v1.1.0 | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20240401170217-c3f982113cda | ||
| google.golang.org/grpc v1.63.2 | ||
| google.golang.org/protobuf v1.33.0 |
There was a problem hiding this comment.
Only thing worth considering is to see if those versions are required to be "current" version; some release branches may be on older image-spec and/or grpc; the API module should define minimum required version so that those branches can use the version they need (usually higher than minimum required version)
| @@ -140,3 +141,6 @@ require ( | |||
| sigs.k8s.io/yaml v1.3.0 // indirect | |||
| tags.cncf.io/container-device-interface/specs-go v0.7.0 // indirect | |||
| ) | |||
|
|
|||
| // Use the relative local source of the github.com/containerd/containerd/api to build | |||
| replace github.com/containerd/containerd/api => ./api | |||
There was a problem hiding this comment.
We must be sure to remove this one in release branches (but somewhat related to removing the directory in those)
Would it also be worth having a CI job to do some minimal testing for the actual version specified in go.mod (once there are releases?) because that's the version "others" will see as being used. I guess less relevant for main (assuming external consumers shouldn't use main / unreleased
There was a problem hiding this comment.
Alternatively, configure a Go Workspace and commit the go.work file. I suspect it'd be easier to remember to delete a file in the release branches than to edit one.
Would it also be worth having a CI job to do some minimal testing for the actual version specified in go.mod (once there are releases?) because that's the version "others" will see as being used.
With testing only on release branches, only tagged releases of containerd would be (somewhat) guaranteed to be importable as a module. Downstream consumers of the containerd client might not be able to try out the HEAD version of the module, e.g. for early integration/acceptance testing with downstream projects, unless the minimum api dependency version is always kept up to date with the actual source code requirements. I think that CI, including PR builds, should always build and run tests using the minimum versions of the dependencies a downstream importer might consume. (Read: CI should test containerd PRs against the minimum version of the api module required in containerd's go.mod.) This would enforce a workflow where api module changes would always have to be committed, and the api dependency bumped, before the changes could be used from the containerd module. I think that enforcing such a strict workflow would be to the benefit of containerd development as it ensures that the github.com/containerd/containerd/v2@main module is always importable.
With a committed go.work, CI could run builds/tests with GOWORK=off in the environment so the file is ignored. Alternatively, .gitignore the go.work path and provide a script/template for containerd developers to quickly get a local workspace configured. You never have to remember to delete the go.work file in release branches if it is never committed in the first place. The go.mod equivalent to either approach is more awkward and error-prone: you can't .gitignore a portion of a file, and CI would have to go mod edit the replace directive away to test PRs how external importers would see them.
There was a problem hiding this comment.
I like the idea of go.work but found it awkward to use when I first tried it out. If it is not committed, how would you be able to have a pull request for an api change and its implementation in the same PR?
There was a problem hiding this comment.
One idea, untested: git push -u my-fork my-pr-branch && go get github.com/containerd/containerd/api@"$(git show --pretty='%H')". Or perhaps some offline go mod edit command could also work. As commits pushed to a fork of a GitHub repo are reachable from the main repo by commit hash, any commit pushed to a PR branch on GitHub is also a valid module pseudo-version.
There was a problem hiding this comment.
It probably isn't an issue for the replace to just come and go, after we tag the version and update, we remove the replace. When we add a change and the replace isn't there, we re-add it until it gets updated back to a tag. We shouldn't need it as a part of any tagged and can add that to the release checklist.
dmcgowan
force-pushed
the
move-proto-to-api
branch
from
bb5759c
to
17ca23e
Compare
|
Updating using minimum version of grpc as currently used in 1.6. Based on the way version resolution in go.mod works, this won't have any effect on containerd builds, but will prevent importers from being forced up. |
| github.com/containerd/ttrpc v1.2.3 | ||
| github.com/containerd/typeurl/v2 v2.1.1 | ||
| github.com/opencontainers/image-spec v1.1.0 | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20240401170217-c3f982113cda |
There was a problem hiding this comment.
The verions of dependencies in the root go.mod and in the api should be same so it dont cause conflicts later. Currently its at google.golang.org/genproto/googleapis/rpc v0.0.0-20240415180920-8c6c420018be https://github.com/containerd/containerd/pull/10151/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R74
There was a problem hiding this comment.
This will not change the version in the main repository, the go.mod only needs to specify a minimum version. Since API isn't compiled directly, the version isn't too important here. I think it would only be an issue for API to be ahead of the main repository, but also don't want it to be ahead of 1.6/1.7 since we intend to backport there.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Allow the api to stay at the same v1 go package name and keep using a 1.x version number. This indicates the API is still at 1.x and allows sharing proto types with containerd 1.6 and 1.7 releases. Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
move-proto-to-api
branch
from
17ca23e
to
171fc14
Compare
|
/retest |
|
Rebased after sandbox api change |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Currently in the v2 rc, the api packages have the new v2 name which are different packages from the api packages in 1.x. Both the 1.x and 2.x packages can be imported by downstream clients and a proto registration error can easily occur since the protos are the same name and version between containerd 1.x and 2.x. While ideally all importers can completely switch to 2.x or stay on 1.x, this is not so easy since any transitive dependency may end up using an API type from the 1.x package. This makes the 2.x upgrade path much more difficult as all dependencies must switch at the same time. However, since containerd 2.x is not a breaking API change, we expect that 1.x API clients to be able to communicate with a 2.x daemon.
This change
After this I propose we restart the API versioning at 1.0.0 to be used by containerd 1.x and 2.x. API versions will be tagged separately from main releases but we can bump the minor api version after each containerd major/minor release.