Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

admission: remove DiskName from StoreSpec.ProvisionedRateSpec #120895

Merged
merged 1 commit into from Mar 26, 2024

Conversation

CheranMahalingam
Copy link
Contributor

@CheranMahalingam CheranMahalingam commented Mar 22, 2024

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor Author

@CheranMahalingam CheranMahalingam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/base/store_spec.go line 167 at r1 (raw file):

type ProvisionedRateSpec struct {
	// ProvisionedBandwidth is the bandwidth provisioned for this store in bytes/s.
	ProvisionedBandwidth int64

Optionally, the ProvisionedBandwidth could be moved out of this struct since it is now the only field. I have left it as is because I am unsure of whether AC will ever extend this struct to include additional options (i.e. read or write bandwidth).


pkg/base/store_spec.go line 309 at r1 (raw file):

//   - 0.2             -> 20% of the available space
//   - attrs=xxx:yyy:zzz A colon separated list of optional attributes.
//   - provisioned-rate=<bandwidth-bytes/s> The provisioned-rate can be used for

Optionally, it makes sense to rename provisioned-rate to provisioned-bandwidth but I have left it as is to avoid confusion for any users that already use provisioned-rate.

@CheranMahalingam CheranMahalingam force-pushed the disk_name_inference branch 3 times, most recently from b52124d to f7f7e03 Compare March 22, 2024 18:13
@CheranMahalingam CheranMahalingam marked this pull request as ready for review March 25, 2024 14:39
@CheranMahalingam CheranMahalingam requested review from a team as code owners March 25, 2024 14:39
@CheranMahalingam CheranMahalingam self-assigned this Mar 25, 2024
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi and @CheranMahalingam)


pkg/base/store_spec.go line 167 at r1 (raw file):

Previously, CheranMahalingam wrote…

Optionally, the ProvisionedBandwidth could be moved out of this struct since it is now the only field. I have left it as is because I am unsure of whether AC will ever extend this struct to include additional options (i.e. read or write bandwidth).

struct is fine for now. We'll probably extend it to include IOPS in the future. In that sense "rate" (in ProvisionedRateSpec) is also an ok-ish term since it doesn't specify whether the rate is bandwidth or IOPS or ...


pkg/base/store_spec.go line 309 at r1 (raw file):

Previously, CheranMahalingam wrote…

Optionally, it makes sense to rename provisioned-rate to provisioned-bandwidth but I have left it as is to avoid confusion for any users that already use provisioned-rate.

Let's make this "provisioned-rate=bandwidth=<bandwidth-bytes/s>


pkg/base/store_spec.go line 309 at r2 (raw file):

//   - 0.2             -> 20% of the available space
//   - attrs=xxx:yyy:zzz A colon separated list of optional attributes.
//   - provisioned-rate=<bandwidth-bytes/s> The provisioned-rate can be used for

since we've made it easier to configure disk bandwidth tokens for AC, we've also made it easier to accidentally turn it on. Can you add a comment in

// ProvisionedBandwidth set a value of the provisioned
// 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",
0,
settings.WithPublic)
that setting kvadmission.store.provisioned_bandwidth to a positive value will enable disk bandwidth based admission control, since admission.disk_bandwidth_tokens.elastic.enabled defaults to true.

This commit cleans up changes from cockroachdb#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: cockroachdb#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.
Copy link
Contributor Author

@CheranMahalingam CheranMahalingam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi and @sumeerbhola)


pkg/base/store_spec.go line 309 at r2 (raw file):

Previously, sumeerbhola wrote…

since we've made it easier to configure disk bandwidth tokens for AC, we've also made it easier to accidentally turn it on. Can you add a comment in

// ProvisionedBandwidth set a value of the provisioned
// 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",
0,
settings.WithPublic)
that setting kvadmission.store.provisioned_bandwidth to a positive value will enable disk bandwidth based admission control, since admission.disk_bandwidth_tokens.elastic.enabled defaults to true.

Done.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aadityasondhi)

@CheranMahalingam
Copy link
Contributor Author

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented Mar 26, 2024

@craig craig bot merged commit 3e02133 into cockroachdb:master Mar 26, 2024
22 checks passed
@CheranMahalingam CheranMahalingam deleted the disk_name_inference branch March 26, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants