Attempt to fix flaky tests after split migration #159397
Merged
Conversation
This file contains 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
gsoldevila
added
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
rudolf
reviewed
Jun 12, 2023
| // Also, https://github.com/elastic/kibana/issues/158918 | ||
| // esArchiver fails with no_shard_available_action_exception after deleting indexes | ||
| // loadTestFile(require.resolve('./disable_legacy_url_aliases')); | ||
| loadTestFile(require.resolve('./disable_legacy_url_aliases')); |
There was a problem hiding this comment.
Looks like this test was flaky from before the split, maybe we should enable the suits mentioned in the issue to test that this works:
Afaik the easiest way to spot the problem is to unskip both https://github.com/elastic/kibana/blob/main/x-pack/test/saved_object_api_integration/spaces_only/apis/resolve_import_errors.ts#L134 and https://github.com/elastic/kibana/blob/main/x-pack/test/saved_object_api_integration/spaces_only/apis/update.ts#L56 and run flaky-test-runner, usually 100 runs is enough
There was a problem hiding this comment.
Unskipped the referenced tests, and triggered 100 runs:
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2376
Flaky test runner is green ✅
| @@ -67,16 +68,12 @@ export function createTestSuiteFactory(esArchiver: any, supertest: SuperTest<any | |||
| (describeFn: DescribeFn) => | |||
| (description: string, { user = {}, spaceId, tests }: CreateTestDefinition) => { | |||
| describeFn(description, () => { | |||
| beforeEach(() => esArchiver.emptyKibanaIndex()); | |||
There was a problem hiding this comment.
Why are we cleaning before the current test instead of after the previous one? Could we continue to use unload since it calls cleanSavedObjectIndices under the hood?
There was a problem hiding this comment.
We cleanup indices on the before hooks because we are reusing the existing ones, created by Kibana, and Kibana might have added some SO already (e.g. the default space).
We cannot rely on unload, cause the delete_index_stream won't be triggered anymore (we deleted mappings.json).
There was a problem hiding this comment.
That makes sense, thanks. Though it would not be very intuitive for me as a developer writing tests that I need to cleanup every time before I import. I wonder if it would make sense to put the cleanSavedObjectIndices into the load action.
So if load process saved objects, then we clean the indices first. This would maintain the existing behaviour because before this PR we would have deleted an saved object index if one existed during the load.
There was a problem hiding this comment.
Fair point.
I created a PR to automatically trigger SO index cleanup whenever SO documents are detected in the data.json:
thomheymann
approved these changes
Jun 14, 2023
kibanamachine
added a commit
that referenced
this pull request
Jun 19, 2023
… are found in data.json (#159582) (#159910) # Backport This will backport the following commits from `main` to `8.8`: - [[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)](#159582) <!--- 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-06-19T11:08:03Z","message":"[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)\n\nThe ultimate goal of this PR is to lay the groundwork to be able to\r\nremove the \"dynamic\" `mappings.json`, which probably should have never\r\nexisted.\r\n\r\nWith the PR, detecting SO documents in the `data.json` will\r\nautomatically trigger a cleanup of the SO indices.\r\nThis, in turn, will allow not having to define \"dynamic\" saved objects\r\nindices (i.e. those with the `$KIBANA_PACKAGE_VERSION` variable in the\r\n`mappings.json`).\r\n\r\nIIUC the idea behind the dynamic indices was to have SO indices that are\r\naligned with the current stack version, avoiding the extra overhead of\r\nhaving to migrate the inserted documents, and reducing overall test\r\ntimes.\r\n\r\nNonetheless, what is happening today is:\r\n1. FTR starts ES and Kibana.\r\n2. Kibana creates current version SO indices at startup (empty ones).\r\n3. `esArchiver.load()` processes the `mappings.json`.\r\n3.1. It detects that we are defining SO indices and **deletes** existing\r\nsaved object indices.\r\n3.2 It then re-creates these indices according to the definitions on\r\n`mappings.json`.\r\n4. `esArchiver.load()` processes the `data.json`. Specifically, it\r\ninserts SO documents present in `data.json`.\r\n5. `esArchiver.load()` calls the _KibanaMigrator_ to make sure that the\r\ninserted documents are up-to-date, hoping they are already aligned with\r\ncurrent stack version (which is not always the case, not even with\r\n\"dynamic\" mappings).\r\n\r\nTwo interesting things to note:\r\n- Steps 3 to 5 happen whilst Kibana is already started and running. If\r\nKibana queries SO indices during `esArchiver.load()`, and a request to\r\nES is made **right after** 3.2, the result might be\r\nhttps://github.com//issues/158918.\r\n- Having dynamic SO indices' definitions, deleting the \"official\"\r\nindices created by Kibana (3.1), and recreating them hoping to be\r\naligned with current stack version (3.2) is non-sense. We could use the\r\nexisting SO indices instead, and simply clean them up whenever we are\r\nabout to insert SO documents.\r\n\r\nPerforming that cleanup is precisely the goal of this PR.\r\nThen, in subsequent PRs like\r\nhttps://github.com//pull/159397/files, tackling the flaky\r\ntests, we'll be able to simply remove the \"dynamic\" `mappings.json`\r\ndefinitions, causing `esArchiver` to rely on SO indices created by\r\nKibana.\r\n\r\nThanks to this PR, the FTR tests won't need to explicitly cleanup saved\r\nobject indices in the `before` hooks.","sha":"bbb5fc4abe7dd530d8248a09a9638cd3438202aa","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","Team:Operations","technical debt","release_note:skip","backport:prev-minor","v8.9.0","FTR","v8.8.2"],"number":159582,"url":"https://github.com/elastic/kibana/pull/159582","mergeCommit":{"message":"[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)\n\nThe ultimate goal of this PR is to lay the groundwork to be able to\r\nremove the \"dynamic\" `mappings.json`, which probably should have never\r\nexisted.\r\n\r\nWith the PR, detecting SO documents in the `data.json` will\r\nautomatically trigger a cleanup of the SO indices.\r\nThis, in turn, will allow not having to define \"dynamic\" saved objects\r\nindices (i.e. those with the `$KIBANA_PACKAGE_VERSION` variable in the\r\n`mappings.json`).\r\n\r\nIIUC the idea behind the dynamic indices was to have SO indices that are\r\naligned with the current stack version, avoiding the extra overhead of\r\nhaving to migrate the inserted documents, and reducing overall test\r\ntimes.\r\n\r\nNonetheless, what is happening today is:\r\n1. FTR starts ES and Kibana.\r\n2. Kibana creates current version SO indices at startup (empty ones).\r\n3. `esArchiver.load()` processes the `mappings.json`.\r\n3.1. It detects that we are defining SO indices and **deletes** existing\r\nsaved object indices.\r\n3.2 It then re-creates these indices according to the definitions on\r\n`mappings.json`.\r\n4. `esArchiver.load()` processes the `data.json`. Specifically, it\r\ninserts SO documents present in `data.json`.\r\n5. `esArchiver.load()` calls the _KibanaMigrator_ to make sure that the\r\ninserted documents are up-to-date, hoping they are already aligned with\r\ncurrent stack version (which is not always the case, not even with\r\n\"dynamic\" mappings).\r\n\r\nTwo interesting things to note:\r\n- Steps 3 to 5 happen whilst Kibana is already started and running. If\r\nKibana queries SO indices during `esArchiver.load()`, and a request to\r\nES is made **right after** 3.2, the result might be\r\nhttps://github.com//issues/158918.\r\n- Having dynamic SO indices' definitions, deleting the \"official\"\r\nindices created by Kibana (3.1), and recreating them hoping to be\r\naligned with current stack version (3.2) is non-sense. We could use the\r\nexisting SO indices instead, and simply clean them up whenever we are\r\nabout to insert SO documents.\r\n\r\nPerforming that cleanup is precisely the goal of this PR.\r\nThen, in subsequent PRs like\r\nhttps://github.com//pull/159397/files, tackling the flaky\r\ntests, we'll be able to simply remove the \"dynamic\" `mappings.json`\r\ndefinitions, causing `esArchiver` to rely on SO indices created by\r\nKibana.\r\n\r\nThanks to this PR, the FTR tests won't need to explicitly cleanup saved\r\nobject indices in the `before` hooks.","sha":"bbb5fc4abe7dd530d8248a09a9638cd3438202aa"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/159582","number":159582,"mergeCommit":{"message":"[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)\n\nThe ultimate goal of this PR is to lay the groundwork to be able to\r\nremove the \"dynamic\" `mappings.json`, which probably should have never\r\nexisted.\r\n\r\nWith the PR, detecting SO documents in the `data.json` will\r\nautomatically trigger a cleanup of the SO indices.\r\nThis, in turn, will allow not having to define \"dynamic\" saved objects\r\nindices (i.e. those with the `$KIBANA_PACKAGE_VERSION` variable in the\r\n`mappings.json`).\r\n\r\nIIUC the idea behind the dynamic indices was to have SO indices that are\r\naligned with the current stack version, avoiding the extra overhead of\r\nhaving to migrate the inserted documents, and reducing overall test\r\ntimes.\r\n\r\nNonetheless, what is happening today is:\r\n1. FTR starts ES and Kibana.\r\n2. Kibana creates current version SO indices at startup (empty ones).\r\n3. `esArchiver.load()` processes the `mappings.json`.\r\n3.1. It detects that we are defining SO indices and **deletes** existing\r\nsaved object indices.\r\n3.2 It then re-creates these indices according to the definitions on\r\n`mappings.json`.\r\n4. `esArchiver.load()` processes the `data.json`. Specifically, it\r\ninserts SO documents present in `data.json`.\r\n5. `esArchiver.load()` calls the _KibanaMigrator_ to make sure that the\r\ninserted documents are up-to-date, hoping they are already aligned with\r\ncurrent stack version (which is not always the case, not even with\r\n\"dynamic\" mappings).\r\n\r\nTwo interesting things to note:\r\n- Steps 3 to 5 happen whilst Kibana is already started and running. If\r\nKibana queries SO indices during `esArchiver.load()`, and a request to\r\nES is made **right after** 3.2, the result might be\r\nhttps://github.com//issues/158918.\r\n- Having dynamic SO indices' definitions, deleting the \"official\"\r\nindices created by Kibana (3.1), and recreating them hoping to be\r\naligned with current stack version (3.2) is non-sense. We could use the\r\nexisting SO indices instead, and simply clean them up whenever we are\r\nabout to insert SO documents.\r\n\r\nPerforming that cleanup is precisely the goal of this PR.\r\nThen, in subsequent PRs like\r\nhttps://github.com//pull/159397/files, tackling the flaky\r\ntests, we'll be able to simply remove the \"dynamic\" `mappings.json`\r\ndefinitions, causing `esArchiver` to rely on SO indices created by\r\nKibana.\r\n\r\nThanks to this PR, the FTR tests won't need to explicitly cleanup saved\r\nobject indices in the `before` hooks.","sha":"bbb5fc4abe7dd530d8248a09a9638cd3438202aa"}},{"branch":"8.8","label":"v8.8.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>
gsoldevila
added
backport:prev-minor
gsoldevila
commented
Jun 21, 2023
| @@ -32,6 +32,7 @@ export function createIndexDocRecordsStream( | |||
| await client.helpers.bulk( | |||
| { | |||
| retries: 5, | |||
| refreshOnCompletion: true, | |||
There was a problem hiding this comment.
@elastic/kibana-operations we were not ensuring indices were refreshed after injecting docs, leading to yet another source of flakiness. E.g.:
https://buildkite.com/elastic/kibana-pull-request/builds/136166#0188d3d0-6c07-4b95-9ab5-248be760ae68
There was a problem hiding this comment.
It turns out this change has uncovered some poorly designed tests that were working by the mere fact that ES indices were not refreshed after esArchiver.load.
I'm looking at the build failures, and I confirm the first one falls in that category. I might use a different PR to address these failures, or this one will end up involving a lot of teams.
There was a problem hiding this comment.
thank you, I had an expectation it is already doing refresh.
There was a problem hiding this comment.
This is getting out of hand for the PR at stakes, so I created a separate PR to tackle this:
I reverted changes in this one. Let's see how CI likes it.
kibanamachine
added a commit
that referenced
this pull request
Jun 29, 2023
#160853) # Backport This will backport the following commits from `main` to `8.9`: - [[esArchiver] Update aliases after creating the indices (#160584)](#160584) <!--- 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-06-29T09:30:15Z","message":"[esArchiver] Update aliases after creating the indices (#160584)\n\nTackles https://github.com/elastic/kibana/issues/158918\r\n\r\nUpdates `esArchiver` so that SO indices are created in two separate\r\nsteps:\r\n\r\n`indices.create()` and `indices.updateAliases()`\r\n\r\nThis way, any Kibana requests that target SO indices (through their\r\naliases) will either find that the indices exist, or that they do not.\r\n\r\nThis is a less invasive approach than\r\nhttps://github.com//pull/159397, as it does not modify the\r\n`esArchiver.load` flow (we don't delete the `mappings.json` files here).","sha":"295b4d4a34bf147ba2d270833355d7c9e6839cc8","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","Team:Operations","release_note:skip","test-failure-flaky","backport:prev-minor","v8.9.0","v8.10.0"],"number":160584,"url":"https://github.com/elastic/kibana/pull/160584","mergeCommit":{"message":"[esArchiver] Update aliases after creating the indices (#160584)\n\nTackles https://github.com/elastic/kibana/issues/158918\r\n\r\nUpdates `esArchiver` so that SO indices are created in two separate\r\nsteps:\r\n\r\n`indices.create()` and `indices.updateAliases()`\r\n\r\nThis way, any Kibana requests that target SO indices (through their\r\naliases) will either find that the indices exist, or that they do not.\r\n\r\nThis is a less invasive approach than\r\nhttps://github.com//pull/159397, as it does not modify the\r\n`esArchiver.load` flow (we don't delete the `mappings.json` files here).","sha":"295b4d4a34bf147ba2d270833355d7c9e6839cc8"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/160584","number":160584,"mergeCommit":{"message":"[esArchiver] Update aliases after creating the indices (#160584)\n\nTackles https://github.com/elastic/kibana/issues/158918\r\n\r\nUpdates `esArchiver` so that SO indices are created in two separate\r\nsteps:\r\n\r\n`indices.create()` and `indices.updateAliases()`\r\n\r\nThis way, any Kibana requests that target SO indices (through their\r\naliases) will either find that the indices exist, or that they do not.\r\n\r\nThis is a less invasive approach than\r\nhttps://github.com//pull/159397, as it does not modify the\r\n`esArchiver.load` flow (we don't delete the `mappings.json` files here).","sha":"295b4d4a34bf147ba2d270833355d7c9e6839cc8"}}]}] BACKPORT--> Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co> Co-authored-by: Alex Szabo <alex.szabo@elastic.co>
gsoldevila
added
v8.10.0
v7.17.12
backport:all-open
gsoldevila
commented
Jul 7, 2023
| "namespaces": ["space_1"] | ||
| "namespaces": ["space_1"], | ||
| "coreMigrationVersion": "8.8.0", | ||
| "typeMigrationVersion": "8.5.0" |
There was a problem hiding this comment.
I have added the coreMigrationVersion and typeMigrationVersion for all saved objects with actual types (not dummy testing types).
Not doing so causes all the "core migrations" to run (as if they were really old SOs), and leads to the "namespaces": ["space_1"]" to be incorrectly handled by internal_transforms.ts, resulting in "namespaces": ["default"].
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @gsoldevila |
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jul 7, 2023
## Summary Addresses root cause of elastic#158918 Underlying cause is that _esArchiver_ is messing up with the SO indices whilst Kibana is already running. This can cause some asynchronous calls made by Kibana (e.g. `GET /.kibana_8.8.0/telemetry:telemetry`) to hit ES at the exact time where the underlying SO indices are **just** recreated, causing the error described in the related issue. The idea of the fix is to delete `mappings.json`, used by _esArchiver_ to create the SO indices. This way, _esArchiver_ will use existing SO indices instead (aka the "official" ones, created by Kibana at startup), thus avoiding the problem altogether. As a side effect: - Documents in `data.json` must be updated so that they are correctly inserted. - The different FTR tests must make sure the SO indices are empty before inserting those documents (done in the `before(), beforeEach()` statements). (cherry picked from commit 79f7bb4)
kibanamachine
added a commit
that referenced
this pull request
Jul 7, 2023
…1468) # Backport This will backport the following commits from `main` to `8.9`: - [Attempt to fix flaky tests after split migration (#159397)](#159397) <!--- 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-07-07T15:05:42Z","message":"Attempt to fix flaky tests after split migration (#159397)\n\n## Summary\r\nAddresses root cause of https://github.com/elastic/kibana/issues/158918\r\n\r\nUnderlying cause is that _esArchiver_ is messing up with the SO indices\r\nwhilst Kibana is already running.\r\n\r\nThis can cause some asynchronous calls made by Kibana (e.g. `GET\r\n/.kibana_8.8.0/telemetry:telemetry`) to hit ES at the exact time where\r\nthe underlying SO indices are **just** recreated, causing the error\r\ndescribed in the related issue.\r\n\r\nThe idea of the fix is to delete `mappings.json`, used by _esArchiver_\r\nto create the SO indices. This way, _esArchiver_ will use existing SO\r\nindices instead (aka the \"official\" ones, created by Kibana at startup),\r\nthus avoiding the problem altogether.\r\n\r\nAs a side effect:\r\n\r\n- Documents in `data.json` must be updated so that they are correctly\r\ninserted.\r\n- The different FTR tests must make sure the SO indices are empty before\r\ninserting those documents (done in the `before(), beforeEach()`\r\nstatements).","sha":"79f7bb45fd44e5c25302dcc23e73c0ff0cda52d0","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Team:Core","technical debt","release_note:skip","test-failure-flaky","Team:Security Solution Platform","backport:prev-minor","backport:prev-MAJOR","backport:all-open","v8.9.0","FTR","v8.10.0","v7.17.12"],"number":159397,"url":"https://github.com/elastic/kibana/pull/159397","mergeCommit":{"message":"Attempt to fix flaky tests after split migration (#159397)\n\n## Summary\r\nAddresses root cause of https://github.com/elastic/kibana/issues/158918\r\n\r\nUnderlying cause is that _esArchiver_ is messing up with the SO indices\r\nwhilst Kibana is already running.\r\n\r\nThis can cause some asynchronous calls made by Kibana (e.g. `GET\r\n/.kibana_8.8.0/telemetry:telemetry`) to hit ES at the exact time where\r\nthe underlying SO indices are **just** recreated, causing the error\r\ndescribed in the related issue.\r\n\r\nThe idea of the fix is to delete `mappings.json`, used by _esArchiver_\r\nto create the SO indices. This way, _esArchiver_ will use existing SO\r\nindices instead (aka the \"official\" ones, created by Kibana at startup),\r\nthus avoiding the problem altogether.\r\n\r\nAs a side effect:\r\n\r\n- Documents in `data.json` must be updated so that they are correctly\r\ninserted.\r\n- The different FTR tests must make sure the SO indices are empty before\r\ninserting those documents (done in the `before(), beforeEach()`\r\nstatements).","sha":"79f7bb45fd44e5c25302dcc23e73c0ff0cda52d0"}},"sourceBranch":"main","suggestedTargetBranches":["8.9","7.17"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/159397","number":159397,"mergeCommit":{"message":"Attempt to fix flaky tests after split migration (#159397)\n\n## Summary\r\nAddresses root cause of https://github.com/elastic/kibana/issues/158918\r\n\r\nUnderlying cause is that _esArchiver_ is messing up with the SO indices\r\nwhilst Kibana is already running.\r\n\r\nThis can cause some asynchronous calls made by Kibana (e.g. `GET\r\n/.kibana_8.8.0/telemetry:telemetry`) to hit ES at the exact time where\r\nthe underlying SO indices are **just** recreated, causing the error\r\ndescribed in the related issue.\r\n\r\nThe idea of the fix is to delete `mappings.json`, used by _esArchiver_\r\nto create the SO indices. This way, _esArchiver_ will use existing SO\r\nindices instead (aka the \"official\" ones, created by Kibana at startup),\r\nthus avoiding the problem altogether.\r\n\r\nAs a side effect:\r\n\r\n- Documents in `data.json` must be updated so that they are correctly\r\ninserted.\r\n- The different FTR tests must make sure the SO indices are empty before\r\ninserting those documents (done in the `before(), beforeEach()`\r\nstatements).","sha":"79f7bb45fd44e5c25302dcc23e73c0ff0cda52d0"}},{"branch":"7.17","label":"v7.17.12","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>
gsoldevila
added
backport:prev-minor
This was referenced Jul 11, 2023
Merged
gsoldevila
added a commit
that referenced
this pull request
Aug 7, 2023
…s are found in data.json (#159582) (#163178) # Backport This will backport the following commits from `main` to `7.17`: - [[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)](#159582) <!--- Backport version: 8.9.8 --> ### 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-06-19T11:08:03Z","message":"[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)\n\nThe ultimate goal of this PR is to lay the groundwork to be able to\r\nremove the \"dynamic\" `mappings.json`, which probably should have never\r\nexisted.\r\n\r\nWith the PR, detecting SO documents in the `data.json` will\r\nautomatically trigger a cleanup of the SO indices.\r\nThis, in turn, will allow not having to define \"dynamic\" saved objects\r\nindices (i.e. those with the `$KIBANA_PACKAGE_VERSION` variable in the\r\n`mappings.json`).\r\n\r\nIIUC the idea behind the dynamic indices was to have SO indices that are\r\naligned with the current stack version, avoiding the extra overhead of\r\nhaving to migrate the inserted documents, and reducing overall test\r\ntimes.\r\n\r\nNonetheless, what is happening today is:\r\n1. FTR starts ES and Kibana.\r\n2. Kibana creates current version SO indices at startup (empty ones).\r\n3. `esArchiver.load()` processes the `mappings.json`.\r\n3.1. It detects that we are defining SO indices and **deletes** existing\r\nsaved object indices.\r\n3.2 It then re-creates these indices according to the definitions on\r\n`mappings.json`.\r\n4. `esArchiver.load()` processes the `data.json`. Specifically, it\r\ninserts SO documents present in `data.json`.\r\n5. `esArchiver.load()` calls the _KibanaMigrator_ to make sure that the\r\ninserted documents are up-to-date, hoping they are already aligned with\r\ncurrent stack version (which is not always the case, not even with\r\n\"dynamic\" mappings).\r\n\r\nTwo interesting things to note:\r\n- Steps 3 to 5 happen whilst Kibana is already started and running. If\r\nKibana queries SO indices during `esArchiver.load()`, and a request to\r\nES is made **right after** 3.2, the result might be\r\nhttps://github.com//issues/158918.\r\n- Having dynamic SO indices' definitions, deleting the \"official\"\r\nindices created by Kibana (3.1), and recreating them hoping to be\r\naligned with current stack version (3.2) is non-sense. We could use the\r\nexisting SO indices instead, and simply clean them up whenever we are\r\nabout to insert SO documents.\r\n\r\nPerforming that cleanup is precisely the goal of this PR.\r\nThen, in subsequent PRs like\r\nhttps://github.com//pull/159397/files, tackling the flaky\r\ntests, we'll be able to simply remove the \"dynamic\" `mappings.json`\r\ndefinitions, causing `esArchiver` to rely on SO indices created by\r\nKibana.\r\n\r\nThanks to this PR, the FTR tests won't need to explicitly cleanup saved\r\nobject indices in the `before` hooks.","sha":"bbb5fc4abe7dd530d8248a09a9638cd3438202aa","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","Team:Operations","technical debt","release_note:skip","backport:prev-minor","backport:prev-MAJOR","v8.9.0","FTR","v8.8.2","v8.10.0","v7.17.13"],"number":159582,"url":"https://github.com/elastic/kibana/pull/159582","mergeCommit":{"message":"[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)\n\nThe ultimate goal of this PR is to lay the groundwork to be able to\r\nremove the \"dynamic\" `mappings.json`, which probably should have never\r\nexisted.\r\n\r\nWith the PR, detecting SO documents in the `data.json` will\r\nautomatically trigger a cleanup of the SO indices.\r\nThis, in turn, will allow not having to define \"dynamic\" saved objects\r\nindices (i.e. those with the `$KIBANA_PACKAGE_VERSION` variable in the\r\n`mappings.json`).\r\n\r\nIIUC the idea behind the dynamic indices was to have SO indices that are\r\naligned with the current stack version, avoiding the extra overhead of\r\nhaving to migrate the inserted documents, and reducing overall test\r\ntimes.\r\n\r\nNonetheless, what is happening today is:\r\n1. FTR starts ES and Kibana.\r\n2. Kibana creates current version SO indices at startup (empty ones).\r\n3. `esArchiver.load()` processes the `mappings.json`.\r\n3.1. It detects that we are defining SO indices and **deletes** existing\r\nsaved object indices.\r\n3.2 It then re-creates these indices according to the definitions on\r\n`mappings.json`.\r\n4. `esArchiver.load()` processes the `data.json`. Specifically, it\r\ninserts SO documents present in `data.json`.\r\n5. `esArchiver.load()` calls the _KibanaMigrator_ to make sure that the\r\ninserted documents are up-to-date, hoping they are already aligned with\r\ncurrent stack version (which is not always the case, not even with\r\n\"dynamic\" mappings).\r\n\r\nTwo interesting things to note:\r\n- Steps 3 to 5 happen whilst Kibana is already started and running. If\r\nKibana queries SO indices during `esArchiver.load()`, and a request to\r\nES is made **right after** 3.2, the result might be\r\nhttps://github.com//issues/158918.\r\n- Having dynamic SO indices' definitions, deleting the \"official\"\r\nindices created by Kibana (3.1), and recreating them hoping to be\r\naligned with current stack version (3.2) is non-sense. We could use the\r\nexisting SO indices instead, and simply clean them up whenever we are\r\nabout to insert SO documents.\r\n\r\nPerforming that cleanup is precisely the goal of this PR.\r\nThen, in subsequent PRs like\r\nhttps://github.com//pull/159397/files, tackling the flaky\r\ntests, we'll be able to simply remove the \"dynamic\" `mappings.json`\r\ndefinitions, causing `esArchiver` to rely on SO indices created by\r\nKibana.\r\n\r\nThanks to this PR, the FTR tests won't need to explicitly cleanup saved\r\nobject indices in the `before` hooks.","sha":"bbb5fc4abe7dd530d8248a09a9638cd3438202aa"}},"sourceBranch":"main","suggestedTargetBranches":["7.17"],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/159582","number":159582,"mergeCommit":{"message":"[esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582)\n\nThe ultimate goal of this PR is to lay the groundwork to be able to\r\nremove the \"dynamic\" `mappings.json`, which probably should have never\r\nexisted.\r\n\r\nWith the PR, detecting SO documents in the `data.json` will\r\nautomatically trigger a cleanup of the SO indices.\r\nThis, in turn, will allow not having to define \"dynamic\" saved objects\r\nindices (i.e. those with the `$KIBANA_PACKAGE_VERSION` variable in the\r\n`mappings.json`).\r\n\r\nIIUC the idea behind the dynamic indices was to have SO indices that are\r\naligned with the current stack version, avoiding the extra overhead of\r\nhaving to migrate the inserted documents, and reducing overall test\r\ntimes.\r\n\r\nNonetheless, what is happening today is:\r\n1. FTR starts ES and Kibana.\r\n2. Kibana creates current version SO indices at startup (empty ones).\r\n3. `esArchiver.load()` processes the `mappings.json`.\r\n3.1. It detects that we are defining SO indices and **deletes** existing\r\nsaved object indices.\r\n3.2 It then re-creates these indices according to the definitions on\r\n`mappings.json`.\r\n4. `esArchiver.load()` processes the `data.json`. Specifically, it\r\ninserts SO documents present in `data.json`.\r\n5. `esArchiver.load()` calls the _KibanaMigrator_ to make sure that the\r\ninserted documents are up-to-date, hoping they are already aligned with\r\ncurrent stack version (which is not always the case, not even with\r\n\"dynamic\" mappings).\r\n\r\nTwo interesting things to note:\r\n- Steps 3 to 5 happen whilst Kibana is already started and running. If\r\nKibana queries SO indices during `esArchiver.load()`, and a request to\r\nES is made **right after** 3.2, the result might be\r\nhttps://github.com//issues/158918.\r\n- Having dynamic SO indices' definitions, deleting the \"official\"\r\nindices created by Kibana (3.1), and recreating them hoping to be\r\naligned with current stack version (3.2) is non-sense. We could use the\r\nexisting SO indices instead, and simply clean them up whenever we are\r\nabout to insert SO documents.\r\n\r\nPerforming that cleanup is precisely the goal of this PR.\r\nThen, in subsequent PRs like\r\nhttps://github.com//pull/159397/files, tackling the flaky\r\ntests, we'll be able to simply remove the \"dynamic\" `mappings.json`\r\ndefinitions, causing `esArchiver` to rely on SO indices created by\r\nKibana.\r\n\r\nThanks to this PR, the FTR tests won't need to explicitly cleanup saved\r\nobject indices in the `before` hooks.","sha":"bbb5fc4abe7dd530d8248a09a9638cd3438202aa"}},{"branch":"8.8","label":"v8.8.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/159910","number":159910,"state":"MERGED","mergeCommit":{"sha":"d2683cba7bfaff545a90c5a7daccb0b731fa1ba0","message":"[8.8] [esArchiver] Automatically cleanup SO indices when SO documents are found in data.json (#159582) (#159910)\n\n# Backport\n\nThis will backport the following commits from `main` to `8.8`:\n- [[esArchiver] Automatically cleanup SO indices when SO documents are\nfound in data.json\n(#159582)](https://github.com/elastic/kibana/pull/159582)\n\n<!--- Backport version: 8.9.7 -->\n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n<!--BACKPORT [{\"author\":{\"name\":\"Gerard\nSoldevila\",\"email\":\"gerard.soldevila@elastic.co\"},\"sourceCommit\":{\"committedDate\":\"2023-06-19T11:08:03Z\",\"message\":\"[esArchiver]\nAutomatically cleanup SO indices when SO documents are found in\ndata.json (#159582)\\n\\nThe ultimate goal of this PR is to lay the\ngroundwork to be able to\\r\\nremove the \\\"dynamic\\\" `mappings.json`,\nwhich probably should have never\\r\\nexisted.\\r\\n\\r\\nWith the PR,\ndetecting SO documents in the `data.json` will\\r\\nautomatically trigger\na cleanup of the SO indices.\\r\\nThis, in turn, will allow not having to\ndefine \\\"dynamic\\\" saved objects\\r\\nindices (i.e. those with the\n`$KIBANA_PACKAGE_VERSION` variable in\nthe\\r\\n`mappings.json`).\\r\\n\\r\\nIIUC the idea behind the dynamic indices\nwas to have SO indices that are\\r\\naligned with the current stack\nversion, avoiding the extra overhead of\\r\\nhaving to migrate the\ninserted documents, and reducing overall\ntest\\r\\ntimes.\\r\\n\\r\\nNonetheless, what is happening today is:\\r\\n1. FTR\nstarts ES and Kibana.\\r\\n2. Kibana creates current version SO indices at\nstartup (empty ones).\\r\\n3. `esArchiver.load()` processes the\n`mappings.json`.\\r\\n3.1. It detects that we are defining SO indices and\n**deletes** existing\\r\\nsaved object indices.\\r\\n3.2 It then re-creates\nthese indices according to the definitions on\\r\\n`mappings.json`.\\r\\n4.\n`esArchiver.load()` processes the `data.json`. Specifically,\nit\\r\\ninserts SO documents present in `data.json`.\\r\\n5.\n`esArchiver.load()` calls the _KibanaMigrator_ to make sure that\nthe\\r\\ninserted documents are up-to-date, hoping they are already\naligned with\\r\\ncurrent stack version (which is not always the case, not\neven with\\r\\n\\\"dynamic\\\" mappings).\\r\\n\\r\\nTwo interesting things to\nnote:\\r\\n- Steps 3 to 5 happen whilst Kibana is already started and\nrunning. If\\r\\nKibana queries SO indices during `esArchiver.load()`, and\na request to\\r\\nES is made **right after** 3.2, the result might\nbe\\r\\nhttps://github.com//issues/158918.\\r\\n- Having\ndynamic SO indices' definitions, deleting the \\\"official\\\"\\r\\nindices\ncreated by Kibana (3.1), and recreating them hoping to be\\r\\naligned\nwith current stack version (3.2) is non-sense. We could use\nthe\\r\\nexisting SO indices instead, and simply clean them up whenever we\nare\\r\\nabout to insert SO documents.\\r\\n\\r\\nPerforming that cleanup is\nprecisely the goal of this PR.\\r\\nThen, in subsequent PRs\nlike\\r\\nhttps://github.com//pull/159397/files, tackling\nthe flaky\\r\\ntests, we'll be able to simply remove the \\\"dynamic\\\"\n`mappings.json`\\r\\ndefinitions, causing `esArchiver` to rely on SO\nindices created by\\r\\nKibana.\\r\\n\\r\\nThanks to this PR, the FTR tests\nwon't need to explicitly cleanup saved\\r\\nobject indices in the `before`\nhooks.\",\"sha\":\"bbb5fc4abe7dd530d8248a09a9638cd3438202aa\",\"branchLabelMapping\":{\"^v8.9.0$\":\"main\",\"^v(\\\\d+).(\\\\d+).\\\\d+$\":\"$1.$2\"}},\"sourcePullRequest\":{\"labels\":[\"Team:Core\",\"Team:Operations\",\"technical\ndebt\",\"release_note:skip\",\"backport:prev-minor\",\"v8.9.0\",\"FTR\",\"v8.8.2\"],\"number\":159582,\"url\":\"https://github.com/elastic/kibana/pull/159582\",\"mergeCommit\":{\"message\":\"[esArchiver]\nAutomatically cleanup SO indices when SO documents are found in\ndata.json (#159582)\\n\\nThe ultimate goal of this PR is to lay the\ngroundwork to be able to\\r\\nremove the \\\"dynamic\\\" `mappings.json`,\nwhich probably should have never\\r\\nexisted.\\r\\n\\r\\nWith the PR,\ndetecting SO documents in the `data.json` will\\r\\nautomatically trigger\na cleanup of the SO indices.\\r\\nThis, in turn, will allow not having to\ndefine \\\"dynamic\\\" saved objects\\r\\nindices (i.e. those with the\n`$KIBANA_PACKAGE_VERSION` variable in\nthe\\r\\n`mappings.json`).\\r\\n\\r\\nIIUC the idea behind the dynamic indices\nwas to have SO indices that are\\r\\naligned with the current stack\nversion, avoiding the extra overhead of\\r\\nhaving to migrate the\ninserted documents, and reducing overall\ntest\\r\\ntimes.\\r\\n\\r\\nNonetheless, what is happening today is:\\r\\n1. FTR\nstarts ES and Kibana.\\r\\n2. Kibana creates current version SO indices at\nstartup (empty ones).\\r\\n3. `esArchiver.load()` processes the\n`mappings.json`.\\r\\n3.1. It detects that we are defining SO indices and\n**deletes** existing\\r\\nsaved object indices.\\r\\n3.2 It then re-creates\nthese indices according to the definitions on\\r\\n`mappings.json`.\\r\\n4.\n`esArchiver.load()` processes the `data.json`. Specifically,\nit\\r\\ninserts SO documents present in `data.json`.\\r\\n5.\n`esArchiver.load()` calls the _KibanaMigrator_ to make sure that\nthe\\r\\ninserted documents are up-to-date, hoping they are already\naligned with\\r\\ncurrent stack version (which is not always the case, not\neven with\\r\\n\\\"dynamic\\\" mappings).\\r\\n\\r\\nTwo interesting things to\nnote:\\r\\n- Steps 3 to 5 happen whilst Kibana is already started and\nrunning. If\\r\\nKibana queries SO indices during `esArchiver.load()`, and\na request to\\r\\nES is made **right after** 3.2, the result might\nbe\\r\\nhttps://github.com//issues/158918.\\r\\n- Having\ndynamic SO indices' definitions, deleting the \\\"official\\\"\\r\\nindices\ncreated by Kibana (3.1), and recreating them hoping to be\\r\\naligned\nwith current stack version (3.2) is non-sense. We could use\nthe\\r\\nexisting SO indices instead, and simply clean them up whenever we\nare\\r\\nabout to insert SO documents.\\r\\n\\r\\nPerforming that cleanup is\nprecisely the goal of this PR.\\r\\nThen, in subsequent PRs\nlike\\r\\nhttps://github.com//pull/159397/files, tackling\nthe flaky\\r\\ntests, we'll be able to simply remove the \\\"dynamic\\\"\n`mappings.json`\\r\\ndefinitions, causing `esArchiver` to rely on SO\nindices created by\\r\\nKibana.\\r\\n\\r\\nThanks to this PR, the FTR tests\nwon't need to explicitly cleanup saved\\r\\nobject indices in the `before`\nhooks.\",\"sha\":\"bbb5fc4abe7dd530d8248a09a9638cd3438202aa\"}},\"sourceBranch\":\"main\",\"suggestedTargetBranches\":[\"8.8\"],\"targetPullRequestStates\":[{\"branch\":\"main\",\"label\":\"v8.9.0\",\"labelRegex\":\"^v8.9.0$\",\"isSourceBranch\":true,\"state\":\"MERGED\",\"url\":\"https://github.com/elastic/kibana/pull/159582\",\"number\":159582,\"mergeCommit\":{\"message\":\"[esArchiver]\nAutomatically cleanup SO indices when SO documents are found in\ndata.json (#159582)\\n\\nThe ultimate goal of this PR is to lay the\ngroundwork to be able to\\r\\nremove the \\\"dynamic\\\" `mappings.json`,\nwhich probably should have never\\r\\nexisted.\\r\\n\\r\\nWith the PR,\ndetecting SO documents in the `data.json` will\\r\\nautomatically trigger\na cleanup of the SO indices.\\r\\nThis, in turn, will allow not having to\ndefine \\\"dynamic\\\" saved objects\\r\\nindices (i.e. those with the\n`$KIBANA_PACKAGE_VERSION` variable in\nthe\\r\\n`mappings.json`).\\r\\n\\r\\nIIUC the idea behind the dynamic indices\nwas to have SO indices that are\\r\\naligned with the current stack\nversion, avoiding the extra overhead of\\r\\nhaving to migrate the\ninserted documents, and reducing overall\ntest\\r\\ntimes.\\r\\n\\r\\nNonetheless, what is happening today is:\\r\\n1. FTR\nstarts ES and Kibana.\\r\\n2. Kibana creates current version SO indices at\nstartup (empty ones).\\r\\n3. `esArchiver.load()` processes the\n`mappings.json`.\\r\\n3.1. It detects that we are defining SO indices and\n**deletes** existing\\r\\nsaved object indices.\\r\\n3.2 It then re-creates\nthese indices according to the definitions on\\r\\n`mappings.json`.\\r\\n4.\n`esArchiver.load()` processes the `data.json`. Specifically,\nit\\r\\ninserts SO documents present in `data.json`.\\r\\n5.\n`esArchiver.load()` calls the _KibanaMigrator_ to make sure that\nthe\\r\\ninserted documents are up-to-date, hoping they are already\naligned with\\r\\ncurrent stack version (which is not always the case, not\neven with\\r\\n\\\"dynamic\\\" mappings).\\r\\n\\r\\nTwo interesting things to\nnote:\\r\\n- Steps 3 to 5 happen whilst Kibana is already started and\nrunning. If\\r\\nKibana queries SO indices during `esArchiver.load()`, and\na request to\\r\\nES is made **right after** 3.2, the result might\nbe\\r\\nhttps://github.com//issues/158918.\\r\\n- Having\ndynamic SO indices' definitions, deleting the \\\"official\\\"\\r\\nindices\ncreated by Kibana (3.1), and recreating them hoping to be\\r\\naligned\nwith current stack version (3.2) is non-sense. We could use\nthe\\r\\nexisting SO indices instead, and simply clean them up whenever we\nare\\r\\nabout to insert SO documents.\\r\\n\\r\\nPerforming that cleanup is\nprecisely the goal of this PR.\\r\\nThen, in subsequent PRs\nlike\\r\\nhttps://github.com//pull/159397/files, tackling\nthe flaky\\r\\ntests, we'll be able to simply remove the \\\"dynamic\\\"\n`mappings.json`\\r\\ndefinitions, causing `esArchiver` to rely on SO\nindices created by\\r\\nKibana.\\r\\n\\r\\nThanks to this PR, the FTR tests\nwon't need to explicitly cleanup saved\\r\\nobject indices in the `before`\nhooks.\",\"sha\":\"bbb5fc4abe7dd530d8248a09a9638cd3438202aa\"}},{\"branch\":\"8.8\",\"label\":\"v8.8.2\",\"labelRegex\":\"^v(\\\\d+).(\\\\d+).\\\\d+$\",\"isSourceBranch\":false,\"state\":\"NOT_CREATED\"}]}]\nBACKPORT-->\n\nCo-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>"}},{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"7.17","label":"v7.17.13","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Addresses root cause of #158918
Underlying cause is that esArchiver is messing up with the SO indices whilst Kibana is already running.
This can cause some asynchronous calls made by Kibana (e.g.
GET /.kibana_8.8.0/telemetry:telemetry) to hit ES at the exact time where the underlying SO indices are just recreated, causing the error described in the related issue.The idea of the fix is to delete
mappings.json, used by esArchiver to create the SO indices. This way, esArchiver will use existing SO indices instead (aka the "official" ones, created by Kibana at startup), thus avoiding the problem altogether.As a side effect:
data.jsonmust be updated so that they are correctly inserted.before(), beforeEach()statements).