[ZDT] DocumentMigrator: support higher version documents #157895

pgayvallet · 2023-05-16T15:48:28Z

Summary

Part of #150312
(next steps depend on #153117)

This PR does two things:

introduce the concept of version persistence schema
adapt the document migrator to support downward migrations for documents of an higher version.

In the follow-up, we will then update the calls from the SOR to the document migrator to allow downward conversions when we're using the ZDT migration algorithm (which requires #153117 to be merged)

Model version persistence schema.

(This is what has also been named 'eviction schema' or 'known fields schema'.)

A new SavedObjectsModelVersion.schemas.backwardConversion property was added to the model version definition.

This 'schema' can either be an arbitrary function, or a schema.object from @kbn/config-schema

type SavedObjectModelVersionBackwardConversionSchema<
  InAttrs = unknown,
  OutAttrs = unknown
> = ObjectType | SavedObjectModelVersionBackwardConversionFn<InAttrs, OutAttrs>;

When specified for a version, the document's attributes will go thought this schema during down conversions by the document migrator.

Adapt the document migrator to support downward migrations for documents of an higher version.

Add an allowDowngrade option to DocumentMigrator.migrate and KibanaMigrator.migrateDocument. When this option is set to true, the document migration will accept to 'downgrade' the document if necessary, instead of throwing an error as done when the option is false or unspecified (which was the only behavior prior to this PR's changes)

…-ref HEAD~1..HEAD --fix'

…nd-schema

pgayvallet

Self-review

Note: now that #153117 is merged, I could do the next steps (leveraging the change from the SOR) in the same PR. However I think it makes sense to validate the document migrator changes / API design first before going further, which is why I'm planning on doing the rest in a distinct PR.

pgayvallet · 2023-05-22T11:50:45Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

+   *
+   * See {@link SavedObjectModelVersionBackwardConversionSchema} for more info.
+   */
+  backwardConversion?: SavedObjectModelVersionBackwardConversionSchema;


I know naming is hard, but it was even harder here. I'm still really not sure how we should name that thing.

I started with persistenceSchema, but it wasn't explicit enough, so I switched to backwardPersistence, higherVersionConversion and then backwardConversion... But nothing seems outstandingly correct here.

If anyone has a better idea, please voice your opinion...

Throwing ideas: versionedModelSchema?

reverseConversion?

pgayvallet · 2023-05-22T11:53:23Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

+export type SavedObjectModelVersionBackwardConversionSchema<
+  InAttrs = unknown,
+  OutAttrs = unknown
+> = ObjectType | SavedObjectModelVersionBackwardConversionFn<InAttrs, OutAttrs>;


We can probably improve this later (e.g with helper methods / factory to build the validation function from another shape of schema or something), but I just KISSed for now. The schema can either be a schema.object of the properties of the document, or a plain JS function taking the attributes as input and returning the filtered / processed attributes as output.

pgayvallet · 2023-05-22T11:54:08Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

 */
-export interface SavedObjectsModelVersionSchemaDefinitions {
-  create?: SavedObjectModelVersionCreateSchema;


NIT: I removed the created schema from SavedObjectsModelVersionSchemaDefinitions given it's unused for now (and now that we have an other, used, schema on this type)

pgayvallet · 2023-05-22T12:07:48Z

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

+    const typeMigrations = this.migrations[doc.type];
+    if (downgradeRequired(doc, typeMigrations?.latestVersion ?? {})) {
+      const currentVersion = doc.typeMigrationVersion ?? doc.migrationVersion?.[doc.type];
+      const latestVersion = this.migrations[doc.type].latestVersion[TransformType.Migrate];
+      if (!allowDowngrade) {
+        throw Boom.badData(
+          `Document "${doc.id}" belongs to a more recent version of Kibana [${currentVersion}] when the last known version is [${latestVersion}].`
+        );
+      }
+      return this.transformDown(doc, { targetTypeVersion: latestVersion! });
+    } else {
+      return this.transformUp(doc, { convertNamespaceTypes, allowDowngrade });
+    }


One of the root change of the PR: DocumentMigrator.migrate (and the underlying .transform) now accepts a allowDowngrade option.

If true, the document will go through the downgrade pipeline instead of throwing an error if the provided document is at an higher version than the last knows by the system.

pgayvallet · 2023-05-22T12:10:34Z

...core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.test.ts

+  describe('down migration', () => {
+    it('accepts to downgrade the document if `allowDowngrade` is true', () => {
+      const registry = createRegistry({});
+
+      const fooType = createType({


NIT: only smoke testing in this file, as the actual coverage is done in the down pipeline's tests

pgayvallet · 2023-05-22T12:15:09Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

      if (!transform.transformDown) {
+        if (this.ignoreMissingTransforms) {
+          continue;
+        }


I decided to reuse the existing downgrade pipeline, even if, realistically given our latest decisions regarding down migrations, we could just rewrite it to only apply the version schema if present.

The reasoning was that

the system is already in place and tested

we may eventually need such downward transformations

the perf impact is negligible given we will just not apply down transforms that are not here.

The only thing is that I had to add this ignoreMissingTransforms option to change the behavior of the down pipeline to now throw when encountering versions that did not register down transformations.

we could just rewrite it to only apply the version schema if present

So this is the version where the down pipeline will throw, but we added a flag. What's the reasoning for not just changing how the implementation works since we are not using ignoreMissingTransforms: false outside of tests?

Yeah, you are right. I will remove the option and just keep the only used behavior

pgayvallet · 2023-05-22T12:17:36Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

+  private applyVersionSchema(doc: SavedObjectUnsanitizedDoc): SavedObjectUnsanitizedDoc {
+    const targetVersion = this.targetTypeVersion;
+    const versionSchema = this.typeTransforms.versionSchemas[targetVersion];
+    if (versionSchema) {
+      return versionSchema(doc);
+    }
+    return doc;
+  }


Where the actual version schema is applied.

Note that, compare to the existing create schema (not yet ported to model versions), here we will only use the schema matching the exact version we're downgrading to. trying to find and use an older version doesn't make sense for such 'persistence shape' schema I think.

pgayvallet · 2023-05-22T12:21:04Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/kibana_migrator.ts

+  public migrateDocument(
+    doc: SavedObjectUnsanitizedDoc,
+    { allowDowngrade = false }: MigrateDocumentOptions = {}
+  ): SavedObjectUnsanitizedDoc {
+    return this.documentMigrator.migrate(doc, { allowDowngrade });


Also added the allowDowngrade option to IKibanaMigrator.migrateDocument, as this is the interface the SOR is using to access the actual document migrator.

…nd-schema

elasticmachine · 2023-05-22T12:23:30Z

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

…nd-schema

jloleysens

Still reviewing just left a comment on what I think is a mistake

jloleysens · 2023-05-22T12:54:16Z

...ore-saved-objects-migration-server-internal/src/document_migrator/build_active_migrations.ts

@@ -48,7 +48,7 @@ export function buildActiveMigrations({
      referenceTransforms,
    });

-    if (typeTransforms.transforms.length) {
+    if (!typeTransforms.transforms.length && !Object.keys(typeTransforms.versionSchemas).length) {


Not quite sure I follow the logic here:

(1) no transforms and (2) no version schemas then we add it to active migration map.

I would have thought we want (1) and (or?) (2) but I may be missing something.

Nope, you're right, I just messed up when I rebased from main. This is what is causing the whole PR to explode btw, thanks :)

jloleysens

Great work @pgayvallet , I left one non-blocker question about the thinking behind schemas, but I expect this is just me missing some context.

jloleysens · 2023-05-22T13:07:12Z

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

  }

-  private transform(
+  private transformUp(


It looks like allowDowngrade is being passed in here but not used. Shall we create an inline type for the options object in this method?

Nice catch, will fix

jloleysens · 2023-05-22T13:09:27Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

 */
-export type SavedObjectModelVersionCreateSchema = ObjectType;
+export interface SavedObjectsModelVersionSchemaDefinitions {


What other (eviction) schemas would we want to define for a model version?

eviction? None.

We will need to implement the create schema at least, but it's a more "traditional" validation schema

jloleysens · 2023-05-22T13:12:44Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

+ * Note that These conversion mechanism shouldn't assert the data itself, only strip unknown fields to convert
+ * the document to the *shape* of the document at the given version.


By shape we also mean format of the fields themselves, the idea is not to transform values -- I am a little concerned that allowing an arbitrary function opens the door for something like that. But I think this doc comment is good enough guidance.

Yeah I agree, we will need to be fairly careful when reviewing PRs adding such functions.

But given we still don't know what the best way to define these schema is, I needed something low level, which is therefor very permissive...

jloleysens · 2023-05-22T13:51:21Z

...ects/core-saved-objects-base-server-internal/src/model_version/backward_conversion_schema.ts

+
+export type ConvertedSchema = (doc: SavedObjectUnsanitizedDoc) => SavedObjectUnsanitizedDoc;
+
+export const convertModelVersionBackwardConversionSchema = (


One other idea:

getSchemaConversionFunction

jloleysens · 2023-05-22T13:56:04Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

      if (!transform.transformDown) {
+        if (this.ignoreMissingTransforms) {
+          continue;
+        }


we could just rewrite it to only apply the version schema if present

So this is the version where the down pipeline will throw, but we added a flag. What's the reasoning for not just changing how the implementation works since we are not using ignoreMissingTransforms: false outside of tests?

afharo

Great job! I added 2 comments I'd like to clarify before approving 😇

afharo · 2023-05-22T20:23:05Z

...re/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/utils.ts

+  if (!docTypeVersion || !latestMigrationVersion) {
+    return false;
+  }
+  return Semver.gt(docTypeVersion, latestMigrationVersion);


Q: is this only for V2 versions? I thought model versions didn't look like Semver, do they?

We're using a compatibility format so that both stack versions (used by old migrations) and model versions are compatible and can share the same logic

kibana/packages/core/saved-objects/core-saved-objects-base-server-internal/src/model_version/version_map.ts

Lines 13 to 23 in 9cddb60

/**

* Represents the virtual version of a given SO type.

* The virtual version is a compatibility format between the old

* migration system's versioning, based on the stack version, and the new model versioning.

*

* A virtual version is a plain semver version. Depending on its major version value, the

* underlying version can be the following:

* - Major < 10: Old migrations system (stack versions), using the equivalent value (e.g `8.7.0` => migration version `8.7.0`)

* - Major == 10: Model versions, using the `10.{modelVersion}.0` format (e.g `10.3.0` => model version 3)

*/

export type VirtualVersion = string;

afharo · 2023-05-22T20:25:54Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

+   *
+   * See {@link SavedObjectModelVersionBackwardConversionSchema} for more info.
+   */
+  backwardConversion?: SavedObjectModelVersionBackwardConversionSchema;


Throwing ideas: versionedModelSchema?

…nd-schema

kibana-ci · 2023-05-23T08:08:31Z

💚 Build Succeeded

Buildkite Build
Commit: 7ed5d5c

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`	57	59	+2
`@kbn/core-saved-objects-migration-server-internal`	88	86	-2
`@kbn/core-saved-objects-server`	117	116	-1
total			-1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	9	11	+2
`@kbn/core-saved-objects-migration-server-internal`	44	45	+1
total			+3

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	83	85	+2
`@kbn/core-saved-objects-migration-server-internal`	122	120	-2
`@kbn/core-saved-objects-server`	531	533	+2
total			+2

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	399	403	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	479	483	+4
total			+6

History

💛 Build #129280 was flaky cffa0fc
💔 Build #129265 failed 7218f95
💚 Build #129223 succeeded a5400ff
💔 Build #129219 failed 465077b
💛 Build #129179 was flaky 6d34540
💚 Build #128548 succeeded d69e49b

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

) ## Summary Part of elastic#150312 (next steps depend on elastic#153117) **This PR does two things:** - introduce the concept of version persistence schema - adapt the document migrator to support downward migrations for documents of an higher version. In the follow-up, we will then update the calls from the SOR to the document migrator to allow downward conversions when we're using the ZDT migration algorithm (which requires elastic#153117 to be merged) ### Model version persistence schema. *(This is what has also been named 'eviction schema' or 'known fields schema'.)* A new `SavedObjectsModelVersion.schemas.backwardConversion` property was added to the model version definition. This 'schema' can either be an arbitrary function, or a `schema.object` from `@kbn/config-schema` ```ts type SavedObjectModelVersionBackwardConversionSchema< InAttrs = unknown, OutAttrs = unknown > = ObjectType | SavedObjectModelVersionBackwardConversionFn<InAttrs, OutAttrs>; ``` When specified for a version, the document's attributes will go thought this schema during down conversions by the document migrator. ### Adapt the document migrator to support downward migrations for documents of an higher version. Add an `allowDowngrade` option to `DocumentMigrator.migrate` and `KibanaMigrator.migrateDocument`. When this option is set to `true`, the document migration will accept to 'downgrade' the document if necessary, instead of throwing an error as done when the option is `false` or unspecified (which was the only behavior prior to this PR's changes) --------- Co-authored-by: kibanamachine <[email protected]>

#158251) ## Summary Follow-up of #157895 Part of #150312 Adapt the SOR to accept retrieving documents on higher versions from the persistence, and convert them back to the latest knows version (using the version schema feature added in #157895).

implement version schema, start to adapt the migrator

2a3381f

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 v8.9.0 labels May 16, 2023

pgayvallet and others added 13 commits May 17, 2023 07:54

fix types

5978514

fix downgrade required logic

fb46372

add tests

823c0da

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

b3b4d2f

…-ref HEAD~1..HEAD --fix'

more tests

d2df4f6

restore boom error...

e673d82

Add the allowDowngrade option to KibanaMigrator.migrateDocument

1ceea39

make options optional...

d69e49b

Merge remote-tracking branch 'upstream/main' into kbn-xxx-downgrade-a…

6d34540

…nd-schema

Merge remote-tracking branch 'upstream/main' into kbn-xxx-downgrade-a…

465077b

…nd-schema

fix test types

a5400ff

NIT on doc

1dc378f

NIT on doc

c2c6603

pgayvallet commented May 22, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-xxx-downgrade-a…

7218f95

…nd-schema

pgayvallet marked this pull request as ready for review May 22, 2023 12:23

pgayvallet requested a review from a team as a code owner May 22, 2023 12:23

pgayvallet added the Feature:Saved Objects label May 22, 2023

Merge remote-tracking branch 'upstream/main' into kbn-xxx-downgrade-a…

61a2421

…nd-schema

jloleysens reviewed May 22, 2023

View reviewed changes

operators are hard

cffa0fc

jloleysens approved these changes May 22, 2023

View reviewed changes

afharo reviewed May 22, 2023

View reviewed changes

pgayvallet added 2 commits May 23, 2023 08:26

review comments

6beed64

naming is hard

c9ded9d

pgayvallet added 2 commits May 23, 2023 09:03

update migration hash generation

1aa2a76

Merge remote-tracking branch 'upstream/main' into kbn-xxx-downgrade-a…

7ed5d5c

…nd-schema

pgayvallet merged commit c24dc35 into elastic:main May 23, 2023

kibanamachine added the backport:skip This commit does not require backporting label May 23, 2023

pgayvallet mentioned this pull request May 23, 2023

[ZDT] SOR: handle higher-version documents retrieved from persistence. #158251

Merged

pgayvallet mentioned this pull request Jun 7, 2023

Adapt the SOR for managed upgrades #150312

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZDT] DocumentMigrator: support higher version documents #157895

[ZDT] DocumentMigrator: support higher version documents #157895

pgayvallet commented May 16, 2023 •

edited

Loading

pgayvallet left a comment •

edited

Loading

pgayvallet May 22, 2023

afharo May 22, 2023

TinaHeiligers May 22, 2023

pgayvallet May 22, 2023

pgayvallet May 22, 2023

pgayvallet May 22, 2023

pgayvallet May 22, 2023

pgayvallet May 22, 2023

jloleysens May 22, 2023

pgayvallet May 23, 2023

pgayvallet May 22, 2023

pgayvallet May 22, 2023

elasticmachine commented May 22, 2023

jloleysens left a comment

jloleysens May 22, 2023 •

edited

Loading

pgayvallet May 22, 2023

jloleysens left a comment

jloleysens May 22, 2023

pgayvallet May 22, 2023

jloleysens May 22, 2023

pgayvallet May 22, 2023

jloleysens May 22, 2023

pgayvallet May 22, 2023

jloleysens May 22, 2023

jloleysens May 22, 2023

afharo left a comment

afharo May 22, 2023

pgayvallet May 23, 2023

afharo May 22, 2023

kibana-ci commented May 23, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

		* Note that These conversion mechanism shouldn't assert the data itself, only strip unknown fields to convert
		* the document to the shape of the document at the given version.


		export type ConvertedSchema = (doc: SavedObjectUnsanitizedDoc) => SavedObjectUnsanitizedDoc;

		export const convertModelVersionBackwardConversionSchema = (

	/**
	* Represents the virtual version of a given SO type.
	* The virtual version is a compatibility format between the old
	* migration system's versioning, based on the stack version, and the new model versioning.
	*
	* A virtual version is a plain semver version. Depending on its major version value, the
	* underlying version can be the following:
	* - Major < 10: Old migrations system (stack versions), using the equivalent value (e.g `8.7.0` => migration version `8.7.0`)
	* - Major == 10: Model versions, using the `10.{modelVersion}.0` format (e.g `10.3.0` => model version 3)
	*/
	export type VirtualVersion = string;

[ZDT] DocumentMigrator: support higher version documents #157895

[ZDT] DocumentMigrator: support higher version documents #157895

Conversation

pgayvallet commented May 16, 2023 • edited Loading

Summary

Model version persistence schema.

Adapt the document migrator to support downward migrations for documents of an higher version.

pgayvallet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 22, 2023

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented May 23, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet commented May 16, 2023 •

edited

Loading

pgayvallet left a comment •

edited

Loading

jloleysens May 22, 2023 •

edited

Loading