[Migrations] Only reindex indices if needed#147371
Merged
gsoldevila merged 52 commits intoelastic:mainfrom Dec 23, 2022
Merged
Conversation
jloleysens
added
Team:Core
…n" and importantly, move the mappings check further down the logic tree separate from the version check
* main: (21 commits) [Profiling] Remove link to 'Other' bucket (elastic#147523) [Synthetics UI] Add missing configuration options to the add/edit monitor forms (elastic#147265) [DOCS] Updates what's new pages (elastic#147483) [Fleet][Endpoint][RBAC V2] Update fleet router and config to allow API access via RBAC controls (elastic#145361) [Guided onboarding] Update guide IDs (elastic#147348) [Synthetics] Add synthetics settings alerting default (elastic#147339) [Security Solution][Endpoint] Fix Policy form being displayed as Read Only when displayed in Fleet pages (elastic#147212) [Cases] Save draft user comment (elastic#146327) [API Docs] Fix `--plugin` filter (elastic#147500) [Fleet] added a logic to use `destinationId` when tagging imported SOs (elastic#147439) Do not skip UPDATE_TARGET_MAPPINGS if upgrading to a newer stack version (elastic#147503) [Discover] Validate if Data View time field exists on Alert creation / editing (elastic#146324) [Discover] Fix Discover navigation from Lens embeddable (elastic#147000) Allow users to Update API Keys (elastic#146237) Update dependency xstate to ^4.35.0 (main) (elastic#147463) [Behavioral Analytics] Remove feature flag to hide functionality (elastic#147429) [Fleet] Add agent policy `inactivity_timeout`experimental setting (elastic#147432) [APM] Switching service groups from grid to flex layout (elastic#147448) [Fleet] Add missing endpoints to openApi specs (elastic#147452) [AO] Allow providing custom time range for Alert Summary Widget (elastic#147253) ...
Contributor
|
Pinging @elastic/kibana-core (Team:Core) |
jloleysens
commented
Dec 15, 2022
packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts
Outdated
Show resolved
Hide resolved
jloleysens
commented
Dec 15, 2022
packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts
Outdated
Show resolved
Hide resolved
jloleysens
commented
Dec 15, 2022
packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts
Show resolved
Hide resolved
pgayvallet
reviewed
Dec 22, 2022
Contributor
pgayvallet
left a comment
pgayvallet
left a comment
There was a problem hiding this comment.
I'll let @rudolf have the final review here, but the approach looks fine to me
rudolf
approved these changes
Dec 23, 2022
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
gsoldevila
added a commit
that referenced
this pull request
Jan 17, 2023
Fixes a bug introduced by #147371. The goal of the aforementioned PR was to avoid reindexing if the SO mappings didn't change. But in that scenario we are still going to perform some operations on the index, and thus we must await for the index to be ready (aka `WAIT_FOR_YELLOW_SOURCE`). This PR moves the `diffMappings` check (and the corresponding "shortcircuit") after the `WAIT_FOR_YELLOW_SOURCE` state + operation. 
gsoldevila
added a commit
that referenced
this pull request
Feb 27, 2023
In the context of migrations, #147371 avoids reindexing during an upgrade, provided that `diffMappings === false`. This _alternative path_ skips some key steps that are performed before reindexing: * `CHECK_UNKNOWN_DOCUMENTS` * `CALCULATE_EXCLUDE_FILTERS` These steps enrich a search query that is used during reindexing, effectively filtering out undesired documents. If the mappings [match](#147371) (or they are [compatible](#149326)) and we _no longer reindex_, this cleanup operation does not happen, leaving undesired documents in our system indices. The goal of this PR is to add an extra step in the state machine (`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system index if we're going the _skip reindexing_ path.  Fixes #150299 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
gsoldevila
added a commit
to gsoldevila/kibana
that referenced
this pull request
Feb 27, 2023
In the context of migrations, elastic#147371 avoids reindexing during an upgrade, provided that `diffMappings === false`. This _alternative path_ skips some key steps that are performed before reindexing: * `CHECK_UNKNOWN_DOCUMENTS` * `CALCULATE_EXCLUDE_FILTERS` These steps enrich a search query that is used during reindexing, effectively filtering out undesired documents. If the mappings [match](elastic#147371) (or they are [compatible](elastic#149326)) and we _no longer reindex_, this cleanup operation does not happen, leaving undesired documents in our system indices. The goal of this PR is to add an extra step in the state machine (`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system index if we're going the _skip reindexing_ path.  Fixes elastic#150299 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 754e868)
gsoldevila
added a commit
that referenced
this pull request
Feb 27, 2023
# Backport This will backport the following commits from `main` to `8.7`: - [Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step (#149931)](#149931) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Gerard Soldevila","email":"gerard.soldevila@elastic.co"},"sourceCommit":{"committedDate":"2023-02-27T10:43:13Z","message":"Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step (#149931)\n\nIn the context of migrations,\r\nhttps://github.com//pull/147371 avoids reindexing during\r\nan upgrade, provided that `diffMappings === false`.\r\n\r\nThis _alternative path_ skips some key steps that are performed before\r\nreindexing:\r\n* `CHECK_UNKNOWN_DOCUMENTS`\r\n* `CALCULATE_EXCLUDE_FILTERS`\r\n\r\nThese steps enrich a search query that is used during reindexing,\r\neffectively filtering out undesired documents.\r\n\r\nIf the mappings [match](https://github.com/elastic/kibana/pull/147371)\r\n(or they are\r\n[compatible](#149326)) and we _no\r\nlonger reindex_, this cleanup operation does not happen, leaving\r\nundesired documents in our system indices.\r\n\r\nThe goal of this PR is to add an extra step in the state machine\r\n(`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system\r\nindex if we're going the _skip reindexing_ path.\r\n\r\n\r\n\r\n\r\nFixes https://github.com/elastic/kibana/issues/150299\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"754e8682d45196e519720693e036017c915c0379","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Team:Core","release_note:skip","Feature:Migrations","backport:prev-minor","v8.7.0","v8.8.0"],"number":149931,"url":"https://github.com/elastic/kibana/pull/149931","mergeCommit":{"message":"Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step (#149931)\n\nIn the context of migrations,\r\nhttps://github.com//pull/147371 avoids reindexing during\r\nan upgrade, provided that `diffMappings === false`.\r\n\r\nThis _alternative path_ skips some key steps that are performed before\r\nreindexing:\r\n* `CHECK_UNKNOWN_DOCUMENTS`\r\n* `CALCULATE_EXCLUDE_FILTERS`\r\n\r\nThese steps enrich a search query that is used during reindexing,\r\neffectively filtering out undesired documents.\r\n\r\nIf the mappings [match](https://github.com/elastic/kibana/pull/147371)\r\n(or they are\r\n[compatible](#149326)) and we _no\r\nlonger reindex_, this cleanup operation does not happen, leaving\r\nundesired documents in our system indices.\r\n\r\nThe goal of this PR is to add an extra step in the state machine\r\n(`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system\r\nindex if we're going the _skip reindexing_ path.\r\n\r\n\r\n\r\n\r\nFixes https://github.com/elastic/kibana/issues/150299\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"754e8682d45196e519720693e036017c915c0379"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/149931","number":149931,"mergeCommit":{"message":"Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step (#149931)\n\nIn the context of migrations,\r\nhttps://github.com//pull/147371 avoids reindexing during\r\nan upgrade, provided that `diffMappings === false`.\r\n\r\nThis _alternative path_ skips some key steps that are performed before\r\nreindexing:\r\n* `CHECK_UNKNOWN_DOCUMENTS`\r\n* `CALCULATE_EXCLUDE_FILTERS`\r\n\r\nThese steps enrich a search query that is used during reindexing,\r\neffectively filtering out undesired documents.\r\n\r\nIf the mappings [match](https://github.com/elastic/kibana/pull/147371)\r\n(or they are\r\n[compatible](#149326)) and we _no\r\nlonger reindex_, this cleanup operation does not happen, leaving\r\nundesired documents in our system indices.\r\n\r\nThe goal of this PR is to add an extra step in the state machine\r\n(`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system\r\nindex if we're going the _skip reindexing_ path.\r\n\r\n\r\n\r\n\r\nFixes https://github.com/elastic/kibana/issues/150299\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"754e8682d45196e519720693e036017c915c0379"}}]}] BACKPORT-->
gsoldevila
added a commit
that referenced
this pull request
Mar 1, 2023
…ex (#149326) Fixes [#147237](#147237) Based on the same principle as [#147371](#147371), the goal of this PR is to **avoid reindexing if possible**. This time, the idea is to check whether the new mappings are still compatible with the ones stored in ES. To to so, we attempt to update the mappings in place in the existing index, introducing a new `CHECK_COMPATIBLE_MAPPINGS` step: * If the update operation fails, we assume the mappings are NOT compatible, and we continue with the normal reindexing flow. * If the update operation succeeds, we assume the mappings ARE compatible, and we skip reindexing, just like [#147371](#147371) does.  --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
bmorelli25
pushed a commit
to bmorelli25/kibana
that referenced
this pull request
Mar 10, 2023
\r\n\r\nFixes){kind=link}
In the context of migrations, elastic#147371 avoids reindexing during an upgrade, provided that `diffMappings === false`. This _alternative path_ skips some key steps that are performed before reindexing: * `CHECK_UNKNOWN_DOCUMENTS` * `CALCULATE_EXCLUDE_FILTERS` These steps enrich a search query that is used during reindexing, effectively filtering out undesired documents. If the mappings [match](elastic#147371) (or they are [compatible](elastic#149326)) and we _no longer reindex_, this cleanup operation does not happen, leaving undesired documents in our system indices. The goal of this PR is to add an extra step in the state machine (`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system index if we're going the _skip reindexing_ path.  Fixes elastic#150299 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
bmorelli25
pushed a commit
to bmorelli25/kibana
that referenced
this pull request
Mar 10, 2023
…ex (elastic#149326) Fixes [elastic#147237](elastic#147237) Based on the same principle as [elastic#147371](elastic#147371), the goal of this PR is to **avoid reindexing if possible**. This time, the idea is to check whether the new mappings are still compatible with the ones stored in ES. To to so, we attempt to update the mappings in place in the existing index, introducing a new `CHECK_COMPATIBLE_MAPPINGS` step: * If the update operation fails, we assume the mappings are NOT compatible, and we continue with the normal reindexing flow. * If the update operation succeeds, we assume the mappings ARE compatible, and we skip reindexing, just like [elastic#147371](elastic#147371) does.  --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
rudolf
added
the
Epic:ScaleMigrations
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #124946
Summary
Takes a step toward optimising our migration paths by only reindexing (an expensive operation) when needed by checking whether the current SO mappings have "changed".
By "changed" we mean that we have detected a new md5 checksum of any registered saved object type relative to the hashes we have stored.
How to test
These changes are constrained to the
model.ts, a test was added for correctly detecting that mappings are the same during theINITphase to send us down the correct migration path.Additionally, we have a new Jest integration test
skip_reindex.test.tsthat runs Kibana and inspects the logs for the expected model transitions.Everything else should remain the same.
Happy path for skipping reindex
Notes
Related
Checklist