-
Notifications
You must be signed in to change notification settings - Fork 729
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
Add support for the desired nodes API #5650
Conversation
e9f88bd
to
6e8a307
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really clean 👌. I left a few minor comments. In my experiments what I found confusing is the interaction between our resource defaults and resource limits/requests I set explicitly but that is nothing to do with your work here.
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
Commit f511711 relies on elastic/elasticsearch#86434 to set the CPU resources, and makes Elasticsearch 8.3.0 the min. supported version. |
This PR is ready for review, however you'll need elastic/elasticsearch#86434 to be merged in |
I've just merged elastic/elasticsearch#86434. It should be available in the next |
Jenkins test this please |
elastic/elasticsearch#86434 is now available in the last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 as I said before its very clean and easy to read. I did some tests with the latest snapshot build of Elasticsearch that contains the processor ranges change. But my tests where not super thorough I just checked that API was called and that it contained desired nodes information. I am not sure how to verify whether Elasticsearch can take advantage of that information.
pkg/controller/elasticsearch/driver/testdata/desired_nodes/happy_path.json
Outdated
Show resolved
Hide resolved
return nil, false, err | ||
} | ||
visit(nil, settings, func(s string) string { | ||
return knownVariablesReplacer.Replace(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to better understand why we have to replace these variables for the desired nodes API. #5528 (comment) it seems that you are worried some of the optimisations that ES does will be negatively affected if there are still variables in the settings. Can you maybe add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment, the main motivation is to provide a value to the node.name
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some very minor comments.
LGTM!
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com> Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
This PR is a draft implementation to add support for the Elasticsearch desired API. I want to raise it now to allow anyone interested in this feature to try it out. There is an ongoing discussion regarding how the CPU capacity should be provided to Elasticsearch (refer to the related ECK issue or this Elasticsearch PR) which is not yet reflected in this draft.
Required resources
For the operator to be able to call the desired node API the following resources are expected in
nodeSets[*].podTemplate.spec.containers["elasticsearch"]
:limit
and therequest
must be set to a same value.limit
is returned if setlimit
is not set thenrequest
is returnedPersistentVolumeClaim
status, or from thePersistentVolumeClaim
specification if it is not bound yet.What if resources are missing?
If the capacity of at least one resource (cpu,memory or storage) cannot be calculated the reconciliation is not stopped, the desired node API is however not called, it is also cleared. The Elasticsearch status subresource helps to understand why, here are a few examples:
If the state of the desired node API can be calculated for a generation:
Testing
Minimum Elasticsearch version to test this PR is
8.1.0