Refactor version number checks #1738

b-deam · 2023-06-20T13:07:15Z

Serverless Elasticsearch doesn't return a version.number field from the Info API (/) , which is actually something we rely on somewhat commonly throughout the codebase.

To fix this, we need a clearly defined way of determining whether or not Rally is talking to a Serverless Elasticsearch, this applies for both clients used to target the system under test, as well as any ancillary clients like those used by a remote metrics store.

In order to do so, we'll take two approaches:

Create a is_serverless | bool property on both existing and new 'serverless' specific clients
Refactor all version checks to default to selecting the 'minimum' version required for a specific conditional, and only use the is_serverless property if we need to do something specific for serverless

I think this is a balanced approach, given that serverless has no concept of 'versioning'.

Note:

The underlying client's use of .options() complicates things, because every call reinstantiates a new copy of itself, but without the ability for us to pass in any custom arguments (e.g. distribution_version). To work around this we must work out upfront whether or not we'd like the factory to create a serverless or non-serverless client, each of which have a static is_serverless attribute. Fixed in abf4320

Example tests

# http_logs with node-stats and data-stream-stats telemetry devices
esrally race --track http_logs --pipeline=benchmark-only --target-hosts https://serverless-es --client-options='{ "default": {"headers": {"X-Found-Cluster": "09895e10-efb8-42da-be30-888eb623d6cf.es"}, "basic_auth_user":"esbench", "basic_auth_password":"changeme", "verify_certs": false}}' --kill-running-processes --test-mode --track-params 'number_of_replicas: 1' --telemetry "node-stats, data-stream-stats"

# create-track
esrally create-track --track test --indices logs-221998 --target-hosts https://serverless-es --client-options '{ "default": {"headers": {"X-Found-Cluster": "09895e10-efb8-42da-be30-888eb623d6cf.es"}, "basic_auth_user":"elastic", "basic_auth_password":"changeme", "verify_certs": false}}'

pquentin · 2023-06-20T15:51:13Z

buildkite build this please after #1739

…ticsearch

b-deam · 2023-06-21T03:13:14Z

esrally/utils/versions.py

+    elif is_serverless(distribution_version):
+        return "master"


We only every select the master branch of rally-tracks when targeting a serverless cluster.

esrally/client/asynchronous.py

esrally/client/factory.py

pquentin

Thanks for iterating! That looks much better.

pquentin · 2023-06-21T13:31:25Z

esrally/client/asynchronous.py

-            _mimetype_header_to_compat("Content-Type", request_headers)
+        # Not applicable to serverless
+        if not self.is_serverless:
+            if self.distribution_version is not None and (


Can we assume that distribution_version will be defined now?

I think we can for the async client, just because we currently only create them in one place where we already know the distribution flavor/version. I mainly did this to be consistent between the two, but I think a better approach is to just check whether the value is a valid identifier as I've done in 5f2db54

pquentin · 2023-06-21T13:33:18Z

esrally/client/factory.py

+        async def on_request_start(session, trace_config_ctx, params):
+            async_client.on_request_start()
+
+        async def on_request_end(session, trace_config_ctx, params):
+            async_client.on_request_end()
+
+        trace_config = aiohttp.TraceConfig()
+        trace_config.on_request_start.append(on_request_start)
+        trace_config.on_request_end.append(on_request_end)
+        # ensure that we also stop the timer when a request "ends" with an exception (e.g. a timeout)
+        trace_config.on_request_exception.append(on_request_end)
+


Would you mind reverting this change? (Move this back up and replace async_client with RallyAsyncElasticsearch in the async methods.)

Addressed in 94d8634

pquentin · 2023-06-21T13:35:30Z

esrally/client/synchronous.py

-            _mimetype_header_to_compat("Accept", request_headers)
-            _mimetype_header_to_compat("Content-Type", request_headers)
+        if not self.is_serverless:
+            if self.distribution_version is not None and (


Same as above

Addressed in 5f2db54

esrally/tracker/tracker.py

pquentin · 2023-06-21T13:38:41Z

esrally/tracker/tracker.py

    ).create()

-    info = client.info()
-    console.info(f"Connected to Elasticsearch cluster [{info['name']}] version [{info['version']['number']}].\n", logger=logger)
+    console.info(f"Connected to Elasticsearch cluster version [{distribution_version}]\n", logger=logger)


Should we log the flavor too?

I thought about that, but when you're targeting stateful elasticsearch you just get default back, which is a little confusing, so I just left it out entirely. Happy to include it if you think it's important though.

Yes I think it's important to have Rally confirm what sort of cluster it thinks it's benchmarking.

Sure, addressed in 27f1f12. I've kicked off a long running test benchmark too. Will likely merge early next week.

…r' and add tests

pquentin

Thanks for all the iterations! LGTM. Only https://github.com/elastic/rally/pull/1738/files#r1238773617 left, but probably does not warrant another round.

dliappis

LGTM thank you for this massive change!

b-deam added :Load Driver Changes that affect the core of the load driver such as scheduling, the measurement approach etc. discuss Needs further clarification from the team :internal Changes for internal, undocumented features: e.g. experimental, release scripts labels Jun 20, 2023

b-deam self-assigned this Jun 20, 2023

pquentin closed this Jun 20, 2023

pquentin reopened this Jun 20, 2023

b-deam added 9 commits June 21, 2023 11:20

Add new 'serverless' client types

899ea8c

Allow factory to create new 'serverless' clients

21978e3

Update racecontrol, driver, and telemetry to handle 'serverless' elas…

d7036a6

…ticsearch

Update metrics store to support serverless

8fb8605

Update 'create-track' to support serverless

b2ebced

Refactor API key delete version checks

7861170

Update 'WaitForCurrentSnapshotsCreate' version check for serverless

ec9e7fc

Fix 'node-stats' telemetry device version checks

7323e81

Fix 'data-stream-stats' telemetry device version checks

944d78c

b-deam force-pushed the verify-serverless branch from 4d89886 to 944d78c Compare June 21, 2023 02:56

b-deam requested review from pquentin, dliappis and inqueue June 21, 2023 03:07

b-deam commented Jun 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into verify-serverless

52bf8f4

pquentin reviewed Jun 21, 2023

View reviewed changes

esrally/client/asynchronous.py Outdated Show resolved Hide resolved

esrally/client/asynchronous.py Outdated Show resolved Hide resolved

esrally/client/factory.py Show resolved Hide resolved

Replace 'serverless' clients via '.options()' override

abf4320

pquentin reviewed Jun 21, 2023

View reviewed changes

b-deam added 2 commits June 22, 2023 09:56

Replace distribution version 'None' checks with 'is_version_identifie…

5f2db54

…r' and add tests

Revert trace config changes

94d8634

b-deam marked this pull request as ready for review June 22, 2023 00:58

pquentin approved these changes Jun 22, 2023

View reviewed changes

Add 'flavor' to 'create-track' console output

27f1f12

dliappis approved these changes Jun 28, 2023

View reviewed changes

b-deam merged commit c3b04f4 into elastic:master Jul 3, 2023

pquentin added this to the 2.9.0 milestone Aug 24, 2023

pquentin removed the discuss Needs further clarification from the team label Aug 24, 2023

gbanasiak mentioned this pull request Jan 5, 2024

Override serverless cluster version #1818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor version number checks #1738

Refactor version number checks #1738

b-deam commented Jun 20, 2023 •

edited

Loading

pquentin commented Jun 20, 2023

b-deam Jun 21, 2023 •

edited

Loading

pquentin left a comment

pquentin Jun 21, 2023

b-deam Jun 22, 2023

pquentin Jun 21, 2023

b-deam Jun 22, 2023

pquentin Jun 21, 2023

b-deam Jun 22, 2023

pquentin Jun 21, 2023

b-deam Jun 22, 2023

pquentin Jun 22, 2023

b-deam Jun 23, 2023

pquentin left a comment

dliappis left a comment

Refactor version number checks #1738

Refactor version number checks #1738

Conversation

b-deam commented Jun 20, 2023 • edited Loading

pquentin commented Jun 20, 2023

b-deam Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

pquentin 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

pquentin left a comment

Choose a reason for hiding this comment

dliappis left a comment

Choose a reason for hiding this comment

b-deam commented Jun 20, 2023 •

edited

Loading

b-deam Jun 21, 2023 •

edited

Loading