Replaces errutil with tools/pkg/merrors; Fixes very ugly misuse of mu…

…lti-error.

Previous multi-error implementation could cause very ugly bug of returnig empty multi-error
that should be treated as success not error by API, but if .Err() is not invoked it will be
used as non nil error.

There were 9 places where we had this bug in handler due to MultiError lib allowing to do so.

Signed-off-by: Bartlomiej Plotka <[email protected]>

Makefile

@@ -299,6 +299,7 @@ go-lint: check-git deps $(GOLANGCI_LINT) $(FAILLINT)
 	@# TODO(bwplotka): Add, Printf, DefaultRegisterer, NewGaugeFunc and MustRegister once exception are accepted. Add fmt.{Errorf}=github.com/pkg/errors.{Errorf} once https://github.com/fatih/faillint/issues/10 is addressed.
 	@$(FAILLINT) -paths "errors=github.com/pkg/errors,\
 github.com/prometheus/tsdb=github.com/prometheus/prometheus/tsdb,\
+github.com/prometheus/prometheus/tsdb/errors=github.com/efficientgo/tools/core/pkg/merrors,\
 github.com/prometheus/prometheus/pkg/testutils=github.com/thanos-io/thanos/pkg/testutil,\
 github.com/prometheus/client_golang/prometheus.{DefaultGatherer,DefBuckets,NewUntypedFunc,UntypedFunc},\
 github.com/prometheus/client_golang/prometheus.{NewCounter,NewCounterVec,NewCounterVec,NewGauge,NewGaugeVec,NewGaugeFunc,\

cmd/thanos/rule.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/efficientgo/tools/core/pkg/merrors"
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/oklog/run"
@@ -29,7 +30,6 @@ import (
 	"github.com/prometheus/prometheus/rules"
 	"github.com/prometheus/prometheus/tsdb"
 	"github.com/prometheus/prometheus/util/strutil"
-	"github.com/thanos-io/thanos/pkg/errutil"
 	"github.com/thanos-io/thanos/pkg/extkingpin"
 
 	"github.com/thanos-io/thanos/pkg/alert"
@@ -811,10 +811,11 @@ func reloadRules(logger log.Logger,
 	ruleFiles []string,
 	ruleMgr *thanosrules.Manager,
 	evalInterval time.Duration,
-	metrics *RuleMetrics) error {
+	metrics *RuleMetrics,
+) error {
 	level.Debug(logger).Log("msg", "configured rule files", "files", strings.Join(ruleFiles, ","))
 	var (
-		errs      errutil.MultiError
+		errs      = merrors.New()
 		files     []string
 		seenFiles = make(map[string]struct{})
 	)

cmd/thanos/tools.go

@@ -6,12 +6,12 @@ package main
 import (
 	"os"
 
+	"github.com/efficientgo/tools/core/pkg/merrors"
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/oklog/run"
 	"github.com/opentracing/opentracing-go"
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/thanos-io/thanos/pkg/errutil"
 	"github.com/thanos-io/thanos/pkg/extkingpin"
 	"github.com/thanos-io/thanos/pkg/rules"
 )
@@ -35,7 +35,7 @@ func registerCheckRules(app extkingpin.AppClause) {
 }
 
 func checkRulesFiles(logger log.Logger, files *[]string) error {
-	var failed errutil.MultiError
+	failed := merrors.New()
 
 	for _, fn := range *files {
 		level.Info(logger).Log("msg", "checking", "filename", fn)
@@ -51,7 +51,7 @@ func checkRulesFiles(logger log.Logger, files *[]string) error {
 		n, errs := rules.ValidateAndCount(f)
 		if errs.Err() != nil {
 			level.Error(logger).Log("result", "FAILED")
-			for _, e := range errs {
+			for _, e := range errs.Err().Errors() {
 				level.Error(logger).Log("error", e.Error())
 				failed.Add(e)
 			}

go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/chromedp/chromedp v0.5.3
 	github.com/cortexproject/cortex v1.7.1-0.20210224085859-66d6fb5b0d42
 	github.com/davecgh/go-spew v1.1.1
+	github.com/efficientgo/tools/core v0.0.0-20210201224146-3d78f4d30648
 	github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb
 	github.com/fatih/structtag v1.1.0
 	github.com/felixge/fgprof v0.9.1

go.sum

@@ -304,6 +304,8 @@ github.com/eclipse/paho.mqtt.golang v1.2.0/go.mod h1:H9keYFcgq3Qr5OUJm/JZI/i6U7j
 github.com/edsrzf/mmap-go v0.0.0-20170320065105-0bce6a688712/go.mod h1:YO35OhQPt3KJa3ryjFM5Bs14WD66h8eGKpfaBNrHW5M=
 github.com/edsrzf/mmap-go v1.0.0 h1:CEBF7HpRnUCSJgGUb5h1Gm7e3VkmVDrR8lvWVLtrOFw=
 github.com/edsrzf/mmap-go v1.0.0/go.mod h1:YO35OhQPt3KJa3ryjFM5Bs14WD66h8eGKpfaBNrHW5M=
+github.com/efficientgo/tools/core v0.0.0-20210201224146-3d78f4d30648 h1:zY9fs6qlXtS/YlrijZ+7vTqduJRybPYwJ8Mjo4zWrS8=
+github.com/efficientgo/tools/core v0.0.0-20210201224146-3d78f4d30648/go.mod h1:cFZoHUhKg31xkPnPjhPKFtevnx0Xcg67ptBRxbpaxtk=
 github.com/elastic/go-sysinfo v1.1.1 h1:ZVlaLDyhVkDfjwPGU55CQRCRolNpc7P0BbyhhQZQmMI=
 github.com/elastic/go-sysinfo v1.1.1/go.mod h1:i1ZYdU10oLNfRzq4vq62BEwD2fH8KaWh6eh0ikPT9F0=
 github.com/elastic/go-windows v1.0.0 h1:qLURgZFkkrYyTTkvYpsZIgf83AUsdIHfvlJaqaZ7aSY=

pkg/block/fetcher.go

@@ -14,6 +14,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/efficientgo/tools/core/pkg/merrors"
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/golang/groupcache/singleflight"
@@ -28,7 +29,6 @@ import (
 	"gopkg.in/yaml.v2"
 
 	"github.com/thanos-io/thanos/pkg/block/metadata"
-	"github.com/thanos-io/thanos/pkg/errutil"
 	"github.com/thanos-io/thanos/pkg/extprom"
 	"github.com/thanos-io/thanos/pkg/model"
 	"github.com/thanos-io/thanos/pkg/objstore"
@@ -285,7 +285,7 @@ type response struct {
 	metas   map[ulid.ULID]*metadata.Meta
 	partial map[ulid.ULID]error
 	// If metaErr > 0 it means incomplete view, so some metas, failed to be loaded.
-	metaErrs errutil.MultiError
+	metaErrs merrors.NilOrMultiError
 
 	noMetas        float64
 	corruptedMetas float64
@@ -362,7 +362,8 @@ func (f *BaseFetcher) fetchMetadata(ctx context.Context) (interface{}, error) {
 		return nil, errors.Wrap(err, "BaseFetcher: iter bucket")
 	}
 
-	if len(resp.metaErrs) > 0 {
+	if resp.metaErrs.Err() != nil {
+		// Incomplete view is fine, errors are handled by metric, but we stil return success.
 		return resp, nil
 	}
 
@@ -433,7 +434,12 @@ func (f *BaseFetcher) fetch(ctx context.Context, metrics *fetcherMetrics, filter
 		metas[id] = m
 	}
 
-	metrics.synced.WithLabelValues(failedMeta).Set(float64(len(resp.metaErrs)))
+	if errs := resp.metaErrs.Err(); errs != nil {
+		metrics.synced.WithLabelValues(failedMeta).Set(float64(len(errs.Errors())))
+	} else {
+		metrics.synced.WithLabelValues(failedMeta).Set(0)
+	}
+
 	metrics.synced.WithLabelValues(noMeta).Set(resp.noMetas)
 	metrics.synced.WithLabelValues(corruptedMeta).Set(resp.corruptedMetas)
 
@@ -454,8 +460,8 @@ func (f *BaseFetcher) fetch(ctx context.Context, metrics *fetcherMetrics, filter
 	metrics.synced.WithLabelValues(loadedMeta).Set(float64(len(metas)))
 	metrics.submit()
 
-	if len(resp.metaErrs) > 0 {
-		return metas, resp.partial, errors.Wrap(resp.metaErrs, "incomplete view")
+	if errs := resp.metaErrs.Err(); errs != nil {
+		return metas, resp.partial, errors.Wrap(errs, "incomplete view")
 	}
 
 	level.Info(f.logger).Log("msg", "successfully synchronized block metadata", "duration", time.Since(start).String(), "cached", len(f.cached), "returned", len(metas), "partial", len(resp.partial))

pkg/block/writer.go

@@ -9,13 +9,13 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/efficientgo/tools/core/pkg/merrors"
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/pkg/errors"
 	"github.com/prometheus/prometheus/pkg/labels"
 	"github.com/prometheus/prometheus/tsdb"
 	"github.com/prometheus/prometheus/tsdb/chunks"
-	tsdb_errors "github.com/prometheus/prometheus/tsdb/errors"
 	"github.com/prometheus/prometheus/tsdb/fileutil"
 	"github.com/prometheus/prometheus/tsdb/index"
 )
@@ -56,6 +56,14 @@ type DiskWriter struct {
 
 const tmpForCreationBlockDirSuffix = ".tmp-for-creation"
 
+func closeAll(closers []io.Closer) error {
+	errs := merrors.New()
+	for _, c := range closers {
+		errs.Add(c.Close())
+	}
+	return errs.Err()
+}
+
 // NewDiskWriter allows to write single TSDB block to disk and returns statistics.
 // Destination block directory has to exists.
 func NewDiskWriter(ctx context.Context, logger log.Logger, bDir string) (_ *DiskWriter, err error) {
@@ -68,7 +76,7 @@ func NewDiskWriter(ctx context.Context, logger log.Logger, bDir string) (_ *Disk
 	}
 	defer func() {
 		if err != nil {
-			err = tsdb_errors.NewMulti(err, tsdb_errors.CloseAll(d.closers)).Err()
+			err = merrors.New(err, closeAll(d.closers)).Err()
 			if err := os.RemoveAll(bTmp); err != nil {
 				level.Error(logger).Log("msg", "removed tmp folder after failed compaction", "err", err.Error())
 			}
@@ -102,7 +110,7 @@ func NewDiskWriter(ctx context.Context, logger log.Logger, bDir string) (_ *Disk
 func (d *DiskWriter) Flush() (_ tsdb.BlockStats, err error) {
 	defer func() {
 		if err != nil {
-			err = tsdb_errors.NewMulti(err, tsdb_errors.CloseAll(d.closers)).Err()
+			err = merrors.New(err, closeAll(d.closers)).Err()
 			if err := os.RemoveAll(d.bTmp); err != nil {
 				level.Error(d.logger).Log("msg", "removed tmp folder failed after block(s) write", "err", err.Error())
 			}
@@ -114,7 +122,7 @@ func (d *DiskWriter) Flush() (_ tsdb.BlockStats, err error) {
 	}
 	defer func() {
 		if df != nil {
-			err = tsdb_errors.NewMulti(err, df.Close()).Err()
+			err = merrors.New(err, df.Close()).Err()
 		}
 	}()
 
@@ -128,7 +136,7 @@ func (d *DiskWriter) Flush() (_ tsdb.BlockStats, err error) {
 	}
 	df = nil
 
-	if err := tsdb_errors.CloseAll(d.closers); err != nil {
+	if err := closeAll(d.closers); err != nil {
 		d.closers = nil
 		return tsdb.BlockStats{}, err
 	}
@@ -180,5 +188,5 @@ func (s *statsGatheringSeriesWriter) WriteChunks(chks ...chunks.Meta) error {
 }
 
 func (s statsGatheringSeriesWriter) Close() error {
-	return tsdb_errors.NewMulti(s.iw.Close(), s.cw.Close()).Err()
+	return merrors.New(s.iw.Close(), s.cw.Close()).Err()
 }

pkg/compact/compact.go

@@ -5,6 +5,7 @@ package compact
 
 import (
 	"context"
+	stderrors "errors" //lint:ignore faillint explicitly using stderrors.As
 	"fmt"
 	"io/ioutil"
 	"math"
@@ -14,6 +15,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/efficientgo/tools/core/pkg/merrors"
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/oklog/ulid"
@@ -27,7 +29,6 @@ import (
 	"github.com/thanos-io/thanos/pkg/block"
 	"github.com/thanos-io/thanos/pkg/block/metadata"
 	"github.com/thanos-io/thanos/pkg/compact/downsample"
-	"github.com/thanos-io/thanos/pkg/errutil"
 	"github.com/thanos-io/thanos/pkg/objstore"
 )
 
@@ -543,17 +544,7 @@ func (e HaltError) Error() string {
 // IsHaltError returns true if the base error is a HaltError.
 // If a multierror is passed, any halt error will return true.
 func IsHaltError(err error) bool {
-	if multiErr, ok := errors.Cause(err).(errutil.MultiError); ok {
-		for _, err := range multiErr {
-			if _, ok := errors.Cause(err).(HaltError); ok {
-				return true
-			}
-		}
-		return false
-	}
-
-	_, ok := errors.Cause(err).(HaltError)
-	return ok
+	return stderrors.As(err, &HaltError{})
 }
 
 // RetryError is a type wrapper for errors that should trigger warning log and retry whole compaction loop, but aborting
@@ -576,17 +567,19 @@ func (e RetryError) Error() string {
 // IsRetryError returns true if the base error is a RetryError.
 // If a multierror is passed, all errors must be retriable.
 func IsRetryError(err error) bool {
-	if multiErr, ok := errors.Cause(err).(errutil.MultiError); ok {
-		for _, err := range multiErr {
-			if _, ok := errors.Cause(err).(RetryError); !ok {
+	if !stderrors.As(err, &RetryError{}) {
+		return false
+	}
+	// Check if it's not multi-error with some non-retry errors.
+	errs, ok := merrors.AsMulti(err)
+	if ok {
+		for _, e := range errs.Errors() {
+			if !IsRetryError(e) {
 				return false
 			}
 		}
-		return true
 	}
-
-	_, ok := errors.Cause(err).(RetryError)
-	return ok
+	return true
 }
 
 func (cg *Group) areBlocksOverlapping(include *metadata.Meta, exclude ...*metadata.Meta) error {
@@ -953,12 +946,12 @@ func (c *BucketCompactor) Compact(ctx context.Context) (rerr error) {
 		level.Info(c.logger).Log("msg", "start of compactions")
 
 		// Send all groups found during this pass to the compaction workers.
-		var groupErrs errutil.MultiError
+		errs := merrors.New()
 	groupLoop:
 		for _, g := range groups {
 			select {
 			case groupErr := <-errChan:
-				groupErrs.Add(groupErr)
+				errs.Add(groupErr)
 				break groupLoop
 			case groupChan <- g:
 			}
@@ -970,12 +963,12 @@ func (c *BucketCompactor) Compact(ctx context.Context) (rerr error) {
 		// while we were waiting for the last batch of groups to run the compaction.
 		close(errChan)
 		for groupErr := range errChan {
-			groupErrs.Add(groupErr)
+			errs.Add(groupErr)
 		}
 
 		workCtxCancel()
-		if len(groupErrs) > 0 {
-			return groupErrs.Err()
+		if err := errs.Err(); err != nil {
+			return err
 		}
 
 		if finishedAllGroups {

pkg/compact/compact_test.go

@@ -6,11 +6,11 @@ package compact
 import (
 	"testing"
 
+	"github.com/efficientgo/tools/core/pkg/merrors"
 	"github.com/pkg/errors"
 	"github.com/prometheus/prometheus/tsdb"
 
 	"github.com/thanos-io/thanos/pkg/block/metadata"
-	"github.com/thanos-io/thanos/pkg/errutil"
 	"github.com/thanos-io/thanos/pkg/testutil"
 )
 
@@ -32,29 +32,28 @@ func TestHaltMultiError(t *testing.T) {
 	haltErr := halt(errors.New("halt error"))
 	nonHaltErr := errors.New("not a halt error")
 
-	errs := errutil.MultiError{nonHaltErr}
-	testutil.Assert(t, !IsHaltError(errs), "should not be a halt error")
+	errs := merrors.New(nonHaltErr)
+	testutil.Assert(t, !IsHaltError(errs.Err()), "should not be a halt error")
 
 	errs.Add(haltErr)
-	testutil.Assert(t, IsHaltError(errs), "if any halt errors are present this should return true")
-	testutil.Assert(t, IsHaltError(errors.Wrap(errs, "wrap")), "halt error with wrap")
+	testutil.Assert(t, IsHaltError(errs.Err()), "if any halt errors are present this should return true")
+	testutil.Assert(t, IsHaltError(errors.Wrap(errs.Err(), "wrap")), "halt error with wrap")
 
 }
 
 func TestRetryMultiError(t *testing.T) {
 	retryErr := retry(errors.New("retry error"))
 	nonRetryErr := errors.New("not a retry error")
 
-	errs := errutil.MultiError{nonRetryErr}
-	testutil.Assert(t, !IsRetryError(errs), "should not be a retry error")
+	errs := merrors.New(nonRetryErr)
+	testutil.Assert(t, !IsRetryError(errs.Err()), "should not be a retry error")
 
-	errs = errutil.MultiError{retryErr}
-	testutil.Assert(t, IsRetryError(errs), "if all errors are retriable this should return true")
+	errs = merrors.New(retryErr)
+	testutil.Assert(t, IsRetryError(errs.Err()), "if all errors are retriable this should return true")
+	testutil.Assert(t, IsRetryError(errors.Wrap(errs.Err(), "wrap")), "retry error with wrap")
 
-	testutil.Assert(t, IsRetryError(errors.Wrap(errs, "wrap")), "retry error with wrap")
-
-	errs = errutil.MultiError{nonRetryErr, retryErr}
-	testutil.Assert(t, !IsRetryError(errs), "mixed errors should return false")
+	errs = merrors.New(nonRetryErr, retryErr)
+	testutil.Assert(t, !IsRetryError(errs.Err()), "mixed errors should return false")
 }
 
 func TestRetryError(t *testing.T) {