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

[SO migration] fail the migration if unknown types are encountered #118300

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Nov 11, 2021

Summary

Fix #107678 by manually reverting #105213

Revert SO migration behavior to what it was before #105213 by having it fail fast when unknown SO types are encountered during the CHECK_UNKNOWN_DOCUMENTS step

Checklist

Release Note

Encountering unknown saved object types during a version migration will now cause Kibana to fail the upgrade. To proceed with the migration, it will be required to either re-enable the plugins owning them, or to delete the documents from the previous version's index.

@pgayvallet pgayvallet added project:ResilientSavedObjectMigrations Reduce Kibana upgrade failures by making saved object migrations more resilient Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 v8.1.0 release_note:breaking labels Nov 11, 2021
@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Nov 12, 2021
@pgayvallet
Copy link
Contributor Author

retest

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

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 -114 to -117
// Do not transform documents of unknown types
if (raw?._source?.type && !knownTypes.has(raw._source.type)) {
processedDocs.push(raw);
} else if (serializer.isRawSavedObject(raw, options)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part of the revert was not strictly necessary, but it felt safer to get back to the behavior where migrating unknown doc fails, to be consistent with our strategy to fail the migration in such case (even if going that far should never occur)

Comment on lines 161 to +162
{
oss: true,
oss: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some test datasets contains objects of types registered from xpack plugins, I had to enable xpack for those tests to avoid re-generating the dataset, as the tests were failing due to the behavior reintroduced in this PR.

@@ -122,7 +122,7 @@ describe('migration v2', () => {
expect(
records.find((rec) =>
rec.message.startsWith(
`Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715275 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.`
`Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715274 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.`
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 have NO idea why, but enabling xpack changed the size of the document by one byte 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

An extra newline maybe or some tiny change between node versions?

@pgayvallet pgayvallet marked this pull request as ready for review November 12, 2021 10:29
@pgayvallet pgayvallet requested a review from a team as a code owner November 12, 2021 10:29
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Revert LGTM

@TinaHeiligers
Copy link
Contributor

@pgayvallet shouldn't "process" be "proceed" in the release note? i.e.

To proceed with the migration, it will be required to either re-enable the plugins owning them, or to delete the documents from the previous version's index.

or

To process the migration, it will be required to either re-enable the plugins owning them, or to delete the documents from the previous version's index.

@pgayvallet
Copy link
Contributor Author

shouldn't "process" be "proceed" in the release note?

Definitely should, thanks.

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@pgayvallet pgayvallet added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 16, 2021
@pgayvallet pgayvallet merged commit e08c633 into elastic:main Nov 16, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 16, 2021
…lastic#118300)

* manually revert elastic#105213

* enabled xpack to register types for some IT tests

* fiz doc size

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 16, 2021
…118300) (#118690)

* manually revert #105213

* enabled xpack to register types for some IT tests

* fiz doc size

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Pierre Gayvallet <[email protected]>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
…118300)

* manually revert #105213

* enabled xpack to register types for some IT tests

* fiz doc size

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed project:ResilientSavedObjectMigrations Reduce Kibana upgrade failures by making saved object migrations more resilient release_note:breaking Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refuse to start with unknown saved object types in 8.0
5 participants