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

Jump to bottom

Prevent future convertToMultiNamespaceType migrations #147369

Merged
merged 5 commits into from
Dec 21, 2022
Merged

Prevent future convertToMultiNamespaceType migrations #147369

merged 5 commits into from
Dec 21, 2022

Conversation

gsoldevila
Copy link
Contributor

@gsoldevila gsoldevila commented Dec 12, 2022 •
edited by kibanamachine
Loading

Addresses #147344

@gsoldevila gsoldevila added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes Feature:Migrations backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.7.0 labels Dec 12, 2022
@gsoldevila gsoldevila requested a review from a team as a code owner December 12, 2022 16:29
@elasticmachine
Copy link
Contributor

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

pgayvallet
pgayvallet reviewed Dec 13, 2022
View reviewed changes
packages/core/saved-objects/core-saved-objects-migration-server-internal/src/kibana_migrator.ts
@@ -37,6 +37,10 @@ import { createIndexMap } from './core/build_index_map';
import { runResilientMigrator } from './run_resilient_migrator';
import { migrateRawDocsSafely } from './core/migrate_raw_docs';

// ensure plugins don't try to convert SO namespaceTypes after 8.0.0
// see https://github.com/elastic/kibana/issues/147344
const ALLOWED_CONVERT_VERSION = '8.0.0';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll defer to @rudolf, but are we sure we don't want to allow type owners to migrate to shareable until some versions to come? This PR basically hard block any type from being converted as soon as merged. I want to make sure we're fine with the consequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I understood correctly, this is a pre-requisite to only migrate an index if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I just saw #147198, but it is converting from "agnostic".

Copy link
Member

Choose a reason for hiding this comment

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

My 2cents are: as claimed in #147344, if we are not reindexing when there are no mapping updates (or, at least, no breaking changes), we cannot support any further version unless we change the current logic to identify that we need reindex anyway because a conversion must happen.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah if someone REALLY REALLY REALLY needs this we'll need to re-enable and detect this to ensure we reindex (which again will introduce downtime).

Re #147198 this is sort of a unique case because it's a new saved object type, there aren't any files saved objects in users clusters.

@afharo afharo mentioned this pull request Dec 14, 2022
Closed
3 tasks
afharo
afharo reviewed Dec 14, 2022
View reviewed changes
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Should we update the docs accordingly?

  1. Deprecate the option:

    1. kibana/packages/core/saved-objects/core-saved-objects-server/src/migration.ts

      Lines 77 to 80 in 4f4c160

      /**
      * The version in which this object type is being converted to a multi-namespace type
      */
      readonly convertToMultiNamespaceTypeVersion?: string;

    2. kibana/packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

      Line 117 in 167797c

      convertToMultiNamespaceTypeVersion?: string;
  2. The documentation page Sharing saved objects

pgayvallet
pgayvallet approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

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

I think this is what we want 😅 . LGTM

...ore/saved-objects/core-saved-objects-migration-server-internal/src/core/document_migrator.ts
@@ -345,6 +338,7 @@ function validateMigrationDefinition(
);
}
if (convertToMultiNamespaceTypeVersion) {
// CHECKPOINT 1
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Checkpoint!

...ore/saved-objects/core-saved-objects-migration-server-internal/src/core/document_migrator.ts
@@ -321,9 +314,9 @@ function validateMigrationDefinition(
throw new Error(
`Invalid convertToMultiNamespaceTypeVersion for type ${type}. Expected value to be a semver, but got '${convertToMultiNamespaceTypeVersion}'.`
);
} else if (Semver.lt(convertToMultiNamespaceTypeVersion, minimumConvertVersion)) {
} else if (convertVersion && Semver.neq(convertToMultiNamespaceTypeVersion, convertVersion)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My gut feeling would have been to keep 8.0.0 as the minimum version and introduces a new maximumConvertVersion that we would set either to 8.0.0 or the current version like 8.6.0. But I'm probably overthinking this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could we useful if in the future we wish to unfreeze the functionality.
But at that point, we will probably have to rollback future changes in migration logic as well, otherwise we would have unexpected results.

gsoldevila
gsoldevila commented Dec 21, 2022
View reviewed changes
docs/developer/advanced/sharing-saved-objects.asciidoc Outdated
@@ -4,6 +4,7 @@
This guide describes the "Sharing saved objects" effort, and the breaking changes that plugin developers need to be aware of for the planned
8.0 release of {kib}. It also describes how developers can take advantage of this feature.

`From *8.7.0*, as a step towards Zero-Down-Time upgrades, plugins are no longer allowed to switch to shareable saved objects.`
Copy link
Contributor Author

@gsoldevila gsoldevila Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

image

@gsoldevila gsoldevila requested review from rudolf and afharo December 21, 2022 11:17
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 61 67 +6
osquery 109 115 +6
securitySolution 439 445 +6
total +20

References to deprecated APIs

id before after diff
@kbn/core-saved-objects-migration-server-internal 4 48 +44
@kbn/core-test-helpers-so-type-serializer 0 8 +8
actions 46 50 +4
alerting 93 95 +2
canvas 113 117 +4
cases 0 10 +10
dashboard 78 80 +2
data 57 59 +2
dataViews 38 40 +2
encryptedSavedObjects 2 6 +4
graph 56 58 +2
lens 16 18 +2
lists 72 74 +2
maps 25 27 +2
savedObjectsTagging 1 3 +2
savedSearch 6 8 +2
securitySolution 339 349 +10
visualizations 71 73 +2
total +106

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 70 76 +6
osquery 110 117 +7
securitySolution 515 521 +6
total +21

History

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

@gsoldevila gsoldevila merged commit 57dad8f into elastic:main Dec 21, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.6 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 147369

Questions ?

Please refer to the Backport tool documentation

@gsoldevila gsoldevila added backport:skip This commit does not require backporting and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 21, 2022
@rudolf rudolf added the Epic:ScaleMigrations Scale upgrade migrations to millions of saved objects label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

@rudolf rudolf rudolf approved these changes

@afharo afharo Awaiting requested review from afharo

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result Epic:ScaleMigrations Scale upgrade migrations to millions of saved objects 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.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gsoldevila @elasticmachine @kibana-ci @kibanamachine @rudolf @pgayvallet @afharo