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

[Index Management] Filter system indices from cat API request #104155

Merged
merged 4 commits into from
Jul 2, 2021

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Jul 1, 2021

Fixes #104079
Fixes #103924
Fixes #64473

Related ES issue: elastic/elasticsearch#74687

Summary:

In order to fetch a user's indices to display in Index Management, we first make a request to fetch indices via the index API (GET /*?expand_wildcards=hidden,all), then make a request to the cat indices API. We use the response of the cat indices API to provide supplemental information about each index.

ES recently made changes that includes a new system index - .geoip_databases- in the cat indices API. However, this same index is not included in the index API. Per elastic/elasticsearch#74687 (comment), this is working as expected; cat APIs should return system indices for debugging purposes. However, the change caused the "Indices" tab in Index Management to error.

As a fix, I have filtered the cat indices response so that it only includes information on indices we fetched from the index API. In the future, we should migrate away from using the cat API as suggested in #57286.

How to test

  1. Remove/comment out the ingest_geoip.downloader.enabled=false setting from cluster.js
  2. Run yarn kbn bootstrap
  3. Restart ES and Kibana (ingest_geoip.downloader.enabled=true should be the default behavior)
  4. The "Indices" tab in Index Management should load successfully 😄

@alisonelizabeth alisonelizabeth added Feature:Index Management Index and index templates UI v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v7.15.0 labels Jul 1, 2021
@alisonelizabeth
Copy link
Contributor Author

Running 50x through flaky test runner: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/1695/

@@ -203,7 +210,8 @@ export default function ({ getService }) {
// We need to sort the keys before comparing then, because race conditions
// can cause enrichers to register in non-deterministic order.
const sortedExpectedKeys = expectedKeys.sort();
const sortedReceivedKeys = Object.keys(body[0]).sort();
const sortedReceivedKeys = Object.keys(indexCreated).sort();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping that by asserting against a specific index this test will be more stable. It's possible for an index to also have a date_stream property (which is not included in the expectedKeys), so this may have been occasionally failing because the index return from body[0] had a date stream associated with. Related comment: #64473 (comment).

@cjcenizal cjcenizal marked this pull request as ready for review July 1, 2021 20:04
@cjcenizal cjcenizal requested a review from a team as a code owner July 1, 2021 20:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Had one perf-related suggestion. Otherwise, code LGTM!

// System indices may show up in _cat APIs, as these APIs are primarily used for troubleshooting
// For now, we filter them out and only return index information for the indices we have
// In the future, we should migrate away from using cat APIs (https://github.com/elastic/kibana/issues/57286)
const filteredCatHits = catHits.filter((hit) => typeof indices[hit.index] !== 'undefined');
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a bit silly, but I'm wondering if performance could be a concern. We know users might have hundreds of thousands (maybe millions) of indices. In this case we are doubling memory consumption by creating the filteredCatHits variable, and also doubling the number of iterations by adding another filter loop. There's a good chance that testing could reveal this to be a non-issue. But we could also just make the code immune to these concerns by sticking with a single loop and doing our undefined check within the loop:

return catHits.reduce((decoratedIndices, hit) => {
  const index = indices[hit.index];
  
  if (typeof index !== 'undefined') {
    const aliases = Object.keys(index.aliases);

    decoratedIndices.push({
      health: hit.health,
      status: hit.status,
      name: hit.index,
      uuid: hit.uuid,
      primary: hit.pri,
      replica: hit.rep,
      documents: hit['docs.count'],
      size: hit['store.size'],
      isFrozen: hit.sth === 'true', // sth value coming back as a string from ES
      aliases: aliases.length ? aliases : 'none',
      hidden: index.settings.index.hidden === 'true',
      data_stream: index.data_stream,
    });
  }
  
  return decoratedIndices;
}, []);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 great point. Thanks for fixing!

@cjcenizal
Copy link
Contributor

cjcenizal commented Jul 1, 2021

When we merge this we should split out the bit about tests from #103924 into a new issue:

Evaluate current API integration tests and add better coverage to cover this scenario.

EDIT: Never mind, I realized we're unskipping the test that would have caught this problem.

@alisonelizabeth
Copy link
Contributor Author

Looks like all runs were successful in the flaky test runner:

Screen Shot 2021-07-01 at 8 24 23 PM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@cjcenizal cjcenizal merged commit a434bc0 into elastic:master Jul 2, 2021
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Jul 2, 2021
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Jul 2, 2021
cjcenizal added a commit that referenced this pull request Jul 2, 2021
… (#104240)

* refactor indices test and unskip

Co-authored-by: Alison Goryachev <[email protected]>
cjcenizal added a commit that referenced this pull request Jul 2, 2021
… (#104239)

* refactor indices test and unskip

Co-authored-by: Alison Goryachev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Index Management Index and index templates UI regression release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.14.0 v7.15.0 v8.0.0
Projects
None yet
4 participants