Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ describe('validateTypeMigrations', () => {
});

expect(() => validate({ type, kibanaVersion: '3.2.3' })).toThrowErrorMatchingInlineSnapshot(
`"Type foo: Uusing modelVersions requires to specify switchToModelVersionAt"`
`"Type foo: Using modelVersions requires to specify switchToModelVersionAt"`
);
});

Expand Down Expand Up @@ -234,6 +234,15 @@ describe('validateTypeMigrations', () => {
`"Type foo: gaps between model versions aren't allowed (missing versions: 2,4,5)"`
);
});

it('does not throw passing an empty model version map', () => {
const type = createType({
name: 'foo',
modelVersions: {},
});

expect(() => validate({ type, kibanaVersion: '3.2.3' })).not.toThrow();
});
});

describe('modelVersions mapping additions', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,45 +73,47 @@ export function validateTypeMigrations({
const modelVersionMap =
typeof type.modelVersions === 'function' ? type.modelVersions() : type.modelVersions ?? {};

if (Object.keys(modelVersionMap).length > 0 && !type.switchToModelVersionAt) {
throw new Error(
`Type ${type.name}: Uusing modelVersions requires to specify switchToModelVersionAt`
);
}
if (Object.keys(modelVersionMap).length > 0) {
if (!type.switchToModelVersionAt) {
throw new Error(
`Type ${type.name}: Using modelVersions requires to specify switchToModelVersionAt`
);
}
Comment on lines +76 to +81
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed a bug causing the validation to throw if an empty model version map was defined

Copy link
Copy Markdown
Contributor

@TinaHeiligers TinaHeiligers May 31, 2023

Choose a reason for hiding this comment

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

This will throw for types that haven't switched to models on the currently running version yet. How do we handle that if a type doesn't need a migration when the switch happens? Will we not allow them to fall back to the most recent (non-model) migration?

The specific scenario I'm thinking about is:

  • Current kibana version: 8.11
  • switchToModelVersionsAt: 8.10
  • Most recent type migration implemented at 8.8. ie. The type didn’t need new migrations, only “legacy” ones.
  • We import a SO of that type from 8.7. (that's still allowed, right?)

On import, we run through migrations to get the existing object (on 8.7) to the current version (8.11). The object needs to run through the 8.8 migration but 8.8 < 8.10 (when the switch happened) and we don’t have a model version. How do we handle that?

I'm concerned that we're forcing folks to add a model when we force the switch over, even if the switch is postponed. Unless I'm missing something, we should have pre-emptively initialized a no-op as a model when we set the switch version. We didn't in #158267, an oversight?

Copy link
Copy Markdown
Contributor Author

@pgayvallet pgayvallet Jun 1, 2023

Choose a reason for hiding this comment

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

This will throw for types that haven't switched to models on the currently running version yet. How do we handle that if a type doesn't need a migration when the switch happens?

No it won't. It's just that it was previously throwing when modelVersions: {} was defined when switchToModelVersionsAt wasn't, and it won't anymore.

You never were forced to define a model version when you switch. It's just that

  • you can't register "old" migrations for versions equal or higher to switchToModelVersionsAt, if set
  • you can't register modelVersions if switchToModelVersionsAt is not set

Copy link
Copy Markdown
Contributor Author

@pgayvallet pgayvallet Jun 1, 2023

Choose a reason for hiding this comment

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

The object needs to run through the 8.8 migration but 8.8 < 8.10 (when the switch happened) and we don’t have a model version. How do we handle that?

As we were doing previously. The document will be upgraded to 8.8 given it's the latest register migration.

Keep in mind that the document migrator doesn't know or care about model versions. When model versions are registered, internally this part of the code only knows that some 10.x.0 migrations are present, and apply the exact same logic to them as it was previously doing.

FWIW your scenario is the exact same than if a customer was trying to import a 8.4 document into a 8.9 stack where the latest version of the type of the document is 8.6: it will upgrade the doc to 8.6 during import, and set the typeMigrationVersion accordingly to 8.6. So yes, this is already, and still will be, supported.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will upgrade the doc to 8.6 during import, and set the typeMigrationVersion accordingly to 8.6. So yes, this is already, and still will be, supported.

Gold to my eyes! Thank you!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will upgrade the doc to 8.6 during import, and set the typeMigrationVersion accordingly to 8.6. So yes, this is already, and still will be, supported.

Gold to my eyes! Thank you!


Object.entries(modelVersionMap).forEach(([version, definition]) => {
assertValidModelVersion(version);
});
Object.entries(modelVersionMap).forEach(([version, definition]) => {
assertValidModelVersion(version);
});

const { min: minVersion, max: maxVersion } = Object.keys(modelVersionMap).reduce(
(minMax, rawVersion) => {
const version = Number.parseInt(rawVersion, 10);
minMax.min = Math.min(minMax.min, version);
minMax.max = Math.max(minMax.max, version);
return minMax;
},
{ min: Infinity, max: -Infinity }
);
const { min: minVersion, max: maxVersion } = Object.keys(modelVersionMap).reduce(
(minMax, rawVersion) => {
const version = Number.parseInt(rawVersion, 10);
minMax.min = Math.min(minMax.min, version);
minMax.max = Math.max(minMax.max, version);
return minMax;
},
{ min: Infinity, max: -Infinity }
);

if (minVersion > 1) {
throw new Error(`Type ${type.name}: model versioning must start with version 1`);
}
if (minVersion > 1) {
throw new Error(`Type ${type.name}: model versioning must start with version 1`);
}

validateAddedMappings(type.name, type.mappings, modelVersionMap);
validateAddedMappings(type.name, type.mappings, modelVersionMap);

const missingVersions = getMissingVersions(
minVersion,
maxVersion,
Object.keys(modelVersionMap).map((v) => Number.parseInt(v, 10))
);
if (missingVersions.length) {
throw new Error(
`Type ${
type.name
}: gaps between model versions aren't allowed (missing versions: ${missingVersions.join(
','
)})`
const missingVersions = getMissingVersions(
minVersion,
maxVersion,
Object.keys(modelVersionMap).map((v) => Number.parseInt(v, 10))
);
if (missingVersions.length) {
throw new Error(
`Type ${
type.name
}: gaps between model versions aren't allowed (missing versions: ${missingVersions.join(
','
)})`
);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

module.exports = {
// TODO replace the line below with
// preset: '@kbn/test/jest_integration_node
// to do so, we must fix all integration tests first
// see https://github.com/elastic/kibana/pull/130255/
preset: '@kbn/test/jest_integration',
rootDir: '../../../../../../..',
roots: ['<rootDir>/src/core/server/integration_tests/saved_objects/migrations/group4'],
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Created a 4th group for the v2 integration tests

// must override to match all test given there is no `integration_tests` subfolder
testMatch: ['**/*.test.{js,mjs,ts,tsx}'],
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import Path from 'path';
import fs from 'fs/promises';
import { range, sortBy } from 'lodash';
import { type TestElasticsearchUtils } from '@kbn/core-test-helpers-kbn-server';
import { SavedObjectsBulkCreateObject } from '@kbn/core-saved-objects-api-server';
import { modelVersionToVirtualVersion } from '@kbn/core-saved-objects-base-server-internal';
import '../jest_matchers';
import { getKibanaMigratorTestKit, startElasticsearch } from '../kibana_migrator_test_kit';
import { delay, createType, parseLogFile } from '../test_utils';
import { getBaseMigratorParams } from '../fixtures/zdt_base.fixtures';

const logFilePath = Path.join(__dirname, 'v2_with_mv_same_stack_version.test.log');

const NB_DOCS_PER_TYPE = 25;

describe('V2 algorithm - using model versions - upgrade without stack version increase', () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Standard upgrade scenario where the stack version doesn't change (only the model versions of the registered types change)

let esServer: TestElasticsearchUtils['es'];

beforeAll(async () => {
await fs.unlink(logFilePath).catch(() => {});
esServer = await startElasticsearch();
});

afterAll(async () => {
await esServer?.stop();
await delay(10);
});

const getTestModelVersionType = ({ beforeUpgrade }: { beforeUpgrade: boolean }) => {
const type = createType({
name: 'test_mv',
namespaceType: 'single',
migrations: {},
switchToModelVersionAt: '8.8.0',
modelVersions: {
1: {
changes: [],
},
},
mappings: {
properties: {
field1: { type: 'text' },
field2: { type: 'text' },
},
},
});

if (!beforeUpgrade) {
Object.assign<typeof type, Partial<typeof type>>(type, {
modelVersions: {
...type.modelVersions,
2: {
changes: [
{
type: 'mappings_addition',
addedMappings: {
field3: { type: 'text' },
},
},
{
type: 'data_backfill',
transform: (document) => {
document.attributes.field3 = 'test_mv-backfilled';
return { document };
},
},
],
},
},
mappings: {
...type.mappings,
properties: {
...type.mappings.properties,
field3: { type: 'text' },
},
},
});
}

return type;
};

const createBaseline = async () => {
const { runMigrations, savedObjectsRepository } = await getKibanaMigratorTestKit({
...getBaseMigratorParams({
migrationAlgorithm: 'v2',
kibanaVersion: '8.8.0',
}),
types: [getTestModelVersionType({ beforeUpgrade: true })],
});
await runMigrations();

const mvObjs = range(NB_DOCS_PER_TYPE).map<SavedObjectsBulkCreateObject>((number) => ({
id: `mv-${String(number).padStart(3, '0')}`,
type: 'test_mv',
attributes: {
field1: `f1-${number}`,
field2: `f2-${number}`,
},
}));

await savedObjectsRepository.bulkCreate(mvObjs);
};

it('migrates the documents', async () => {
await createBaseline();

const modelVersionType = getTestModelVersionType({ beforeUpgrade: false });

const { runMigrations, client, savedObjectsRepository } = await getKibanaMigratorTestKit({
...getBaseMigratorParams({ migrationAlgorithm: 'v2', kibanaVersion: '8.8.0' }),
logFilePath,
types: [modelVersionType],
});
await runMigrations();

const indices = await client.indices.get({ index: '.kibana*' });
expect(Object.keys(indices)).toEqual(['.kibana_8.8.0_001']);

const index = indices['.kibana_8.8.0_001'];
const mappings = index.mappings ?? {};
const mappingMeta = mappings._meta ?? {};

expect(mappings.properties).toEqual(
expect.objectContaining({
test_mv: modelVersionType.mappings,
})
);

expect(mappingMeta).toEqual({
indexTypesMap: {
'.kibana': ['test_mv'],
},
migrationMappingPropertyHashes: expect.any(Object),
});

const { saved_objects: testMvDocs } = await savedObjectsRepository.find({
type: 'test_mv',
perPage: 1000,
});

expect(testMvDocs).toHaveLength(NB_DOCS_PER_TYPE);

const testMvData = sortBy(testMvDocs, 'id').map((object) => ({
id: object.id,
type: object.type,
attributes: object.attributes,
version: object.typeMigrationVersion,
}));

expect(testMvData).toEqual(
range(NB_DOCS_PER_TYPE).map((number) => ({
id: `mv-${String(number).padStart(3, '0')}`,
type: 'test_mv',
attributes: {
field1: `f1-${number}`,
field2: `f2-${number}`,
field3: 'test_mv-backfilled',
},
version: modelVersionToVirtualVersion(2),
}))
);

const records = await parseLogFile(logFilePath);
expect(records).toContainLogEntries(
[
'INIT -> WAIT_FOR_YELLOW_SOURCE',
'CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES',
'Migration completed',
],
{
ordered: true,
}
);
});
});
Loading