Skip to content

Check diffMappings after WAIT_FOR_YELLOW_SOURCE #148980

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

Merged
gsoldevila merged 3 commits into from
Jan 17, 2023
Merged

Check diffMappings after WAIT_FOR_YELLOW_SOURCE #148980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
074dc2b
Check diffMappings after WAIT_FOR_YELLOW_SOURCE
gsoldevila Jan 16, 2023
4488dcc
it => test
gsoldevila Jan 16, 2023
738b8e4
Update integration test
gsoldevila Jan 16, 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
219 changes: 129 additions & 90 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 @@ -652,6 +652,11 @@ describe('migrations v2 model', () => {

expect(newState.controlState).toBe('WAIT_FOR_YELLOW_SOURCE');
expect(newState.sourceIndex.value).toBe('.kibana_7.invalid.0_001');
expect(newState.aliases).toEqual({
'.kibana': '.kibana_7.invalid.0_001',
'.kibana_7.11.0': '.kibana_7.11.0_001',
'.kibana_7.12.0': '.kibana_7.invalid.0_001',
});
});

test('INIT -> WAIT_FOR_YELLOW_SOURCE when migrating from a v2 migrations index (>= 7.11.0)', () => {
Expand Down Expand Up @@ -683,6 +688,10 @@ describe('migrations v2 model', () => {
expect(newState.sourceIndex.value).toBe('.kibana_7.11.0_001');
expect(newState.retryCount).toEqual(0);
expect(newState.retryDelay).toEqual(0);
expect(newState.aliases).toEqual({
'.kibana': '.kibana_7.11.0_001',
'.kibana_7.11.0': '.kibana_7.11.0_001',
});
});

test('INIT -> WAIT_FOR_YELLOW_SOURCE when migrating from a v1 migrations index (>= 6.5 < 7.11.0)', () => {
Expand All @@ -701,6 +710,9 @@ describe('migrations v2 model', () => {
expect(newState.sourceIndex.value).toBe('.kibana_3');
expect(newState.retryCount).toEqual(0);
expect(newState.retryDelay).toEqual(0);
expect(newState.aliases).toEqual({
'.kibana': '.kibana_3',
});
});
test('INIT -> LEGACY_SET_WRITE_BLOCK when migrating from a legacy index (>= 6.0.0 < 6.5)', () => {
const res: ResponseType<'INIT'> = Either.right({
Expand Down Expand Up @@ -807,67 +819,6 @@ describe('migrations v2 model', () => {
expect(newState.retryCount).toEqual(0);
expect(newState.retryDelay).toEqual(0);
});

describe('when upgrading to a new stack version', () => {
const unchangedMappingsState: State = {
...baseState,
controlState: 'INIT',
kibanaVersion: '7.12.0', // new version!
currentAlias: '.kibana',
versionAlias: '.kibana_7.12.0',
versionIndex: '.kibana_7.11.0_001',
};
it('INIT -> PREPARE_COMPATIBLE_MIGRATION when the mappings have not changed', () => {
const res: ResponseType<'INIT'> = Either.right({
'.kibana_7.11.0_001': {
aliases: {
'.kibana': {},
'.kibana_7.11.0': {},
},
mappings: indexMapping,
settings: {},
},
});
const newState = model(unchangedMappingsState, res) as PrepareCompatibleMigration;

expect(newState.controlState).toEqual('PREPARE_COMPATIBLE_MIGRATION');
expect(newState.targetIndexRawMappings).toEqual({
_meta: {
migrationMappingPropertyHashes: {
new_saved_object_type: '4a11183eee21e6fbad864f7a30b39ad0',
},
},
properties: {
new_saved_object_type: {
properties: {
value: {
type: 'text',
},
},
},
},
});
expect(newState.versionAlias).toEqual('.kibana_7.12.0');
expect(newState.currentAlias).toEqual('.kibana');
// will point to
expect(newState.targetIndex).toEqual('.kibana_7.11.0_001');
expect(newState.preTransformDocsActions).toEqual([
{
add: {
alias: '.kibana_7.12.0',
index: '.kibana_7.11.0_001',
},
},
{
remove: {
alias: '.kibana_7.11.0',
index: '.kibana_7.11.0_001',
must_exist: true,
},
},
]);
});
});
});
});

Expand Down Expand Up @@ -1259,25 +1210,129 @@ describe('migrations v2 model', () => {
});

describe('WAIT_FOR_YELLOW_SOURCE', () => {
const someMappings = {
properties: {},
} as const;

const waitForYellowSourceState: WaitForYellowSourceState = {
...baseState,
controlState: 'WAIT_FOR_YELLOW_SOURCE',
sourceIndex: Option.some('.kibana_3') as Option.Some<string>,
sourceIndexMappings: someMappings,
sourceIndex: Option.some('.kibana_7.11.0_001') as Option.Some<string>,
sourceIndexMappings: baseState.targetIndexMappings,
aliases: {
'.kibana': '.kibana_7.11.0_001',
'.kibana_7.11.0': '.kibana_7.11.0_001',
},
};

test('WAIT_FOR_YELLOW_SOURCE -> CHECK_UNKNOWN_DOCUMENTS if action succeeds', () => {
const res: ResponseType<'WAIT_FOR_YELLOW_SOURCE'> = Either.right({});
const newState = model(waitForYellowSourceState, res);
expect(newState.controlState).toEqual('CHECK_UNKNOWN_DOCUMENTS');
describe('if action succeeds', () => {
test('it resets retry count and delay', () => {
const res: ResponseType<'WAIT_FOR_YELLOW_SOURCE'> = Either.right({});
const testState = {
...waitForYellowSourceState,
retryCount: 1,
retryDelay: 2000,
};
const newState = model(testState, res);
expect(newState.retryCount).toEqual(0);
expect(newState.retryDelay).toEqual(0);
});

expect(newState).toMatchObject({
controlState: 'CHECK_UNKNOWN_DOCUMENTS',
sourceIndex: Option.some('.kibana_3'),
describe('and mappings match (diffMappings == false)', () => {
const unchangedMappingsState: State = {
...waitForYellowSourceState,
controlState: 'WAIT_FOR_YELLOW_SOURCE',
kibanaVersion: '7.12.0', // new version!
currentAlias: '.kibana',
versionAlias: '.kibana_7.12.0',
versionIndex: '.kibana_7.11.0_001',
};

test('WAIT_FOR_YELLOW_SOURCE -> PREPARE_COMPATIBLE_MIGRATION', () => {
const res: ResponseType<'WAIT_FOR_YELLOW_SOURCE'> = Either.right({
'.kibana_7.11.0_001': {
aliases: {
'.kibana': {},
'.kibana_7.11.0': {},
},
mappings: indexMapping,
settings: {},
},
});
const newState = model(unchangedMappingsState, res) as PrepareCompatibleMigration;

expect(newState.controlState).toEqual('PREPARE_COMPATIBLE_MIGRATION');
expect(newState.targetIndexRawMappings).toEqual({
_meta: {
migrationMappingPropertyHashes: {
new_saved_object_type: '4a11183eee21e6fbad864f7a30b39ad0',
},
},
properties: {
new_saved_object_type: {
properties: {
value: {
type: 'text',
},
},
},
},
});
expect(newState.versionAlias).toEqual('.kibana_7.12.0');
expect(newState.currentAlias).toEqual('.kibana');
// will point to
expect(newState.targetIndex).toEqual('.kibana_7.11.0_001');
expect(newState.preTransformDocsActions).toEqual([
{
add: {
alias: '.kibana_7.12.0',
index: '.kibana_7.11.0_001',
},
},
{
remove: {
alias: '.kibana_7.11.0',
index: '.kibana_7.11.0_001',
must_exist: true,
},
},
]);
});
});

describe('and mappings DO NOT match (diffMappings == true)', () => {
const actualMappings: IndexMapping = {
properties: {
new_saved_object_type: {
properties: {
value: { type: 'integer' },
},
},
},
_meta: {
migrationMappingPropertyHashes: {
new_saved_object_type: '5b11183eee21e6fbad864f7a30b39be1',
},
},
};

const changedMappingsState: State = {
...waitForYellowSourceState,
controlState: 'WAIT_FOR_YELLOW_SOURCE',
kibanaVersion: '7.12.0', // new version!
currentAlias: '.kibana',
versionAlias: '.kibana_7.12.0',
versionIndex: '.kibana_7.11.0_001',
sourceIndexMappings: actualMappings,
};

test('WAIT_FOR_YELLOW_SOURCE -> CHECK_UNKNOWN_DOCUMENTS', () => {
const res: ResponseType<'WAIT_FOR_YELLOW_SOURCE'> = Either.right({});
const newState = model(changedMappingsState, res);
expect(newState.controlState).toEqual('CHECK_UNKNOWN_DOCUMENTS');

expect(newState).toMatchObject({
controlState: 'CHECK_UNKNOWN_DOCUMENTS',
sourceIndex: Option.some('.kibana_7.11.0_001'),
sourceIndexMappings: actualMappings,
});
});
});
});

Expand All @@ -1297,22 +1352,6 @@ describe('migrations v2 model', () => {
}
`);
});

test('WAIT_FOR_YELLOW_SOURCE -> CHECK_UNKNOWN_DOCUMENTS resets retry count and delay if action succeeds', () => {
const res: ResponseType<'WAIT_FOR_YELLOW_SOURCE'> = Either.right({});
const testState = {
...waitForYellowSourceState,
retryCount: 1,
retryDelay: 2000,
};
const newState = model(testState, res);
expect(newState.controlState).toEqual('CHECK_UNKNOWN_DOCUMENTS');

expect(newState).toMatchObject({
controlState: 'CHECK_UNKNOWN_DOCUMENTS',
sourceIndex: Option.some('.kibana_3'),
});
});
});

describe('CHECK_UNKNOWN_DOCUMENTS', () => {
Expand Down
86 changes: 46 additions & 40 deletions packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,48 +158,14 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
},
],
};
} else if (
// source exists
Boolean(indices[source!]?.mappings?._meta?.migrationMappingPropertyHashes) &&
// ...and mappings are unchanged
!diffMappings(
/* actual */
indices[source!].mappings,
/* expected */
stateP.targetIndexMappings
)
) {
const targetIndex = source!;
const sourceMappings = indices[source!].mappings;

return {
...stateP,
controlState: 'PREPARE_COMPATIBLE_MIGRATION',
sourceIndex: Option.none,
targetIndex,
targetIndexRawMappings: sourceMappings,
targetIndexMappings: mergeMigrationMappingPropertyHashes(
stateP.targetIndexMappings,
sourceMappings
),
preTransformDocsActions: [
// Point the version alias to the source index. This let's other Kibana
// instances know that a migration for the current version is "done"
// even though we may be waiting for document transformations to finish.
{ add: { index: source!, alias: stateP.versionAlias } },
...buildRemoveAliasActions(source!, Object.keys(aliases), [
stateP.currentAlias,
stateP.versionAlias,
]),
],
versionIndexReadyActions: Option.none,
};
} else if (
// If the `.kibana` alias exists
source != null
) {
// CHECKPOINT here we decide to go for yellow source
return {
...stateP,
aliases,
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens Jan 16, 2023

Choose a reason for hiding this comment

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

Just wanted to get your thoughts:

By passing aliases here it seems more likely that this object could be stale if other Kibana's see yellow source before us. We would reach the PREPARE_COMPATIBLE_MIGRATION step, get an alias_not_found exception and still go to OUTDATED_DOCUMENTS_SEARCH_OPEN_PIT. So all good?

Copy link
Copy Markdown
Member Author

@gsoldevila gsoldevila Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Fair point!

The only way I see to get fresher aliases information would be to perform a GET .kibana again (like the INIT step does), and even in that case, there's no way to guarantee that another instance will not delete the aliases before us, so I'm not sure it is worth it.

  • If the other instance is the same version, and it performs the same cleanup of aliases before us, it's alright to proceed to OUTDATED_DOCUMENTS_SEARCH_OPEN_PIT on alias_not_found. As you said, odds of this happening are increasing with this fix, but the behavior / handling is still correct IMHO.

  • If the other instance is a more recent version, it will obtain the aliases in the INIT step:

    • if it obtains them before we update / delete them in the PREPARE_COMPATIBLE_MIGRATION step, it will proceed to CHECK_UNKNOWN_DOCUMENTS + all the reindex steps, which don't use the aliases anymore.
    • if it obtains them after we update / delete them in the PREPARE_COMPATIBLE_MIGRATION step, it will reindex the source index into a new index without using the alias for anything. Then, it would likely override our .kibana alias and make it point to the newly created index. In this case we'd have some garbage version aliases pointing to an older index version, but that's already the case without this fix.

Copy link
Copy Markdown
Contributor

@rudolf rudolf Jan 16, 2023

Choose a reason for hiding this comment

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

++ there's always a potential race condition between the time we get the fetchAliases respose and performing the preTransformDocsActions. Before it was probably in the order of < 100ms and now it could potentially be many minutes. So this makes any potential race conditions more likely to occur but doesn't introduce anything new.

Like you both said, any race conditions should be handled in PREPARE_COMPATIBLE_MIGRATION already.

controlState: 'WAIT_FOR_YELLOW_SOURCE',
sourceIndex: Option.some(source!) as Option.Some<string>,
sourceIndexMappings: indices[source!].mappings,
Expand Down Expand Up @@ -481,10 +447,50 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
} else if (stateP.controlState === 'WAIT_FOR_YELLOW_SOURCE') {
const res = resW as ExcludeRetryableEsError<ResponseType<typeof stateP.controlState>>;
if (Either.isRight(res)) {
return {
...stateP,
controlState: 'CHECK_UNKNOWN_DOCUMENTS',
};
// check the existing mappings to see if we can avoid reindexing
if (
// source exists
Boolean(stateP.sourceIndexMappings._meta?.migrationMappingPropertyHashes) &&
// ...and mappings are unchanged
!diffMappings(
/* actual */
stateP.sourceIndexMappings,
/* expected */
stateP.targetIndexMappings
)
) {
// The source index .kibana is pointing to. E.g: ".xx8.7.0_001"
const source = stateP.sourceIndex.value;

return {
...stateP,
controlState: 'PREPARE_COMPATIBLE_MIGRATION',
sourceIndex: Option.none,
targetIndex: source!,
targetIndexRawMappings: stateP.sourceIndexMappings,
targetIndexMappings: mergeMigrationMappingPropertyHashes(
stateP.targetIndexMappings,
stateP.sourceIndexMappings
),
preTransformDocsActions: [
// Point the version alias to the source index. This let's other Kibana
// instances know that a migration for the current version is "done"
// even though we may be waiting for document transformations to finish.
{ add: { index: source!, alias: stateP.versionAlias } },
...buildRemoveAliasActions(source!, Object.keys(stateP.aliases), [
stateP.currentAlias,
stateP.versionAlias,
]),
],
versionIndexReadyActions: Option.none,
};
} else {
// the mappings have changed, but changes might still be compatible
return {
...stateP,
controlState: 'CHECK_UNKNOWN_DOCUMENTS',
};
}
} else if (Either.isLeft(res)) {
const left = res.left;
if (isTypeof(left, 'index_not_yellow_timeout')) {
Expand Down
1 change: 1 addition & 0 deletions 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 @@ -215,6 +215,7 @@ export interface WaitForYellowSourceState extends BaseState {
readonly controlState: 'WAIT_FOR_YELLOW_SOURCE';
readonly sourceIndex: Option.Some<string>;
readonly sourceIndexMappings: IndexMapping;
readonly aliases: Record<string, string | undefined>;
}

export interface CheckUnknownDocumentsState extends BaseState {
Expand Down
3 changes: 2 additions & 1 deletion src/core/server/integration_tests/saved_objects/migrations/skip_reindex.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ describe('skip reindexing', () => {

logs = await fs.readFile(logFilePath, 'utf-8');

expect(logs).toMatch('INIT -> PREPARE_COMPATIBLE_MIGRATION');
expect(logs).toMatch('INIT -> WAIT_FOR_YELLOW_SOURCE');
expect(logs).toMatch('WAIT_FOR_YELLOW_SOURCE -> PREPARE_COMPATIBLE_MIGRATION');
expect(logs).toMatch('PREPARE_COMPATIBLE_MIGRATION -> OUTDATED_DOCUMENTS_SEARCH_OPEN_PIT');
expect(logs).toMatch('CHECK_TARGET_MAPPINGS -> CHECK_VERSION_INDEX_READY_ACTIONS');
expect(logs).toMatch('CHECK_VERSION_INDEX_READY_ACTIONS -> DONE');
Expand Down