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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Jun 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ef4aa10
WIP don't auto-create temp index
rudolf May 22, 2023
1314315
Fix type bugs
rudolf May 24, 2023
2c57bc0
Fix bulk indexing batches
rudolf May 24, 2023
2cf38ce
Fix tests
rudolf May 24, 2023
e40d63f
Less invasive way of pointing temp index batches at alias
rudolf May 31, 2023
1108baa
Merge branch 'main' into migrations-unavailable-shards
kibanamachine Jun 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...-migration-server-internal/src/__snapshots__/migrations_state_action_machine.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 8 additions & 1 deletion ...ved-objects-migration-server-internal/src/actions/bulk_overwrite_transformed_documents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ export interface BulkOverwriteTransformedDocumentsParams {
index: string;
operations: BulkOperation[];
refresh?: estypes.Refresh;
/**
* 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;
Comment on lines +29 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

/**
Expand All @@ -38,6 +44,7 @@ export const bulkOverwriteTransformedDocuments =
index,
operations,
refresh = false,
useAliasToPreventAutoCreate = false,
}: BulkOverwriteTransformedDocumentsParams): TaskEither.TaskEither<
| RetryableEsClientError
| TargetIndexHadWriteBlock
Expand All @@ -56,7 +63,7 @@ export const bulkOverwriteTransformedDocuments =
// probably unlikely so for now we'll accept this risk and wait till
// system indices puts in place a hard control.
index,
require_alias: false,
require_alias: useAliasToPreventAutoCreate,
wait_for_active_shards: WAIT_FOR_ALL_SHARDS_TO_BE_ACTIVE,
refresh,
filter_path: ['items.*.error'],
Expand Down
1 change: 1 addition & 0 deletions ...core/saved-objects/core-saved-objects-migration-server-internal/src/initial_state.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ describe('createInitialState', () => {
},
},
"tempIndex": ".kibana_task_manager_8.1.0_reindex_temp",
"tempIndexAlias": ".kibana_task_manager_8.1.0_reindex_temp_alias",
"tempIndexMappings": Object {
"dynamic": false,
"properties": Object {
Expand Down
1 change: 1 addition & 0 deletions ...ages/core/saved-objects/core-saved-objects-migration-server-internal/src/initial_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export const createInitialState = ({
versionAlias: `${indexPrefix}_${kibanaVersion}`,
versionIndex: `${indexPrefix}_${kibanaVersion}_001`,
tempIndex: getTempIndexName(indexPrefix, kibanaVersion),
tempIndexAlias: getTempIndexName(indexPrefix, kibanaVersion) + '_alias',
kibanaVersion,
preMigrationScript: Option.fromNullable(preMigrationScript),
targetIndexMappings,
Expand Down
12 changes: 6 additions & 6 deletions ...ved-objects/core-saved-objects-migration-server-internal/src/model/create_batches.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe('createBatches', () => {
expect(
createBatches({
documents,
maxBatchSizeBytes: (DOCUMENT_SIZE_BYTES + 43) * 2, // add extra length for 'index' property
maxBatchSizeBytes: (DOCUMENT_SIZE_BYTES + 49) * 2, // add extra length for 'index' property
typeIndexMap: buildTempIndexMap(
{
'.kibana': ['dashboard'],
Expand All @@ -108,7 +108,7 @@ describe('createBatches', () => {
{
index: {
_id: '',
_index: '.kibana_8.8.0_reindex_temp',
_index: '.kibana_8.8.0_reindex_temp_alias',
},
},
{ type: 'dashboard', title: 'my saved object title ¹' },
Expand All @@ -117,7 +117,7 @@ describe('createBatches', () => {
{
index: {
_id: '',
_index: '.kibana_8.8.0_reindex_temp',
_index: '.kibana_8.8.0_reindex_temp_alias',
},
},
{ type: 'dashboard', title: 'my saved object title ²' },
Expand All @@ -128,7 +128,7 @@ describe('createBatches', () => {
{
index: {
_id: '',
_index: '.kibana_cases_8.8.0_reindex_temp',
_index: '.kibana_cases_8.8.0_reindex_temp_alias',
},
},
{ type: 'cases', title: 'a case' },
Expand All @@ -137,7 +137,7 @@ describe('createBatches', () => {
{
index: {
_id: '',
_index: '.kibana_cases_8.8.0_reindex_temp',
_index: '.kibana_cases_8.8.0_reindex_temp_alias',
},
},
{ type: 'cases-comments', title: 'a case comment #1' },
Expand All @@ -148,7 +148,7 @@ describe('createBatches', () => {
{
index: {
_id: '',
_index: '.kibana_cases_8.8.0_reindex_temp',
_index: '.kibana_cases_8.8.0_reindex_temp_alias',
},
},
{ type: 'cases-user-actions', title: 'a case user action' },
Expand Down
2 changes: 1 addition & 1 deletion ...core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ describe('createBulkIndexOperationTuple', () => {
Object {
"index": Object {
"_id": "",
"_index": ".kibana_cases_8.8.0_reindex_temp",
"_index": ".kibana_cases_8.8.0_reindex_temp_alias",
},
},
Object {
Expand Down
4 changes: 3 additions & 1 deletion ...ages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,9 @@ export const createBulkIndexOperationTuple = (
{
index: {
_id: doc._id,
...(typeIndexMap[doc._source.type] && { _index: typeIndexMap[doc._source.type] }),
...(typeIndexMap[doc._source.type] && {
_index: typeIndexMap[doc._source.type] + '_alias',
}),
Copy link
Contributor

@pgayvallet pgayvallet May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Screenshot 2023-05-30 at 08 40 34

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// use optimistic concurrency control to ensure that outdated
// documents are only overwritten once with the latest version
...(typeof doc._seq_no !== 'undefined' && { if_seq_no: doc._seq_no }),
Expand Down
1 change: 1 addition & 0 deletions ...s/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ describe('migrations v2 model', () => {
versionAlias: '.kibana_7.11.0',
versionIndex: '.kibana_7.11.0_001',
tempIndex: '.kibana_7.11.0_reindex_temp',
tempIndexAlias: '.kibana_7.11.0_reindex_temp_alias',
excludeOnUpgradeQuery: {
bool: {
must_not: [
Expand Down
9 changes: 8 additions & 1 deletion packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export const nextActionMap = (
Actions.createIndex({
client,
indexName: state.tempIndex,
aliases: [state.tempIndexAlias],
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, aliases are "attached" to an index.

mappings: state.tempIndexMappings,
}),
READY_TO_REINDEX_SYNC: () => Actions.synchronizeMigrators(readyToReindex),
Expand All @@ -163,7 +164,13 @@ export const nextActionMap = (
REINDEX_SOURCE_TO_TEMP_INDEX_BULK: (state: ReindexSourceToTempIndexBulk) =>
Actions.bulkOverwriteTransformedDocuments({
client,
index: state.tempIndex,
/*
* Since other nodes can delete the temp index while we're busy writing
* to it, we use the alias to prevent the auto-creation of the index if
* it doesn't exist.
*/
index: state.tempIndexAlias,
useAliasToPreventAutoCreate: true,
operations: state.bulkOperationBatches[state.currentBatch],
/**
* Since we don't run a search against the target index, we disable "refresh" to speed up
Expand Down
8 changes: 7 additions & 1 deletion packages/core/saved-objects/core-saved-objects-migration-server-internal/src/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,16 @@ export interface BaseState extends ControlState {
*/
readonly versionIndex: string;
/**
* An alias on the target index used as part of an "reindex block" that
* A temporary index used as part of an "reindex block" that
* prevents lost deletes e.g. `.kibana_7.11.0_reindex`.
*/
readonly tempIndex: string;
/**
* 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;
Comment on lines +139 to +144
Copy link
Contributor

Choose a reason for hiding this comment

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

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

/**
* When upgrading to a more recent kibana version, some saved object types
* might be conflicting or no longer used.
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/integration_tests/saved_objects/migrations/group2/batch_size_bytes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe('migration v2', () => {
await root.preboot();
await root.setup();
await expect(root.start()).rejects.toMatchInlineSnapshot(
`[Error: Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715312 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.]`
`[Error: Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715318 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.]`
);

await retryAsync(
Expand All @@ -131,7 +131,7 @@ describe('migration v2', () => {
expect(
records.find((rec) =>
rec.message.startsWith(
`Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715312 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.`
`Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715318 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.`
)
)
).toBeDefined();
Expand Down
33 changes: 31 additions & 2 deletions src/core/server/integration_tests/saved_objects/migrations/group3/actions/actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('migration actions', () => {
await createIndex({
client,
indexName: 'existing_index_with_docs',
aliases: ['existing_index_with_docs_alias'],
mappings: {
dynamic: true,
properties: {
Expand Down Expand Up @@ -149,7 +150,9 @@ describe('migration actions', () => {
expect(res.right).toEqual(
expect.objectContaining({
existing_index_with_docs: {
aliases: {},
aliases: {
existing_index_with_docs_alias: {},
},
mappings: expect.anything(),
settings: expect.anything(),
},
Expand All @@ -166,7 +169,9 @@ describe('migration actions', () => {
expect(res.right).toEqual(
expect.objectContaining({
existing_index_with_docs: {
aliases: {},
aliases: {
existing_index_with_docs_alias: {},
},
mappings: {
// FIXME https://github.com/elastic/elasticsearch-js/issues/1796
dynamic: 'true',
Expand Down Expand Up @@ -1905,6 +1910,30 @@ describe('migration actions', () => {
}
`);
});
it('resolves left index_not_found_exception if the index does not exist and useAliasToPreventAutoCreate=true', async () => {
const newDocs = [
{ _source: { title: 'doc 5' } },
{ _source: { title: 'doc 6' } },
{ _source: { title: 'doc 7' } },
] as unknown as SavedObjectsRawDoc[];
await expect(
bulkOverwriteTransformedDocuments({
client,
index: 'existing_index_with_docs_alias_that_does_not_exist',
useAliasToPreventAutoCreate: true,
operations: newDocs.map((doc) => createBulkIndexOperationTuple(doc)),
refresh: 'wait_for',
})()
).resolves.toMatchInlineSnapshot(`
Object {
"_tag": "Left",
"left": Object {
"index": "existing_index_with_docs_alias_that_does_not_exist",
"type": "index_not_found_exception",
},
}
`);
});
it('resolves left target_index_had_write_block if there are write_block errors', async () => {
const newDocs = [
{ _source: { title: 'doc 5' } },
Expand Down