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

Allow node to enroll to cluster on startup #77718

Merged
merged 68 commits into from
Oct 27, 2021

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Sep 14, 2021

The functionality to enroll a new node to a cluster was
introduced in #77292 as a CLI tool. This change allows to trigger
the enrollment functionality on startup of elasticsearch via a
named argument that can be passed to the es
startup script (--enrollment-token) so that the users that want to
do this do not need to run two commands instead of one.

In a followup PR we are introducing a CLI tool version of this
functionality, that can be used to reconfigure packaged
installations.

The functionality to enroll a new node to a cluster was
introduced in elastic#77292 as a CLI tool. This change brings forward two
additions:

- It allows to trigger the enrollment functionality on startup of
elasticsearch via a named argument that can be passed to the es
startup script (--enrollment-token) so that the users that want to
do this do not need to run two commands instead of one.
- It adds a `-f, --force` flag to the standalone CLI version of
this which bypasses the checks for whether there are data in the
current node or whether security related features are already
configured. The primary use case for this flag is enrolling new
nodes in packaged installations. There we auto-configure each
installation as a first node of a cluster by default because we
don't have a user-friendly and generic way to allow the user
decide on installation time. Thus, we want to allow users to
easily reconfigure the node to enroll to an existing cluster
without requiring many manual steps.
@jkakavas jkakavas added >feature :Delivery/Build Build or test infrastructure :Core/Infra/Core Core issues without another label v8.0.0 labels Sep 14, 2021
@elasticmachine elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Delivery Meta label for Delivery team labels Sep 14, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@jkakavas
Copy link
Member Author

@albertzaharovits anything in EnrollNodeToCluster class goes, you were not here when it was merged, happy to get your thoughts/feedback 🙏

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Reminder, we'll need packaging tests here to actually verify these shell/batch script changes work as intended.

distribution/src/bin/elasticsearch Outdated Show resolved Hide resolved
@jkakavas
Copy link
Member Author

jkakavas commented Sep 15, 2021

Again, we'll need packaging tests here to actually verify these shell/batch script changes work as intended.

Thank you for the feedback. The original plan is to add QA project(s) for the enrollment process as separate PRs once this and #77231 are merged but you are right it does make sense to test for the scripts behavior here too. I added a class

It did uncover a number of issues too, so thanks for the nudge to add the tests sooner than later! This is ready for a review

@jkakavas
Copy link
Member Author

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

@jkakavas
Copy link
Member Author

Thanks for your comments @mark-vieira , can you please clarify what you mean by

I'm missing the bit that actually calls into EnrollNodeToCluster

@jkakavas
Copy link
Member Author

@elasticmachine run elasticsearch-ci/part-1

@@ -129,3 +162,8 @@ ECHO.!KEYSTORE_PASSWORD!| %JAVA% %ES_JAVA_OPTS% -Delasticsearch ^
endlocal
endlocal
exit /b %ERRORLEVEL%

Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: I'm more than happy to remove this ugliness if we don't care about exiting with 0 (instead of an 1) when a user passes multiple --enrollment-token parameters

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

The shell changes look ok to me. I hope we (core/infra) can simplify startup in the future so changes like this are not so cryptic or difficult to make.

@jkakavas
Copy link
Member Author

I hope we (core/infra) can simplify startup in the future so changes like this are not so cryptic or difficult to make.

Thanks ,happy to re-work and simplify this when possible

@jkakavas jkakavas merged commit 5d3b6bf into elastic:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label :Delivery/Build Build or test infrastructure >feature Team:Core/Infra Meta label for core/infra team Team:Delivery Meta label for Delivery team v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants