Merge #121609 #121815

121609: GNUmakefile: add .PHONY: all and help r=rickystewart a=imalasong



121815: server/profiler: introduce separate limit for each profiler r=yuzefovich a=yuzefovich

This commit introduces a separate cluster setting for each profiler that
writes into `heapprofiler` directory. There are five of these (heap, mem
monitoring, jemalloc, mem stats, and active query), and previously all
of them use the same limit for the combined size (that was initially
introduced for the heap profiles). However, `DumpStore.GC` logic only
looks at the files owned by each profiler when calculating the
footprint, so each profiler would get up to 256MiB of space. This commit
makes the following limits the default:
- heap - 256MiB
- mem monitor - 4MiB (these files are usually in single KiBs range)
- jemalloc - 32MiB (don't have a good intuition about this one)
- mem stats - 32MiB (these files seems like an order of magnitude
smaller than heap profiles
- active query - 64MiB (these files seem smaller than heap profiles, but
preserving longer history might be beneficial in investigations).

Note that the heap profile limit was increased from 128MiB to 256MiB
when the mem monitoring profiler was introduced under incorrect
understanding that the limit was on the total size of the `heapprofiler`
directory. We could consider reducing it back down to 128MiB.

Epic: None

Release note: None

Co-authored-by: imalasong <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

GNUmakefile

@@ -11,10 +11,11 @@
 # 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.
-
+.PHONY: all
 all: build
 	$(MAKE) help
 
+.PHONY: help
 help:
 	@echo
 	@echo "Tip: use ./dev instead of 'make'."

pkg/server/profiler/BUILD.bazel

@@ -48,6 +48,7 @@ go_test(
     deps = [
         "//pkg/server/dumpstore",
         "//pkg/settings/cluster",
+        "//pkg/util/log",
         "//pkg/util/mon",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_redact//:redact",

pkg/server/profiler/activequeryprofiler.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/server/debug"
 	"github.com/cockroachdb/cockroach/pkg/server/dumpstore"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/util/cgroups"
@@ -63,6 +64,13 @@ const (
 	QueryFileNameSuffix = ".csv"
 )
 
+var activeQueryCombinedFileSize = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"server.active_query.total_dump_size_limit",
+	"maximum combined disk size of preserved active query profiles",
+	64<<20, // 64MiB
+)
+
 // NewActiveQueryProfiler creates a NewQueryProfiler. dir is the directory in which
 // profiles are to be stored.
 func NewActiveQueryProfiler(
@@ -72,7 +80,7 @@ func NewActiveQueryProfiler(
 		return nil, errors.AssertionFailedf("need to specify dir for NewQueryProfiler")
 	}
 
-	dumpStore := dumpstore.NewStore(dir, maxCombinedFileSize, st)
+	dumpStore := dumpstore.NewStore(dir, activeQueryCombinedFileSize, st)
 
 	maxMem, warn, err := memLimitFn()
 	if err != nil {

pkg/server/profiler/activequeryprofiler_test.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/server/dumpstore"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/stretchr/testify/require"
 )
 
@@ -52,7 +53,7 @@ func TestNewActiveQueryProfiler(t *testing.T) {
 			profiler: &ActiveQueryProfiler{
 				profiler: makeProfiler(
 					newProfileStore(
-						dumpstore.NewStore(heapProfilerDirName, maxCombinedFileSize, nil),
+						dumpstore.NewStore(heapProfilerDirName, activeQueryCombinedFileSize, nil),
 						QueryFileNamePrefix,
 						QueryFileNameSuffix,
 						nil),
@@ -80,12 +81,13 @@ func TestNewActiveQueryProfiler(t *testing.T) {
 			test.profiler.resetInterval = nil
 			profiler.resetInterval = nil
 			require.Equal(t, test.profiler, profiler)
-			require.Equal(t, test.profiler, profiler)
 		})
 	}
 }
 
 func TestShouldDump(t *testing.T) {
+	defer log.Scope(t).Close(t)
+
 	ctx := context.Background()
 	createSettingFn := func(settingEnabled bool) *cluster.Settings {
 		s := cluster.MakeClusterSettings()
@@ -96,7 +98,7 @@ func TestShouldDump(t *testing.T) {
 	profiler := &ActiveQueryProfiler{
 		profiler: profiler{
 			store: newProfileStore(
-				dumpstore.NewStore(heapProfilerDirName, maxCombinedFileSize, nil),
+				dumpstore.NewStore(heapProfilerDirName, activeQueryCombinedFileSize, nil),
 				QueryFileNamePrefix,
 				QueryFileNameSuffix,
 				nil),
@@ -201,6 +203,8 @@ func TestShouldDump(t *testing.T) {
 }
 
 func TestMaybeDumpQueries_PanicHandler(t *testing.T) {
+	defer log.Scope(t).Close(t)
+
 	ctx := context.Background()
 	memLimitFn = cgroupFnWithReturn(mbToBytes(256), "", nil)
 	s := &cluster.Settings{}

pkg/server/profiler/cgoprofiler.go

@@ -14,6 +14,7 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/server/dumpstore"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
@@ -38,6 +39,13 @@ const jemallocFileNamePrefix = "jeprof"
 // jemallocFileNameSuffix is the file name extension of jemalloc profile dumps.
 const jemallocFileNameSuffix = ".jeprof"
 
+var jemallCombinedFileSize = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"server.jemalloc.total_dump_size_limit",
+	"maximum combined disk size of preserved jemalloc profiles",
+	32<<20, // 32MiB
+)
+
 // NewNonGoAllocProfiler creates a NonGoAllocProfiler. dir is the
 // directory in which profiles are to be stored.
 func NewNonGoAllocProfiler(
@@ -47,7 +55,7 @@ func NewNonGoAllocProfiler(
 		return nil, errors.AssertionFailedf("need to specify dir for NewHeapProfiler")
 	}
 
-	dumpStore := dumpstore.NewStore(dir, maxCombinedFileSize, st)
+	dumpStore := dumpstore.NewStore(dir, jemallCombinedFileSize, st)
 
 	hp := &NonGoAllocProfiler{
 		profiler: makeProfiler(

pkg/server/profiler/heapprofiler.go

@@ -16,6 +16,7 @@ import (
 	"runtime/pprof"
 
 	"github.com/cockroachdb/cockroach/pkg/server/dumpstore"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
@@ -39,6 +40,23 @@ const heapFileNamePrefix = "memprof"
 // heapFileNameSuffix is the suffix of files containing pprof data.
 const heapFileNameSuffix = ".pprof"
 
+var maxCombinedFileSize = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"server.mem_profile.total_dump_size_limit",
+	"maximum combined disk size of preserved memory profiles",
+	256<<20, // 256MiB
+)
+
+func init() {
+	_ = settings.RegisterByteSizeSetting(
+		settings.ApplicationLevel,
+		"server.heap_profile.total_dump_size_limit",
+		"use server.mem_profile.total_dump_size_limit instead",
+		256<<20, // 256MiB
+		settings.Retired,
+	)
+}
+
 // NewHeapProfiler creates a HeapProfiler. dir is the directory in which
 // profiles are to be stored.
 func NewHeapProfiler(ctx context.Context, dir string, st *cluster.Settings) (*HeapProfiler, error) {

pkg/server/profiler/memory_monitoring_profiler.go

@@ -44,6 +44,13 @@ type MemoryMonitoringProfiler struct {
 const memMonitoringFileNamePrefix = "memmonitoring"
 const memMonitoringFileNameSuffix = ".txt"
 
+var memMonitoringCombinedFileSize = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"server.mem_monitoring.total_dump_size_limit",
+	"maximum combined disk size of preserved mem monitoring profiles",
+	4<<20, // 4MiB
+)
+
 // NewMemoryMonitoringProfiler returns a new MemoryMonitoringProfiler. dir is
 // the directory in which memory monitoring dumps are to be stored.
 func NewMemoryMonitoringProfiler(
@@ -53,7 +60,7 @@ func NewMemoryMonitoringProfiler(
 		return nil, errors.AssertionFailedf("need to specify dir for MemoryMonitoringProfiler")
 	}
 
-	dumpStore := dumpstore.NewStore(dir, maxCombinedFileSize, st)
+	dumpStore := dumpstore.NewStore(dir, memMonitoringCombinedFileSize, st)
 	mmp := &MemoryMonitoringProfiler{
 		profiler: makeProfiler(
 			newProfileStore(dumpStore, memMonitoringFileNamePrefix, memMonitoringFileNameSuffix, st),

pkg/server/profiler/profilestore.go

@@ -26,36 +26,19 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-var (
-	maxProfiles = settings.RegisterIntSetting(
-		settings.ApplicationLevel,
-		"server.mem_profile.max_profiles",
-		"maximum number of profiles to be kept per ramp-up of memory usage. "+
-			"A ramp-up is defined as a sequence of profiles with increasing usage.",
-		5,
-	)
-
-	maxCombinedFileSize = settings.RegisterByteSizeSetting(
-		settings.ApplicationLevel,
-		"server.mem_profile.total_dump_size_limit",
-		"maximum combined disk size of preserved memory profiles",
-		256<<20, // 256MiB
-	)
+var maxProfiles = settings.RegisterIntSetting(
+	settings.ApplicationLevel,
+	"server.mem_profile.max_profiles",
+	"maximum number of profiles to be kept per ramp-up of memory usage. "+
+		"A ramp-up is defined as a sequence of profiles with increasing usage.",
+	5,
 )
 
 func init() {
 	_ = settings.RegisterIntSetting(
 		settings.ApplicationLevel,
 		"server.heap_profile.max_profiles", "use server.mem_profile.max_profiles instead", 5,
 		settings.Retired)
-
-	_ = settings.RegisterByteSizeSetting(
-		settings.ApplicationLevel,
-		"server.heap_profile.total_dump_size_limit",
-		"use server.mem_profile.total_dump_size_limit instead",
-		256<<20, // 256MiB
-		settings.Retired,
-	)
 }
 
 // profileStore represents the directory where heap profiles are stored.
@@ -160,15 +143,6 @@ func (s *profileStore) parseFileName(
 		// Not for us. Silently ignore.
 		return
 	}
-	if len(parts[2]) < 3 {
-		// At some point in the v20.2 cycle the timestamps were generated
-		// with format .999, which caused the trailing zeroes to be
-		// omitted. During parsing, they must be present, so re-add them
-		// here.
-		//
-		// TODO(knz): Remove this code in v21.1.
-		parts[2] += "000"[:3-len(parts[2])]
-	}
 	maybeTimestamp := parts[1] + "." + parts[2]
 	var err error
 	timestamp, err = time.Parse(timestampFormat, maybeTimestamp)

pkg/server/profiler/profilestore_test.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/server/dumpstore"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -42,6 +43,8 @@ func TestMakeFileName(t *testing.T) {
 }
 
 func TestParseFileName(t *testing.T) {
+	defer log.Scope(t).Close(t)
+
 	z := time.Time{}
 	testData := []struct {
 		f         string
@@ -58,10 +61,6 @@ func TestParseFileName(t *testing.T) {
 
 		// New format.
 		{"memprof.2020-06-15T13_19_19.543.123456", time.Date(2020, 6, 15, 13, 19, 19, 543000000, time.UTC), 123456, false},
-		// v20.2 transition formats.
-		// TODO(knz): Remove in v21.1.
-		{"memprof.2020-06-15T13_19_19.54.123456", time.Date(2020, 6, 15, 13, 19, 19, 540000000, time.UTC), 123456, false},
-		{"memprof.2020-06-15T13_19_19.5.123456", time.Date(2020, 6, 15, 13, 19, 19, 500000000, time.UTC), 123456, false},
 	}
 
 	s := profileStore{prefix: heapFileNamePrefix}
@@ -77,6 +76,8 @@ func TestParseFileName(t *testing.T) {
 }
 
 func TestCleanupLastRampup(t *testing.T) {
+	defer log.Scope(t).Close(t)
+
 	testData := []struct {
 		startFiles []string
 		maxP       int64

pkg/server/profiler/statsprofiler.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/server/dumpstore"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
@@ -43,6 +44,13 @@ const statsFileNamePrefix = "memstats"
 // statsFileNameSuffix is the suffix of memory stats dumps.
 const statsFileNameSuffix = ".txt"
 
+var memStatsCombinedFileSize = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"server.mem_stats.total_dump_size_limit",
+	"maximum combined disk size of preserved memstats profiles",
+	32<<20, // 32MiB
+)
+
 // NewStatsProfiler creates a StatsProfiler. dir is the
 // directory in which profiles are to be stored.
 func NewStatsProfiler(
@@ -52,7 +60,7 @@ func NewStatsProfiler(
 		return nil, errors.AssertionFailedf("need to specify dir for NewStatsProfiler")
 	}
 
-	dumpStore := dumpstore.NewStore(dir, maxCombinedFileSize, st)
+	dumpStore := dumpstore.NewStore(dir, memStatsCombinedFileSize, st)
 
 	hp := &StatsProfiler{
 		profiler: makeProfiler(