Introduce max headroom for disk watermark stages #88639

kingherc · 2022-07-20T10:32:22Z

Introduce max headroom settings for the low, high, and flood disk watermark stages, similar to the existing max headroom setting for the flood stage of the frozen tier. Also, convert the disk watermarks to RelativeByteSizeValue, similar to the existing setting for the flood stage of the frozen tier.

Introduce new max headrooms in HealthMetadata and in ReactiveStorageDeciderService.

Add multiple tests in DiskThresholdDeciderUnitTests, DiskThresholdDeciderTests and DiskThresholdMonitorTests.

Fixes #81406

Introduce max headroom settings for the low, high, and flood disk watermark stages, similar to the existing max headroom setting for the flood stage of the frozen tier. Also, convert the disk watermarks to RelativeByteSizeValue, similar to the existing setting for the flood stage of the frozen tier. Introduce new max headrooms in HealthMetadata and in ReactiveStorageDeciderService. Add multiple tests in DiskThresholdDeciderUnitTests, DiskThresholdDeciderTests and DiskThresholdMonitorTests. Fixes elastic#81406

elasticsearchmachine · 2022-07-20T12:35:54Z

Hi @kingherc, I've created a changelog YAML for you.

kingherc · 2022-07-20T14:22:23Z

Hi!

@DaveCTurner , because @henningandersen may see this later , I added you as reviewer because I noticed your name in several tests I modified; please tell me if you'd prefer to add someone else or wait for Henning. @gmarouli , I added you as secondary reviewer because I touched some HealthMetadata parts you had recently written -- with any luck if this PR makes the 8.4.0 version, there will not be any compatibility issues to handle. But feel free to tell me any tips for handling backwards compatibility in case this PR slips the deadline for the 8.4.0 version.

Since this is my first large PR, please tell me tips if you think I should do something differently.

Some notes and questions for you:

Feel free to follow this reviewing order for your ease:

disk_allocator.asciidoc
DiskThresholdSettings.java
DiskThresholdMonitor.java
DiskThresholdDecider.java
HealthMetadata.java
HealthMetadataService.java
Then the rest, including the tests.

I introduced a boolean switch in several tests to make sure we test the new max headroom settings. On purpose, I split tests to test both percentage watermarks and the new max headroom settings. I could, alternatively, not split the tests and rather introduce the switch with a testMaxHeadroom=randomBoolean() inside the test, so as to randomly test either the percentage settings or the max headrooms. Please tell me if you think this would be better.
In modifying disk_allocator.asciidoc to add the new max headroom settings, I saw a {ess-icon} next to the watermark settings. I added the same icon. But, should I do anything related to ESS or notify the ESS team?
There was a rounding error happening calculating disk usage in some cases. Corrected it in a commit.

DaveCTurner

This change touches a lot of code (+2,021 −885 !!) and does a handful of different things and I wonder if it would be possible for you to split it up into steps for ease of reviewing. For instance, it looks like we could move DiskThresholdSettings to use RelativeByteSizeValue as a preliminary step before introducing the max headroom parameters. Likewise the move to using formatNoTrailingZerosPercent and the change from 100.0 * (freeBytes / totalBytes) to (100.0 * freeBytes) / totalBytes seem independent of this change (or maybe they're prerequisites?) so could happen first.

That said the overall shape of the change looks sensible to me. I think adding a boolean parameter to each test and running them all each time is fine.

DaveCTurner · 2022-07-20T16:12:37Z

I wonder if it would be possible for you to split it up into steps for ease of reviewing.

By "split it up into steps" I probably mean doing each step as a separate PR. Sometimes it works to have a sequence of commits in a single PR but it's hard to iterate on that kind of change.

kingherc · 2022-07-20T18:32:37Z

Hi @DaveCTurner , I get you. I also thought of this problem in the last couple of days. I was mostly working on the tests so that they are comprehensive, and the test-related changes of the new max headroom settings are the bulk of the PR. The good thing is that the test changes can be reviewed without going back-and-forward. I believe following the order of reading I recommended will get you through the main code changes, while the rest of the test-related changes can be read in a forward-reading manner at your own leisure/pace.

Some of the changes that you mentioned could be done in separate commits, to ease reviewing. I can do that tomorrow. I am a bit hesitant to split into separate PRs, because:

I would ideally see this through sooner than later to make it in the 8.4.0 version FF, if there are no major concerns of course, else I would need to figure out the compatibility issues with the HealthMetadataService (which may not be major, but I did not need to care for them now), and
of the additional effort to extricate seemingly orthogonal changes (which I am not yet 100% sure they will be totally orthogonal) that can be self-standing PRs passing the merge gates separately. While with separate commits, this will not be an issue for the merge gate.

Taking the above in consideration, if it is OK with you to review separate commits, it would be great. However, if you believe that I should split in separate PRs, that is also perfectly fine, I can go ahead and do that!

DaveCTurner · 2022-07-21T06:28:09Z

As things stand I don't think I'll be able to review this satisfactorily by 8.4.0 FF - there's just too much going on in this one PR and this stuff is important to get right. Let's not fixate on that date too much, we will be able to make this compatible in a later version if needed.

kingherc · 2022-07-21T08:16:17Z

Hi @DaveCTurner , that makes sense, it is a very tight window until FF. No problem, please review when you can, and indeed I will make it compatible as/if needed after the FF. I will go ahead and split commits for easier reviewing, but please tell me if I should split PRs. BTW, if you do not review it this week, then @henningandersen can review instead of you next week, since I believe either one of your reviews would be sufficient and since he seems more relevant to how he introduced the max headroom setting for the frozen tier. Of course if you still want to review, feel free, your comments are definitely appreciated :)

@gmarouli , when you also review, please comment on compatibility implications, assuming this PR makes it after the 8.4.0 FF. E.g., if there would be any issues introducing the new max headroom in the HealthMetadata after 8.4.0 with some version if switches in the serialization and/or XContent (is HealthMetadata exposed to REST?).

I am thinking I should not merge this with only one approval. So I will wait for at least two approvals (@gmarouli and at least one of @DaveCTurner or @henningandersen ). Please tell me if I should do differently. Thanks so much for taking the time to review this!

DaveCTurner · 2022-07-21T08:34:48Z

👍 I think it might be tight to have Henning review all of this before FF too but let's see. I'm very comfortable reviewing the bits that I highlighted as possible preliminaries, especially since they are refactorings which shouldn't change behaviour, and I expect that will make the behaviour-changing side of this PR much more manageable. That's why I'd rather approve it in steps so I'd still like to see separate PRs please.

One LGTM is usually acceptable unless the reviewer indicates otherwise.

kingherc · 2022-07-21T10:07:48Z

Thanks for the tips @DaveCTurner ! If one LGTM is enough, I believe I could disengage you & Henning, and ask @gmarouli if you would be comfortable to review this, maybe in time for the FF? Feel free to tell me if you'd like separate commits/PRs and I can do them immediately.

kingherc · 2022-08-18T14:48:38Z

Hi @henningandersen ! gentle reminder for giving another review

kingherc · 2022-08-25T08:29:54Z

Gentle ping @henningandersen :)

kingherc · 2022-09-07T08:55:35Z

@henningandersen gentle reminder for a final review/approval now that we know we can merge this without unintended repercussions.

henningandersen

Had another look and left a few more comments. I did not check the health part though, but I think @gmarouli went into detail there.

docs/reference/modules/cluster/disk_allocator.asciidoc

...lClusterTest/java/org/elasticsearch/cluster/routing/allocation/decider/MockDiskUsagesIT.java

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java

Co-authored-by: Henning Andersen <[email protected]>

To org.elasticsearch.cluster.routing.allocation.decider.MockDiskUsagesIT#testRerouteOccursOnDiskPassingHighWatermark

That if it is set, then the watermarks need to be ratios.

kingherc · 2022-09-12T15:25:38Z

Thanks @henningandersen for leaving feedback! 👍
I resolved them, feel free to check again. Thanks!

henningandersen

I only have one comment left on this now.

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java

And tests for corner edge cases where they could default to -1 and the user needs to explicitly set any higher-level headrooms.

kingherc · 2022-09-19T08:12:20Z

Hi @henningandersen ! Just pushed a commit to disallow explicitly setting headrooms to -1. There are still the corner edge cases where they could default to -1, so I leave those checks in place for these corner cases. I added appropriate tests for all these.

Please check the commit, and if that was your last worry, feel free to approve, or otherwise comment. Thanks!

henningandersen

LGTM.

docs/reference/modules/cluster/disk_allocator.asciidoc

So set headrooms only if they are not -1

idegtiarenko · 2023-07-12T08:58:07Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java

+                        + lowHeadroom.getStringRep()
+                        + "]"
+                );
+            }


I think there is an issue with this validation. Imagine a user is upgrading a cluster from a version prior to this change to a version with it.

The prior settings are:

cluster.routing.allocation.disk.watermark.low=null (is not set) cluster.routing.allocation.disk.watermark.high=91% (any non-default value)

This way the new settings are initialized as:

cluster.routing.allocation.disk.watermark.low.max_headroom=200GB cluster.routing.allocation.disk.watermark.high.max_headroom=-1

that would fail this validation.

I believe the similar could happen for below validation of highHeadroom & floodHeadroom.

Hi @idegtiarenko, indeed, reading the code I believe you're right. This would make the user need to define the max headroom explicitly. Seems like an issue that could break upgrades indeed.

I think a solution out of this would be that the default values of the headrooms return -1 if any of the watermarks are explicitly set. Currently they return -1 only if the respective watermark is explicity set. So with this correction, it would mean that the low max headroom in your example would return -1 instead of 200GB.

Have you created a ticket for this?

I believe this is covered by this issue: #93468

elasticsearchmachine added the v8.4.0 label Jul 20, 2022

kingherc force-pushed the feature/81406-disk-max-headrooms branch from 6c1bd74 to 10bca27 Compare July 20, 2022 10:42

kingherc force-pushed the feature/81406-disk-max-headrooms branch from 10bca27 to 61d4f32 Compare July 20, 2022 11:40

kingherc self-assigned this Jul 20, 2022

kingherc added 3 commits July 20, 2022 15:35

Update docs/changelog/88639.yaml

64a04a6

Fix changelog

0f2ff24

Fix double rounding errors for disk usage

378bb4d

kingherc marked this pull request as ready for review July 20, 2022 14:15

kingherc requested review from DaveCTurner and gmarouli July 20, 2022 14:16

kingherc added the cloud-deploy Publish cloud docker image for Cloud-First-Testing label Jul 20, 2022

kingherc requested a review from henningandersen July 20, 2022 14:49

DaveCTurner reviewed Jul 20, 2022

View reviewed changes

Use IllegalArgumentException for subtraction -1

7361f18

henningandersen self-requested a review August 18, 2022 16:02

Merge branch 'main' into feature/81406-disk-max-headrooms

43f9802

Merge branch 'main' into feature/81406-disk-max-headrooms

6422477

henningandersen reviewed Sep 12, 2022

View reviewed changes

kingherc and others added 5 commits September 12, 2022 16:05

Update docs/reference/modules/cluster/disk_allocator.asciidoc

7406677

Co-authored-by: Henning Andersen <[email protected]>

Update docs/reference/modules/cluster/disk_allocator.asciidoc

c37c0ea

Co-authored-by: Henning Andersen <[email protected]>

Add more randomness to test

756d44e

To org.elasticsearch.cluster.routing.allocation.decider.MockDiskUsagesIT#testRerouteOccursOnDiskPassingHighWatermark

Fix headroom validator for flood number

bbf610a

That if it is set, then the watermarks need to be ratios.

Merge branch 'main' into feature/81406-disk-max-headrooms

04277ab

kingherc requested a review from henningandersen September 12, 2022 15:25

henningandersen reviewed Sep 15, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java Show resolved Hide resolved

kingherc added 3 commits September 15, 2022 16:36

Add comment around max headroom validator settings

4b04aec

Merge branch 'main' into feature/81406-disk-max-headrooms

2c1325c

Disallow headroom values to be set to -1 by user

2910426

And tests for corner edge cases where they could default to -1 and the user needs to explicitly set any higher-level headrooms.

henningandersen approved these changes Sep 19, 2022

View reviewed changes

docs/reference/modules/cluster/disk_allocator.asciidoc Outdated Show resolved Hide resolved

kingherc added 4 commits September 19, 2022 12:41

Fix tests due to not being able to set to -1

e716d00

So set headrooms only if they are not -1

Simplify documentation

34aedce

Fix test

8a64f56

Fix test

445e114

kingherc merged commit 34471b1 into elastic:main Sep 19, 2022

kingherc deleted the feature/81406-disk-max-headrooms branch September 19, 2022 11:59

fcofdez mentioned this pull request Jan 23, 2023

Disk health indicator doesn't take into account watermark.low/high.max_headroom #93155

Closed

idegtiarenko reviewed Jul 12, 2023

View reviewed changes

kingherc mentioned this pull request Jul 12, 2023

Cannot update a subset of the disk watermark settings #93468

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce max headroom for disk watermark stages #88639

Introduce max headroom for disk watermark stages #88639

kingherc commented Jul 20, 2022 •

edited

Loading

elasticsearchmachine commented Jul 20, 2022

kingherc commented Jul 20, 2022

DaveCTurner left a comment

DaveCTurner commented Jul 20, 2022

kingherc commented Jul 20, 2022

DaveCTurner commented Jul 21, 2022

kingherc commented Jul 21, 2022

DaveCTurner commented Jul 21, 2022

kingherc commented Jul 21, 2022

kingherc commented Aug 18, 2022

kingherc commented Aug 25, 2022

kingherc commented Sep 7, 2022

henningandersen left a comment

kingherc commented Sep 12, 2022

henningandersen left a comment

kingherc commented Sep 19, 2022

henningandersen left a comment

idegtiarenko Jul 12, 2023

kingherc Jul 12, 2023

idegtiarenko Jul 12, 2023

Introduce max headroom for disk watermark stages #88639

Introduce max headroom for disk watermark stages #88639

Conversation

kingherc commented Jul 20, 2022 • edited Loading

elasticsearchmachine commented Jul 20, 2022

kingherc commented Jul 20, 2022

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Jul 20, 2022

kingherc commented Jul 20, 2022

DaveCTurner commented Jul 21, 2022

kingherc commented Jul 21, 2022

DaveCTurner commented Jul 21, 2022

kingherc commented Jul 21, 2022

kingherc commented Aug 18, 2022

kingherc commented Aug 25, 2022

kingherc commented Sep 7, 2022

henningandersen left a comment

Choose a reason for hiding this comment

kingherc commented Sep 12, 2022

henningandersen left a comment

Choose a reason for hiding this comment

kingherc commented Sep 19, 2022

henningandersen left a comment

Choose a reason for hiding this comment

idegtiarenko Jul 12, 2023

Choose a reason for hiding this comment

kingherc Jul 12, 2023

Choose a reason for hiding this comment

idegtiarenko Jul 12, 2023

Choose a reason for hiding this comment

kingherc commented Jul 20, 2022 •

edited

Loading