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

Add waitForReady to base config for FTR serverless #165522

Merged
merged 7 commits into from
Sep 5, 2023

Conversation

Ikuni17
Copy link
Contributor

@Ikuni17 Ikuni17 commented Sep 1, 2023

Summary

waitForReady was added in #165467. This PR utilizes it so that FTR doesn't continue with starting Kibana until ES has reported its ready. We get quite a few of these errors every time, and it could be misleading. It adds 20-30s to startup time.

@Ikuni17 Ikuni17 added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting ci:serverless-test-all v8.11.0 labels Sep 1, 2023
@Ikuni17 Ikuni17 self-assigned this Sep 1, 2023
@Ikuni17 Ikuni17 requested review from a team as code owners September 1, 2023 21:22
Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

Other than the little comment I left, LGTM

Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Is it safe to say ES is ready when client.info() is returning result?
I tried client.cluster.health({ wait_for_status: 'green' }); and time frame is pretty the same, but it verifies ES cluster state is valid, e.g. "green" but not "yellow".

It looks like we already do health check for stateful https://github.com/elastic/kibana/blob/main/packages/kbn-es/src/cluster.js#L523-L560

@dmlemeshko
Copy link
Member

@Ikuni17 I opened #165605 to unify wait logic across stateful & serverless as it seems like a valid check for both cases. I will try to convert cluster.js in TS separately so that it can also re-use waitForClusterReady.

Does it make sense?

@Ikuni17
Copy link
Contributor Author

Ikuni17 commented Sep 5, 2023

@dmlemeshko

Is it safe to say ES is ready when client.info() is returning result?
I tried client.cluster.health({ wait_for_status: 'green' }); and time frame is pretty the same, but it verifies ES cluster state is valid, e.g. "green" but not "yellow".

There are times where master node election can take longer, and keeps the cluster in yellow health. So, I think your solution is more appropriate for a true ready state. client.info returns basic information once the cluster is started, but does not necessarily mean healthy.

@Ikuni17 I opened #165605 to unify wait logic across stateful & serverless as it seems like a valid check for both cases. I will try to convert cluster.js in TS separately so that it can also re-use waitForClusterReady.

Should we combine the two PRs? We will need a bit of the client setup from this one. We decided to merge this now to help with pipeline stability and it should be easier to incorporate the changes into your PR. LMK if you need any help!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Ikuni17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:serverless-test-all release_note:skip Skip the PR/issue when compiling release notes v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants