Fetch health info action by masseyke · Pull Request #89820 · elastic/elasticsearch

masseyke · 2022-09-06T13:17:32Z

This PR adds FetchHealthInfoCacheAction, which fetches the data sent to the health node by the UpdateHealthInfoCacheAction (#89275).

Relates to #84811

elasticsearchmachine · 2022-09-06T13:17:57Z

Pinging @elastic/es-data-management (Team:Data Management)

elasticsearchmachine · 2022-09-06T13:17:57Z

Hi @masseyke, I've created a changelog YAML for you.

…asseyke/elasticsearch into feature/health-api-fetch-health-info-2

andreidan

LGTM thanks Keith

Can you please link this PR (and future PRs) to the disk indicator meta issue?

I'd like to have an IT for the fetching bit too
(similar to UpdateHealthInfoCacheIT).
What do you think? // @gmarouli

server/src/main/java/org/elasticsearch/health/node/HealthInfo.java

gmarouli

LGTM, thanks for working on this @masseyke . Let me know what you think about the last comments, we are almost ready to ship this too!

docs/changelog/89820.yaml

server/src/test/java/org/elasticsearch/health/node/FetchHealthInfoCacheActionTests.java

masseyke · 2022-09-07T13:22:18Z

I'd like to have an IT for the fetching bit too (similar to UpdateHealthInfoCacheIT). What do you think? // @gmarouli

What would the integration test test in this case? I thought about adding one but decided not to because the only thing beyond what FetchHealthInfoCacheActionTests is testing that be testing is TransportHealthNodeAction functionality. And that ought to have an integration test of its own (I'm not sure if it does).

andreidan · 2022-09-07T13:39:45Z

What would the integration test test in this case?

If HealthInfoCache had a bug that would, say, return an empty Map in certain conditions (irrespective of live values being sent to it) we wouldn't catch it with a unit test that stubs a return value.
I think integration tests are useful to make sure the end2end flow is working

masseyke · 2022-09-07T14:46:46Z

If HealthInfoCache had a bug that would, say, return an empty Map in certain conditions (irrespective of live values being sent to it) we wouldn't catch it with a unit test that stubs a return value. I think integration tests are useful to make sure the end2end flow is working

Wouldn't that be really easy to test in a unit test? And really hard to test in an integration test (because I'm guessing that HealthInfoCache doesn't have such a bug and if it did we'd just fix it)? I'm just trying to get at what would be useful and meaningful to test in an integration test that might protect us from future bugs in this code.

…asseyke/elasticsearch into feature/health-api-fetch-health-info-2

andreidan · 2022-09-07T16:59:58Z

@masseyke I think the workflow for health reporting is rather complex (many moving parts in terms of reporting, caching, and fetching) and would benefit for as much testing as possible.

masseyke · 2022-09-07T19:00:52Z

@elasticsearchmachine run elasticsearch-ci/part-1

masseyke · 2022-09-07T19:01:21Z

@elasticmachine run elasticsearch-ci/part-1

masseyke · 2022-09-07T22:31:34Z

@elasticmachine run elasticsearch-ci/bwc

gmarouli · 2022-09-08T12:08:01Z

I agree with @andreidan , maybe an integration test doesn't have to simulate a weird edge case but the edge case might present itself. What we are trying to achieve with such an integration test is to test a situation as close to reality as possible and effectively test our assumptions. @masseyke what do you think? Does this make sense?

masseyke · 2022-09-08T12:32:47Z

I strongly disagree with both of you, but I added several integration tests yesterday.

gmarouli · 2022-09-08T12:53:37Z

That doesn't feel very good to me, you are leading this PR I wouldn't want you to do something you strongly disagree :). I would like to understand better your objection. Do you think that there is no value to have an integration test for this as a sanity check, or that you were planning to add it in the next PR that includes the disk indicator where it would test a more end to end scenario?

masseyke · 2022-09-08T13:17:48Z

The reason is that there is nothing to really integration test here that's not already covered elsewhere. It doesn't tell us anything new or mitigate any risk. There's no complex interaction in the code in this PR -- unlike the code that runs automatically on all nodes to generate and upload the health data, this just runs on a single node when someone calls it (and doesn't even transform the data in any way). Assuming TransportHealthNodeAction has been thoroughly tested and the unit test for FetchHealthInfoCacheAction is decent, I haven't figured out how to mitigate any additional risk with an integration test (other than maybe that HealthInfoCache can't be injected).
Each test we write takes time and money to run (both in our CI system and for the hundreds of people building the code on their own), and integration tests are especially expensive. If they mitigate any risk at all, they're probably worth the cost. I just haven't been able to understand what risk we're targeting here. In order to minimize the cost, I've piggybacked the FetchHealthInfoCacheAction tests onto UpdateHealthInfoCacheIT (since they're complimentary actions and we'd probably want to test the same situations for both without copy/pasting lots of code).

gmarouli · 2022-09-08T13:18:50Z

server/src/internalClusterTest/java/org/elasticsearch/health/UpdateHealthInfoCacheIT.java

@@ -121,12 +135,14 @@ public void testMasterFailure() throws Exception {
            assertThat(newHealthNode, notNullValue());
            assertBusy(() -> {
                Map<String, DiskHealthInfo> healthInfoCache = internalCluster.getInstance(HealthInfoCache.class, newHealthNode.getName())


I think we should replace this check all together, and compare the info we get from the action you introduced. What do you think?

Makes sense (I just pushed that).

masseyke · 2022-09-08T14:09:56Z

@elasticmachine update branch

gmarouli · 2022-09-08T14:48:12Z

Thank you for sharing your view on the matter. I like your proposal to piggy back on the UpdateHealthInfoCacheIT and I would suggest to replace the class retrieval by the FetchHealthInfoCacheAction. Code wise it's much nicer to use this action than try to find the correct node to get the instance from a class. Is this approach something that you feel comfortable with? We reduce the costs and we use the action to retrieve the data for the assertions.

masseyke · 2022-09-08T14:48:27Z

@elasticmachine run elasticsearch-ci/part-1

gmarouli

I had a few minor comments, but it looks good thanks for carrying it through. I hope you are happy with the result too.

server/src/internalClusterTest/java/org/elasticsearch/health/UpdateHealthInfoCacheIT.java

gmarouli · 2022-09-08T15:38:03Z

server/src/internalClusterTest/java/org/elasticsearch/health/UpdateHealthInfoCacheIT.java

+     * @param notExpectedNodeId A nullable nodeId that we do _not_ expect to see in the results
+     * @throws Exception
+     */
+    private void assertResultsCanBeFetched(


To simplify this what if we pick a node randomly? Isn't this good enough?

That's what it's doing right? It picks a random non-health node, and then also the health node (to cover the 2 main cases).

Yes, it does both, I was wondering since as you said the routing is tested in the TransportHealthNodeAction, that maybe it's sufficient to just pick a node and fetch the data once. Without differentiating between the health and other nodes. WDYT?

…asseyke/elasticsearch into feature/health-api-fetch-health-info-2

masseyke · 2022-09-08T17:27:04Z

@elasticmachine update branch

* main: (34 commits) Make sure ivy repo directory exists before downloading artifacts Use 'file://' scheme for local repository URL Use DRA artifacts for release build CI jobs Log unsuccessful attempts to get credentials from web identity tokens (elastic#88241) Script: Write Field API path manipulation (elastic#89889) Fetch health info action (elastic#89820) Fix memory leak in TransportDeleteExpiredDataAction (elastic#89935) [ML] Performance improvements for categorization jobs (elastic#89824) [DOCS] Revert changes for ES_JAVA_OPTS (elastic#89931) Fix deadlock bug exposed by a test (elastic#89934) [Downsampling] Remove `FieldValueFetcher` validator (elastic#89497) Fix segment stats in tsdb (elastic#89754) Synthetic _source: support dense_vector (elastic#89840) REST tests fetching fields with synthetic _source (elastic#89888) Do not deserialize back BytesTransportRequest to clone a request in MockTransportService (elastic#89926) Add SDK request logging to debug failures of S3BlobStoreRepositoryTests#testRequestStats (elastic#89912) Fix SnapshotStatusApisIT.testGetSnapshotsWithSnapshotInProgress (elastic#89925) Document synthetic source for text and keyword (elastic#89893) Fix CloneSnapshotIT.testRemoveFailedCloneFromCSWithQueuedSnapshotInProgress (elastic#89914) Add missing index.mapping.total_fields.limit setting to the target index (elastic#89875) ...

* main: (176 commits) Fix RandomSamplerAggregatorTests testAggregationSamplingNestedAggsScaled test failure (elastic#89958) [Downsampling] Replace document map with SMILE encoded doc (elastic#89495) Remove full cluster state from error logging in MasterService (elastic#89960) [ML] Truncate categorization fields (elastic#89827) [TSDB] Removed `summary` and `histogram` metric types (elastic#89937) Update testNodeSelectorRouting so that it does not depend on iteration order (elastic#89879) Make sure listener is resolved when file queue is cleared (elastic#89929) [Stable plugin api] Extensible annotation (elastic#89903) Fix double sending of response in TransportOpenIdConnectPrepareAuthenticationAction (elastic#89930) Make sure ivy repo directory exists before downloading artifacts Use 'file://' scheme for local repository URL Use DRA artifacts for release build CI jobs Log unsuccessful attempts to get credentials from web identity tokens (elastic#88241) Script: Write Field API path manipulation (elastic#89889) Fetch health info action (elastic#89820) Fix memory leak in TransportDeleteExpiredDataAction (elastic#89935) [ML] Performance improvements for categorization jobs (elastic#89824) [DOCS] Revert changes for ES_JAVA_OPTS (elastic#89931) Fix deadlock bug exposed by a test (elastic#89934) [Downsampling] Remove `FieldValueFetcher` validator (elastic#89497) ...

This PR updates HealthService to fetch HealthInfo data using FetchHealthInfoCacheAction (#89820), and to pass it to the HealthIndicatorServices' calculate method. This will allow HealthIndicatorServices to use any data from the health node's HealthInfoCache.

Fetch health info action

e9da364

masseyke added >enhancement :Distributed/Health Issues for the health report API v8.5.0 labels Sep 6, 2022

masseyke requested review from andreidan and gmarouli September 6, 2022 13:17

elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Sep 6, 2022

Update docs/changelog/89820.yaml

88b67a5

masseyke mentioned this pull request Sep 6, 2022

Fetch health info action gmarouli/elasticsearch#3

Closed

masseyke added 2 commits September 6, 2022 08:31

fixing compilation error

2fd6c8e

Merge branch 'feature/health-api-fetch-health-info-2' of github.com:m…

7b85ce8

…asseyke/elasticsearch into feature/health-api-fetch-health-info-2

andreidan approved these changes Sep 7, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/health/node/HealthInfo.java Outdated Show resolved Hide resolved

gmarouli requested changes Sep 7, 2022

View reviewed changes

docs/changelog/89820.yaml Outdated Show resolved Hide resolved

server/src/test/java/org/elasticsearch/health/node/FetchHealthInfoCacheActionTests.java Show resolved Hide resolved

masseyke added >non-issue and removed >enhancement labels Sep 7, 2022

Delete docs/changelog/89820.yaml

a208586

code review feedback

8db8ee5

masseyke added 2 commits September 7, 2022 09:50

Merge branch 'feature/health-api-fetch-health-info-2' of github.com:m…

74f624c

…asseyke/elasticsearch into feature/health-api-fetch-health-info-2

Merge branch 'main' into feature/health-api-fetch-health-info-2

5e06a4c

gmarouli mentioned this pull request Sep 7, 2022

Disk Usage health indicator #84811

Closed

9 tasks

adding to the integration test

8d7280e

Merge branch 'main' into feature/health-api-fetch-health-info-2

83db4db

masseyke requested a review from gmarouli September 8, 2022 12:33

gmarouli reviewed Sep 8, 2022

View reviewed changes

simplifying

42f7d08

masseyke requested a review from gmarouli September 8, 2022 13:47

Merge branch 'main' into feature/health-api-fetch-health-info-2

772689f

gmarouli approved these changes Sep 8, 2022

View reviewed changes

masseyke added 2 commits September 8, 2022 12:19

removing curly braces

06f56d6

Merge branch 'feature/health-api-fetch-health-info-2' of github.com:m…

045a3aa

…asseyke/elasticsearch into feature/health-api-fetch-health-info-2

masseyke added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 8, 2022

Merge branch 'main' into feature/health-api-fetch-health-info-2

34a54ff

elasticsearchmachine merged commit 691c08a into elastic:main Sep 8, 2022

masseyke deleted the feature/health-api-fetch-health-info-2 branch September 8, 2022 18:06

masseyke mentioned this pull request Sep 8, 2022

Updating HealthService to use FetchHealthInfoCacheAction #89947

Merged

Conversation

masseyke commented Sep 6, 2022 • edited by andreidan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 6, 2022

Uh oh!

elasticsearchmachine commented Sep 6, 2022

Uh oh!

andreidan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmarouli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

masseyke commented Sep 7, 2022

Uh oh!

andreidan commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masseyke commented Sep 7, 2022

Uh oh!

andreidan commented Sep 7, 2022

Uh oh!

masseyke commented Sep 7, 2022

Uh oh!

masseyke commented Sep 7, 2022

Uh oh!

masseyke commented Sep 7, 2022

Uh oh!

gmarouli commented Sep 8, 2022

Uh oh!

masseyke commented Sep 8, 2022

Uh oh!

gmarouli commented Sep 8, 2022

Uh oh!

masseyke commented Sep 8, 2022

Uh oh!

gmarouli Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

masseyke Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

masseyke commented Sep 8, 2022

Uh oh!

gmarouli commented Sep 8, 2022

Uh oh!

masseyke commented Sep 8, 2022

Uh oh!

gmarouli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmarouli Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

masseyke Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

gmarouli Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

masseyke commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

masseyke commented Sep 6, 2022 •

edited by andreidan

Loading

andreidan left a comment •

edited

Loading

andreidan commented Sep 7, 2022 •

edited

Loading