Improve error reporting, add errorlint, fix its warnings; drop pkg/errors #3011

kolyshkin · 2021-06-09T04:02:45Z

This commit does a few things related to error reporting and handling.

Fix and improve a bunch of error messages and associated error handling,
using features that appear in Go 1.13 (fmt.Errorf with %w to wrap errors,
errors.Is and errors.As for unwrapping) and some other methods
(such as using os.PathError, introducing and using ParseError etc.)
Fix all errorlint linter warnings, enable errorlint it in CI.
Remove all uses of pkg/errors, switching to native Go error (un)wrapping.

~~TODO (not in this PR, as it's already too big):~~

modernize and simplify libcontainer/error (done in libcontainer: rm own error system #3033)

kolyshkin · 2021-06-09T19:52:51Z

Hmm, weird failure in update devices [minimal transition rules] test; filed #3014; CI restarted.

libcontainer/restored_process.go

kolyshkin · 2021-06-09T21:24:01Z

Hmm, weird failure in update devices [minimal transition rules] test; filed #3014; CI restarted.

OK it seems that this error is repeatable. Kicked CI again, created #3015 to investigate if this is indeed caused by this PR.

kolyshkin · 2021-06-11T20:01:51Z

Rebased; set milestone to 1.1

kolyshkin · 2021-06-12T00:35:40Z

Added commits that switch from pkg/error to Go native error (un)wrapping.

kolyshkin · 2021-06-14T19:50:41Z

Rebased on top of merged #3024

libcontainer/cgroups/fscommon/utils.go

An errror from ioutil.WriteFile already contains file name, so there is no need to duplicate that information. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2021-06-22T22:50:11Z

Working on addressing my own review comments :)

Errors returned by unix are bare. In some cases it's impossible to find out what went wrong because there's is not enough context. Add a mountError type (mostly copy-pasted from github.com/moby/sys/mount), and mount/unmount helpers. Use these where appropriate, and convert error checks to use errors.Is. Signed-off-by: Kir Kolyshkin <[email protected]>

Errors from os.Open, os.Symlink etc do not need to be wrapped, as they are already wrapped into os.PathError. Error from unix are bare errnos and need to be wrapped. Same os.PathError is a good candidate. Signed-off-by: Kir Kolyshkin <[email protected]>

This should result in no change when the error is printed, but make the errors returned unwrappable, meaning errors.As and errors.Is will work. Signed-off-by: Kir Kolyshkin <[email protected]>

This one is tough as errorlint insists on using errors.Is, and the latter is known to not work for Go 1.13 which we still support. So, add a nolint annotation to suppress the warning, and a TODO to address it later. For intelrdt, we can do the same, but it is easier to reuse the very same function from fscommon (note we can't use fscommon for other stuff as it expects cgroupfs). Signed-off-by: Kir Kolyshkin <[email protected]>

Use fmt.Errorf with %w instead. Convert the users to the new wrapping. This fixes an errorlint warning. Signed-off-by: Kir Kolyshkin <[email protected]>

Instead of using errors.Wrap, use fmt.Errorf with %w for error wrapping. Also, use errors.Is instead of direct error comparison. Signed-off-by: Kir Kolyshkin <[email protected]>

Do this for all errors except one from unix.*. This fixes a bunch of errorlint warnings, like these libcontainer/generic_error.go:25:15: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint) if le, ok := err.(Error); ok { ^ libcontainer/factory_linux_test.go:145:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint) lerr, ok := err.(Error) ^ libcontainer/state_linux_test.go:28:11: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint) _, ok := err.(*stateTransitionError) ^ libcontainer/seccomp/patchbpf/enosys_linux.go:88:4: switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors (errorlint) switch err { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Errors from unix.* are always bare and thus can be used directly. Add //nolint:errorlint annotation to ignore errors such as these: libcontainer/system/xattrs_linux.go:18:7: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint) case errno == unix.ERANGE: ^ libcontainer/container_linux.go:1259:9: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint) if e != unix.EINVAL { ^ libcontainer/rootfs_linux.go:919:7: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint) if err != unix.EINVAL && err != unix.EPERM { ^ libcontainer/rootfs_linux.go:1002:4: switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors (errorlint) switch err { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

Use Go native errors wrapping. Introduce wrapErr helper to minimise the patch size. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2021-06-22T23:44:39Z

(my own) review comments addressed; no longer WIP.

cyphar

One nit, two things need changing, and one question.

libcontainer/cgroups/fs/blkio_test.go

libcontainer/cgroups/fs/pids.go

libcontainer/cgroups/fs2/pids.go

libcontainer/container_linux.go

kolyshkin · 2021-06-23T18:10:09Z

One nit, two things need changing, and one question.

Sorry, what was the question? I can't find it.

cyphar · 2021-06-24T12:13:21Z

@kolyshkin It was about removing genericError but after posting it I noticed that there was a separate PR for that. #3011 (comment)

cyphar

LGTM, my suggestions missed that the pids.max handling already existed in the fscommon utilities.

kolyshkin force-pushed the errlinter branch 2 times, most recently from 13bcbba to 34cbc22 Compare June 9, 2021 17:07

This was referenced Jun 9, 2021

libcontainer/cgroups/fs/blkio: support BFQ weight[_device] #3010

Merged

(regression in master) not ok 117 update devices [minimal transition rules] #3014

Closed

thaJeztah reviewed Jun 9, 2021

View reviewed changes

libcontainer/restored_process.go Show resolved Hide resolved

kolyshkin changed the title ~~[WIP] improve error reporting, add errorlint, fix its warnings~~ Improve error reporting, add errorlint, fix its warnings Jun 10, 2021

kolyshkin marked this pull request as ready for review June 10, 2021 14:57

kolyshkin added this to the post-1.0 milestone Jun 10, 2021

kolyshkin force-pushed the errlinter branch from aed7a3c to 17f9209 Compare June 11, 2021 19:47

kolyshkin modified the milestones: post-1.0, 1.1.0 Jun 11, 2021

kolyshkin changed the title ~~Improve error reporting, add errorlint, fix its warnings~~ Improve error reporting, add errorlint, fix its warnings; drop pkg/errors Jun 11, 2021

kolyshkin force-pushed the errlinter branch from d80ce57 to fa370c9 Compare June 12, 2021 00:34

kolyshkin force-pushed the errlinter branch from fa370c9 to 7f55214 Compare June 12, 2021 03:13

kolyshkin added the kind/refactor refactoring label Jun 12, 2021

kolyshkin force-pushed the errlinter branch 3 times, most recently from 59d43b0 to d8de1a7 Compare June 14, 2021 19:49

kolyshkin requested a review from cyphar June 15, 2021 20:01

kolyshkin mentioned this pull request Jun 22, 2021

libcontainer: rm own error system #3033

Merged

kolyshkin force-pushed the errlinter branch from d8de1a7 to 9230410 Compare June 22, 2021 01:56

kolyshkin requested review from AkihiroSuda and mrunalp June 22, 2021 18:05

kolyshkin commented Jun 22, 2021

View reviewed changes

libcontainer/cgroups/fscommon/utils.go Outdated Show resolved Hide resolved

libct/intelrdt: error message nits

92e8d9b

An errror from ioutil.WriteFile already contains file name, so there is no need to duplicate that information. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin changed the title ~~Improve error reporting, add errorlint, fix its warnings; drop pkg/errors~~ [WIP] Improve error reporting, add errorlint, fix its warnings; drop pkg/errors Jun 22, 2021

kolyshkin force-pushed the errlinter branch from a8977f0 to 9e25772 Compare June 22, 2021 23:01

kolyshkin added 13 commits June 22, 2021 16:09

*: fmt.Errorf: use %w when appropriate

7be93a6

This should result in no change when the error is printed, but make the errors returned unwrappable, meaning errors.As and errors.Is will work. Signed-off-by: Kir Kolyshkin <[email protected]>

libct/keys: stop using pkg/errors

c6fed26

Use fmt.Errorf with %w instead. Convert the users to the new wrapping. This fixes an errorlint warning. Signed-off-by: Kir Kolyshkin <[email protected]>

tty.go: don't use pkg/errors, use errors.Is

5d2a11a

Instead of using errors.Wrap, use fmt.Errorf with %w for error wrapping. Also, use errors.Is instead of direct error comparison. Signed-off-by: Kir Kolyshkin <[email protected]>

.golangci.yml: enable errorlint

ebb0812

Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cg/devices: stop using pkg/errors

f137aaa

Use Go native errors wrapping. Introduce wrapErr helper to minimise the patch size. Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cg/ebpf: stop using pkg/errors

a6cc36a

Signed-off-by: Kir Kolyshkin <[email protected]>

libct/cg: stop using pkg/errors

b60e2ed

Signed-off-by: Kir Kolyshkin <[email protected]>

*: stop using pkg/errors

a7cfb23

Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the errlinter branch from 9e25772 to a7cfb23 Compare June 22, 2021 23:10

kolyshkin changed the title ~~[WIP] Improve error reporting, add errorlint, fix its warnings; drop pkg/errors~~ Improve error reporting, add errorlint, fix its warnings; drop pkg/errors Jun 22, 2021

AkihiroSuda approved these changes Jun 23, 2021

View reviewed changes

cyphar requested changes Jun 23, 2021

View reviewed changes

libcontainer/cgroups/fs/blkio_test.go Show resolved Hide resolved

libcontainer/cgroups/fs/pids.go Show resolved Hide resolved

libcontainer/cgroups/fs2/pids.go Show resolved Hide resolved

libcontainer/container_linux.go Show resolved Hide resolved

cyphar approved these changes Jun 24, 2021

View reviewed changes

cyphar closed this in 51beb5c Jun 24, 2021

cyphar merged commit 51beb5c into opencontainers:master Jun 24, 2021

rata mentioned this pull request Jun 24, 2021

Implement seccomp notify support #2682

Merged

kolyshkin mentioned this pull request Jun 24, 2021

remove pkg/errors from vendor #3039

Closed

10 tasks

zouyee mentioned this pull request Sep 25, 2021

remove pkg/errors from vendor containerd/containerd#6049

Closed

kolyshkin mentioned this pull request Nov 11, 2021

libct: wrap more unix errors #3270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error reporting, add errorlint, fix its warnings; drop pkg/errors #3011

Improve error reporting, add errorlint, fix its warnings; drop pkg/errors #3011

kolyshkin commented Jun 9, 2021 •

edited

Loading

kolyshkin commented Jun 9, 2021

kolyshkin commented Jun 9, 2021

kolyshkin commented Jun 11, 2021

kolyshkin commented Jun 12, 2021

kolyshkin commented Jun 14, 2021

kolyshkin commented Jun 22, 2021

kolyshkin commented Jun 22, 2021

cyphar left a comment

kolyshkin commented Jun 23, 2021

cyphar commented Jun 24, 2021

cyphar left a comment

Improve error reporting, add errorlint, fix its warnings; drop pkg/errors #3011

Improve error reporting, add errorlint, fix its warnings; drop pkg/errors #3011

Conversation

kolyshkin commented Jun 9, 2021 • edited Loading

kolyshkin commented Jun 9, 2021

kolyshkin commented Jun 9, 2021

kolyshkin commented Jun 11, 2021

kolyshkin commented Jun 12, 2021

kolyshkin commented Jun 14, 2021

kolyshkin commented Jun 22, 2021

kolyshkin commented Jun 22, 2021

cyphar left a comment

Choose a reason for hiding this comment

kolyshkin commented Jun 23, 2021

cyphar commented Jun 24, 2021

cyphar left a comment

Choose a reason for hiding this comment

kolyshkin commented Jun 9, 2021 •

edited

Loading