Remove hashicorp/go-multierror dependency by djdongjin · Pull Request #8992 · containerd/containerd

djdongjin · 2023-08-19T07:40:01Z

Remove github.com/hashicorp/go-multierror and replace with errors.Join in std.

Some concept equivalents:

var errs multierror.Error = var errs []error
errs = multierror.Append(errs, err) = errs = append(errs, err)
multierror.ErrorOrNil(errs) = errors.Join(errs)
errs.WrappedErrors() = err.(interface{ Unwrap() []error }).Unwrap()

One behavior difference between multierror.Append and errors.Join is that the former will append to an existing multierror if it is (so the new one has a length of N + 1), while the latter will create a new joinError which includes previous error and the new one (so the new one has a length of 2)

https://go.dev/play/p/ryeimUucDyb

I think the recommended use pattern of errors.Join is to have a errs [] error to save all the errors along the way and a errors.Join(errs...) at the end to produce one joinError.

https://pkg.go.dev/errors#Join

golang/go#60209

golang/go#53435 (comment)

k8s-ci-robot · 2023-08-19T07:40:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

djdongjin · 2023-08-19T08:46:57Z

/test all

AkihiroSuda · 2023-08-19T09:43:58Z

/retest

djdongjin · 2023-08-19T23:18:38Z

/retest

djdongjin · 2023-08-20T19:11:52Z

snapshots/devmapper/snapshotter.go

 			status, sErr := dmsetup.Status(s.pool.poolName)
 			if sErr != nil {
-				multierror.Append(err, sErr)
+				errs = append(errs, sErr)


The previous code actually has a bug. If err is a error type, multierror.Append(err, sErr) will create a new multierror.Error type and append sErr there. It should be err = multierror.Append(err, sErr).

Signed-off-by: Jin Dong <jin.dong@databricks.com>

djdongjin · 2023-08-21T01:08:08Z

pkg/process/io.go

+			result = append(result, fn())
 		}
-		err = multierror.Flatten(result)
+		err = errors.Join(result...)


multierror.Flatten is only called here, so not sure if it's necessary. If so, maybe we can write a simple helper to do that.

djdongjin · 2023-08-21T01:08:30Z

/test all

mxpv · 2023-08-21T18:06:34Z

snapshots/devmapper/pool_device.go

-		return e.Errors[0]
+	if joinErr, ok := e.(interface{ Unwrap() []error }); ok {
+		if errs := joinErr.Unwrap(); len(errs) == 1 {
+			return errs[0]


If I recall this right, we required this unwrapping helper for error comparisons to work. Which can now be replaced with error.Is. I wonder if we can now remove this?

thanks for the context. I remember there're 2 places where the unwrap is used. Let me take a look at both.

Digging into a little bit, unwrapError was introduced in #4437. At that time it seems multierror 1.0.0 is used (or at least older than 1.1.0).

My guess is unwrapError was required to expose the underlying unix.ENODATA error within multierror which was handled below

containerd/snapshots/devmapper/pool_device.go

Line 545 in fb786b2

if e != nil && !errors.Is(e, unix.ENODATA) {

It was needed because at that time, errors.Is seems not work with multierror:

In this playground with multierror 1.0.0, errors.Is returns false: https://go.dev/play/p/29rAHWAV310
In this playground with multierror 1.1.0, errors.Is returns true: https://go.dev/play/p/UpFMxRkmjcc

And now since errors.Is works with both multierror 1.1.0 and errors.Join, we can safely remove this regardless :)

Signed-off-by: Jin Dong <jin.dong@databricks.com>

djdongjin · 2023-08-22T02:20:42Z

snapshots/storage/metastore.go

-		// Unwrap if just one error
-		if len(result.Errors) == 1 {
-			return result.Errors[0]
-		}


I also removed this Unwrap since most cases don't have such Unwrap logic unless specifically needed.

Only such case I found is the code below (to handle cleanupErr separately).

containerd/pkg/cri/sbserver/sandbox_run.go

Lines 256 to 266 in fb786b2

var cerr podsandbox.CleanupErr

if errors.As(err, &cerr) {

cleanupErr = fmt.Errorf("failed to cleanup sandbox: %w", cerr)

// Strip last error as cleanup error to handle separately

if merr, ok := err.(*multierror.Error); ok {

if errs := merr.WrappedErrors(); len(errs) > 0 {

err = errs[0]

}

}

}

mxpv

LGTM

fuweid

Changes look good.

djdongjin · 2025-03-06T15:20:49Z

/cherry-pick release/1.7

k8s-infra-cherrypick-robot · 2025-03-06T15:21:32Z

@djdongjin: #8992 failed to apply on top of branch "release/1.7":

Applying: Remove hashicorp/go-multierror
Using index info to reconstruct a base tree...
M	go.mod
M	pkg/cri/sbserver/podsandbox/sandbox_run.go
M	pkg/cri/sbserver/sandbox_run.go
M	pkg/cri/sbserver/sandbox_stats_list.go
M	pkg/cri/server/sandbox_stats_list.go
M	pkg/process/io.go
M	runtime/v2/shim.go
M	snapshots/devmapper/pool_device.go
M	snapshots/devmapper/snapshotter.go
M	snapshots/devmapper/snapshotter_test.go
M	snapshots/storage/metastore.go
Falling back to patching base and 3-way merge...
Auto-merging snapshots/storage/metastore.go
CONFLICT (content): Merge conflict in snapshots/storage/metastore.go
Auto-merging snapshots/devmapper/snapshotter_test.go
Auto-merging snapshots/devmapper/snapshotter.go
CONFLICT (content): Merge conflict in snapshots/devmapper/snapshotter.go
Auto-merging snapshots/devmapper/pool_device.go
Auto-merging runtime/v2/shim.go
Auto-merging pkg/process/io.go
CONFLICT (content): Merge conflict in pkg/process/io.go
Auto-merging pkg/cri/server/sandbox_stats_list.go
CONFLICT (content): Merge conflict in pkg/cri/server/sandbox_stats_list.go
Auto-merging pkg/cri/sbserver/sandbox_stats_list.go
CONFLICT (content): Merge conflict in pkg/cri/sbserver/sandbox_stats_list.go
Auto-merging pkg/cri/sbserver/sandbox_run.go
CONFLICT (content): Merge conflict in pkg/cri/sbserver/sandbox_run.go
Auto-merging pkg/cri/sbserver/podsandbox/sandbox_run.go
Auto-merging go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Remove hashicorp/go-multierror

Details

In response to this:

/cherry-pick release/1.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

djdongjin force-pushed the remove-hashicorp-multierror branch from 1597909 to 2ab1bd7 Compare August 19, 2023 07:55

djdongjin commented Aug 20, 2023

View reviewed changes

djdongjin force-pushed the remove-hashicorp-multierror branch from 2ab1bd7 to 4da1749 Compare August 21, 2023 00:57

Remove hashicorp/go-multierror

cd8c8ae

Signed-off-by: Jin Dong <jin.dong@databricks.com>

djdongjin force-pushed the remove-hashicorp-multierror branch from 4da1749 to cd8c8ae Compare August 21, 2023 00:59

djdongjin commented Aug 21, 2023

View reviewed changes

djdongjin marked this pull request as ready for review August 21, 2023 15:45

mxpv reviewed Aug 21, 2023

View reviewed changes

Remove unnecessary joinError unwrap

6d8329d

Signed-off-by: Jin Dong <jin.dong@databricks.com>

djdongjin commented Aug 22, 2023

View reviewed changes

mxpv approved these changes Aug 23, 2023

View reviewed changes

AkihiroSuda approved these changes Aug 23, 2023

View reviewed changes

fuweid approved these changes Aug 23, 2023

View reviewed changes

fuweid merged commit 738c153 into containerd:main Aug 23, 2023

djdongjin deleted the remove-hashicorp-multierror branch August 24, 2023 23:34

thaJeztah mentioned this pull request Sep 18, 2023

[release/0.11 backport] [ci] Fix integration test failure microsoft/hcsshim#1909

Closed

This was referenced Mar 6, 2025

[release/1.7] build(deps): bump containerd/project-checks from 1.1.0 to 1.2.1 #11494

Closed

[release/1.7] Remove hashicorp/go-multierror dependency #11498

Closed

[release/1.7] Remove hashicorp/go-multierror dependency and fix CI #11499

Merged

	var cerr podsandbox.CleanupErr
	if errors.As(err, &cerr) {
	cleanupErr = fmt.Errorf("failed to cleanup sandbox: %w", cerr)

	// Strip last error as cleanup error to handle separately
	if merr, ok := err.(*multierror.Error); ok {
	if errs := merr.WrappedErrors(); len(errs) > 0 {
	err = errs[0]
	}
	}
	}

Conversation

djdongjin commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 19, 2023

Uh oh!

djdongjin commented Aug 19, 2023

Uh oh!

AkihiroSuda commented Aug 19, 2023

Uh oh!

djdongjin commented Aug 19, 2023

Uh oh!

djdongjin Aug 20, 2023

Choose a reason for hiding this comment

Uh oh!

djdongjin Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

djdongjin commented Aug 21, 2023

Uh oh!

mxpv Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

djdongjin Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

djdongjin Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

djdongjin Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

mxpv left a comment

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

djdongjin commented Mar 6, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

djdongjin commented Aug 19, 2023 •

edited

Loading