Add support for CPU ranges in desired nodes #86434

fcofdez · 2022-05-04T16:46:45Z

This commit adds support for CPU ranges in the desired nodes API.

This aligns better with environments where administrators/orchestrators
can define lower and upper bounds for the amount of CPUs that the
desired node would get once deployed.

This allows to provide information about the expected CPU and possible
allowed overcommit that the desired node will run on.

This was the previous expected body for the desired nodes API (we still support it):

PUT /_internal/desired_nodes/history/1
{
    "nodes" : [
        {
            "settings" : {
                 "node.name" : "instance-000187",
                 "node.external_id": "instance-000187",
                 "node.roles" : ["data_hot", "master"],
                 "node.attr.data" : "hot",
                 "node.attr.logical_availability_zone" : "zone-0"
            },
            "processors" : 8, 
            "memory" : "58gb",
            "storage" : "1700gb",
            "node_version" : "8.3.0"
        }
    ]
}

Now it's possible to define processors or processors_range as in:

PUT /_internal/desired_nodes/history/1
{
    "nodes" : [
        {
            "settings" : {
                 "node.name" : "instance-000187",
                 "node.external_id": "instance-000187",
                 "node.roles" : ["data_hot", "master"],
                 "node.attr.data" : "hot",
                 "node.attr.logical_availability_zone" : "zone-0"
            },
            "processors_range" : {"min": 8.0, "max": 16.0},
            "memory" : "58gb",
            "storage" : "1700gb",
            "node_version" : "8.3.0"
        }
    ]
}

Note that max in processors_range is optional.

This commit also moves from representing CPUs as integers to
accept floating point numbers.

Note: I disabled the bwc yamlRestTests for versions < 8.3 since we introduced
a few "breaking changes" but since this is an internal API it should be fine.

elasticmachine · 2022-05-05T10:07:03Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2022-05-05T10:07:23Z

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

elasticmachine · 2022-05-05T11:44:01Z

Pinging @elastic/clients-team (Team:Clients)

sethmlarson

LGTM, two questions:

Are the memory and storage fields going to also have this change eventually?
Can the processors.min be set without processors.max or is the value always required?

fcofdez · 2022-05-05T15:09:57Z

Thanks for the review @sethmlarson

Are the memory and storage fields going to also have this change eventually?

It might change as more internal users adopt this API.

Can the processors.min be set without processors.max or is the value always required?

When processors is represented as an object, min and max are required.
The API still supports the old version, where you can specify the number of processors as a number. In that case min and max are the supplied value.

henningandersen

Did an initial read and left a few comments.

henningandersen · 2022-05-05T16:58:18Z

docs/reference/cluster/update-desired-nodes.asciidoc

@@ -77,7 +77,7 @@ PUT /_internal/desired_nodes/Ywkh3INLQcuPT49f6kcppA/100
                 "node.attr.data" : "hot",
                 "node.attr.logical_availability_zone" : "zone-0"
            },
-            "processors" : 8,


I wonder if we would not want to keep the processors: 8 case in the documentation (perhaps using 8.0 instead). I think the best world is if we have just the one number rather than multiple and having that case be simpler for those orchestrations that can supply it seems desirable.

Let us also mention that processors can be specified as a range, that makes it easier to see which releases the range syntax is supported in (and documents it).

henningandersen · 2022-05-05T17:00:14Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml

@@ -774,3 +767,135 @@ teardown:
            - { settings: { "node.external_id": "instance-000187"}, processors: 64, memory: "1b", storage: "1b", node_version: null }
  - match: { status: 400 }
  - match: { error.type: x_content_parse_exception }
+---
+"Test update desired nodes using legacy processors field":


I would not call this legacy, the min/max is not really the desired way to use this.

henningandersen · 2022-05-05T17:01:42Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml

+"Test update desired nodes using legacy processors field":
+  - skip:
+      reason: "contains is a newly added assertion"
+      features: contains


It looks like we are not using contains here (as well as the rest of the new tests)?

Copy-paste... 🤦

henningandersen · 2022-05-05T17:05:23Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

+        }
+
+        public int roundedMin() {
+            return Math.round(min);


I wonder if we should round this down and round max up? That seems more intuitive, since then the actual quota of processors is still within the range.

henningandersen · 2022-05-05T17:16:30Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

@@ -86,21 +91,23 @@ private static Version parseVersion(String version) {
    }

    private final Settings settings;
-    private final int processors;
+    private final Processors processors;


I am not sure this will work in a mixed cluster, since we are changing the type of a field stored in cluster state (regardless of whether the new API has been used) and an old version cannot read it. I think we need a rolling upgrade test. Also, I wonder if we should simply make it two fields instead like processors and processors_range - to simplify parsing both for ourselves and clients? We could still make the two fields mutually exclusive.

I made the assumption that we would be able to break this since we don't have known users of this API, but that might be a risky assumption. I'm wondering if we should prevent upgrades during mixed-version clusters (at least when there are 8.2 and 8.3 master nodes) as we should be able to accept a processor range but the older nodes won't have that notion and we would have to send them the lower bound, maybe it's better to reject the updates in that phase to simplify things? wdty @henningandersen

but that might be a risky assumption

Yes, it can be a bit hard to control that it is never called on an 8.2 cluster. And if so, it will essentially break the cluster during upgrade, which sounds bad.

we would have to send them the lower bound, maybe it's better to reject the updates in that phase to simplify things?

It sounds reasonable to me to reject a range specification during an upgrade to the release allowing the range specification, since otherwise we would be throwing away data.

I think we need to still accept a single processors specification in a mixed cluster though, i.e., as long as the client conforms to the old version API, everything should still work.

henningandersen · 2022-05-05T17:20:39Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

+
+        static {
+            PROCESSORS_PARSER.declareFloat(ConstructingObjectParser.constructorArg(), MIN_FIELD);
+            PROCESSORS_PARSER.declareFloat(ConstructingObjectParser.constructorArg(), MAX_FIELD);


Should this be optional? I think k8s might only have requests set, leading to no max cpu. In that case we would still like to know the minimum value.

…ssors

…search into desired-nodes-processors

…ssors

barkbay

I made a few tests using a dev version of the ECK operator (from this PR) and it works great 👍

Some comments/questions:

Setting a value of 0 for the max CPU capacity does not seem to clear a previous max value. It also does not raise an error as I think it is the case in the current implementation. I'm wondering if setting a value of 0 should raise an error as it is the case now? (and also maybe explain it in the documentation?)
I also want to double check the units: if a memory capacity of 2333Mi is set in the K8S resource, it is translated by the ECK operator as 2446327808b:

In K8S:

  resources:
    requests:
      memory: 2333Mi
      cpu: 2222m
    limits:
      memory: 2333Mi
      cpu: 3141m

This the content of the API request by the ECK operator:

PUT /_internal/desired_nodes/history/version

"processors_range": {
	"min": 2.222,
	"max": 3.141
},
"memory": "2446327808b"

It is however then stored in Elasticsearch as 2.2gb in the desired node state:

GET _internal/desired_nodes/_latest

      "processors_range" : {
        "min" : 2.222,
        "max" : 3.141
      },
      "memory" : "2.2gb"

I just want to be sure it is the expected result given that it should be either 2.2783203125 GiB or 2.446327808 GB. I think this is because "2.2gb" actually means "2.2GiB"?

Thanks!

henningandersen

This looks mostly good. My only real concerns is with how ProcessorsRange.max() returns the min value if not specified, that looks wrong to me.

henningandersen · 2022-05-16T11:59:08Z

docs/reference/cluster/update-desired-nodes.asciidoc

@@ -77,7 +77,7 @@ PUT /_internal/desired_nodes/Ywkh3INLQcuPT49f6kcppA/100
                 "node.attr.data" : "hot",
                 "node.attr.logical_availability_zone" : "zone-0"
            },
-            "processors" : 8,


Let us also mention that processors can be specified as a range, that makes it easier to see which releases the range syntax is supported in (and documents it).

henningandersen · 2022-05-16T12:03:41Z

qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/DesiredNodesUpgradeIT.java

+public class DesiredNodesUpgradeIT extends AbstractRollingTestCase {
+    public void testUpgradeDesiredNodes() throws Exception {
+        // Desired nodes was introduced in 8.2
+        if (UPGRADE_FROM_VERSION.before(Version.V_8_2_0)) {


I think we introduced it in 8.1, so we should test with upgrade from version = 8.1+?

henningandersen · 2022-05-16T12:23:09Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

+
+        @Override
+        public Float max() {
+            return max == null ? min : max;


I would have thought we would make this return null if max is null? Or infinity.

henningandersen · 2022-05-17T09:26:02Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java

+            IllegalArgumentException.class,
+            () -> new DesiredNode(
+                settings,
+                new DesiredNode.ProcessorsRange(randomFloat() + 1, randomInvalidFloatProcessor()),


Suggested change

new DesiredNode.ProcessorsRange(randomFloat() + 1, randomInvalidFloatProcessor()),

new DesiredNode.ProcessorsRange(randomFloat() + 0.1, randomInvalidFloatProcessor()),

henningandersen · 2022-05-17T09:28:05Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java

+            IllegalArgumentException.class,
+            () -> new DesiredNode(
+                settings,
+                new DesiredNode.ProcessorsRange(3.0f, 2.0f),


This would pass even if comparing ints. Perhaps we can pick the lower bound randomly between 0.1-10 and the upper bound between 0.01 and lower bound-Math.ulp(lower bound)?

TIL about ulp. Thanks for the tip!

henningandersen · 2022-05-17T11:14:14Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java

+    }
+
+    private Float randomInvalidFloatProcessor() {
+        return randomFrom(-1.0f, Float.NaN, Float.POSITIVE_INFINITY, Float.NEGATIVE_INFINITY);


Should we also include 0.0f?

henningandersen · 2022-05-17T11:16:11Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java

+            case 1 -> randomDesiredNode(version, randomIntBetween(1, 256), settingsProvider);
+            case 2 -> randomDesiredNode(version, randomIntBetween(1, 256) + randomFloat(), settingsProvider);
+            default -> throw new IllegalStateException();
+        };
    }

    public static DesiredNode randomDesiredNode(Version version, int processors, Consumer<Settings.Builder> settingsProvider) {


Can we simply remove this overload?

…ssors

fcofdez · 2022-05-18T14:24:06Z

Thanks for testing the PR @barkbay!

Setting a value of 0 for the max CPU capacity does not seem to clear a previous max value. It also does not raise an error as I think it is the case in the current implementation. I'm wondering if setting a value of 0 should raise an error as it is the case now? (and also maybe explain it in the documentation?)

This is strange, it should fail (I even added a couple of tests that cover that case). When you mention that it does not clear the previous max value, do you mean that the Desired nodes remain in the previous version?

I also want to double check the units: if a memory capacity of 2333Mi is set in the K8S resource, it is translated by the ECK operator as 2446327808b:

Yes, it's GiB but it states that the unit is gb. Is that problematic?

barkbay · 2022-05-19T06:10:48Z

This is strange, it should fail (I even added a couple of tests that cover that case). When you mention that it does not clear the previous max value, do you mean that the Desired nodes remain in the previous version?

I'm not able to reproduce, it might be a problem on my side.

Yes, it's GiB but it states that the unit is gb. Is that problematic?

No, just want to be sure I was understanding the units correctly.

…ssors

henningandersen

LGTM, thanks Francisco.

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

…dNode.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

fcofdez · 2022-05-20T07:32:55Z

@elasticmachine upgrade branch

fcofdez · 2022-05-20T09:47:45Z

Thanks all for the reviews!

elasticsearchmachine added the v8.3.0 label May 4, 2022

fcofdez added 5 commits May 5, 2022 10:52

Add support for CPU ranges in desired nodes

c7eb3e2

Fix mistake

7384292

Change backwards compatible policy

4a6d0b9

Update desired nodes docs

39f3603

Add a few tests

b2b4f26

fcofdez force-pushed the desired-nodes-processors branch from 4a031f9 to b2b4f26 Compare May 5, 2022 08:52

fcofdez added Team:Distributed Meta label for distributed team :Distributed/Autoscaling >enhancement labels May 5, 2022

fcofdez marked this pull request as ready for review May 5, 2022 10:07

fcofdez requested a review from henningandersen May 5, 2022 10:07

Update docs/changelog/86434.yaml

7751acb

fcofdez mentioned this pull request May 5, 2022

Call desired nodes API upon desired state changes elastic/cloud-on-k8s#5528

Closed

sethmlarson added the Team:Clients Meta label for clients team label May 5, 2022

sethmlarson approved these changes May 5, 2022

View reviewed changes

henningandersen reviewed May 5, 2022

View reviewed changes

fcofdez added 5 commits May 9, 2022 11:03

Reviews

6cd28c2

Support processor ranges and processors as separate fields

ab44132

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

cb24fdb

…ssors

Merge branch 'desired-nodes-processors' of github.com:fcofdez/elastic…

33964ec

…search into desired-nodes-processors

Max is not rendered

74d4ef8

barkbay mentioned this pull request May 10, 2022

Add support for the desired nodes API elastic/cloud-on-k8s#5650

Merged

fcofdez added 3 commits May 10, 2022 16:44

Take into account newer versions in upgrade test

32934e4

Spotless

5bb3f51

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

9e1990f

…ssors

fcofdez added 7 commits May 11, 2022 10:00

Clarification

3cc813e

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

c922cc1

…ssors

Remove unused contains

493e241

Upgrade test

4ac2461

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

40190a4

…ssors

Change size

f56b63e

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

2dfa5e7

…ssors

fcofdez requested a review from henningandersen May 11, 2022 11:48

barkbay reviewed May 17, 2022

View reviewed changes

henningandersen reviewed May 17, 2022

View reviewed changes

fcofdez added 2 commits May 18, 2022 16:02

Review comments

02dceff

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

1142f69

…ssors

Docs

7016271

fcofdez added 2 commits May 19, 2022 11:02

Fix docs

68c97cf

Merge remote-tracking branch 'origin/master' into desired-nodes-proce…

9906fe0

…ssors

fcofdez requested a review from henningandersen May 19, 2022 10:06

henningandersen approved these changes May 19, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java Outdated Show resolved Hide resolved

Update server/src/main/java/org/elasticsearch/cluster/metadata/Desire…

ea58538

…dNode.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com>

fcofdez merged commit e91e7e6 into elastic:master May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CPU ranges in desired nodes #86434

Add support for CPU ranges in desired nodes #86434

fcofdez commented May 4, 2022 •

edited

Loading

elasticmachine commented May 5, 2022

elasticsearchmachine commented May 5, 2022

elasticmachine commented May 5, 2022

sethmlarson left a comment

fcofdez commented May 5, 2022

henningandersen left a comment

henningandersen May 5, 2022

henningandersen May 16, 2022

henningandersen May 5, 2022

henningandersen May 5, 2022

fcofdez May 9, 2022

henningandersen May 5, 2022

henningandersen May 5, 2022

fcofdez May 9, 2022

henningandersen May 9, 2022

henningandersen May 5, 2022

barkbay left a comment

henningandersen left a comment

henningandersen May 16, 2022

henningandersen May 16, 2022

henningandersen May 16, 2022

henningandersen May 17, 2022

henningandersen May 17, 2022

fcofdez May 18, 2022

henningandersen May 17, 2022

henningandersen May 17, 2022

fcofdez commented May 18, 2022

barkbay commented May 19, 2022

henningandersen left a comment

fcofdez commented May 20, 2022

fcofdez commented May 20, 2022

	new DesiredNode.ProcessorsRange(randomFloat() + 1, randomInvalidFloatProcessor()),
	new DesiredNode.ProcessorsRange(randomFloat() + 0.1, randomInvalidFloatProcessor()),

Add support for CPU ranges in desired nodes #86434

Add support for CPU ranges in desired nodes #86434

Conversation

fcofdez commented May 4, 2022 • edited Loading

elasticmachine commented May 5, 2022

elasticsearchmachine commented May 5, 2022

elasticmachine commented May 5, 2022

sethmlarson left a comment

Choose a reason for hiding this comment

fcofdez commented May 5, 2022

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barkbay left a comment

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcofdez commented May 18, 2022

barkbay commented May 19, 2022

henningandersen left a comment

Choose a reason for hiding this comment

fcofdez commented May 20, 2022

fcofdez commented May 20, 2022

fcofdez commented May 4, 2022 •

edited

Loading