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

Detect serverless operator status automatically #1768

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

pquentin
Copy link
Member

This can be tested like #1760, but you no longer need to explicitly set the operator status in rally.ini, instead it should be detected automatically.

@pquentin pquentin added enhancement Improves the status quo :Load Driver Changes that affect the core of the load driver such as scheduling, the measurement approach etc. labels Aug 23, 2023
@pquentin pquentin self-assigned this Aug 23, 2023
@@ -346,7 +347,16 @@ def cluster_distribution_version(hosts, client_options, client_factory=EsClientF
# version number does not exist for serverless
version_number = version.get("number", version_build_flavor)

return version_build_flavor, version_number, version_build_hash
serverless_operator = False
if version_build_flavor == "serverless":
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the is_serverless helper function for this?

def is_serverless(text):
return text == "serverless"

authentication_info = es.perform_request(method="GET", path="/_security/_authenticate")
serverless_operator = authentication_info.body.get("operator", False)

if not version_build_flavor == "serverless" or serverless_operator is True:
Copy link
Member

Choose a reason for hiding this comment

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

# unconditionally wait for the REST layer - if it's not up by then, we'll intentionally raise the original error
wait_for_rest_layer(es)

# wait_for_rest_layer calls the Cluster Health API, which is not available for unprivileged users on Serverless
Copy link
Member

Choose a reason for hiding this comment

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

Can we please update the docstring to include the new serverless_operator return value?

Copy link
Member

@b-deam b-deam left a comment

Choose a reason for hiding this comment

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

I get the sense that it might be better to split out the retrieval of the versions, operator status, and REST API health check into discrete functions, but I don't think it's worth blocking this PR.

Copy link
Member

@gareth-ellis gareth-ellis left a comment

Choose a reason for hiding this comment

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

LGTM

@pquentin
Copy link
Member Author

I get the sense that it might be better to split out the retrieval of the versions, operator status, and REST API health check into discrete functions, but I don't think it's worth blocking this PR.

Well this is definitely quite ugly. I want to use a single Elasticsearch client here but did not want to affect the stateful uses of this function too much. This is the best compromise I found. However, if we think the logic of this function is fine, then I'm not convinced separating it into other functions would have helped that much. The logic is complicated but 20 lines is not that much in my opinion, even though Uncle Bob would disagree.

@b-deam
Copy link
Member

b-deam commented Aug 25, 2023

I get the sense that it might be better to split out the retrieval of the versions, operator status, and REST API health check into discrete functions, but I don't think it's worth blocking this PR.

Well this is definitely quite ugly. I want to use a single Elasticsearch client here but did not want to affect the stateful uses of this function too much. This is the best compromise I found. However, if we think the logic of this function is fine, then I'm not convinced separating it into other functions would have helped that much. The logic is complicated but 20 lines is not that much in my opinion, even though Uncle Bob would disagree.

I agree, no need for any changes. :shipit:

@pquentin pquentin merged commit 8dae2c3 into elastic:master Aug 28, 2023
14 checks passed
@pquentin pquentin deleted the detect-operator-status branch August 28, 2023 07:51
@pquentin pquentin added this to the 2.9.1 milestone Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :Load Driver Changes that affect the core of the load driver such as scheduling, the measurement approach etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants