Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Update createSpec to use errdefs #38770

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@dperny
Copy link
Contributor

dperny commented Feb 21, 2019

Signed-off-by: Drew Erny drew.erny@docker.com

- What I did

Updates the createSpec method, for both Windows and Linux systems, to return all errors in terms of errdefs. Makes easy changes in dependencies of those methods as well, if bringing them to use all errdefs errors was trivial. Includes comments annotating why each errdef type was chosen for each particular error.

/cc @cpuguy83, done as promised in exchange for #38632 being accepted.

- How I did it

Carefully step through createSpec and its dependencies, and wrapped each error in an errdefs helper function before it was returned

- How to verify it

You sort of have to review it by hand. This function is really bad in terms of complexity, so a comprehensive test case to cover it would be prohibitively difficult to write.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Update createSpec to use errdefs
Updates the createSpec method, for both Windows and Linux systems, to
return all errors in terms of errdefs. Makes easy changes in
dependencies of those methods as well, if bringing them to use all
errdefs errors was trivial. Includes comments annotating why each errdef
type was chosen for each particular error.

Signed-off-by: Drew Erny <drew.erny@docker.com>

@dperny dperny force-pushed the dperny:createspec-use-errdefs branch from 001435c to 59c7083 Feb 21, 2019

@cpuguy83
Copy link
Contributor

cpuguy83 left a comment

Thanks for following up.

Some general comments.
In some cases it seems improper to make assumptions about the errors returned by a lower level function unless it's clear (e.g. validateUserInput would by definition always be an InvaldParam error).
Otherwise we end up with the same risk as we have for the caller of createSpec assuming a System error.

@@ -36,12 +37,12 @@ func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]s

for linkAlias, child := range children {
if !child.IsRunning() {
return nil, fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias)
return nil, errdefs.InvalidParameter(fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

Maybe this should be Conflict?

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

Agree, will fix.

@@ -723,7 +738,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
s.Linux.CgroupsPath = cgroupsPath

if err := setResources(&s, c.HostConfig.Resources); err != nil {
return nil, fmt.Errorf("linux runtime spec resources: %v", err)
return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec resources: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

Can you change this to errors.Wrap?

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

Yeah, I realize I discovered that errors.Wrap works with errdefs, but I forgot to go back and change the ones I'd already written.

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

To be clear, do you think this:

errdefs.InvalidParameter(errors.Wrap(err, "linux runtime spec resources:")

or this:

errors.Wrap(errdefs.InvalidParameter(err), "linux runtime spec resources:")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2019

Member

I think the first one would be clearer (and the InvalidParameter would not be nested deeper)

}
if err := setUser(&s, c); err != nil {
return nil, fmt.Errorf("linux spec user: %v", err)
// setUser fails if you specify a bad user, so it's an InvalidParameter
return nil, errdefs.InvalidParameter(fmt.Errorf("linux spec user: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

errors.Wrap

return nil, fmt.Errorf("linux runtime spec rlimits: %v", err)
// as of this writing, setRlimits doesn't return any errors at all, so
// we return error type Unknown if it happens.
return nil, errdefs.Unknown(fmt.Errorf("linux runtime spec rlimits: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

errors.Wrap
But we should maybe not use errdefs here, in case setRLimits wants to use errors int he future? Or if it's really not returning errors, maybe the method definition should be updated.

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

I could do either. I suppose it would be the ideal to enforce that if setRLimits ever did return errors, they would have to be of errdefs types.

return nil, fmt.Errorf("linux runtime spec devices: %v", err)
// The errors resulting from devices are generally more of a
// user-configured-it-wrong error, so return InvalidParameter
return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec devices: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

errors.Wrap

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 21, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@cbb885b). Click here to learn what that means.
The diff coverage is 15.38%.

@@            Coverage Diff            @@
##             master   #38770   +/-   ##
=========================================
  Coverage          ?   36.42%           
=========================================
  Files             ?      613           
  Lines             ?    45847           
  Branches          ?        0           
=========================================
  Hits              ?    16700           
  Misses            ?    26852           
  Partials          ?     2295
@@ -26,6 +26,7 @@ import (
)

func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]string, error) {
// NOTE(dperny): this method has been fixed to return only errdefs errors.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2019

Member

Think we can remove this note (as it's not a "todo") 😅

return nil, fmt.Errorf("linux spec capabilities: %v", err)
// SetCapabilities returned no error in this revision, so if it fails,
// we don't know why
return nil, errdefs.Unknown(fmt.Errorf("linux spec capabilities: %v", err))

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2019

Member

I see there's some more fmt.Errorf('s here (and below); should all ideally be changed to errors.Wrap so that the original error doesn't get lost.

// have changed. To ensure that, no matter what, we get a valid error
// type, we will match on all known error types, and then wrap this
// error in "Unknown" if it is none of them
if !errdefs.IsKnownErrorType(err) {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 8, 2019

Member

Wondering if we need this, as we already log errors that don't have a type assigned, and treat them as a 500;

logrus.WithFields(logrus.Fields{
"module": "api",
"error_type": fmt.Sprintf("%T", err),
}).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err)

(basically; if we didn't handle the error, it's a server error, so a 500)

@@ -45,6 +53,9 @@ func TestForbidden(t *testing.T) {
if !IsForbidden(e) {
t.Fatalf("expected forbidden error, got: %T", e)
}
if !IsKnownErrorType(e) {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 8, 2019

Member

This looks redundant; we checked if it's a Forbidden error above, and otherwise the test fails (in which case it prints the %T)

This comment has been minimized.

Copy link
@dperny

dperny Mar 11, 2019

Author Contributor

right, but this test is to make sure that Forbidden is a known error type.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 12, 2019

Member

Not sure I understand; IsForbidden checks that it's a known error type (must be ErrForbidden);

moby/errdefs/is.go

Lines 62 to 66 in 8e610b2

// IsForbidden returns if the passed in error is an ErrForbidden
func IsForbidden(err error) bool {
_, ok := getImplementer(err).(ErrForbidden)
return ok
}

So when we reach this line, we know it's a ErrForbidden, thus a known error-type?

This comment has been minimized.

Copy link
@dperny

dperny Mar 12, 2019

Author Contributor

the test isn't for IsForbidden, it's for IsKnownErrorType.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 12, 2019

Member

If it's to test if IsKnownErrorType is handling all error-types, it should be a separate test. This test is to test if IsForbidden is working as expected.

But I'm still not sure what the IsKnownErrorType utility is solving (see my comment above: https://github.com/moby/moby/pull/38770/files#r263735447)

By wrapping "unknown error types" as an errdefs.Unknown, we're actually masking unhandled errors, and won't be logging them as errors that have not been mapped;

case errdefs.IsSystem(err) || errdefs.IsUnknown(err) || errdefs.IsDataLoss(err) || errdefs.IsDeadline(err) || errdefs.IsCancelled(err):
statusCode = http.StatusInternalServerError
default:
statusCode = statusCodeFromGRPCError(err)
if statusCode != http.StatusInternalServerError {
return statusCode
}
if e, ok := err.(causer); ok {
return GetHTTPErrorStatusCode(e.Cause())
}
logrus.WithFields(logrus.Fields{
"module": "api",
"error_type": fmt.Sprintf("%T", err),
}).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err)
}

This comment has been minimized.

Copy link
@dperny

dperny Mar 13, 2019

Author Contributor

Yeah, I see that now. I'm gonna change it when I spend some time working on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.