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.

Sign up for GitHub

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

Jump to bottom

kvserver: allocator should use io threshold instead of l0 sublevels #85084

Closed
kvoli opened this issue Jul 26, 2022 · 0 comments · Fixed by #97142
Closed

kvserver: allocator should use io threshold instead of l0 sublevels #85084

kvoli opened this issue Jul 26, 2022 · 0 comments · Fixed by #97142
Assignees
@kvoli
Labels
A-kv-distribution Relating to rebalancing and leasing. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@kvoli
Copy link
Collaborator

kvoli commented Jul 26, 2022 •
edited
Loading

io threshold encapsulates the io overload a store is experiencing. We currently use the smoohted (max over 10 minutes) L0 sublevel count to exclude stores from rebalance targets.

cockroach/pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go

Line 2085 in d7b901d

func (o StoreHealthOptions) rebalanceToReadAmpIsHealthy(

This should be swapped with the io threshold instead, as this encapsulates the files and sublevels together.

cockroach/pkg/kv/kvserver/store.go

Lines 945 to 948 in d7b901d

ioThreshold struct {
syncutil.Mutex
t *admissionpb.IOThreshold // never nil
}

The value used should remain smoothed, using the swag utility on the receiver end to track the maximum.

Added in #78608

cockroach/pkg/util/slidingwindow/sliding_window.go

Line 32 in d7b901d

type Swag struct {

Jira issue: CRDB-18032
@kvoli kvoli added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-distribution Relating to rebalancing and leasing. labels Jul 26, 2022
@kvoli kvoli self-assigned this Jul 26, 2022
@blathers-crl blathers-crl bot added the T-kv KV Team label Jul 26, 2022
@kvoli kvoli mentioned this issue Feb 3, 2023
Closed
@kvoli kvoli mentioned this issue Feb 11, 2023
Closed
@kvoli kvoli mentioned this issue Feb 14, 2023
Merged
kvoli added a commit to kvoli/cockroach that referenced this issue Feb 14, 2023
@kvoli
kvserver: replace read amp with io threshold
fa3ad10 
L0-sublevels used to be checked as a gauge of store health in order to
exclude unhealthy stores from being allocation targets. In cockroachdb#83720,
`IOThreshold` was included into gossip. `IOThreshold` includes
the L0-sublevels, L0-filecount and may be used for additional
measures of store health, making it more informative than looking at
`L0-sublevels` alone.

This commit stops gossiping the L0-sublevels and replaces uses of
L0-sublevels in the allocator and storepool with `IOThreshold`.

The prior cluster settings which controlled how store health was
considered in allocation are now deprecated:

`kv.allocator.L0_sublevels_threshold`
`kv.allocator.L0_sublevels_threshold_enforce`

The new cluster settings which perform an identical function:

`kv.allocator.io_overload_threshold`
Which has the same concept as `L0-sublevels_threshold`, however is
adjusted for `IOThreshold` where a value above 1 indicates overload. The
default cluster setting value is set to `0.8`, which is intentional to
prevent allocation before overload occurs.

`kv.allocator.io_overload_threshold_enforce`
Which is identical to the `L0_sublevels_threshold_enforce`.

Resolves: cockroachdb#85084

Release note (ops change): Deprecate cluster settings
`kv.allocator.l0_sublevels_threshold` and
`kv.allocator.L0_sublevels_threshold_enforce` and introduce
`kv.allocator.io_overload_threshold` and
`kv.allocator.io_overload_threshold_enforce`. The enforcement cluster
setting is unchanged, however renamed. The threshold cluster setting is
adapted to `IOThreshold`, where a value greater or equal to 1 indicates
IO overload. The default is set to 0.8 to prevent allocation prior to
overload occurring.
kvoli added a commit to kvoli/cockroach that referenced this issue Feb 16, 2023
@kvoli
kvserver: replace read amp with io threshold
d00e3da 
L0-sublevels used to be checked as a gauge of store health in order to
exclude unhealthy stores from being allocation targets. In cockroachdb#83720,
`IOThreshold` was included into gossip. `IOThreshold` includes
the L0-sublevels, L0-filecount and may be used for additional
measures of store health, making it more informative than looking at
`L0-sublevels` alone.

This commit stops gossiping the L0-sublevels and replaces uses of
L0-sublevels in the allocator and storepool with `IOThreshold`.

The prior cluster settings which controlled how store health was
considered in allocation are now deprecated:

`kv.allocator.L0_sublevels_threshold`
`kv.allocator.L0_sublevels_threshold_enforce`

The new cluster settings which perform an identical function:

`kv.allocator.io_overload_threshold`
Which has the same concept as `L0-sublevels_threshold`, however is
adjusted for `IOThreshold` where a value above 1 indicates overload. The
default cluster setting value is set to `0.8`, which is intentional to
prevent allocation before overload occurs.

`kv.allocator.io_overload_threshold_enforce`
Which is identical to the `L0_sublevels_threshold_enforce`.

Resolves: cockroachdb#85084

Release note (ops change): Deprecate cluster settings
`kv.allocator.l0_sublevels_threshold` and
`kv.allocator.L0_sublevels_threshold_enforce` and introduce
`kv.allocator.io_overload_threshold` and
`kv.allocator.io_overload_threshold_enforce`. The enforcement cluster
setting is unchanged, however renamed. The threshold cluster setting is
adapted to `IOThreshold`, where a value greater or equal to 1 indicates
IO overload. The default is set to 0.8 to prevent allocation prior to
overload occurring.
kvoli added a commit to kvoli/cockroach that referenced this issue Feb 21, 2023
@kvoli
allocator: replace read amp with io thresh
01ac8be 
Previously, L0-sublevels used to be checked as a gauge of store health
in order to exclude unhealthy stores from being allocation targets. In
the L0-sublevels, L0-filecount and may be used for additional measures
of store health, making it more informative than looking at
`L0-sublevels` alone.

This commit replaces uses of L0-sublevels in the allocator and storepool
with `IOThreshold`. L0-sublevels is still gossiped for compatibility
with mixed version (23.1 <-> 22.2) clusters.

The prior cluster settings which controlled how store health was
considered in allocation decisions is now deprecated:

`kv.allocator.L0_sublevels_threshold`
`kv.allocator.L0_sublevels_threshold_enforce`

The new cluster settings which perform an identical function is:

`kv.allocator.io_overload_threshold`

Which has the same concept as `L0-sublevels_threshold`, however is
adjusted for `IOThreshold` where a value above 1 indicates overload. The
default cluster setting value is set to `0.8`, which is intentional to
prevent allocation before overload occurs.

`kv.allocator.io_overload_threshold_enforcement` Is introduced as a
replacement for `L0_sublevels_threshold_enforce`, the options are
identical.

The 10 minute max L0-sublevel tracking period is not continued for
IOThreshold. Instead, the value is taken as is from gossip.

Resolves: cockroachdb#85084

Release note (ops change): Deprecate cluster settings
`kv.allocator.l0_sublevels_threshold` and
`kv.allocator.L0_sublevels_threshold_enforce` and introduce
`kv.allocator.io_overload_threshold` and
`kv.allocator.io_overload_threshold_enforcement`. The enforcement cluster
setting is unchanged, however renamed. The threshold cluster setting is
adapted to `IOThreshold`, where a value greater or equal to 1 indicates
IO overload. The default is set to 0.8 to prevent allocation prior to
overload occurring.
craig bot pushed a commit that referenced this issue Feb 22, 2023
@michae2 @Xiang-Gu
@kvoli @msbutler
Merge #96914 #97065 #97142 #97495
b3231c0 
96914: sql: add spaces to delimiter in uniqueness violation error r=rharding6373 a=michae2

Whenever a mutation statement violates a uniqueness constraint we return a specific error which shows the affected row. This error is formatted to match the corresponding error in Postgres.

We create this error in two places. In one place (`pkg/sql/opt/exec/execbuilder/mutation.go`) we were using ", " as the delimter between values, which matches Postgres. But in the other place (`pkg/sql/row/errors.go`) we were using "," (without a space).

This patch adds the space.

Epic: None

Release note (bug fix): Fix formatting of uniqueness violation errors to match the corresponding errors from PostgreSQL.

97065: schemachanger: Support dropping index cascade with dependent inbound FK r=healthy-pod a=Xiang-Gu

Dropping a unique index cascade will need to drop any inbound FK constraint 
if this index is the only uniqueness provider.

This commit enables the declarative schema changer to support this behavior.

Fixes: #96731
Epic: None

97142: allocator: replace read amp with io thresh r=irfansharif a=kvoli

We previously checked stores' L0-sublevels to exclude IO overloaded
stores from being allocation targets (#78608). This commit replaces the signal
with the normalized IO overload score instead, which also factors in the
L0-filecount. We started gossiping this value as of #83720. We continue
gossiping L0-sublevels for mixed-version compatibility; we can stop doing this
in 23.2.

Resolves: #85084

Release note (ops change): We've deprecated two cluster settings:
- kv.allocator.l0_sublevels_threshold
- kv.allocator.l0_sublevels_threshold_enforce.
The pair of them were used to control rebalancing and upreplication behavior in
the face of IO overloaded stores. This has been now been replaced by other
internal mechanisms.

97495: roachprod: run scheduled backup init without timeout r=renatolabs a=msbutler

Previously, several roachtests failed during a cluster restart because a node serving the default scheduled backup command was not ready to serve requests. At this time, when roachprod start returns, not every node may be ready to serve requests.

To prevent this failure mode, this patch changes the scheduled backup cmd during roachprod.Start() to run with infinite timeout and only on the the first node in the cluster.

Fixes #97010, #97232

Release note: None

Epic: none

Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
Co-authored-by: Xiang Gu <xiang@cockroachlabs.com>
Co-authored-by: Austen McClernon <austen@cockroachlabs.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
@craig craig bot closed this as completed in 4b11002 Feb 22, 2023
@cockroach-teamcity cockroach-teamcity mentioned this issue Feb 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kvoli kvoli

Labels
A-kv-distribution Relating to rebalancing and leasing. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

allocator: replace read amp with io thresh kvoli/cockroach
1 participant
@kvoli