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

[Upgrade Assistant] Better handling of closed indices #58890

Merged
merged 33 commits into from
Mar 9, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Feb 28, 2020

Summary

Fix #41572

We use the _cluster/state endpoint to determine whether an index is open or closed. We inform the user in the UI but we still allow them to request a reindex with the understanding that the UA will open, reindex, alias and close the index again.

How to test

  • Replace line 57 with the snippet below in x-pack/plugins/upgrade_assistant/server/lib/es_migration_apis.ts.
  • Make sure that indices test1, test2 and test3 actually exist in the cluster. (e.g. in Console PUT test1 etc).
  • Close test1. POST /test1/_close (more information on this API here)
  • Navigate to the Upgrade Assistant "Indices" tab and expand the accordions until you find test1. There should be an alert icon next to the button with some more information displayed on hover.
  • Follow the steps to start the reindex process. Once reindexing has completed, go back to Console and use GET _cluster/state/metadata/test1 and check that the state key is still close.
  • Attempt reindexing test2, this should work as before and leave the index in open state (use same step as above but for index test2 to verify).

Release note

TODO

Additional information

Snippet:

      indices: indices.concat([
      {
        level: 'none',
        message: 'none level test',
        url: 'test/url',
        details: 'test details',
        index: 'test1',
        reindex: true,
        needsDefaultFields: false,
        blockerForReindexing: 'index-closed', // Faking that test1 is closed!
      },
      {
        level: 'info',
        message: 'info level test',
        url: 'test/url',
        details: 'test details',
        index: 'test2',
        reindex: true,
        needsDefaultFields: false,
      },
      {
        level: 'warning',
        message: 'warning level test2',
        url: 'test/url',
        details: 'test details',
        index: 'test3',
        reindex: true,
        needsDefaultFields: false,
      },
      {
        level: 'critical',
        message: 'metricbeat-1 successful fix',
        url: 'test/url',
        details: 'test details',
        index: 'metricbeat-1',
        reindex: false,
        needsDefaultFields: true,
      },
      {
        level: 'critical',
        message: 'metricbeat-2 failed fix',
        url: 'test/url',
        details: 'test details',
        index: 'metricbeat-2',
        reindex: false,
        needsDefaultFields: true,
      },
    ]),

Screenshots

Reindex Button state when index is closed

Screenshot 2020-03-06 at 13 44 40

Flyout state (another callout, see emphasis)

Screenshot 2020-03-06 at 13 44 46

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@jloleysens jloleysens added release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Upgrade Assistant v7.7.0 labels Feb 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

Updated the handling of reindexOptions to make it more
backwards compatible (treat it as if it could be undefined).

Also update the logic for checking for open or closed indices.
No optional chaining! It should break if the response does not
exactly match.
Rather check for the specific value we want, as this is what is
also gauranteed by the tests. In this way, the information we
send back to the client is also more accurate regarding the index
status. If, in future, more index states are introduced this will
need to be revisited if it affects the ability for an index to be
re-indexed.
@jloleysens jloleysens marked this pull request as ready for review March 2, 2020 15:14
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

"openAndClose" should just happen automatically. The user should
not have to pass the flag in, that would be a weird API. We just
need to warn the user about that reindexing a closed index will
take more resources
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for makings those changes! I left one copy suggestion, but nothing blocking.

@jloleysens jloleysens changed the title [Upgrade Assistant] Fix for better handling of closed indices [Upgrade Assistant] Better handling of closed indices Mar 6, 2020
Reindexing test was generating index name, could just get it from
server response. Also removed openAndClose from all integration
tests
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jloleysens jloleysens merged commit 492a97e into elastic:master Mar 9, 2020
@jloleysens jloleysens deleted the ua/fix/reindex-closed-index branch March 9, 2020 13:11
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 9, 2020
* Exclude disallowed, private setting at index creation

* Remove intl from tabs component

* Added logic for checking the current index status

* Added ES contract integration test

Using _cluster/state is considered internal. This adds an integration
test for checking the contract in CI.

* Add the client side notification for closed indices

* First version of end-to-end functionality working

* Clean up unused, incorrect type information

* Fix type issues and added a comment about the new reindex options

* Fixed server side tests, added comments and logic updates

Updated the handling of reindexOptions to make it more
backwards compatible (treat it as if it could be undefined).

Also update the logic for checking for open or closed indices.
No optional chaining! It should break if the response does not
exactly match.

* Clean up unused code

* Improved idempotency of test and check explicitly for "close".

Rather check for the specific value we want, as this is what is
also gauranteed by the tests. In this way, the information we
send back to the client is also more accurate regarding the index
status. If, in future, more index states are introduced this will
need to be revisited if it affects the ability for an index to be
re-indexed.

* Update client-side tests

* Fix types

* Handle a case where the index name provided may be an alias

* Fix types

* merge-conflict: finish merge conflict resolution

* Update x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/closed_warning_icon.tsx

Co-Authored-By: Alison Goryachev <alisonmllr20@gmail.com>

* merge-conflict: Remove duplicate import

VSCode does not auto-save as expected :sigh:

* ui: Revisit the UI

Moved the warning icon to inside of the button and tooltip to
on the button.

Added a callout to the reindex flyout for when an index is closed.

* logic: slight update to when the index closed callout is shown

We only show the index closed callout in the flyout when the
reindex operation is not considered "completed"

* tests: fix jest tests

* refactor: remove "openAndClose" from reindex endpoints

"openAndClose" should just happen automatically. The user should
not have to pass the flag in, that would be a weird API. We just
need to warn the user about that reindexing a closed index will
take more resources

* test: update upgrade assistant integration test

* fix: types

* copy: use sentence case

* refactor: use the in scope declaration of reindex op

* test: Clean up tests

Reindexing test was generating index name, could just get it from
server response. Also removed openAndClose from all integration
tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>
# Conflicts:
#	x-pack/plugins/upgrade_assistant/common/types.ts
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/cell.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/index_table.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/list.test.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/list.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/button.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/flyout/checklist_step.test.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/flyout/checklist_step.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/flyout/container.tsx
#	x-pack/plugins/upgrade_assistant/server/lib/__snapshots__/es_migration_apis.test.ts.snap
#	x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 10, 2020
* master: (22 commits)
  Generate docs from data plugin (elastic#56955)
  Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514)
  Harden creation of child processes (elastic#55697)
  [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475)
  [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553)
  chore: 🤖 hide Drilldowns in master (elastic#59698)
  [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175)
  Use HTTP request schemas to create types, use those types in the client (elastic#59340)
  [Maps] Support categorical styling for numbers and dates (elastic#57908)
  [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665)
  Fix slm_ui setting by changing camel case back to snake case. (elastic#59663)
  removes beta tag (elastic#59618)
  [DOCS] Removed spatial references (elastic#59595)
  fix outdated docs (elastic#58729)
  [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639)
  [ML] Refactoring anomaly detector job types (elastic#59556)
  [Upgrade Assistant] Better handling of closed indices (elastic#58890)
  additional visualizations plugin cleanup before moving to NP (elastic#59318)
  In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285)
  [Visualize] Remove global state in visualize (elastic#58352)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 10, 2020
…s/kibana into alerting/fix-flaky-instance-test

* 'alerting/fix-flaky-instance-test' of github.com:gmmorris/kibana: (176 commits)
  Generate docs from data plugin (elastic#56955)
  Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514)
  Harden creation of child processes (elastic#55697)
  [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475)
  [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553)
  chore: 🤖 hide Drilldowns in master (elastic#59698)
  [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175)
  Use HTTP request schemas to create types, use those types in the client (elastic#59340)
  [Maps] Support categorical styling for numbers and dates (elastic#57908)
  [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665)
  Fix slm_ui setting by changing camel case back to snake case. (elastic#59663)
  removes beta tag (elastic#59618)
  [DOCS] Removed spatial references (elastic#59595)
  fix outdated docs (elastic#58729)
  [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639)
  [ML] Refactoring anomaly detector job types (elastic#59556)
  [Upgrade Assistant] Better handling of closed indices (elastic#58890)
  additional visualizations plugin cleanup before moving to NP (elastic#59318)
  In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285)
  [Visualize] Remove global state in visualize (elastic#58352)
  ...
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 10, 2020
jloleysens added a commit that referenced this pull request Mar 10, 2020
…59650)

* [Upgrade Assistant] Better handling of closed indices (#58890)

* Exclude disallowed, private setting at index creation

* Remove intl from tabs component

* Added logic for checking the current index status

* Added ES contract integration test

Using _cluster/state is considered internal. This adds an integration
test for checking the contract in CI.

* Add the client side notification for closed indices

* First version of end-to-end functionality working

* Clean up unused, incorrect type information

* Fix type issues and added a comment about the new reindex options

* Fixed server side tests, added comments and logic updates

Updated the handling of reindexOptions to make it more
backwards compatible (treat it as if it could be undefined).

Also update the logic for checking for open or closed indices.
No optional chaining! It should break if the response does not
exactly match.

* Clean up unused code

* Improved idempotency of test and check explicitly for "close".

Rather check for the specific value we want, as this is what is
also gauranteed by the tests. In this way, the information we
send back to the client is also more accurate regarding the index
status. If, in future, more index states are introduced this will
need to be revisited if it affects the ability for an index to be
re-indexed.

* Update client-side tests

* Fix types

* Handle a case where the index name provided may be an alias

* Fix types

* merge-conflict: finish merge conflict resolution

* Update x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/closed_warning_icon.tsx

Co-Authored-By: Alison Goryachev <alisonmllr20@gmail.com>

* merge-conflict: Remove duplicate import

VSCode does not auto-save as expected :sigh:

* ui: Revisit the UI

Moved the warning icon to inside of the button and tooltip to
on the button.

Added a callout to the reindex flyout for when an index is closed.

* logic: slight update to when the index closed callout is shown

We only show the index closed callout in the flyout when the
reindex operation is not considered "completed"

* tests: fix jest tests

* refactor: remove "openAndClose" from reindex endpoints

"openAndClose" should just happen automatically. The user should
not have to pass the flag in, that would be a weird API. We just
need to warn the user about that reindexing a closed index will
take more resources

* test: update upgrade assistant integration test

* fix: types

* copy: use sentence case

* refactor: use the in scope declaration of reindex op

* test: Clean up tests

Reindexing test was generating index name, could just get it from
server response. Also removed openAndClose from all integration
tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>
# Conflicts:
#	x-pack/plugins/upgrade_assistant/common/types.ts
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/cell.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/index_table.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/list.test.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/list.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/button.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/flyout/checklist_step.test.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/flyout/checklist_step.tsx
#	x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/flyout/container.tsx
#	x-pack/plugins/upgrade_assistant/server/lib/__snapshots__/es_migration_apis.test.ts.snap
#	x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts

* Merge: Resolve remaining merge conflicts

Also updated existing Jest snapshots.

* test: Update Jest snapshots and reference the 6.0 index

The integration test on master is reference 7.0 because it aims
toward helping users migrate from 7.0 -> 8.0. The 7.x branch
is running 6.0 data as it helps migrate from older indices.

* Remove test data 🤦🏼‍♂️

* Clean up Jest snapshot

After removing the test data, this snapshot also needed to be
updated

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 10, 2020
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Mar 12, 2020
* Exclude disallowed, private setting at index creation

* Remove intl from tabs component

* Added logic for checking the current index status

* Added ES contract integration test

Using _cluster/state is considered internal. This adds an integration
test for checking the contract in CI.

* Add the client side notification for closed indices

* First version of end-to-end functionality working

* Clean up unused, incorrect type information

* Fix type issues and added a comment about the new reindex options

* Fixed server side tests, added comments and logic updates

Updated the handling of reindexOptions to make it more
backwards compatible (treat it as if it could be undefined).

Also update the logic for checking for open or closed indices.
No optional chaining! It should break if the response does not
exactly match.

* Clean up unused code

* Improved idempotency of test and check explicitly for "close".

Rather check for the specific value we want, as this is what is
also gauranteed by the tests. In this way, the information we
send back to the client is also more accurate regarding the index
status. If, in future, more index states are introduced this will
need to be revisited if it affects the ability for an index to be
re-indexed.

* Update client-side tests

* Fix types

* Handle a case where the index name provided may be an alias

* Fix types

* merge-conflict: finish merge conflict resolution

* Update x-pack/plugins/upgrade_assistant/public/application/components/tabs/checkup/deprecations/reindex/closed_warning_icon.tsx

Co-Authored-By: Alison Goryachev <alisonmllr20@gmail.com>

* merge-conflict: Remove duplicate import

VSCode does not auto-save as expected :sigh:

* ui: Revisit the UI

Moved the warning icon to inside of the button and tooltip to
on the button.

Added a callout to the reindex flyout for when an index is closed.

* logic: slight update to when the index closed callout is shown

We only show the index closed callout in the flyout when the
reindex operation is not considered "completed"

* tests: fix jest tests

* refactor: remove "openAndClose" from reindex endpoints

"openAndClose" should just happen automatically. The user should
not have to pass the flag in, that would be a weird API. We just
need to warn the user about that reindexing a closed index will
take more resources

* test: update upgrade assistant integration test

* fix: types

* copy: use sentence case

* refactor: use the in scope declaration of reindex op

* test: Clean up tests

Reindexing test was generating index name, could just get it from
server response. Also removed openAndClose from all integration
tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>
@bhavyarm
Copy link
Contributor

@jloleysens do I need to start from 5.6.16 to test this PR? Thanks!

@jloleysens
Copy link
Contributor Author

Hi @bhavyarm ! The PR description has a hacky version of to test the UI, but for an actual test we would need to load up a v7.7.0 stack (ES + Kibana) and have the following conditions met:

  1. There is an index using known deprecated functionality that requires reindexing to fix
  2. That same index is currently closed

The upgrade assistant should now be able to both detect the need for reindexing and that the index is closed. Reindexing should still be possible, though. We just want to let the user know that we need to open the index and re-close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Upgrade Assistant release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade assistant fails to reindex closed index
5 participants