Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[release/1.7] Revert errdefs package #10712

Merged
merged 6 commits into from
Sep 23, 2024

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Sep 20, 2024

Switching to the errdefs package in 1.7 is a breaking change since the new errdefs package was not stabilized. We can consider moving to the errdefs package once it is marked as 1.0 after the containerd 2.0 release.

As part of the changes in errdefs, we made sure that even if we don't have exact type compatibility, we can rely on interface compability. This will ensure that any version of the errdefs package or any similar errdefs package which uses the same interfaces will have error matches. Switching this in 1.7 was premature and unnecessary considering we can get compatibility with the interface checks. This has now been reverted and the error definitions synchronized with errdefs to ensure the interfaces are included.

This is currently causing some 1.7 builds to break after version resolution and is a blocker for 2.0 since it prevents us from moving up the errdefs package due to a circular dependency. This dependency could also be solved by bumping 1.7 to errdefs 0.2.0, but that is introducing another breaking change to 1.7 for a non-1.0 package.

This reverts commit f5ce2f2.

Signed-off-by: Derek McGowan <derek@mcg.dev>
This reverts commit 136e1b7.

Signed-off-by: Derek McGowan <derek@mcg.dev>
This reverts commit c7d5e43.

Signed-off-by: Derek McGowan <derek@mcg.dev>
This reverts commit 308341a.

Signed-off-by: Derek McGowan <derek@mcg.dev>
This reverts commit 47ff8cf.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Since errdefs has been updated to also rely on interface compatibility,
there is no need to have a breaking errdefs package change in 1.7.

Signed-off-by: Derek McGowan <derek@mcg.dev>
@thaJeztah
Copy link
Member

Ugh; this will be a painful one. Wasn't the errdefs package specifically MEANT so that errors between v2 and v1 could be handled? At least this will be a painful one with moby <--> buildkit <--> containerd

@dmcgowan
Copy link
Member Author

Wasn't the errdefs package specifically MEANT so that errors between v2 and v1 could be handled? At least this will be a painful one with moby <--> buildkit <--> containerd

Yes, but errdefs 0.1.0 didn't solve that and can makes the situation more difficult since the package isn't 1.0 and not stable. Also buildkit and moby haven't adopted the errdefs package for their definitions and errdefs.Is functions will still work with this change. The interface approach is more reliable for now and what we already do for compatibility with Moby errors. This change still ensures errors will be handled between v1 and v2.

@thaJeztah
Copy link
Member

Yeah, mostly concerned that code consumed from BuildKit will match different errors than Moby code if only one will update, and updating only the containerd dependency likely means that none of the cerrdefs.Is(...) will match anything

git describe --tags --match="v[0-9]*" HEAD
v0.16.0

git grep '"github.com/containerd/errdefs"' | wc -l
      97
git describe --tags --match="v[0-9]*" HEAD
v27.3.0
git grep '"github.com/containerd/errdefs"' | wc -l
     126
git grep 'cerrdefs.Is' | wc -l
      90

@dmcgowan
Copy link
Member Author

BuildKit is doing its own thing with error handling and not directly handling errors from containerd yet, so that is not a concern yet. There is definitely still ongoing work in both BuildKit and Moby to get the error handling unified, but we shouldn't even try to rely on strict type matching until we have a 1.0 of errdefs, we should use interface matching via Is functions for now when needed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants