Migrations: Don't auto-create temp index #158182

rudolf · 2023-05-22T13:17:08Z

Summary

Release notes

Fixes a race condition that could cause intermittent upgrade migration failures when Kibana connects to a single node Elasticsearch cluster.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-05-24T19:15:59Z

Pinging @elastic/kibana-core (Team:Core)

afharo

LGTM!

pgayvallet · 2023-05-30T06:33:54Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts

@@ -139,6 +139,7 @@ export const nextActionMap = (
      Actions.createIndex({
        client,
        indexName: state.tempIndex,
+        aliases: [state.tempIndexAlias],


Is the alias automatically deleted when we associated index is removed (asking before the PR apparently doesn't add logic to delete the alias when the temp index is delete)?

Yes, aliases are "attached" to an index.

pgayvallet · 2023-05-30T06:35:21Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/state.ts

+  /**
+   * An alias to the tempIndex used to prevent ES from auto-creating the temp
+   * index if one node deletes it while another writes to it
+   * e.g. `.kibana_7.11.0_reindex_temp_alias`.
+   */
+  readonly tempIndexAlias: string;


Still amazed this is the 'only' way to prevent automatic index creation...

pgayvallet · 2023-05-30T06:37:06Z

...-saved-objects-migration-server-internal/src/actions/bulk_overwrite_transformed_documents.ts

+  /**
+   * If true, we prevent Elasticsearch from auto-creating the index if it
+   * doesn't exist. We use the ES paramater require_alias: true so `index`
+   * must be an alias, otherwise the bulk index will fail.
+   */
+  useAliasToPreventAutoCreate?: boolean;


NIT: personal opinion, but given it's a 1-1 mapping to an ES option and it's only used internally, I would have just called this requireAlias.

yeah I was going back and forth... this requireAlias "hack" / workaround / trick is so weird that it felt like I need to explain it because we don't actually care about using an alias...

Yeah, I would have gone with the actual option name + your tsdoc to explain why it should be used, but I'm fine with both

pgayvallet · 2023-05-30T06:39:32Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.ts

+        ...(typeIndexMap[doc._source.type] && {
+          _index: typeIndexMap[doc._source.type] + '_alias',
+        }),


createBulkIndexOperationTuple is a fairly low-level utility. Are we sure it's fine to hard-code this here? Isn't it used in scenarios where we're not targeting the temp index (alias)?

createBulkIndexOperationTuple is used by createBatches which is used both for v2 and zdt.

And even for v2, I think this change is wrong for reindex-into-the-same-index scenario?

(but OTOH it didn't break any integration test, so maybe I just don't get it here?)

Yes I think you're right, I would expect it to fail here https://github.com/elastic/kibana/blob/main/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts#L1291

Yeah, we do have integration tess for ZDT too, I'm not sure to get why those are not failing with the PR's changes to be honest

pgayvallet

Main question in #158182 (comment)

pgayvallet

LGTM

rudolf · 2023-06-04T21:32:14Z

@elasticmachine merge upstream

kibana-ci · 2023-06-04T22:34:08Z

💚 Build Succeeded

Buildkite Build
Commit: 1108baa

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	414	418	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	498	502	+4
total			+6

History

💚 Build #131366 succeeded e40d63f
💚 Build #130018 succeeded 2cf38ce
💔 Build #129946 failed c027911
💔 Build #129826 failed 0d59121
💔 Build #129286 failed 8066585

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

cc @rudolf

kibanamachine · 2023-06-04T22:40:19Z

💚 All backports created successfully

Status	Branch	Result
✅	8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

## Summary Try to fix elastic#156117 (comment) ## Release notes Fixes a race condition that could cause intermittent upgrade migration failures when Kibana connects to a single node Elasticsearch cluster. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 8e7e263)

# Backport This will backport the following commits from `main` to `8.8`: - [Migrations: Don't auto-create temp index (#158182)](#158182)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Rudolf Meijering <skaapgif@gmail.com>

## Summary Try to fix elastic#156117 (comment) ## Release notes Fixes a race condition that could cause intermittent upgrade migration failures when Kibana connects to a single node Elasticsearch cluster. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

rudolf added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Migrations labels May 22, 2023

rudolf changed the title ~~migrations: Don't auto-create temp index~~ Migrations: Don't auto-create temp index May 22, 2023

rudolf self-assigned this May 23, 2023

rudolf force-pushed the migrations-unavailable-shards branch from 8066585 to 0d59121 Compare May 24, 2023 10:37

rudolf mentioned this pull request May 24, 2023

Migrations fail on single node clusters due to unavailable shards exception #157968

Closed

rudolf added 4 commits May 24, 2023 21:15

WIP don't auto-create temp index

ef4aa10

Fix type bugs

1314315

Fix bulk indexing batches

2c57bc0

Fix tests

2cf38ce

rudolf force-pushed the migrations-unavailable-shards branch from 643f204 to 2cf38ce Compare May 24, 2023 19:15

rudolf added the backport:prev-minor Backport to the previous minor version (i.e. one version back from main) label May 24, 2023

rudolf marked this pull request as ready for review May 24, 2023 19:15

rudolf requested a review from a team as a code owner May 24, 2023 19:15

afharo approved these changes May 25, 2023

View reviewed changes

rudolf added the release_note:fix label May 25, 2023

pgayvallet reviewed May 30, 2023

View reviewed changes

Less invasive way of pointing temp index batches at alias

e40d63f

pgayvallet approved these changes Jun 2, 2023

View reviewed changes

Merge branch 'main' into migrations-unavailable-shards

1108baa

rudolf enabled auto-merge (squash) June 4, 2023 21:32

rudolf merged commit 8e7e263 into main Jun 4, 2023
16 checks passed

rudolf deleted the migrations-unavailable-shards branch June 4, 2023 22:34

kibanamachine added the v8.9.0 label Jun 4, 2023

kibanamachine mentioned this pull request Jun 4, 2023

[8.8] Migrations: Don't auto-create temp index (#158182) #158996

Merged

kibanamachine added the v8.8.1 label Jun 4, 2023

rudolf mentioned this pull request Jun 9, 2023

Failing test: Jest Integration Tests.src/core/server/integration_tests/saved_objects/migrations/group2 - migration v2 migrates saved objects normally when multiple Kibana instances are started with an average interval #156117

Closed

dokmic mentioned this pull request Jul 27, 2023

[Migrations] Unskip migration integration tests after fixing #162648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrations: Don't auto-create temp index #158182

Migrations: Don't auto-create temp index #158182

rudolf commented May 22, 2023 •

edited by kibanamachine

elasticmachine commented May 24, 2023

afharo left a comment

pgayvallet May 30, 2023

rudolf May 30, 2023

pgayvallet May 30, 2023

pgayvallet May 30, 2023

rudolf May 30, 2023

pgayvallet May 31, 2023

pgayvallet May 30, 2023 •

edited

rudolf May 30, 2023

pgayvallet May 31, 2023

pgayvallet left a comment

pgayvallet left a comment

rudolf commented Jun 4, 2023

kibana-ci commented Jun 4, 2023

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Jun 4, 2023

Migrations: Don't auto-create temp index #158182

Migrations: Don't auto-create temp index #158182

Conversation

rudolf commented May 22, 2023 • edited by kibanamachine

Summary

Release notes

Checklist

Risk Matrix

For maintainers

elasticmachine commented May 24, 2023

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet May 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

rudolf commented Jun 4, 2023

kibana-ci commented Jun 4, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Jun 4, 2023

💚 All backports created successfully

Questions ?

rudolf commented May 22, 2023 •

edited by kibanamachine

pgayvallet May 30, 2023 •

edited