From 72f279c4c3b96085e178e878ad09d7bbfb091e72 Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Wed, 6 Nov 2019 13:29:42 +0100 Subject: [PATCH] *: improve error handling * objectstorage: remove `types` package and move `ErrNotExist` in base package * objectstorage: Implement .Is and add helper `IsErrNotExist` for `ErrNotExist` * util: Rename `ErrNotFound` to `ErrNotExist` * util: Add `IsErr*` helpers and use them in place of `errors.Is()` * datamanager: add `ErrNoDataStatus` to report when there's not data status in ost * runservice/common: remove `ErrNotExist` and use errors in util package --- internal/datamanager/data.go | 22 +++++++----- internal/datamanager/datamanager_test.go | 13 +++---- internal/datamanager/wal.go | 13 +++---- internal/objectstorage/objectstorage.go | 21 ++++++++++- internal/objectstorage/posix.go | 6 ++-- internal/objectstorage/posixflat.go | 6 ++-- internal/objectstorage/s3.go | 4 +-- internal/services/configstore/action/org.go | 2 +- .../services/configstore/action/project.go | 2 +- .../configstore/action/projectgroup.go | 6 ++-- .../services/configstore/action/secret.go | 2 +- internal/services/configstore/action/user.go | 2 +- internal/services/configstore/api/api.go | 22 ++++++------ internal/services/configstore/api/org.go | 2 +- .../services/configstore/api/remotesource.go | 2 +- internal/services/configstore/api/user.go | 8 ++--- .../services/configstore/readdb/readdb.go | 5 +-- internal/services/gateway/action/action.go | 2 +- internal/services/gateway/api/api.go | 22 ++++++------ internal/services/gateway/api/run.go | 2 +- internal/services/runservice/action/action.go | 2 +- internal/services/runservice/api/api.go | 36 +++++++++---------- internal/services/runservice/api/executor.go | 18 +++++----- internal/services/runservice/common/common.go | 12 ------- internal/services/runservice/readdb/readdb.go | 5 +-- internal/services/runservice/scheduler.go | 6 ++-- internal/services/runservice/store/store.go | 2 +- internal/util/errors.go | 36 +++++++++++++++---- 28 files changed, 155 insertions(+), 126 deletions(-) diff --git a/internal/datamanager/data.go b/internal/datamanager/data.go index b557e39..cdc5c8c 100644 --- a/internal/datamanager/data.go +++ b/internal/datamanager/data.go @@ -28,11 +28,15 @@ import ( "agola.io/agola/internal/objectstorage" "agola.io/agola/internal/sequence" + "agola.io/agola/internal/util" uuid "github.com/satori/go.uuid" errors "golang.org/x/xerrors" ) +// ErrNoDataStatus represent when there's no data status files in the ost +var ErrNoDataStatus = errors.New("no data status files") + const ( DefaultMaxDataFileSize = 10 * 1024 * 1024 dataStatusToKeep = 3 @@ -165,7 +169,7 @@ func (d *DataManager) writeDataSnapshot(ctx context.Context, wals []*WalData) er } curDataStatus, err := d.GetLastDataStatus() - if err != nil && err != objectstorage.ErrNotExist { + if err != nil && !errors.Is(err, ErrNoDataStatus) { return err } @@ -321,10 +325,10 @@ func (d *DataManager) writeDataType(ctx context.Context, wi walIndex, dataType s if actionGroup.DataStatusFile != nil { // TODO(sgotti) instead of reading all entries in memory decode it's contents one by one when needed oldDataf, err := d.ost.ReadObject(d.DataFilePath(dataType, actionGroup.DataStatusFile.ID)) - if err != nil && err != objectstorage.ErrNotExist { + if err != nil && !objectstorage.IsNotExist(err) { return nil, err } - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { dec := json.NewDecoder(oldDataf) for { var de *DataEntry @@ -481,7 +485,7 @@ func (d *DataManager) Read(dataType, id string) (io.Reader, error) { var matchingDataFileID string // get the matching data file for the action entry ID if len(curFiles[dataType]) == 0 { - return nil, objectstorage.ErrNotExist + return nil, util.NewErrNotExist(errors.Errorf("datatype %q doesn't exists", dataType)) } matchingDataFileID = curFiles[dataType][0].ID @@ -507,7 +511,7 @@ func (d *DataManager) Read(dataType, id string) (io.Reader, error) { pos, ok := dataFileIndex.Index[id] if !ok { - return nil, objectstorage.ErrNotExist + return nil, util.NewErrNotExist(errors.Errorf("datatype %q, id %q doesn't exists", dataType, id)) } dataf, err := d.ost.ReadObject(d.DataFilePath(dataType, matchingDataFileID)) @@ -560,7 +564,7 @@ func (d *DataManager) GetFirstDataStatusSequences(n int) ([]*sequence.Sequence, } if len(dataStatusSequences) == 0 { - return nil, objectstorage.ErrNotExist + return nil, ErrNoDataStatus } return dataStatusSequences, nil @@ -601,7 +605,7 @@ func (d *DataManager) GetLastDataStatusSequences(n int) ([]*sequence.Sequence, e }) if len(dataStatusSequences) == 0 { - return nil, objectstorage.ErrNotExist + return nil, ErrNoDataStatus } return dataStatusSequences, nil @@ -862,7 +866,7 @@ func (d *DataManager) cleanOldCheckpoints(ctx context.Context, dataStatusSequenc if _, ok := dataStatusPathsMap[object.Path]; !ok { d.log.Infof("removing %q", object.Path) if err := d.ost.DeleteObject(object.Path); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { return err } } @@ -930,7 +934,7 @@ func (d *DataManager) cleanOldCheckpoints(ctx context.Context, dataStatusSequenc if _, ok := files[pne]; !ok { d.log.Infof("removing %q", object.Path) if err := d.ost.DeleteObject(object.Path); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { return err } } diff --git a/internal/datamanager/datamanager_test.go b/internal/datamanager/datamanager_test.go index 470a223..19c4b52 100644 --- a/internal/datamanager/datamanager_test.go +++ b/internal/datamanager/datamanager_test.go @@ -32,11 +32,12 @@ import ( slog "agola.io/agola/internal/log" "agola.io/agola/internal/objectstorage" "agola.io/agola/internal/testutil" - "github.com/google/go-cmp/cmp" - errors "golang.org/x/xerrors" + "agola.io/agola/internal/util" + "github.com/google/go-cmp/cmp" "go.uber.org/zap" "go.uber.org/zap/zapcore" + errors "golang.org/x/xerrors" ) var level = zap.NewAtomicLevelAt(zapcore.InfoLevel) @@ -558,8 +559,8 @@ func TestReadObject(t *testing.T) { // should not exists _, _, err = dm.ReadObject("datatype01", "object1", nil) - if err != objectstorage.ErrNotExist { - t.Fatalf("expected err %v, got: %v", objectstorage.ErrNotExist, err) + if !util.IsNotExist(err) { + t.Fatalf("expected err %v, got: %v", &util.ErrNotExist{}, err) } // should exist _, _, err = dm.ReadObject("datatype01", "object19", nil) @@ -582,8 +583,8 @@ func TestReadObject(t *testing.T) { // should not exists _, _, err = dm.ReadObject("datatype01", "object1", nil) - if err != objectstorage.ErrNotExist { - t.Fatalf("expected err %v, got: %v", objectstorage.ErrNotExist, err) + if !util.IsNotExist(err) { + t.Fatalf("expected err %v, got: %v", &util.ErrNotExist{}, err) } // should exist _, _, err = dm.ReadObject("datatype01", "object19", nil) diff --git a/internal/datamanager/wal.go b/internal/datamanager/wal.go index 3f67086..d752094 100644 --- a/internal/datamanager/wal.go +++ b/internal/datamanager/wal.go @@ -28,6 +28,7 @@ import ( "agola.io/agola/internal/etcd" "agola.io/agola/internal/objectstorage" "agola.io/agola/internal/sequence" + "agola.io/agola/internal/util" uuid "github.com/satori/go.uuid" etcdclientv3 "go.etcd.io/etcd/clientv3" @@ -123,7 +124,7 @@ func (d *DataManager) ReadObject(dataType, id string, cgNames []string) (io.Read } } } - return nil, nil, errors.Errorf("no datatype %q, id %q in wal %s", dataType, id, walseq) + return nil, nil, util.NewErrNotExist(errors.Errorf("no datatype %q, id %q in wal %s", dataType, id, walseq)) } f, err := d.Read(dataType, id) @@ -132,7 +133,7 @@ func (d *DataManager) ReadObject(dataType, id string, cgNames []string) (io.Read func (d *DataManager) HasOSTWal(walseq string) (bool, error) { _, err := d.ost.Stat(d.storageWalStatusFile(walseq) + ".committed") - if err == objectstorage.ErrNotExist { + if objectstorage.IsNotExist(err) { return false, nil } if err != nil { @@ -909,7 +910,7 @@ func (d *DataManager) storageWalCleaner(ctx context.Context) error { walStatusFilePath := d.storageWalDataFile(header.WalDataFileID) d.log.Infof("removing %q", walStatusFilePath) if err := d.ost.DeleteObject(walStatusFilePath); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { return err } } @@ -917,7 +918,7 @@ func (d *DataManager) storageWalCleaner(ctx context.Context) error { // then remove wal status files d.log.Infof("removing %q", object.Path) if err := d.ost.DeleteObject(object.Path); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { return err } } @@ -928,7 +929,7 @@ func (d *DataManager) storageWalCleaner(ctx context.Context) error { if ext == ".checkpointed" { d.log.Infof("removing %q", object.Path) if err := d.ost.DeleteObject(object.Path); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { return err } } @@ -1149,7 +1150,7 @@ func (d *DataManager) InitEtcd(ctx context.Context, dataStatus *DataStatus) erro firstWal = dataStatus.WalSequence } else { dataStatus, err = d.GetLastDataStatus() - if err != nil && err != objectstorage.ErrNotExist { + if err != nil && !errors.Is(err, ErrNoDataStatus) { return err } // set the first wal to import in etcd if there's a snapshot. In this way we'll diff --git a/internal/objectstorage/objectstorage.go b/internal/objectstorage/objectstorage.go index 08d7f71..3a6c0de 100644 --- a/internal/objectstorage/objectstorage.go +++ b/internal/objectstorage/objectstorage.go @@ -34,7 +34,26 @@ type Storage interface { List(prefix, startWith, delimiter string, doneCh <-chan struct{}) <-chan ObjectInfo } -var ErrNotExist = errors.New("does not exist") +type ErrNotExist struct { + err error +} + +func NewErrNotExist(err error) error { + return &ErrNotExist{err: err} +} + +func (e *ErrNotExist) Error() string { + return e.err.Error() +} + +func (*ErrNotExist) Is(err error) bool { + _, ok := err.(*ErrNotExist) + return ok +} + +func IsNotExist(err error) bool { + return errors.Is(err, &ErrNotExist{}) +} type ReadSeekCloser interface { io.Reader diff --git a/internal/objectstorage/posix.go b/internal/objectstorage/posix.go index 75e1ff6..88d0028 100644 --- a/internal/objectstorage/posix.go +++ b/internal/objectstorage/posix.go @@ -65,7 +65,7 @@ func (s *PosixStorage) Stat(p string) (*ObjectInfo, error) { fi, err := os.Stat(fspath) if err != nil { if os.IsNotExist(err) { - return nil, ErrNotExist + return nil, NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return nil, err } @@ -81,7 +81,7 @@ func (s *PosixStorage) ReadObject(p string) (ReadSeekCloser, error) { f, err := os.Open(fspath) if err != nil && os.IsNotExist(err) { - return nil, ErrNotExist + return nil, NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return f, err } @@ -114,7 +114,7 @@ func (s *PosixStorage) DeleteObject(p string) error { if err := os.Remove(fspath); err != nil { if os.IsNotExist(err) { - return ErrNotExist + return NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return err } diff --git a/internal/objectstorage/posixflat.go b/internal/objectstorage/posixflat.go index 85dbad5..de4a316 100644 --- a/internal/objectstorage/posixflat.go +++ b/internal/objectstorage/posixflat.go @@ -239,7 +239,7 @@ func (s *PosixFlatStorage) Stat(p string) (*ObjectInfo, error) { fi, err := os.Stat(fspath) if err != nil { if os.IsNotExist(err) { - return nil, ErrNotExist + return nil, NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return nil, err } @@ -255,7 +255,7 @@ func (s *PosixFlatStorage) ReadObject(p string) (ReadSeekCloser, error) { f, err := os.Open(fspath) if err != nil && os.IsNotExist(err) { - return nil, ErrNotExist + return nil, NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return f, err } @@ -288,7 +288,7 @@ func (s *PosixFlatStorage) DeleteObject(p string) error { if err := os.Remove(fspath); err != nil { if os.IsNotExist(err) { - return ErrNotExist + return NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return err } diff --git a/internal/objectstorage/s3.go b/internal/objectstorage/s3.go index da33d1c..7b26413 100644 --- a/internal/objectstorage/s3.go +++ b/internal/objectstorage/s3.go @@ -65,7 +65,7 @@ func (s *S3Storage) Stat(p string) (*ObjectInfo, error) { if err != nil { merr := minio.ToErrorResponse(err) if merr.StatusCode == http.StatusNotFound { - return nil, ErrNotExist + return nil, NewErrNotExist(errors.Errorf("object %q doesn't exist", p)) } return nil, merr } @@ -77,7 +77,7 @@ func (s *S3Storage) ReadObject(filepath string) (ReadSeekCloser, error) { if _, err := s.minioClient.StatObject(s.bucket, filepath, minio.StatObjectOptions{}); err != nil { merr := minio.ToErrorResponse(err) if merr.StatusCode == http.StatusNotFound { - return nil, ErrNotExist + return nil, NewErrNotExist(errors.Errorf("object %q doesn't exist", filepath)) } return nil, merr } diff --git a/internal/services/configstore/action/org.go b/internal/services/configstore/action/org.go index 761fe1e..c26f028 100644 --- a/internal/services/configstore/action/org.go +++ b/internal/services/configstore/action/org.go @@ -51,7 +51,7 @@ func (h *ActionHandler) GetOrgMembers(ctx context.Context, orgRef string) ([]*Or return err } if org == nil { - return util.NewErrNotFound(errors.Errorf("org %q doesn't exist", orgRef)) + return util.NewErrNotExist(errors.Errorf("org %q doesn't exist", orgRef)) } orgUsers, err = h.readDB.GetOrgUsers(tx, org.ID) diff --git a/internal/services/configstore/action/project.go b/internal/services/configstore/action/project.go index bafc690..a7c01ca 100644 --- a/internal/services/configstore/action/project.go +++ b/internal/services/configstore/action/project.go @@ -76,7 +76,7 @@ func (h *ActionHandler) GetProject(ctx context.Context, projectRef string) (*typ } if project == nil { - return nil, util.NewErrNotFound(errors.Errorf("project %q doesn't exist", projectRef)) + return nil, util.NewErrNotExist(errors.Errorf("project %q doesn't exist", projectRef)) } return project, nil diff --git a/internal/services/configstore/action/projectgroup.go b/internal/services/configstore/action/projectgroup.go index a17b634..bcf8a38 100644 --- a/internal/services/configstore/action/projectgroup.go +++ b/internal/services/configstore/action/projectgroup.go @@ -41,7 +41,7 @@ func (h *ActionHandler) GetProjectGroup(ctx context.Context, projectGroupRef str } if projectGroup == nil { - return nil, util.NewErrNotFound(errors.Errorf("project group %q doesn't exist", projectGroupRef)) + return nil, util.NewErrNotExist(errors.Errorf("project group %q doesn't exist", projectGroupRef)) } return projectGroup, nil @@ -57,7 +57,7 @@ func (h *ActionHandler) GetProjectGroupSubgroups(ctx context.Context, projectGro } if projectGroup == nil { - return util.NewErrNotFound(errors.Errorf("project group %q doesn't exist", projectGroupRef)) + return util.NewErrNotExist(errors.Errorf("project group %q doesn't exist", projectGroupRef)) } projectGroups, err = h.readDB.GetProjectGroupSubgroups(tx, projectGroup.ID) @@ -80,7 +80,7 @@ func (h *ActionHandler) GetProjectGroupProjects(ctx context.Context, projectGrou } if projectGroup == nil { - return util.NewErrNotFound(errors.Errorf("project group %q doesn't exist", projectGroupRef)) + return util.NewErrNotExist(errors.Errorf("project group %q doesn't exist", projectGroupRef)) } projects, err = h.readDB.GetProjectGroupProjects(tx, projectGroup.ID) diff --git a/internal/services/configstore/action/secret.go b/internal/services/configstore/action/secret.go index a64576c..bf846a6 100644 --- a/internal/services/configstore/action/secret.go +++ b/internal/services/configstore/action/secret.go @@ -39,7 +39,7 @@ func (h *ActionHandler) GetSecret(ctx context.Context, secretID string) (*types. } if secret == nil { - return nil, util.NewErrNotFound(errors.Errorf("secret %q doesn't exist", secretID)) + return nil, util.NewErrNotExist(errors.Errorf("secret %q doesn't exist", secretID)) } return secret, nil diff --git a/internal/services/configstore/action/user.go b/internal/services/configstore/action/user.go index 4a228b7..cdad65a 100644 --- a/internal/services/configstore/action/user.go +++ b/internal/services/configstore/action/user.go @@ -651,7 +651,7 @@ func (h *ActionHandler) GetUserOrgs(ctx context.Context, userRef string) ([]*Use return err } if user == nil { - return util.NewErrNotFound(errors.Errorf("user %q doesn't exist", userRef)) + return util.NewErrNotExist(errors.Errorf("user %q doesn't exist", userRef)) } userOrgs, err = h.readDB.GetUserOrgs(tx, user.ID) diff --git a/internal/services/configstore/api/api.go b/internal/services/configstore/api/api.go index a552922..825ef17 100644 --- a/internal/services/configstore/api/api.go +++ b/internal/services/configstore/api/api.go @@ -34,23 +34,23 @@ func ErrorResponseFromError(err error) *ErrorResponse { var aerr error // use inner errors if of these types switch { - case errors.Is(err, &util.ErrBadRequest{}): + case util.IsBadRequest(err): var cerr *util.ErrBadRequest errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrNotFound{}): - var cerr *util.ErrNotFound + case util.IsNotExist(err): + var cerr *util.ErrNotExist errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrForbidden{}): + case util.IsForbidden(err): var cerr *util.ErrForbidden errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrUnauthorized{}): + case util.IsUnauthorized(err): var cerr *util.ErrUnauthorized errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrInternal{}): + case util.IsInternal(err): var cerr *util.ErrInternal errors.As(err, &cerr) aerr = cerr @@ -76,19 +76,19 @@ func httpError(w http.ResponseWriter, err error) bool { return true } switch { - case errors.Is(err, &util.ErrBadRequest{}): + case util.IsBadRequest(err): w.WriteHeader(http.StatusBadRequest) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrNotFound{}): + case util.IsNotExist(err): w.WriteHeader(http.StatusNotFound) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrForbidden{}): + case util.IsForbidden(err): w.WriteHeader(http.StatusForbidden) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrUnauthorized{}): + case util.IsUnauthorized(err): w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrInternal{}): + case util.IsInternal(err): w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write(resj) default: diff --git a/internal/services/configstore/api/org.go b/internal/services/configstore/api/org.go index 5ad4df3..3d427b7 100644 --- a/internal/services/configstore/api/org.go +++ b/internal/services/configstore/api/org.go @@ -58,7 +58,7 @@ func (h *OrgHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } if org == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("org %q doesn't exist", orgRef))) + httpError(w, util.NewErrNotExist(errors.Errorf("org %q doesn't exist", orgRef))) return } diff --git a/internal/services/configstore/api/remotesource.go b/internal/services/configstore/api/remotesource.go index baa5012..d24dcfd 100644 --- a/internal/services/configstore/api/remotesource.go +++ b/internal/services/configstore/api/remotesource.go @@ -57,7 +57,7 @@ func (h *RemoteSourceHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) } if remoteSource == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("remote source %q doesn't exist", rsRef))) + httpError(w, util.NewErrNotExist(errors.Errorf("remote source %q doesn't exist", rsRef))) return } diff --git a/internal/services/configstore/api/user.go b/internal/services/configstore/api/user.go index 0c639d5..6485856 100644 --- a/internal/services/configstore/api/user.go +++ b/internal/services/configstore/api/user.go @@ -58,7 +58,7 @@ func (h *UserHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } if user == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("user %q doesn't exist", userRef))) + httpError(w, util.NewErrNotExist(errors.Errorf("user %q doesn't exist", userRef))) return } @@ -235,7 +235,7 @@ func (h *UsersHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } if user == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("user with required token doesn't exist"))) + httpError(w, util.NewErrNotExist(errors.Errorf("user with required token doesn't exist"))) return } users = []*types.User{user} @@ -253,7 +253,7 @@ func (h *UsersHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } if user == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("user with linked account %q token doesn't exist", linkedAccountID))) + httpError(w, util.NewErrNotExist(errors.Errorf("user with linked account %q token doesn't exist", linkedAccountID))) return } users = []*types.User{user} @@ -272,7 +272,7 @@ func (h *UsersHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } if user == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("user with remote user %q for remote source %q token doesn't exist", remoteUserID, remoteSourceID))) + httpError(w, util.NewErrNotExist(errors.Errorf("user with remote user %q for remote source %q token doesn't exist", remoteUserID, remoteSourceID))) return } users = []*types.User{user} diff --git a/internal/services/configstore/readdb/readdb.go b/internal/services/configstore/readdb/readdb.go index 707a702..ff8f579 100644 --- a/internal/services/configstore/readdb/readdb.go +++ b/internal/services/configstore/readdb/readdb.go @@ -129,12 +129,9 @@ func (r *ReadDB) ResetDB(ctx context.Context) error { func (r *ReadDB) SyncFromDump(ctx context.Context) (string, error) { dumpIndex, err := r.dm.GetLastDataStatus() - if err != nil && err != objectstorage.ErrNotExist { + if err != nil { return "", err } - if err == objectstorage.ErrNotExist { - return "", nil - } for dataType, files := range dumpIndex.Files { for _, file := range files { dumpf, err := r.ost.ReadObject(r.dm.DataFilePath(dataType, file.ID)) diff --git a/internal/services/gateway/action/action.go b/internal/services/gateway/action/action.go index bdd61cc..4749098 100644 --- a/internal/services/gateway/action/action.go +++ b/internal/services/gateway/action/action.go @@ -57,7 +57,7 @@ func ErrFromRemote(resp *http.Response, err error) error { case http.StatusBadRequest: return util.NewErrBadRequest(err) case http.StatusNotFound: - return util.NewErrNotFound(err) + return util.NewErrNotExist(err) } } diff --git a/internal/services/gateway/api/api.go b/internal/services/gateway/api/api.go index d18b5b8..a3ced3b 100644 --- a/internal/services/gateway/api/api.go +++ b/internal/services/gateway/api/api.go @@ -34,23 +34,23 @@ func ErrorResponseFromError(err error) *ErrorResponse { var aerr error // use inner errors if of these types switch { - case errors.Is(err, &util.ErrBadRequest{}): + case util.IsBadRequest(err): var cerr *util.ErrBadRequest errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrNotFound{}): - var cerr *util.ErrNotFound + case util.IsNotExist(err): + var cerr *util.ErrNotExist errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrForbidden{}): + case util.IsForbidden(err): var cerr *util.ErrForbidden errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrUnauthorized{}): + case util.IsUnauthorized(err): var cerr *util.ErrUnauthorized errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrInternal{}): + case util.IsInternal(err): var cerr *util.ErrInternal errors.As(err, &cerr) aerr = cerr @@ -76,19 +76,19 @@ func httpError(w http.ResponseWriter, err error) bool { return true } switch { - case errors.Is(err, &util.ErrBadRequest{}): + case util.IsBadRequest(err): w.WriteHeader(http.StatusBadRequest) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrNotFound{}): + case util.IsNotExist(err): w.WriteHeader(http.StatusNotFound) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrForbidden{}): + case util.IsForbidden(err): w.WriteHeader(http.StatusForbidden) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrUnauthorized{}): + case util.IsUnauthorized(err): w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrInternal{}): + case util.IsInternal(err): w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write(resj) default: diff --git a/internal/services/gateway/api/run.go b/internal/services/gateway/api/run.go index e6a866a..f54f291 100644 --- a/internal/services/gateway/api/run.go +++ b/internal/services/gateway/api/run.go @@ -197,7 +197,7 @@ func (h *RuntaskHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { rt, ok := run.Tasks[taskID] if !ok { - httpError(w, util.NewErrNotFound(errors.Errorf("run %q task %q not found", runID, taskID))) + httpError(w, util.NewErrNotExist(errors.Errorf("run %q task %q not found", runID, taskID))) return } rct := rc.Tasks[rt.ID] diff --git a/internal/services/runservice/action/action.go b/internal/services/runservice/action/action.go index 9da61d3..2010fcd 100644 --- a/internal/services/runservice/action/action.go +++ b/internal/services/runservice/action/action.go @@ -592,7 +592,7 @@ func (h *ActionHandler) GetExecutorTask(ctx context.Context, etID string) (*type return nil, err } if et == nil { - return nil, util.NewErrNotFound(errors.Errorf("executor task %q not found", etID)) + return nil, util.NewErrNotExist(errors.Errorf("executor task %q not found", etID)) } r, _, err := store.GetRun(ctx, h.e, et.Spec.RunID) diff --git a/internal/services/runservice/api/api.go b/internal/services/runservice/api/api.go index fd2ae22..65d27f5 100644 --- a/internal/services/runservice/api/api.go +++ b/internal/services/runservice/api/api.go @@ -50,23 +50,23 @@ func ErrorResponseFromError(err error) *ErrorResponse { var aerr error // use inner errors if of these types switch { - case errors.Is(err, &util.ErrBadRequest{}): + case util.IsBadRequest(err): var cerr *util.ErrBadRequest errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrNotFound{}): - var cerr *util.ErrNotFound + case util.IsNotExist(err): + var cerr *util.ErrNotExist errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrForbidden{}): + case util.IsForbidden(err): var cerr *util.ErrForbidden errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrUnauthorized{}): + case util.IsUnauthorized(err): var cerr *util.ErrUnauthorized errors.As(err, &cerr) aerr = cerr - case errors.Is(err, &util.ErrInternal{}): + case util.IsInternal(err): var cerr *util.ErrInternal errors.As(err, &cerr) aerr = cerr @@ -92,19 +92,19 @@ func httpError(w http.ResponseWriter, err error) bool { return true } switch { - case errors.Is(err, &util.ErrBadRequest{}): + case util.IsBadRequest(err): w.WriteHeader(http.StatusBadRequest) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrNotFound{}): + case util.IsNotExist(err): w.WriteHeader(http.StatusNotFound) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrForbidden{}): + case util.IsForbidden(err): w.WriteHeader(http.StatusForbidden) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrUnauthorized{}): + case util.IsUnauthorized(err): w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write(resj) - case errors.Is(err, &util.ErrInternal{}): + case util.IsInternal(err): w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write(resj) default: @@ -193,8 +193,8 @@ func (h *LogsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err, sendError := h.readTaskLogs(ctx, runID, taskID, setup, step, w, follow); err != nil { h.log.Errorf("err: %+v", err) if sendError { - switch err.(type) { - case common.ErrNotExist: + switch { + case util.IsNotExist(err): http.Error(w, err.Error(), http.StatusNotFound) default: http.Error(w, err.Error(), http.StatusInternalServerError) @@ -230,8 +230,8 @@ func (h *LogsHandler) readTaskLogs(ctx context.Context, runID, taskID string, se } f, err := h.ost.ReadObject(logPath) if err != nil { - if err == objectstorage.ErrNotExist { - return common.NewErrNotExist(err), true + if objectstorage.IsNotExist(err) { + return util.NewErrNotExist(err), true } return err, true } @@ -248,7 +248,7 @@ func (h *LogsHandler) readTaskLogs(ctx context.Context, runID, taskID string, se return err, true } if executor == nil { - return common.NewErrNotExist(errors.Errorf("executor with id %q doesn't exist", et.Spec.ExecutorID)), true + return util.NewErrNotExist(errors.Errorf("executor with id %q doesn't exist", et.Spec.ExecutorID)), true } var url string @@ -267,7 +267,7 @@ func (h *LogsHandler) readTaskLogs(ctx context.Context, runID, taskID string, se defer req.Body.Close() if req.StatusCode != http.StatusOK { if req.StatusCode == http.StatusNotFound { - return common.NewErrNotExist(errors.New("no log on executor")), true + return util.NewErrNotExist(errors.New("no log on executor")), true } return errors.Errorf("received http status: %d", req.StatusCode), true } @@ -393,7 +393,7 @@ func (h *RunHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } if run == nil { - httpError(w, util.NewErrNotFound(errors.Errorf("run %q doesn't exist", runID))) + httpError(w, util.NewErrNotExist(errors.Errorf("run %q doesn't exist", runID))) return } diff --git a/internal/services/runservice/api/executor.go b/internal/services/runservice/api/executor.go index 2c78963..f58a1ba 100644 --- a/internal/services/runservice/api/executor.go +++ b/internal/services/runservice/api/executor.go @@ -234,8 +234,8 @@ func (h *ArchivesHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("Cache-Control", "no-cache") if err := h.readArchive(taskID, step, w); err != nil { - switch err.(type) { - case common.ErrNotExist: + switch { + case util.IsNotExist(err): http.Error(w, err.Error(), http.StatusNotFound) default: http.Error(w, err.Error(), http.StatusInternalServerError) @@ -248,8 +248,8 @@ func (h *ArchivesHandler) readArchive(rtID string, step int, w io.Writer) error archivePath := store.OSTRunTaskArchivePath(rtID, step) f, err := h.ost.ReadObject(archivePath) if err != nil { - if err == objectstorage.ErrNotExist { - return common.NewErrNotExist(err) + if objectstorage.IsNotExist(err) { + return util.NewErrNotExist(err) } return err } @@ -307,8 +307,8 @@ func (h *CacheHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("Cache-Control", "no-cache") if err := h.readCache(matchedKey, w); err != nil { - switch err.(type) { - case common.ErrNotExist: + switch { + case util.IsNotExist(err): http.Error(w, err.Error(), http.StatusNotFound) default: http.Error(w, err.Error(), http.StatusInternalServerError) @@ -344,7 +344,7 @@ func matchCache(ost *objectstorage.ObjStorage, key string, prefix bool) (string, } _, err := ost.Stat(cachePath) - if err == objectstorage.ErrNotExist { + if objectstorage.IsNotExist(err) { return "", nil } if err != nil { @@ -357,8 +357,8 @@ func (h *CacheHandler) readCache(key string, w io.Writer) error { cachePath := store.OSTCachePath(key) f, err := h.ost.ReadObject(cachePath) if err != nil { - if err == objectstorage.ErrNotExist { - return common.NewErrNotExist(err) + if objectstorage.IsNotExist(err) { + return util.NewErrNotExist(err) } return err } diff --git a/internal/services/runservice/common/common.go b/internal/services/runservice/common/common.go index 85a49f6..e5aeb08 100644 --- a/internal/services/runservice/common/common.go +++ b/internal/services/runservice/common/common.go @@ -28,18 +28,6 @@ const ( MaxCacheKeyLength = 200 ) -type ErrNotExist struct { - err error -} - -func NewErrNotExist(err error) error { - return ErrNotExist{err: err} -} - -func (e ErrNotExist) Error() string { - return e.err.Error() -} - var ( EtcdSchedulerBaseDir = "scheduler" diff --git a/internal/services/runservice/readdb/readdb.go b/internal/services/runservice/readdb/readdb.go index f971d63..52ab5c0 100644 --- a/internal/services/runservice/readdb/readdb.go +++ b/internal/services/runservice/readdb/readdb.go @@ -667,12 +667,9 @@ func (r *ReadDB) SyncObjectStorage(ctx context.Context) error { func (r *ReadDB) SyncFromDump(ctx context.Context) (string, error) { dumpIndex, err := r.dm.GetLastDataStatus() - if err != nil && err != objectstorage.ErrNotExist { + if err != nil { return "", err } - if err == objectstorage.ErrNotExist { - return "", nil - } for dataType, files := range dumpIndex.Files { for _, file := range files { dumpf, err := r.ost.ReadObject(r.dm.DataFilePath(dataType, file.ID)) diff --git a/internal/services/runservice/scheduler.go b/internal/services/runservice/scheduler.go index cfd5387..eb7ba90 100644 --- a/internal/services/runservice/scheduler.go +++ b/internal/services/runservice/scheduler.go @@ -865,7 +865,7 @@ func (s *Runservice) runTasksUpdater(ctx context.Context) error { func (s *Runservice) OSTFileExists(path string) (bool, error) { _, err := s.ost.Stat(path) - if err != nil && err != objectstorage.ErrNotExist { + if err != nil && !objectstorage.IsNotExist(err) { return false, err } return err == nil, nil @@ -1359,7 +1359,7 @@ func (s *Runservice) cacheCleaner(ctx context.Context, cacheExpireInterval time. } if object.LastModified.Add(cacheExpireInterval).Before(time.Now()) { if err := s.ost.DeleteObject(object.Path); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { log.Warnf("failed to delete cache object %q: %v", object.Path, err) } } @@ -1411,7 +1411,7 @@ func (s *Runservice) workspaceCleaner(ctx context.Context, workspaceExpireInterv } if object.LastModified.Add(workspaceExpireInterval).Before(time.Now()) { if err := s.ost.DeleteObject(object.Path); err != nil { - if err != objectstorage.ErrNotExist { + if !objectstorage.IsNotExist(err) { log.Warnf("failed to delete workspace object %q: %v", object.Path, err) } } diff --git a/internal/services/runservice/store/store.go b/internal/services/runservice/store/store.go index 7a86552..35ae086 100644 --- a/internal/services/runservice/store/store.go +++ b/internal/services/runservice/store/store.go @@ -504,7 +504,7 @@ func GetRunEtcdOrOST(ctx context.Context, e *etcd.Store, dm *datamanager.DataMan } if r == nil { r, err = OSTGetRun(dm, runID) - if err != nil && err != objectstorage.ErrNotExist { + if err != nil && !objectstorage.IsNotExist(err) { return nil, err } } diff --git a/internal/util/errors.go b/internal/util/errors.go index cac2dc8..27119ad 100644 --- a/internal/util/errors.go +++ b/internal/util/errors.go @@ -16,6 +16,8 @@ package util import ( "strings" + + errors "golang.org/x/xerrors" ) // Errors is an error that contains multiple errors @@ -75,25 +77,33 @@ func (*ErrBadRequest) Is(err error) bool { return ok } -// ErrNotFound represent a not found error +func IsBadRequest(err error) bool { + return errors.Is(err, &ErrBadRequest{}) +} + +// ErrNotExist represent a not exist error // it's used to differentiate an internal error from an user error -type ErrNotFound struct { +type ErrNotExist struct { Err error } -func (e *ErrNotFound) Error() string { +func (e *ErrNotExist) Error() string { return e.Err.Error() } -func NewErrNotFound(err error) *ErrNotFound { - return &ErrNotFound{Err: err} +func NewErrNotExist(err error) *ErrNotExist { + return &ErrNotExist{Err: err} } -func (*ErrNotFound) Is(err error) bool { - _, ok := err.(*ErrNotFound) +func (*ErrNotExist) Is(err error) bool { + _, ok := err.(*ErrNotExist) return ok } +func IsNotExist(err error) bool { + return errors.Is(err, &ErrNotExist{}) +} + // ErrForbidden represent an error caused by an forbidden operation // it's used to differentiate an internal error from an user error type ErrForbidden struct { @@ -113,6 +123,10 @@ func (*ErrForbidden) Is(err error) bool { return ok } +func IsForbidden(err error) bool { + return errors.Is(err, &ErrForbidden{}) +} + // ErrUnauthorized represent an error caused by an unauthorized request // it's used to differentiate an internal error from an user error type ErrUnauthorized struct { @@ -132,6 +146,10 @@ func (*ErrUnauthorized) Is(err error) bool { return ok } +func IsUnauthorized(err error) bool { + return errors.Is(err, &ErrUnauthorized{}) +} + type ErrInternal struct { Err error } @@ -149,3 +167,7 @@ func (*ErrInternal) Is(err error) bool { _, ok := err.(*ErrInternal) return ok } + +func IsInternal(err error) bool { + return errors.Is(err, &ErrInternal{}) +}