Merge pull request #194 from taskcluster/refactor-errors

Refactor errors

Makefile

@@ -53,4 +53,5 @@ lint:
 	go get github.com/alecthomas/gometalinter
 	gometalinter --install
 	# not enabled: aligncheck, deadcode, dupl, errcheck, gas, gocyclo, structcheck, unused, varcheck
-	gometalinter --deadline=10m --line-length=180 --vendor --vendored-linters --disable-all --enable=goconst --enable=gofmt --enable=goimports --enable=golint --enable=gosimple --enable=gotype --enable=ineffassign --enable=interfacer --enable=lll --enable=misspell --enable=staticcheck --enable=test --enable=testify --enable=unconvert --enable=vet --enable=vetshadow ./...
+	# Disabled: testify, test (these two show test errors, hence, they run tests)
+	gometalinter --deadline=10m --line-length=180 --vendor --vendored-linters --disable-all --enable=goconst --enable=gofmt --enable=goimports --enable=golint --enable=gosimple --enable=gotype --enable=ineffassign --enable=interfacer --enable=lll --enable=misspell --enable=staticcheck --enable=unconvert --enable=vet --enable=vetshadow ./...

engines/enginetest/environmentvariable.go

@@ -5,6 +5,7 @@ import (
 	"sync"
 
 	"github.com/taskcluster/taskcluster-worker/engines"
+	"github.com/taskcluster/taskcluster-worker/runtime"
 )
 
 // The EnvVarTestCase contains information sufficient to setting an environment
@@ -56,7 +57,7 @@ func (c *EnvVarTestCase) TestInvalidVariableNames() {
 	r.NewSandboxBuilder(c.Payload)
 	for _, name := range c.InvalidVariableNames {
 		err := r.sandboxBuilder.SetEnvironmentVariable(name, "hello world")
-		if _, ok := err.(*engines.MalformedPayloadError); ok {
+		if _, ok := runtime.IsMalformedPayloadError(err); !ok {
 			log.Panic("Expected MalformedPayloadError from invalid variable name: ", name)
 		}
 	}

engines/errors.go

@@ -1,9 +1,6 @@
 package engines
 
-import (
-	"errors"
-	"fmt"
-)
+import "errors"
 
 // ErrFeatureNotSupported is a common error that may be returned from optional
 // Engine methods to indicate the engine implementation doesn't support the
@@ -55,10 +52,6 @@ var ErrNoSuchDisplay = errors.New("No such display exists")
 // ErrNamingConflict is used to indicate that a name is already in use.
 var ErrNamingConflict = errors.New("Conflicting name is already in use")
 
-// ErrNonFatalInternalError is used to indicate that the operation failed
-// because of internal error that isn't expected to affect other tasks.
-var ErrNonFatalInternalError = errors.New("Engine encountered a non-fatal internal error")
-
 // ErrContractViolation is returned when a contract with the engine has been
 // violated.
 var ErrContractViolation = errors.New("Engine has detected a contract violation")
@@ -71,69 +64,3 @@ var ErrMaxConcurrencyExceeded = errors.New("Engine is cannot run more than " +
 // ErrEngineNotSupported is used to indicate that the engine isn't supported in
 // the current configuration.
 var ErrEngineNotSupported = errors.New("Engine is not available in the current configuration")
-
-// TODO: MalformedPayloadError should be define in the runtime
-// TODO: MalformedPayloadError should have a merge to join two of these
-//       errors (useful if we have multiple of them)
-
-// The MalformedPayloadError error type is used to indicate that some operation
-// failed because of malformed-payload. For example a string expected to be
-// path contained invalid characters, a required property was missing, or an
-// integer was outside the permitted range.
-type MalformedPayloadError struct {
-	messages []string
-}
-
-// Error returns the error message and adheres to the Error interface
-func (e MalformedPayloadError) Error() string {
-	if len(e.messages) == 1 {
-		return e.messages[0]
-	}
-	//TODO: Make this smarter in some way please!
-	msg := ""
-	for _, m := range e.messages {
-		msg += m + "\n"
-	}
-	return msg
-}
-
-// NewMalformedPayloadError creates a MalformedPayloadError object, please
-// make sure to include a detailed description of the error, preferably using
-// multiple lines and with examples.
-//
-// These will be printed in the logs and end-users will rely on them to debug
-// their tasks.
-func NewMalformedPayloadError(a ...interface{}) MalformedPayloadError {
-	return MalformedPayloadError{messages: []string{fmt.Sprint(a...)}}
-}
-
-// MergeMalformedPayload merges a list of MalformedPayloadError objects
-func MergeMalformedPayload(errors ...MalformedPayloadError) MalformedPayloadError {
-	messages := []string{}
-	for _, e := range errors {
-		messages = append(messages, e.messages...)
-	}
-	return MalformedPayloadError{messages: messages}
-}
-
-// InternalError are errors that could not be completed because of issues related to the
-// host.  These issues could include issues with the engine, host resources, and worker
-// configuration.
-type InternalError struct {
-	message string
-}
-
-// Error returns the error message and adheres to the Error interface
-func (e InternalError) Error() string {
-	return e.message
-}
-
-// NewInternalError creates an InternalError object, please
-// make sure to include a detailed description of the error, preferably using
-// multiple lines and with examples.
-//
-// These will be printed in the logs and end-users will rely on them to debug
-// their tasks.
-func NewInternalError(message string) InternalError {
-	return InternalError{message: message}
-}

engines/mock/mocksandbox.go

@@ -61,7 +61,7 @@ func (s *sandbox) AttachVolume(mountpoint string, v engines.Volume, readOnly boo
 	s.Lock()
 	defer s.Unlock()
 	if strings.ContainsAny(mountpoint, " ") {
-		return engines.NewMalformedPayloadError("MockEngine mountpoints cannot contain space")
+		return runtime.NewMalformedPayloadError("MockEngine mountpoints cannot contain space")
 	}
 	if s.mounts[mountpoint] != nil {
 		return engines.ErrNamingConflict
@@ -78,7 +78,7 @@ func (s *sandbox) AttachProxy(name string, handler http.Handler) error {
 	s.Lock()
 	defer s.Unlock()
 	if strings.ContainsAny(name, " ") {
-		return engines.NewMalformedPayloadError(
+		return runtime.NewMalformedPayloadError(
 			"MockEngine proxy names cannot contain space.",
 			"Was given proxy name: '", name, "' which isn't allowed!",
 		)
@@ -94,7 +94,7 @@ func (s *sandbox) SetEnvironmentVariable(name string, value string) error {
 	s.Lock()
 	defer s.Unlock()
 	if strings.Contains(name, " ") {
-		return engines.NewMalformedPayloadError(
+		return runtime.NewMalformedPayloadError(
 			"MockEngine environment variable names cannot contain space.",
 			"Was given environment variable name: '", name, "' which isn't allowed!",
 		)
@@ -181,7 +181,7 @@ func (s *sandbox) WaitForResult() (engines.ResultSet, error) {
 		// No need to lock access mounts and proxies either
 		f := functions[s.payload.Function]
 		if f == nil {
-			return nil, engines.NewMalformedPayloadError("Unknown function")
+			return nil, runtime.NewMalformedPayloadError("Unknown function")
 		}
 		result := f(s, s.payload.Argument)
 		s.Lock()

engines/native/engine.go

@@ -41,12 +41,10 @@ func (engineProvider) NewEngine(options engines.EngineOptions) (engines.Engine,
 	for _, name := range c.Groups {
 		group, err := system.FindGroup(name)
 		if err != nil {
-			errorMsg := fmt.Sprintf(
-				"Unable to find system user-group: %s from engine config.",
-				name,
+			return nil, fmt.Errorf(
+				"unable to find system user-group: %s from engine config, error: %s",
+				name, err,
 			)
-			options.Monitor.ReportError(err, errorMsg)
-			return nil, engines.NewInternalError(errorMsg)
 		}
 		groups = append(groups, group)
 	}

engines/native/resultset.go

@@ -33,7 +33,7 @@ func (r *resultSet) ExtractFile(path string) (ioext.ReadSeekCloser, error) {
 		if _, ok := err.(*os.PathError); ok {
 			return nil, engines.ErrResourceNotFound
 		}
-		return nil, engines.NewMalformedPayloadError(
+		return nil, runtime.NewMalformedPayloadError(
 			"Unable to evaluate path: ", path,
 		)
 	}
@@ -77,7 +77,7 @@ func (r *resultSet) ExtractFolder(path string, handler engines.FileHandler) erro
 		if _, ok := err.(*os.PathError); ok {
 			return engines.ErrResourceNotFound
 		}
-		return engines.NewMalformedPayloadError(
+		return runtime.NewMalformedPayloadError(
 			"Unable to evaluate path: ", path,
 		)
 	}

engines/native/sandbox.go

@@ -69,7 +69,7 @@ func newSandbox(b *sandboxBuilder) (s *sandbox, err error) {
 
 	if b.payload.Context != "" {
 		if err = fetchContext(b.payload.Context, user); err != nil {
-			err = engines.NewMalformedPayloadError(
+			err = runtime.NewMalformedPayloadError(
 				fmt.Sprintf("Error downloading %s: %v", b.payload.Context, err),
 			)
 			b.context.LogError(err)
@@ -99,7 +99,7 @@ func newSandbox(b *sandboxBuilder) (s *sandbox, err error) {
 	if err != nil {
 		// StartProcess provides human-readable error messages (see docs)
 		// We'll convert it to a MalformedPayloadError
-		err = engines.NewMalformedPayloadError(
+		err = runtime.NewMalformedPayloadError(
 			"Unable to start specified command: ", b.payload.Command, "error: ", err,
 		)
 		b.context.LogError(err)
@@ -172,7 +172,7 @@ func (s *sandbox) NewShell(command []string, tty bool) (engines.Shell, error) {
 	if err != nil {
 		debug("Failed to start shell, error: %s", err)
 		s.wg.Done()
-		return nil, engines.NewMalformedPayloadError(
+		return nil, runtime.NewMalformedPayloadError(
 			"Unable to spawn command: ", command, " error: ", err,
 		)
 	}

engines/native/sandboxbuilder.go

@@ -20,7 +20,7 @@ var envVarPattern = regexp.MustCompile("^[a-zA-Z0-9_-]+$")
 
 func (b *sandboxBuilder) SetEnvironmentVariable(name string, value string) error {
 	if !envVarPattern.MatchString(name) {
-		return engines.NewMalformedPayloadError(
+		return runtime.NewMalformedPayloadError(
 			"Environment variables name: '", name, "' doesn't match: ",
 			envVarPattern.String(),
 		)

engines/qemu/image/extract.go

@@ -6,8 +6,8 @@ import (
 	"os/exec"
 	"path/filepath"
 
-	"github.com/taskcluster/taskcluster-worker/engines"
 	"github.com/taskcluster/taskcluster-worker/engines/qemu/vm"
+	"github.com/taskcluster/taskcluster-worker/runtime"
 	"github.com/taskcluster/taskcluster-worker/runtime/ioext"
 )
 
@@ -47,7 +47,7 @@ func extractImage(imageFile, imageFolder string) (*vm.Machine, error) {
 		return nil, fmt.Errorf("extractImage: imageFile is not a file")
 	}
 	if !ioext.IsFileLessThan(imageFile, maxImageSize) {
-		return nil, engines.NewMalformedPayloadError("Image file is larger than ", maxImageSize, " bytes")
+		return nil, runtime.NewMalformedPayloadError("Image file is larger than ", maxImageSize, " bytes")
 	}
 
 	// Using zstd | tar so we get sparse files (sh to get OS pipes)
@@ -58,7 +58,7 @@ func extractImage(imageFile, imageFolder string) (*vm.Machine, error) {
 	_, err := tar.Output()
 	if err != nil {
 		if ee, ok := err.(*exec.ExitError); ok {
-			return nil, engines.NewMalformedPayloadError(
+			return nil, runtime.NewMalformedPayloadError(
 				"Failed to extract image archive, error: ", string(ee.Stderr),
 			)
 		}
@@ -71,10 +71,10 @@ func extractImage(imageFile, imageFolder string) (*vm.Machine, error) {
 	for _, name := range []string{"disk.img", "layer.qcow2", "machine.json"} {
 		f := filepath.Join(imageFolder, name)
 		if !ioext.IsPlainFile(f) {
-			return nil, engines.NewMalformedPayloadError("Image file is missing '", name, "'")
+			return nil, runtime.NewMalformedPayloadError("Image file is missing '", name, "'")
 		}
 		if !ioext.IsFileLessThan(f, maxImageSize) {
-			return nil, engines.NewMalformedPayloadError("Image file contains '",
+			return nil, runtime.NewMalformedPayloadError("Image file contains '",
 				name, "' larger than ", maxImageSize, " bytes")
 		}
 	}
@@ -90,43 +90,43 @@ func extractImage(imageFile, imageFolder string) (*vm.Machine, error) {
 	diskFile := filepath.Join(imageFolder, "disk.img")
 	diskInfo := inspectImageFile(diskFile, imageRawFormat)
 	if diskInfo == nil || diskInfo.Format != formatRaw {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'disk.img' which is not a RAW image file")
 	}
 	if diskInfo.VirtualSize > maxImageSize {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'disk.img' has virtual size larger than ", maxImageSize, " bytes")
 	}
 	if diskInfo.DirtyFlag {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'disk.img' which has the dirty-flag set")
 	}
 	if diskInfo.BackingFile != "" {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'disk.img' which has a backing file, this is not permitted")
 	}
 
 	// Inspect the QCOW2 layer file
 	layerFile := filepath.Join(imageFolder, "layer.qcow2")
 	layerInfo := inspectImageFile(layerFile, imageQCOW2Format)
 	if layerInfo == nil || layerInfo.Format != formatQCOW2 {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'layer.qcow2' which is not a QCOW2 file")
 	}
 	if layerInfo.VirtualSize > maxImageSize {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'layer.qcow2' has virtual size larger than ", maxImageSize, " bytes")
 	}
 	if layerInfo.DirtyFlag {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'layer.qcow2' which has the dirty-flag set")
 	}
 	if layerInfo.BackingFile != "disk.img" {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'layer.qcow2' which has a backing file that isn't: 'disk.img'")
 	}
 	if layerInfo.BackingFormat != formatRaw {
-		return nil, engines.NewMalformedPayloadError("Image file contains ",
+		return nil, runtime.NewMalformedPayloadError("Image file contains ",
 			"'layer.qcow2' which has a backing file format that isn't 'raw'")
 	}
 

engines/qemu/image/util.go

@@ -7,7 +7,7 @@ import (
 	"os"
 
 	"github.com/taskcluster/go-got"
-	"github.com/taskcluster/taskcluster-worker/engines"
+	"github.com/taskcluster/taskcluster-worker/runtime"
 )
 
 // copyFile copies source to destination, and returns an error if one occurs
@@ -84,7 +84,7 @@ func DownloadImage(url string) Downloader {
 				goto retry
 			}
 			if res.StatusCode != 200 {
-				return engines.NewMalformedPayloadError(
+				return runtime.NewMalformedPayloadError(
 					"Image download failed with status code: ", res.StatusCode,
 				)
 			}

engines/qemu/metaservice/metaservice.go

@@ -355,7 +355,7 @@ func (s *MetaService) getArtifactWithoutRetry(path string) (
 	// Create result values to be set in the callback
 	var File ioext.ReadSeekCloser
 	var Err error
-	Err = engines.ErrNonFatalInternalError
+	Err = runtime.ErrNonFatalInternalError
 
 	s.asyncRequest(Action{
 		Type: "get-artifact",
@@ -416,7 +416,7 @@ func (s *MetaService) GetArtifact(path string) (ioext.ReadSeekCloser, error) {
 	for {
 		f, err := s.getArtifactWithoutRetry(path)
 		retries--
-		if err == engines.ErrNonFatalInternalError && retries > 0 {
+		if err == runtime.ErrNonFatalInternalError && retries > 0 {
 			continue
 		}
 		return f, err
@@ -426,7 +426,7 @@ func (s *MetaService) GetArtifact(path string) (ioext.ReadSeekCloser, error) {
 func (s *MetaService) listFolderWithoutRetries(path string) ([]string, error) {
 	var Result []string
 	var Err error
-	Err = engines.ErrNonFatalInternalError
+	Err = runtime.ErrNonFatalInternalError
 
 	s.asyncRequest(Action{
 		Type: "list-folder",
@@ -483,7 +483,7 @@ func (s *MetaService) ListFolder(path string) ([]string, error) {
 	for {
 		files, err := s.listFolderWithoutRetries(path)
 		retries--
-		if err == engines.ErrNonFatalInternalError && retries > 0 {
+		if err == runtime.ErrNonFatalInternalError && retries > 0 {
 			continue
 		}
 		return files, err
@@ -502,7 +502,7 @@ var upgrader = websocket.Upgrader{
 func (s *MetaService) ExecShell(command []string, tty bool) (engines.Shell, error) {
 	var Shell engines.Shell
 	var Err error
-	Err = engines.ErrNonFatalInternalError
+	Err = runtime.ErrNonFatalInternalError
 
 	s.asyncRequest(Action{
 		Type:    "exec-shell",
@@ -512,7 +512,7 @@ func (s *MetaService) ExecShell(command []string, tty bool) (engines.Shell, erro
 		ws, err := upgrader.Upgrade(w, r, nil)
 		if err != nil {
 			debug("Failed to upgrade request to websocket, error: %s", err)
-			Err = engines.ErrNonFatalInternalError
+			Err = runtime.ErrNonFatalInternalError
 			return
 		}