Merge #159917

159917: admission: move yielding into ElasticCPUWorkHandle r=dt a=sumeerbhola

ElasticCPUWorkHandle.Overlimit is expected to be called in a tight loop, so yielding there is conceptually the right place. More importantly, this will allow in the future for KV work that is not holding latches to also yield.

As part of this change, elastic work that does not wish to wait in admission control queues (due to cluster settings), is now accounted for in the elastic tokens, and in the admission.elastic_cpu_bypassed.utilization metric. One side-effect of this accounting is that work that needs to wait in admission queues may have fewer tokens available to it, and may wait longer. This is considered acceptable since:
- Elastic work that bypasses queueing is still elastic work, and our overarching goal is to reduce impact to foreground work.
- Due to the default on use of runtime.Yield, all elastic work yields, which allows the system to run at higher elastic CPU utilization without impacting the latency of foreground work.

Epic: none

Release note: None

Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>

Author: craig[bot]

docs/generated/metrics/metrics.yaml

@@ -10841,6 +10841,14 @@ layers:
       unit: PERCENT
       aggregation: AVG
       derivative: NONE
+    - name: admission.elastic_cpu_bypassed.utilization
+      exported_name: admission_elastic_cpu_bypassed_utilization
+      description: CPU utilization by elastic work that bypassed admission
+      y_axis_label: CPU Time
+      type: GAUGE
+      unit: PERCENT
+      aggregation: AVG
+      derivative: NONE
     - name: admission.errored.elastic-cpu
       exported_name: admission_errored_elastic_cpu
       description: Number of requests not admitted due to error

pkg/backup/datadriven_test.go

@@ -27,7 +27,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
-	"github.com/cockroachdb/cockroach/pkg/kv/bulk"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
@@ -200,8 +199,7 @@ func (d *datadrivenTestState) addCluster(t *testing.T, cfg clusterCfg) error {
 	// and overload the host. Generally overload would mean bulk work, which only
 	// uses strictly spare capacitym gets starved, but these tests expect it to
 	// still run (just slowly, along with everything else).
-	bulk.YieldIfNoPacer.Override(context.Background(), &settings.SV, false)
-	admission.YieldInPacer.Override(context.Background(), &settings.SV, false)
+	admission.YieldForElasticCPU.Override(context.Background(), &settings.SV, false)
 	params.ServerArgs.Settings = settings
 
 	clusterSize := cfg.nodes

pkg/kv/bulk/cpu_pacer.go

@@ -25,26 +25,18 @@ var cpuPacerRequestDuration = settings.RegisterDurationSetting(
 	50*time.Millisecond,
 )
 
-// YieldIfNoPacer is exported so it can be overridden in tests.
-var YieldIfNoPacer = settings.RegisterBoolSetting(
-	settings.ApplicationLevel,
-	"bulkio.elastic_cpu_control.always_yield.enabled",
-	"if true, yield the CPU as needed even when time-based elastic pacing is not enabled",
-	true,
-)
-
-// NewCPUPacer creates a new AC pacer for SST batcher. It may return an empty
-// Pacer which noops if pacing is disabled or its arguments are nil.
+// NewCPUPacer creates a new AC pacer for SST batcher. It will return an empty
+// Pacer which noops if db or db.AdmissionPacerFactory is nil.
+//
+// The setting specifies where waiting in the elastic admission control queue
+// is enabled. If disabled, the CPU consumed will be accounted for in
+// admission control, but pacing will not wait in admission control.
 func NewCPUPacer(ctx context.Context, db *kv.DB, setting *settings.BoolSetting) *admission.Pacer {
-	if db == nil || db.AdmissionPacerFactory == nil || !setting.Get(db.SettingsValues()) {
-		log.Dev.Infof(ctx, "admission control is not configured to pace bulk ingestion")
-
-		if db != nil && YieldIfNoPacer.Get(db.SettingsValues()) {
-			// Return a Pacer that just yields.
-			return &admission.Pacer{Yield: true}
-		}
+	if db == nil || db.AdmissionPacerFactory == nil {
+		log.Dev.Infof(ctx, "admission control is not configured to pace this bulk work")
 		return nil
 	}
+	bypassACQueue := !setting.Get(db.SettingsValues())
 	tenantID, ok := roachpb.ClientTenantFromContext(ctx)
 	if !ok {
 		tenantID = roachpb.SystemTenantID
@@ -55,6 +47,6 @@ func NewCPUPacer(ctx context.Context, db *kv.DB, setting *settings.BoolSetting)
 			TenantID:        tenantID,
 			Priority:        admissionpb.BulkNormalPri,
 			CreateTime:      timeutil.Now().UnixNano(),
-			BypassAdmission: false,
+			BypassAdmission: bypassACQueue,
 		})
 }

pkg/kv/kvserver/kvadmission/kvadmission.go

@@ -396,7 +396,7 @@ func (n *controllerImpl) AdmitKVWork(
 			// reads, so in some sense, the integration is incomplete. This is
 			// probably harmless.
 			elasticWorkHandle, err := n.elasticCPUGrantCoordinator.ElasticCPUWorkQueue.Admit(
-				ctx, admitDuration, admissionInfo,
+				ctx, admitDuration, admissionInfo, false,
 			)
 			if err != nil {
 				return Handle{}, err

pkg/roachprod/agents/opentelemetry/files/cockroachdb_metrics.yaml

@@ -65,6 +65,7 @@ admission_admitted_sql_sql_response_locking_normal_pri: admission.admitted.sql_s
 admission_admitted_sql_sql_response_normal_pri: admission.admitted.sql_sql_response.normal_pri
 admission_elastic_cpu_acquired_nanos: admission.elastic_cpu.acquired_nanos
 admission_elastic_cpu_available_nanos: admission.elastic_cpu.available_nanos
+admission_elastic_cpu_bypassed_utilization: admission.elastic_cpu_bypassed.utilization
 admission_elastic_cpu_max_available_nanos: admission.elastic_cpu.max_available_nanos
 admission_elastic_cpu_nanos_exhausted_duration: admission.elastic_cpu.nanos_exhausted_duration
 admission_elastic_cpu_over_limit_durations: admission.elastic_cpu.over_limit_durations

pkg/server/testserver.go

@@ -26,7 +26,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
-	"github.com/cockroachdb/cockroach/pkg/kv/bulk"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvprober"
@@ -1494,8 +1493,7 @@ func (ts *testServer) StartSharedProcessTenant(
 
 	// Disable yield AC for tenant servers in tests, for the same reason as the
 	// system tenant (see comment in serverutils.NewServer).
-	admission.YieldInPacer.Override(ctx, &sqlServer.cfg.Settings.SV, false)
-	bulk.YieldIfNoPacer.Override(ctx, &sqlServer.cfg.Settings.SV, false)
+	admission.YieldForElasticCPU.Override(ctx, &sqlServer.cfg.Settings.SV, false)
 
 	hts := &httpTestServer{}
 	hts.t.authentication = sqlServerWrapper.authentication
@@ -1887,8 +1885,7 @@ func (ts *testServer) StartTenant(
 
 	// Disable yield AC for tenant servers in tests, for the same reason as the
 	// system tenant (see comment in serverutils.NewServer).
-	admission.YieldInPacer.Override(ctx, &st.SV, false)
-	bulk.YieldIfNoPacer.Override(ctx, &st.SV, false)
+	admission.YieldForElasticCPU.Override(ctx, &st.SV, false)
 
 	hts := &httpTestServer{}
 	hts.t.authentication = sw.authentication