From b85673ba39a5237ee22d476d2694e29e7c375669 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Wed, 24 Aug 2016 11:26:41 +0200 Subject: [PATCH 1/3] *: replace err.Error() with err In principle we should avoid returning err.Error(). Instead we should just return err, to be able to inspect the error in a flexible way. --- api/machines_test.go | 2 +- api/state_test.go | 2 +- api/units_test.go | 2 +- fleetd/fleetd.go | 8 ++++---- functional/systemd_test.go | 8 ++++---- pkg/filesystem_test.go | 6 +++--- systemd/manager_test.go | 14 +++++++------- 7 files changed, 21 insertions(+), 21 deletions(-) diff --git a/api/machines_test.go b/api/machines_test.go index a2938ce02..349cda87c 100644 --- a/api/machines_test.go +++ b/api/machines_test.go @@ -77,7 +77,7 @@ func TestMachinesListBadNextPageToken(t *testing.T) { err = assertErrorResponse(rw, http.StatusBadRequest) if err != nil { - t.Error(err.Error()) + t.Error(err) } } diff --git a/api/state_test.go b/api/state_test.go index 418e08dbf..4107c31b1 100644 --- a/api/state_test.go +++ b/api/state_test.go @@ -189,7 +189,7 @@ func TestUnitStateListBadNextPageToken(t *testing.T) { err = assertErrorResponse(rw, http.StatusBadRequest) if err != nil { - t.Error(err.Error()) + t.Error(err) } } diff --git a/api/units_test.go b/api/units_test.go index 308d9bc75..ca065cbf2 100644 --- a/api/units_test.go +++ b/api/units_test.go @@ -114,7 +114,7 @@ func TestUnitsListBadNextPageToken(t *testing.T) { err = assertErrorResponse(rw, http.StatusBadRequest) if err != nil { - t.Error(err.Error()) + t.Error(err) } } diff --git a/fleetd/fleetd.go b/fleetd/fleetd.go index 59dddefcb..849c1d4de 100644 --- a/fleetd/fleetd.go +++ b/fleetd/fleetd.go @@ -100,13 +100,13 @@ func main() { globalconf.Register("", cfgset) cfg, err := getConfig(cfgset, *cfgPath) if err != nil { - log.Fatalf(err.Error()) + log.Fatal(err) } log.Debugf("Creating Server") srv, err := server.New(*cfg, nil) if err != nil { - log.Fatalf("Failed creating Server: %v", err.Error()) + log.Fatalf("Failed creating Server: %v", err) } srv.Run() @@ -120,7 +120,7 @@ func main() { cfg, err := getConfig(cfgset, *cfgPath) if err != nil { - log.Fatalf(err.Error()) + log.Fatal(err) } log.Infof("Restarting server components") @@ -135,7 +135,7 @@ func main() { // The new server takes the original listeners. srv, err = server.New(*cfg, oldListeners) if err != nil { - log.Fatalf(err.Error()) + log.Fatal(err) } srv.Run() diff --git a/functional/systemd_test.go b/functional/systemd_test.go index 408543bee..d1a37448e 100644 --- a/functional/systemd_test.go +++ b/functional/systemd_test.go @@ -75,22 +75,22 @@ ExecStart=/usr/bin/sleep 3000 err = waitForUnitState(mgr, name, unit.UnitState{"loaded", "inactive", "dead", "", hash, ""}) if err != nil { - t.Error(err.Error()) + t.Error(err) } err = mgr.TriggerStart(name) if err != nil { - t.Error(err.Error()) + t.Error(err) } err = waitForUnitState(mgr, name, unit.UnitState{"loaded", "active", "running", "", hash, ""}) if err != nil { - t.Error(err.Error()) + t.Error(err) } err = mgr.TriggerStop(name) if err != nil { - t.Error(err.Error()) + t.Error(err) } mgr.Unload(name) diff --git a/pkg/filesystem_test.go b/pkg/filesystem_test.go index b259508c7..0da1a336f 100644 --- a/pkg/filesystem_test.go +++ b/pkg/filesystem_test.go @@ -25,7 +25,7 @@ import ( func TestListDirectory(t *testing.T) { dir, err := ioutil.TempDir("", "fleet-testing-") if err != nil { - t.Fatal(err.Error()) + t.Fatal(err) } defer os.RemoveAll(dir) @@ -33,7 +33,7 @@ func TestListDirectory(t *testing.T) { for _, name := range []string{"ping", "pong", "foo", "bar", "baz"} { err := ioutil.WriteFile(path.Join(dir, name), []byte{}, 0400) if err != nil { - t.Fatal(err.Error()) + t.Fatal(err) } } @@ -43,7 +43,7 @@ func TestListDirectory(t *testing.T) { got, err := ListDirectory(dir, filterFunc) if err != nil { - t.Fatal(err.Error()) + t.Fatal(err) } want := []string{"baz", "ping", "pong"} diff --git a/systemd/manager_test.go b/systemd/manager_test.go index 9e73ad49c..e9a7650cc 100644 --- a/systemd/manager_test.go +++ b/systemd/manager_test.go @@ -25,7 +25,7 @@ import ( func TestHashUnitFile(t *testing.T) { f, err := ioutil.TempFile("", "fleet-testing-") if err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } defer os.Remove(f.Name()) @@ -36,16 +36,16 @@ ExecStart=/usr/bin/sleep infinity ` if _, err := f.Write([]byte(contents)); err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } if err := f.Close(); err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } hash, err := hashUnitFile(f.Name()) if err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } want := "40ea6646945809f4b420a50475ee68503088f127" @@ -58,7 +58,7 @@ ExecStart=/usr/bin/sleep infinity func TestHashUnitFileDirectory(t *testing.T) { dir, err := ioutil.TempDir("", "fleet-testing-") if err != nil { - t.Fatal(err.Error()) + t.Fatal(err) } defer os.RemoveAll(dir) @@ -88,13 +88,13 @@ func TestHashUnitFileDirectory(t *testing.T) { for _, f := range fixtures { err := ioutil.WriteFile(path.Join(dir, f.name), []byte(f.contents), 0400) if err != nil { - t.Fatal(err.Error()) + t.Fatal(err) } } hashes, err := hashUnitFiles(dir) if err != nil { - t.Fatal(err.Error()) + t.Fatal(err) } got := make(map[string]string, len(hashes)) From c27234a16bd9ca6f127e310c36041a1bae732b67 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Wed, 24 Aug 2016 12:09:34 +0200 Subject: [PATCH 2/3] engine: check for an exact error code from etcd rpcRenewLeadership() should check for an exact error code from etcd, EcodeKeyNotFound, instead of comparing error string with a particular string pattern. Doing that, we can also avoid using err.Error(). --- engine/rpcengine.go | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/engine/rpcengine.go b/engine/rpcengine.go index 93e574eda..2e8504578 100644 --- a/engine/rpcengine.go +++ b/engine/rpcengine.go @@ -1,4 +1,4 @@ -// Copyright 2014 CoreOS, Inc. +// Copyright 2016 The fleet Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,9 +16,10 @@ package engine import ( "errors" - "strings" "time" + etcdErr "github.com/coreos/etcd/error" + "github.com/coreos/fleet/log" "github.com/coreos/fleet/machine" "github.com/coreos/fleet/pkg/lease" @@ -145,22 +146,23 @@ func rpcAcquireLeadership(reg registry.Registry, lManager lease.Manager, machID func rpcRenewLeadership(lManager lease.Manager, l lease.Lease, ver int, ttl time.Duration) lease.Lease { err := l.Renew(ttl) - if err != nil && strings.Contains(err.Error(), "Key not found") { - log.Errorf("Retry renew etcd operation that failed due to %v", err) - l, err = lManager.AcquireLease(engineLeaseName, l.MachineID(), ver, ttl) - if err != nil { - log.Errorf("Engine leadership re-acquisition failed: %v", err) - return nil - } else if l == nil { - log.Infof("Unable to re-acquire engine leadership") + if err != nil { + if eerr, ok := err.(*etcdErr.Error); ok && eerr.ErrorCode == etcdErr.EcodeKeyNotFound { + log.Errorf("Retry renew etcd operation that failed due to %v", err) + l, err = lManager.AcquireLease(engineLeaseName, l.MachineID(), ver, ttl) + if err != nil { + log.Errorf("Engine leadership re-acquisition failed: %v", err) + return nil + } else if l == nil { + log.Infof("Unable to re-acquire engine leadership") + return nil + } + log.Infof("Engine leadership re-acquired") + return l + } else { + log.Errorf("Engine leadership lost, renewal failed: %v", err) return nil } - log.Infof("Engine leadership re-acquired") - return l - - } else if err != nil { - log.Errorf("Engine leadership lost, renewal failed: %v", err) - return nil } log.Debugf("Engine leadership renewed") From dbc9102225be80c7857eed608c821dbff5be4346 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Wed, 24 Aug 2016 12:26:05 +0200 Subject: [PATCH 3/3] vendor: add github.com/coreos/etcd/error Add github.com/coreos/etcd/error for dependencies. --- glide.lock | 1 + glide.yaml | 1 + vendor/github.com/coreos/etcd/error/error.go | 162 +++++++++++++++++++ 3 files changed, 164 insertions(+) create mode 100644 vendor/github.com/coreos/etcd/error/error.go diff --git a/glide.lock b/glide.lock index 46d57ce1c..047610c77 100644 --- a/glide.lock +++ b/glide.lock @@ -9,6 +9,7 @@ imports: version: 4a8dce34cb7176d68b9031857e7a0b94e1413421 subpackages: - client + - error - pkg/pathutil - pkg/types - name: github.com/coreos/go-semver diff --git a/glide.yaml b/glide.yaml index f7a9c733e..5051e7a00 100644 --- a/glide.yaml +++ b/glide.yaml @@ -3,6 +3,7 @@ import: - package: github.com/coreos/etcd subpackages: - client + - error - package: github.com/coreos/go-semver subpackages: - semver diff --git a/vendor/github.com/coreos/etcd/error/error.go b/vendor/github.com/coreos/etcd/error/error.go new file mode 100644 index 000000000..8cf83cc71 --- /dev/null +++ b/vendor/github.com/coreos/etcd/error/error.go @@ -0,0 +1,162 @@ +// Copyright 2015 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package error describes errors in etcd project. When any change happens, +// Documentation/v2/errorcode.md needs to be updated correspondingly. +package error + +import ( + "encoding/json" + "fmt" + "net/http" +) + +var errors = map[int]string{ + // command related errors + EcodeKeyNotFound: "Key not found", + EcodeTestFailed: "Compare failed", //test and set + EcodeNotFile: "Not a file", + ecodeNoMorePeer: "Reached the max number of peers in the cluster", + EcodeNotDir: "Not a directory", + EcodeNodeExist: "Key already exists", // create + ecodeKeyIsPreserved: "The prefix of given key is a keyword in etcd", + EcodeRootROnly: "Root is read only", + EcodeDirNotEmpty: "Directory not empty", + ecodeExistingPeerAddr: "Peer address has existed", + EcodeUnauthorized: "The request requires user authentication", + + // Post form related errors + ecodeValueRequired: "Value is Required in POST form", + EcodePrevValueRequired: "PrevValue is Required in POST form", + EcodeTTLNaN: "The given TTL in POST form is not a number", + EcodeIndexNaN: "The given index in POST form is not a number", + ecodeValueOrTTLRequired: "Value or TTL is required in POST form", + ecodeTimeoutNaN: "The given timeout in POST form is not a number", + ecodeNameRequired: "Name is required in POST form", + ecodeIndexOrValueRequired: "Index or value is required", + ecodeIndexValueMutex: "Index and value cannot both be specified", + EcodeInvalidField: "Invalid field", + EcodeInvalidForm: "Invalid POST form", + EcodeRefreshValue: "Value provided on refresh", + EcodeRefreshTTLRequired: "A TTL must be provided on refresh", + + // raft related errors + EcodeRaftInternal: "Raft Internal Error", + EcodeLeaderElect: "During Leader Election", + + // etcd related errors + EcodeWatcherCleared: "watcher is cleared due to etcd recovery", + EcodeEventIndexCleared: "The event in requested index is outdated and cleared", + ecodeStandbyInternal: "Standby Internal Error", + ecodeInvalidActiveSize: "Invalid active size", + ecodeInvalidRemoveDelay: "Standby remove delay", + + // client related errors + ecodeClientInternal: "Client Internal Error", +} + +var errorStatus = map[int]int{ + EcodeKeyNotFound: http.StatusNotFound, + EcodeNotFile: http.StatusForbidden, + EcodeDirNotEmpty: http.StatusForbidden, + EcodeUnauthorized: http.StatusUnauthorized, + EcodeTestFailed: http.StatusPreconditionFailed, + EcodeNodeExist: http.StatusPreconditionFailed, + EcodeRaftInternal: http.StatusInternalServerError, + EcodeLeaderElect: http.StatusInternalServerError, +} + +const ( + EcodeKeyNotFound = 100 + EcodeTestFailed = 101 + EcodeNotFile = 102 + ecodeNoMorePeer = 103 + EcodeNotDir = 104 + EcodeNodeExist = 105 + ecodeKeyIsPreserved = 106 + EcodeRootROnly = 107 + EcodeDirNotEmpty = 108 + ecodeExistingPeerAddr = 109 + EcodeUnauthorized = 110 + + ecodeValueRequired = 200 + EcodePrevValueRequired = 201 + EcodeTTLNaN = 202 + EcodeIndexNaN = 203 + ecodeValueOrTTLRequired = 204 + ecodeTimeoutNaN = 205 + ecodeNameRequired = 206 + ecodeIndexOrValueRequired = 207 + ecodeIndexValueMutex = 208 + EcodeInvalidField = 209 + EcodeInvalidForm = 210 + EcodeRefreshValue = 211 + EcodeRefreshTTLRequired = 212 + + EcodeRaftInternal = 300 + EcodeLeaderElect = 301 + + EcodeWatcherCleared = 400 + EcodeEventIndexCleared = 401 + ecodeStandbyInternal = 402 + ecodeInvalidActiveSize = 403 + ecodeInvalidRemoveDelay = 404 + + ecodeClientInternal = 500 +) + +type Error struct { + ErrorCode int `json:"errorCode"` + Message string `json:"message"` + Cause string `json:"cause,omitempty"` + Index uint64 `json:"index"` +} + +func NewRequestError(errorCode int, cause string) *Error { + return NewError(errorCode, cause, 0) +} + +func NewError(errorCode int, cause string, index uint64) *Error { + return &Error{ + ErrorCode: errorCode, + Message: errors[errorCode], + Cause: cause, + Index: index, + } +} + +// Error is for the error interface +func (e Error) Error() string { + return e.Message + " (" + e.Cause + ")" +} + +func (e Error) toJsonString() string { + b, _ := json.Marshal(e) + return string(b) +} + +func (e Error) StatusCode() int { + status, ok := errorStatus[e.ErrorCode] + if !ok { + status = http.StatusBadRequest + } + return status +} + +func (e Error) WriteTo(w http.ResponseWriter) { + w.Header().Add("X-Etcd-Index", fmt.Sprint(e.Index)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(e.StatusCode()) + fmt.Fprintln(w, e.toJsonString()) +}