Skip to content

Commit

Permalink
util/must: add runtime assertion API
Browse files Browse the repository at this point in the history
This patch adds a convenient and canonical API for runtime assertions,
inspired by the Testify package used for Go test assertions. It is
intended to encourage liberal use of runtime assertions throughout the
code base, by making it as easy as possible to write assertions that
follow best practices. It does not attempt to reinvent the wheel, but
instead builds on existing infrastructure.

Assertion failures are fatal in all non-release builds, including
roachprod clusters and roachtests, to ensure they will be noticed. In
release builds, they instead log the failure and report it to Sentry (if
enabled), and return an assertion error to the caller for propagation.
This avoids excessive disruption in production environments, where an
assertion failure is often scoped to an individual RPC request,
transaction, or range, and crashing the node can turn a minor problem
into a full-blown outage. It is still possible to kill the node when
appropriate via `log.Fatalf`, but this should be the exception rather
than the norm.

It also supports expensive assertions that must be compiled out of
normal dev/test/release builds for performance reasons. These are
instead enabled in special test builds.

This is intended to be used instead of other existing assertion
mechanisms, which have various shortcomings:

* `log.Fatalf`: kills the node even in release builds, which can cause
  severe disruption over often minor issues.

* `errors.AssertionFailedf`: only suitable when we have an error return
  path, does not fatal in non-release builds, and are not always
  notified in release builds.

* `logcrash.ReportOrPanic`: panics rather than fatals, which can leave
  the node limping along. Requires the caller to implement separate
  assertion handling in release builds, which is easy to forget. Also
  requires propagating cluster settings, which aren't always available.

* `buildutil.CrdbTestBuild`: only enabled in Go tests, not roachtests,
  roachprod clusters, or production clusters.

* `util.RaceEnabled`: only enabled in race builds. Expensive assertions
  should be possible to run without the additional overhead of the race
  detector.

For more details and examples, see the `must` package documentation.

Epic: none
Release note: None
  • Loading branch information
erikgrinaker committed Jul 28, 2023
1 parent 4ffc54c commit 3250477
Show file tree
Hide file tree
Showing 33 changed files with 1,225 additions and 1 deletion.
3 changes: 3 additions & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,7 @@ ALL_TESTS = [
"//pkg/util/metric/aggmetric:aggmetric_test",
"//pkg/util/metric:metric_test",
"//pkg/util/mon:mon_test",
"//pkg/util/must:must_test",
"//pkg/util/netutil/addr:addr_test",
"//pkg/util/netutil:netutil_test",
"//pkg/util/optional:optional_test",
Expand Down Expand Up @@ -2337,6 +2338,8 @@ GO_TARGETS = [
"//pkg/util/metric:metric_test",
"//pkg/util/mon:mon",
"//pkg/util/mon:mon_test",
"//pkg/util/must:must",
"//pkg/util/must:must_test",
"//pkg/util/netutil/addr:addr",
"//pkg/util/netutil/addr:addr_test",
"//pkg/util/netutil:netutil",
Expand Down
1 change: 1 addition & 0 deletions pkg/testutils/lint/gcassert_paths.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ util
util/admission
util/hlc
util/intsets
util/must
29 changes: 29 additions & 0 deletions pkg/testutils/lint/passes/fmtsafe/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,35 @@ var requireConstFmt = map[string]bool{

"(github.com/cockroachdb/cockroach/pkg/kv/kvpb.TestPrinter).Printf": true,

// must assertions.
"github.com/cockroachdb/cockroach/pkg/util/must.Fail": true,
"github.com/cockroachdb/cockroach/pkg/util/must.failDepth": true,
"github.com/cockroachdb/cockroach/pkg/util/must.True": true,
"github.com/cockroachdb/cockroach/pkg/util/must.False": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Equal": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotEqual": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Greater": true,
"github.com/cockroachdb/cockroach/pkg/util/must.GreaterOrEqual": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Less": true,
"github.com/cockroachdb/cockroach/pkg/util/must.LessOrEqual": true,
"github.com/cockroachdb/cockroach/pkg/util/must.EqualBytes": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotEqualBytes": true,
"github.com/cockroachdb/cockroach/pkg/util/must.PrefixBytes": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotPrefixBytes": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Len": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Contains": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotContains": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Empty": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotEmpty": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Nil": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotNil": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Same": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotSame": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Zero": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NotZero": true,
"github.com/cockroachdb/cockroach/pkg/util/must.Error": true,
"github.com/cockroachdb/cockroach/pkg/util/must.NoError": true,

// Error things are populated in the init() message.
}

Expand Down
1 change: 1 addition & 0 deletions pkg/util/log/logcrash/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ go_library(
"//pkg/util/envutil",
"//pkg/util/log",
"//pkg/util/log/severity",
"//pkg/util/must",
"//pkg/util/timeutil",
"@com_github_cockroachdb_errors//:errors",
"@com_github_getsentry_sentry_go//:sentry-go",
Expand Down
6 changes: 5 additions & 1 deletion pkg/util/log/logcrash/crash_reporting.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/envutil"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/log/severity"
"github.com/cockroachdb/cockroach/pkg/util/must"
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/cockroachdb/errors"
sentry "github.com/getsentry/sentry-go"
Expand Down Expand Up @@ -318,7 +319,7 @@ const (
// was generated via a log.Fatal call.
ReportTypeLogFatal
// ReportTypeAssertionFailure signifies that an assertion was violated (see
// expect package).
// must package).
ReportTypeAssertionFailure
)

Expand Down Expand Up @@ -453,4 +454,7 @@ func init() {
log.MaybeSendCrashReport = func(ctx context.Context, err error) {
maybeSendCrashReport(ctx, err, ReportTypeLogFatal)
}
must.MaybeSendReport = func(ctx context.Context, err error) {
maybeSendCrashReport(ctx, err, ReportTypeAssertionFailure)
}
}
36 changes: 36 additions & 0 deletions pkg/util/must/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "must",
srcs = ["must.go"],
importpath = "github.com/cockroachdb/cockroach/pkg/util/must",
visibility = ["//visibility:public"],
deps = [
"//pkg/build",
"//pkg/util",
"//pkg/util/envutil",
"//pkg/util/log",
"@com_github_cockroachdb_errors//:errors",
"@org_golang_x_exp//constraints",
],
)

go_test(
name = "must_test",
srcs = ["must_test.go"],
args = ["-test.timeout=295s"],
data = glob(["testdata/**"]),
deps = [
":must",
"//pkg/build",
"//pkg/keys",
"//pkg/testutils/datapathutils",
"//pkg/testutils/echotest",
"//pkg/util",
"//pkg/util/hlc",
"//pkg/util/leaktest",
"@com_github_cockroachdb_errors//:errors",
"@com_github_cockroachdb_redact//:redact",
"@com_github_stretchr_testify//require",
],
)
Loading

0 comments on commit 3250477

Please sign in to comment.