libct/errors.go: remove ConfigError in favor of ErrInvalidConfig

[kolyshkin]

Inspired by #3033 (comment).

Replace ConfigError type (introduced in #3033) with an ErrInvalidConfig
variable, for better and more uniform errors returned by libcontainer.

This is somewhat complicated, as we need to make all errors returned
from config validator to be ErrInvalidConfig, while keeping them
unwrappable.

Implemented by introducing a type which holds an underlying error,
and has the Is method that answers that the error is
ErrInvalidConfig.

[kolyshkin]

ci failure in centos7 is a epel repo glitch. Will retry later.

[kolyshkin]

Draft until #3049 is merged

[kolyshkin]

OK, it looks like #3049 is not going anywhere, so I'll redo this without it.

[kolyshkin]

Removed dependency on #3049, rebased, ready for review.

[thaJeztah]

as we need to make all errors returned from config validator to be ErrInvalidConfig

Can you provide more details on why it needs an ErrInvalidConfig ?

while keeping them unwrappable

This confuses me somewhat; if the validator invalidated the config, applying / wrapping it in ErrInvalidConfig means the error was handled. Is there code that needs to know the underlying error to handle it?

I feel like it should be either one of the above; don't wrap the error in an ErrInvalidConfig (if underlying errors must be handled), or wrap it, and consider ErrInvalidConfig to be "it was invalid for (whatever reason), and we're done".

Looking at how it's used;

runc/libcontainer/factory_linux.go

Lines 259 to 261 in e918d02

	if err := l.Validator.Validate(config); err != nil {
	return nil, &ConfigError{err.Error()}
	}

runc/utils_linux.go

Line 246 in e918d02

return factory.Create(id, config)

runc/utils_linux.go

Lines 413 to 416 in e918d02

	container, err := createContainer(context, id, spec)
	if err != nil {
	return -1, err
	}

[kolyshkin]

as we need to make all errors returned from config validator to be ErrInvalidConfig

Can you provide more details on why it needs an ErrInvalidConfig ?

So, we want to have some predefined errors returned by libcontainer. Those are listed in libcontainer/error.go.

PR #3033 did a good job of dismantling the libcontainer's own error system (in favor of Go 1.13+ errors), while keeping these predefined errors (see PR description).

So, a libcontainer user can check for a particular error using errors.Is, for example:

if errors.Is(err, libcontainer.ErrNotExist)

One thing, though, where the above PR falls short, is "invalid config" error. There used to be ConfigInvalid error, which is replaced by a ConfigError type. Since this is a type, the check for a particular error (to answer a question "do we have an invalid config?") can't be in the same manner as for all other such errors (using errors.Is) -- instead you have to use something like errors.As().

This PR is solving this problem, so you can now use errors.Is(err, libcontainer.ErrInvalidConfig), same as for other "generic" errors.

while keeping them unwrappable

This confuses me somewhat; if the validator invalidated the config, applying / wrapping it in ErrInvalidConfig means the error was handled. Is there code that needs to know the underlying error to handle it?

Wrapping it in ErrInvalidConfig makes it possible to use errors.Is(err, ErrInvalidConfig), that's it.

The "keeping then unwrappable" part means that you can still get the exact underlying error.

I feel like it should be either one of the above; don't wrap the error in an ErrInvalidConfig (if underlying errors must be handled), or wrap it, and consider ErrInvalidConfig to be "it was invalid for (whatever reason), and we're done".

Looking at how it's used;

runc/libcontainer/factory_linux.go

Lines 259 to 261 in e918d02

	if err := l.Validator.Validate(config); err != nil {
	return nil, &ConfigError{err.Error()}
	}

This PR removes ConfigError type.

runc/utils_linux.go

Line 246 in e918d02

return factory.Create(id, config)

runc/utils_linux.go

Lines 413 to 416 in e918d02

	container, err := createContainer(context, id, spec)
	if err != nil {
	return -1, err
	}

[thaJeztah]

The "keeping then unwrappable" part means that you can still get the exact underlying error.

Right, so question is: is it needed? Looking at the code in this repository, the only relevant error that's checked is ErrNotExist;

runc/delete.go

Line 58 in 5547b57

if errors.Is(err, libcontainer.ErrNotExist) {

Which is not on "create", but on "delete" (container already gone situation).

Outside of runc itself, it also looks to be the only error that some users check for (but using the "old" errors); https://grep.app/search?q=libcontainer.E

Mostly wondering; are there practical situations where the caller can implement logic to handle these errors, other than "it's invalid"?

[kolyshkin]

@thaJeztah I don't know if anyone needs it. I did it mostly to not lose any functionality compared to what we had before #3033, and generally thinking it's good to have more structured errors.

If that's not needed, I can simplify the whole thing, removing both ConfigError type and ErrInvalidConfig.

@AkihiroSuda WDYT?

[thaJeztah]

generally thinking it's good to have more structured errors.

Bit on the fence if errors should be wrapped multiple times. I think that's what github.com/pkg/errors provided, wasn't it? But the Go maintainers explicitly considered "wrapping an error, means it's handled, and is the end".

Happy to hear others input though.

[kolyshkin]

But the Go maintainers explicitly considered "wrapping an error, means it's handled, and is the end".

I don't quite understand this. Can you share a link or something?

Wrapping an error, in many cases, means adding more context to it. For example, os returns many errors wrapped into os.PathError or os.SyscallError, to add context like file name and/or the failed operation or syscall. This certainly does not mean the error is handled.

In case of this PR, though, we wrap the error into a newly introduced type for the sole reason of making errors.Is(err, libcontainer.ErrInvalidConfig) work. I agree that maybe no one is using this, and it is not needed at all, and this is why I suggested to drop both ConfigError and ErrInvalidConfig in #3056 (comment).

But I totally don't understand how wrapping an error means handling it.

[kolyshkin]

I agree that maybe no one is using this, and it is not needed at all, and this is why I suggested to drop both ConfigError and ErrInvalidConfig in #3056 (comment).

Here's the alternative: #3176
PTAL if it makes more sense.

[kolyshkin]

PTAL @thaJeztah (comments above)

[kolyshkin]

Closing in favor of #3176 which is simpler.