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.

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

Adapt the ZDT migration algorithm to support v2 migrations #155981

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Apr 27, 2023

Summary

Part of #150309

Adapt the zdt migration algorithm to run the v2 migrations in addition to the model version transformations.

The intent is to be able to use the current migration system in a zero-downtime upgrade friendly-ish way

Technicals

#148656 was a precondition, as the zdt algo requires that mapping changes are compatible (given we keep the same index).

Technically, it means we're now storing the virtualVersion instead of the modelVersion in the index's meta (mappingVersions and docVersions), to allow keeping track of mixed stack and model versions per type.

Note that switching to model versioning is still a one way, non-revertible action. Once using model versioning (by specifying switchToModelVersionAt on your type), there is no going back.

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes Feature:Migrations v8.9.0 labels Apr 27, 2023
Copy link
Contributor Author

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Self-review

Comment on lines +102 to +103
/** @internal */
export type IndexMappingMeta = V2AlgoIndexMappingMeta & ZdtAlgoIndexMappingMeta;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got tired of having to check which properties are used by each algo everytime I read the type, so it's now split (still using an union though, otherwise we would need to manually specify which one we're using everytime)

Comment on lines +27 to +31
const virtualVersion = virtualVersions[type.name];
return {
bool: {
must: [
{ term: { type: type.name } },
{
bool: {
should: [
{
bool: {
must: { exists: { field: 'migrationVersion' } },
must_not: { term: { [`migrationVersion.${type.name}`]: virtualVersion } },
},
},
{
bool: {
must_not: [
{ exists: { field: 'migrationVersion' } },
{ term: { typeMigrationVersion: virtualVersion } },
],
},
},
],
},
},
],
must: [{ term: { type: type.name } }],
must_not: [{ term: { typeMigrationVersion: virtualVersion } }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The query was copied from the v2 algo, but we effectively don't need to support the old migrationVersion.{type} syntax given that zdt will be introduced this change, so I cleaned that up.

Comment on lines +16 to +17
roots: ['<rootDir>/src/core/server/integration_tests/saved_objects/migrations/zdt_v2_compat'],
// must override to match all test given there is no `integration_tests` subfolder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a dedicated suite for the v2 compat tests. The reasoning was that we may eventually want to get rid of the support later, so having dedicated test suites would make the removal easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking the same thing: Isolate our newest changes in new CI groups.

@pgayvallet pgayvallet marked this pull request as ready for review May 1, 2023 12:06
@pgayvallet pgayvallet requested a review from a team as a code owner May 1, 2023 12:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @pgayvallet , a lot of comments stem from missing some parts of the context in which these changes were made, but I think I could piece most of them together. FWIW, perhaps some of my questions can translate into doc comments that could help other devs grok how the different parts interact.

Comment on lines +230 to +235
migrations: {
'7.17.2': dummyMigration,
'8.6.0': dummyMigration,
},
modelVersions: {
1: dummyModelVersion(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to make sure I understand this correctly, but what is the plan moving forward for these two sets of values? Will consumers specify when they are switching to model versions and then we will assume all "virtual" version migrations should be run first after which the "model versions" take over?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will consumers specify when they are switching to model versions

Correct, via switchToModelVersionAt


/**
* Returns the current virtual version for the given type.
* If will either be the latest model version if the type
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If will either be the latest model version if the type
* It will either be the latest model version if the type


export type ModelVersionMap = Record<string, number>;
export type VirtualVersion = string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a doc comment to this type explaining what VirtualVersion is, perhaps in relation to ModelVersion?

Comment on lines 18 to 23
const meta: IndexMappingMeta = {
mappingVersions: {
foo: 1,
bar: 1,
foo: '10.1.0',
bar: '10.1.0',
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Exciting (and a little scary) times! Will we, at some point in time, potentially have mixed state of VirtualVersions and "regular" stack versions in this map?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel like the "missing link" for my understanding is the intended co-existence of these values in the future and what form that will take specifically.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I think I got the answer to my own q when reading this test:

https://github.com/elastic/kibana/blob/8a2e3e147fd2fdf2373932b3127c4e3fa5c2606b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/utils/outdated_documents_query.test.ts

VirtualVersion is a "special" major combined with model version as minor - fully decoupled from stack (or so it seems).

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, virtual versions is a compatibility format between stack versions and model versions, making the assumption that we will no longer be using stack versioning before this arbitrary 10 version, that serves as a delimiter / identifier for model versions (10.{modelVersion}.0)

@pgayvallet pgayvallet enabled auto-merge (squash) May 2, 2023 10:32
@pgayvallet pgayvallet merged commit 9cddb60 into elastic:main May 2, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-saved-objects-base-server-internal 52 55 +3
Unknown metric groups

API count

id before after diff
@kbn/core-saved-objects-base-server-internal 72 79 +7

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 399 402 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 479 482 +3
total +5

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 2, 2023
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request May 2, 2023
…55981)

## Summary

Adapt the `zdt` migration algorithm to run the v2 migrations in addition
to the model version transformations.

The intent is to be able to use the current migration system in a
zero-downtime upgrade friendly-ish way

### Technicals  

elastic#148656 was a precondition, as the
zdt algo requires that mapping changes are compatible (given we keep the
same index).

Technically, it means we're now storing the `virtualVersion` instead of
the `modelVersion` in the index's meta (`mappingVersions` and
`docVersions`), to allow keeping track of mixed stack and model versions
per type.

Note that switching to model versioning is still a one way,
non-revertible action. Once using model versioning (by specifying
`switchToModelVersionAt` on your type), there is no going back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Migrations release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants