admission: remove DiskName from StoreSpec.ProvisionedRateSpec

This commit cleans up changes from #119885. There is no longer a need for
users to specify a disk name when specifying the provisioned bandwidth
since we can now automatically infer disk names from the `StoreSpec.Path`
and the underlying block device.

Informs: #86857.

Epic: None.
Release note (ops change): The provisioned-rate field, if specified,
should no longer accept a disk-name or an optional bandwidth field. To
use the disk bandwidth constraint the store-spec must contain
provisioned-rate=bandwidth=<bandwidth-bytes/s>, otherwise the cluster
setting kv.store.admission.provisioned_bandwidth will be used.

docs/generated/settings/settings.html

@@ -91,7 +91,7 @@
 <tr><td><div id="setting-kv-transaction-max-intents-bytes" class="anchored"><code>kv.transaction.max_intents_bytes</code></div></td><td>integer</td><td><code>4194304</code></td><td>maximum number of bytes used to track locks in transactions</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-max-refresh-spans-bytes" class="anchored"><code>kv.transaction.max_refresh_spans_bytes</code></div></td><td>integer</td><td><code>4194304</code></td><td>maximum number of bytes used to track refresh spans in serializable transactions</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-reject-over-max-intents-budget-enabled" class="anchored"><code>kv.transaction.reject_over_max_intents_budget.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if set, transactions that exceed their lock tracking budget (kv.transaction.max_intents_bytes) are rejected instead of having their lock spans imprecisely compressed</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-kvadmission-store-provisioned-bandwidth" class="anchored"><code>kvadmission.store.provisioned_bandwidth</code></div></td><td>byte size</td><td><code>0 B</code></td><td>if set to a non-zero value, this is used as the provisioned bandwidth (in bytes/s), for each store. It can be over-ridden on a per-store basis using the --store flag</td><td>Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-kvadmission-store-provisioned-bandwidth" class="anchored"><code>kvadmission.store.provisioned_bandwidth</code></div></td><td>byte size</td><td><code>0 B</code></td><td>if set to a non-zero value, this is used as the provisioned bandwidth (in bytes/s), for each store. It can be overridden on a per-store basis using the --store flag. Note that setting the provisioned bandwidth to a positive value may enable disk bandwidth based admission control, since admission.disk_bandwidth_tokens.elastic.enabled defaults to true</td><td>Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-schedules-backup-gc-protection-enabled" class="anchored"><code>schedules.backup.gc_protection.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>enable chaining of GC protection across backups run as part of a schedule</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-security-ocsp-mode" class="anchored"><code>security.ocsp.mode</code></div></td><td>enumeration</td><td><code>off</code></td><td>use OCSP to check whether TLS certificates are revoked. If the OCSP server is unreachable, in strict mode all certificates will be rejected and in lax mode all certificates will be accepted. [off = 0, lax = 1, strict = 2]</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-security-ocsp-timeout" class="anchored"><code>security.ocsp.timeout</code></div></td><td>duration</td><td><code>3s</code></td><td>timeout before considering the OCSP server unreachable</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/base/store_spec.go

@@ -162,83 +162,42 @@ func (ss *SizeSpec) Set(value string) error {
 }
 
 // ProvisionedRateSpec is an optional part of the StoreSpec.
-//
-// TODO(sumeer): We should map the file path specified in the store spec to
-// the disk name. df can be used to map paths to names like /dev/nvme1n1 and
-// /dev/sdb (these examples are from AWS EBS and GCP PD respectively) and the
-// corresponding names produced by disk_counters.go are nvme1n1 and sdb
-// respectively. We need to find or write a platform independent library --
-// see the discussion on
-// https://github.com/cockroachdb/cockroach/pull/86063#pullrequestreview-1074487018.
-// With that change, the ProvisionedRateSpec would only be needed to override
-// the cluster setting when there are heterogenous bandwidth limits in a
-// cluster (there would be no more DiskName field).
 type ProvisionedRateSpec struct {
-	// DiskName is the name of the disk observed by the code in disk_counters.go
-	// when retrieving stats for this store.
-	DiskName string
-	// ProvisionedBandwidth is the bandwidth provisioned for this store in
-	// bytes/s.
+	// ProvisionedBandwidth is the bandwidth provisioned for this store in bytes/s.
 	ProvisionedBandwidth int64
 }
 
 func newStoreProvisionedRateSpec(
 	field redact.SafeString, value string,
 ) (ProvisionedRateSpec, error) {
-	var spec ProvisionedRateSpec
-	used := make(map[string]struct{})
-	for _, split := range strings.Split(value, ":") {
-		if len(split) == 0 {
-			continue
-		}
-		subSplits := strings.Split(split, "=")
-		if len(subSplits) != 2 {
-			return ProvisionedRateSpec{}, errors.Errorf("%s field has invalid value %s", field, value)
-		}
-		subField := subSplits[0]
-		subValue := subSplits[1]
-		if _, ok := used[subField]; ok {
-			return ProvisionedRateSpec{}, errors.Errorf("%s field has duplicate sub-field %s",
-				field, subField)
-		}
-		used[subField] = struct{}{}
-		if len(subField) == 0 {
-			continue
-		}
-		if len(subValue) == 0 {
-			return ProvisionedRateSpec{},
-				errors.Errorf("%s field has no value specified for sub-field %s", field, subField)
-		}
-		switch subField {
-		case "disk-name":
-			spec.DiskName = subValue
-		case "bandwidth":
-			if len(subValue) <= 2 || subValue[len(subValue)-2:] != "/s" {
-				return ProvisionedRateSpec{},
-					errors.Errorf("%s field does not have bandwidth sub-field %s ending in /s",
-						field, subValue)
-			}
-			subValue = subValue[:len(subValue)-2]
-			var err error
-			spec.ProvisionedBandwidth, err = humanizeutil.ParseBytes(subValue)
-			if err != nil {
-				return ProvisionedRateSpec{},
-					errors.Wrapf(err, "could not parse bandwidth in field %s", field)
-			}
-			if spec.ProvisionedBandwidth == 0 {
-				return ProvisionedRateSpec{},
-					errors.Errorf("%s field is trying to set bandwidth to 0", field)
-			}
-		default:
-			return ProvisionedRateSpec{}, errors.Errorf("%s field has unknown sub-field %s",
-				field, subField)
-		}
+	split := strings.Split(value, "=")
+	if len(split) != 2 {
+		return ProvisionedRateSpec{}, errors.Errorf("%s field has invalid value %s", field, value)
 	}
-	if len(spec.DiskName) == 0 {
+	subField := split[0]
+	subValue := split[1]
+	if subField != "bandwidth" {
+		return ProvisionedRateSpec{}, errors.Errorf("%s field does not have bandwidth sub-field", field)
+	}
+	if len(subValue) == 0 {
+		return ProvisionedRateSpec{}, errors.Errorf("%s field has no value specified for bandwidth", field)
+	}
+	if len(subValue) <= 2 || subValue[len(subValue)-2:] != "/s" {
 		return ProvisionedRateSpec{},
-			errors.Errorf("%s field did not specify disk-name", field)
+			errors.Errorf("%s field does not have bandwidth sub-field %s ending in /s",
+				field, subValue)
 	}
-	return spec, nil
+	bandwidthString := subValue[:len(subValue)-2]
+	bandwidth, err := humanizeutil.ParseBytes(bandwidthString)
+	if err != nil {
+		return ProvisionedRateSpec{},
+			errors.Wrapf(err, "could not parse bandwidth in field %s", field)
+	}
+	if bandwidth == 0 {
+		return ProvisionedRateSpec{},
+			errors.Errorf("%s field is trying to set bandwidth to 0", field)
+	}
+	return ProvisionedRateSpec{ProvisionedBandwidth: bandwidth}, nil
 }
 
 // StoreSpec contains the details that can be specified in the cli pertaining
@@ -311,15 +270,9 @@ func (ss StoreSpec) String() string {
 		fmt.Fprint(&buffer, optsStr)
 		fmt.Fprint(&buffer, ",")
 	}
-	if len(ss.ProvisionedRateSpec.DiskName) > 0 {
-		fmt.Fprintf(&buffer, "provisioned-rate=disk-name=%s",
-			ss.ProvisionedRateSpec.DiskName)
-		if ss.ProvisionedRateSpec.ProvisionedBandwidth > 0 {
-			fmt.Fprintf(&buffer, ":bandwidth=%s/s,",
-				humanizeutil.IBytes(ss.ProvisionedRateSpec.ProvisionedBandwidth))
-		} else {
-			fmt.Fprintf(&buffer, ",")
-		}
+	if ss.ProvisionedRateSpec.ProvisionedBandwidth > 0 {
+		fmt.Fprintf(&buffer, "provisioned-rate=bandwidth=%s/s,",
+			humanizeutil.IBytes(ss.ProvisionedRateSpec.ProvisionedBandwidth))
 	}
 	// Trim the extra comma from the end if it exists.
 	if l := buffer.Len(); l > 0 {
@@ -350,8 +303,8 @@ var fractionRegex = regexp.MustCompile(`^([-]?([0-9]+\.[0-9]*|[0-9]*\.[0-9]+|[0-
 // NewStoreSpec parses the string passed into a --store flag and returns a
 // StoreSpec if it is correctly parsed.
 // There are five possible fields that can be passed in, comma separated:
-//   - path=xxx The directory in which to the rocks db instance should be
-//     located, required unless using a in memory storage.
+//   - path=xxx The directory in which the rocks db instance should be
+//     located, required unless using an in memory storage.
 //   - type=mem This specifies that the store is an in memory storage instead of
 //     an on disk one. mem is currently the only other type available.
 //   - size=xxx The optional maximum size of the storage. This can be in one of a
@@ -363,10 +316,9 @@ var fractionRegex = regexp.MustCompile(`^([-]?([0-9]+\.[0-9]*|[0-9]*\.[0-9]+|[0-
 //   - 20%             -> 20% of the available space
 //   - 0.2             -> 20% of the available space
 //   - attrs=xxx:yyy:zzz A colon separated list of optional attributes.
-//   - provisioned-rate=disk-name=<disk-name>[:bandwidth=<bandwidth-bytes/s>] The
-//     provisioned-rate can be used for admission control for operations on the
-//     store. The bandwidth is optional, and if unspecified, a cluster setting
-//     (kvadmission.store.provisioned_bandwidth) will be used.
+//   - provisioned-rate=bandwidth=<bandwidth-bytes/s> The provisioned-rate can be
+//     used for admission control for operations on the store and if unspecified,
+//     a cluster setting (kvadmission.store.provisioned_bandwidth) will be used.
 //
 // Note that commas are forbidden within any field name or value.
 func NewStoreSpec(value string) (StoreSpec, error) {

pkg/base/store_spec_test.go

@@ -151,12 +151,11 @@ target_file_size=2097152`
 		{"path=/mnt/hda1,type=mem,size=20GiB", "path specified for in memory store", StoreSpec{}},
 
 		// provisioned rate
-		{"path=/mnt/hda1,provisioned-rate=disk-name=nvme1n1:bandwidth=200MiB/s", "",
-			StoreSpec{Path: "/mnt/hda1", ProvisionedRateSpec: base.ProvisionedRateSpec{
-				DiskName: "nvme1n1", ProvisionedBandwidth: 200 << 20}}},
-		{"path=/mnt/hda1,provisioned-rate=disk-name=sdb", "", StoreSpec{
-			Path: "/mnt/hda1", ProvisionedRateSpec: base.ProvisionedRateSpec{
-				DiskName: "sdb", ProvisionedBandwidth: 0}}},
+		{"path=/mnt/hda1,provisioned-rate=bandwidth=200MiB/s", "",
+			StoreSpec{Path: "/mnt/hda1", ProvisionedRateSpec: base.ProvisionedRateSpec{ProvisionedBandwidth: 200 << 20}}},
+		{"path=/mnt/hda1,provisioned-rate=bandwidth=200MiB", "provisioned-rate field does not have bandwidth sub-field 200MiB ending in /s", StoreSpec{}},
+		{"path=/mnt/hda1,provisioned-rate=200MiB/s", "provisioned-rate field has invalid value 200MiB/s", StoreSpec{}},
+		{"path=/mnt/hda1,provisioned-rate=bandwidth=0B/s", "provisioned-rate field is trying to set bandwidth to 0", StoreSpec{}},
 
 		// RocksDB
 		{"path=/,rocksdb=key1=val1;key2=val2", "", StoreSpec{Path: "/", RocksDBOptions: "key1=val1;key2=val2"}},

pkg/kv/kvserver/kvadmission/kvadmission.go

@@ -63,7 +63,7 @@ var elasticCPUDurationPerInternalLowPriRead = settings.RegisterDurationSetting(
 var internalLowPriReadElasticControlEnabled = settings.RegisterBoolSetting(
 	settings.SystemOnly,
 	"kvadmission.low_pri_read_elastic_control.enabled",
-	"determines whether the internally submitted low priority reads reads integrate with elastic CPU control",
+	"determines whether the internally submitted low priority reads integrate with elastic CPU control",
 	true,
 )
 
@@ -100,8 +100,10 @@ var rangefeedCatchupScanElasticControlEnabled = settings.RegisterBoolSetting(
 // bandwidth for each store in the cluster.
 var ProvisionedBandwidth = settings.RegisterByteSizeSetting(
 	settings.SystemOnly, "kvadmission.store.provisioned_bandwidth",
-	"if set to a non-zero value, this is used as the provisioned bandwidth (in bytes/s), "+
-		"for each store. It can be over-ridden on a per-store basis using the --store flag",
+	"if set to a non-zero value, this is used as the provisioned bandwidth (in bytes/s), for "+
+		"each store. It can be overridden on a per-store basis using the --store flag. Note that "+
+		"setting the provisioned bandwidth to a positive value may enable disk bandwidth based "+
+		"admission control, since admission.disk_bandwidth_tokens.elastic.enabled defaults to true",
 	0,
 	settings.WithPublic)
 

pkg/server/node_test.go

@@ -976,15 +976,13 @@ func TestDiskStatsMap(t *testing.T) {
 	specs := []base.StoreSpec{
 		{
 			ProvisionedRateSpec: base.ProvisionedRateSpec{
-				DiskName: "foo",
 				// ProvisionedBandwidth is 0 so the cluster setting will be used.
 				ProvisionedBandwidth: 0,
 			},
 			Path: path.Join(dir, "foo"),
 		},
 		{
 			ProvisionedRateSpec: base.ProvisionedRateSpec{
-				DiskName:             "bar",
 				ProvisionedBandwidth: 200,
 			},
 			Path: path.Join(dir, "bar"),