-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Saved object migrations] Collect all documents that fail to transform before stopping the migration #96986
[Saved object migrations] Collect all documents that fail to transform before stopping the migration #96986
Conversation
fbfa86b
to
d307ea4
Compare
d307ea4
to
993a5fa
Compare
This comment has been minimized.
This comment has been minimized.
…RMED_DOCUMENTS_BULK_INDEX
2362862
to
0b14388
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job entering in the SO migration v2 code. You're now a migration expert!
Implementation looks fine to me. What's currently missing in the change in the OUTDATED_DOCUMENTS_SEARCH
step to used a PIT instead of just looping on search
, to avoid trying to migrate the failing documents indefinitely.
It seems that it's not currently handled in #97222, so not sure which PR should take care of that.
src/core/server/saved_objects/migrations/core/document_migrator.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/core/transform_saved_object_document_error.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/kibana/kibana_migrator.ts
Outdated
Show resolved
Hide resolved
this.id = id; | ||
this.namespace = namespace; | ||
this.type = type; | ||
// Removed because not including still seems to work, it may have been an old Typescript 2.1 issue: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgayvallet ideally, we also want to capture the original stack trace. I think we are, since we're on node v14.16.1 but I'm not as familiar as you might be. Do you happen to know if I need to add something along the lines of:
// Removed because not including still seems to work, it may have been an old Typescript 2.1 issue: | |
// Maintains proper stack trace for where our error was thrown (only available on V8) | |
if (Error.captureStackTrace) { | |
Error.captureStackTrace(this, TransformSavedObjectDocumentError) | |
} | |
// Removed because not including still seems to work, it may have been an old Typescript 2.1 issue: |
as suggested in the "ES6 Custom Error Class" https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#instance_properties section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error.captureStackTrace
captures the stacktrace at the time it was invoked, so when the TransformSavedObjectDocumentError
will be created. Is that what you want?
If you want to use the stack from the original error instead (originalError
), You can just copy it in the constructor (this.stack = originalError.stack
), or even access it from consumer code, as the property is public transformError.originalError.stack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want the stack trace from the original error because that's ultimately what's adding to the reason the migration will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add the stack traces to the FATAL
controlState reason but realized it's going to clutter up the logs badly! For now, we'll go with adding the raw id and the transform that failed. We can tack on the stack trace later if we see that it's not enough info for debugging failed migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add the stack traces to the FATAL controlState reason but realized it's going to clutter up the logs badly!
How much worse are they getting? I'd say keeping the error stack is a must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can improve it in a follow-up
…, cleans up comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ecifies namespace as undefined for multi-namespace saved objects
…on failure reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good. A few nits and questions
src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts
Outdated
Show resolved
Hide resolved
err, | ||
}); | ||
} else { | ||
transformErrors.push({ rawId: 'unknown', err }); // cases we haven't accounted for yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error might be from a specific transformation script that threw, as an example
Having the document._id of the doc that encountered the failure (or at least the doc we were processing while encountering this unhandled error) seems like valuable information though, and is available in raw
, so I think we ideally want to surface it in the logs when ! err instanceof TransformSavedObjectDocumentError
src/core/server/saved_objects/migrations/core/migrate_raw_docs.test.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/core/migrate_raw_docs.test.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrationsv2/integration_tests/actions.test.ts
Outdated
Show resolved
Hide resolved
}; | ||
} else { | ||
const left = res.left; | ||
const left = Either.left; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, even with your comment, I don't understand this change, given that Either
is the library import, not one of our constant?
import * as Either from 'fp-ts/lib/Either';
How does const left = Either.left
make sense here?
…cuments the same way
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last few minor NITs, but overall LGTM.
// const savedObject = convertToRawAddMigrationVersion(raw, options, serializer); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: commented line can be removed
const resultsWithProcessDocs = ((await transformTask()) as Either.Right<DocumentsTransformSuccess>) | ||
.right.processedDocs; | ||
expect(resultsWithProcessDocs.length).toEqual(2); | ||
// const foo2 = hits.find((h) => h._id === 'foo:2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: can be removed
[...err.message.matchAll(corruptFooSOs)].concat( | ||
[...err.message.matchAll(corruptBarSOs)], | ||
[...err.message.matchAll(corruptBazSOs)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: no use to concat
[
...err.message.matchAll(corruptFooSOs),
...err.message.matchAll(corruptBarSOs),
...err.message.matchAll(corruptBazSOs)
]
const corruptFooSOs = /foo:/g; | ||
const corruptBarSOs = /bar:/g; | ||
const corruptBazSOs = /baz:/g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can the regexps be a little more 'precise'?
- can we also add a test on the 'prefix' of the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll address those changes in the follow up PR. The test will likely change anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a few nits
const processedDocs: SavedObjectsRawDoc[] = []; | ||
const transformErrors: TransformErrorObjects[] = []; | ||
const corruptSavedObjectIds: string[] = []; | ||
const options = { namespaceTreatment: 'lax' as const }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional nit: let's move it outside to reduce GC pressure
const soParseOptions = { namespaceTreatment: 'lax' } as const;
export function migrateRawDocsSafely(...
const options = { namespaceTreatment: 'lax' as const }; | ||
for (const raw of rawDocs) { | ||
if (serializer.isRawSavedObject(raw, options)) { | ||
// const savedObject = convertToRawAddMigrationVersion(raw, options, serializer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove it
// the doc id we get from the error is only the uuid part | ||
// we transform the id to a raw saved object id. | ||
transformErrors.push({ | ||
rawId: serializer.generateRawId(err.namespace, err.type, err.id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use the original raw._id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good question!
import { TransformSavedObjectDocumentError } from '.'; | ||
|
||
export interface DocumentsTransformFailed { | ||
type: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: readonly
for all the properties. And below.
expect(result.left.transformErrors.length).toEqual(1); | ||
expect(result.left.transformErrors[0].err.message).toMatchInlineSnapshot(` | ||
"Failed to transform document b. Transform: a1.2.3 | ||
Doc: {\\"type\\":\\"a\\",\\"id\\":\\"b\\",\\"attributes\\":{\\"name\\":\\"AAA\\"},\\"references\\":[],\\"migrationVersion\\":{}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransformSavedObjectDocumentError
shows the problem SO, but not the source of the problem. Doesn't it? Let's refactor it to include the original error message as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the original error is included in the transformation error as originalError
.
} | ||
|
||
/** | ||
* Sanitizes the raw saved object document and sets the migration version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't set the migration version
. Does it?
const resultsWithProcessDocs = ((await transformTask()) as Either.Right<DocumentsTransformSuccess>) | ||
.right.processedDocs; | ||
expect(resultsWithProcessDocs.length).toEqual(2); | ||
// const foo2 = hits.find((h) => h._id === 'foo:2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove it.
.map((errObj) => `${errObj.rawId}: ${errObj.err.message}\n ${errObj.err.stack ?? ''}`) | ||
.join('/n') | ||
: ''; | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could return the final error message Migrations failed. Reason:...
to encapsulate all the error message logic
const corruptBarSOs = /bar:/g; | ||
const corruptBazSOs = /baz:/g; | ||
expect( | ||
[...err.message.matchAll(corruptFooSOs)].concat( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind extending the test case to include transformErrors
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that in a follow up PR if you don't mind. I wasn't sure how to inject a failing transform and need to research that a bit more.
// foo: '7.13.0', | ||
// }, | ||
// }, | ||
// contains migrated index with 8.0 aliases to skip migration, but run outdated doc search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: it's better to use 7.x
index unless you want to test OUTDATED_DOCUMENTS_*
steps. otherwise, we have to skip the test on 7.x
branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, testing OUTDATED_DOCUMENTS_*
should be a good start and we can explicitly test the changes introduced to REINDEX_SOURCE_TO_TEMP_*
in a follow up PR.
💚 Build SucceededMetrics [docs]Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
…m before stopping the migration (elastic#96986) Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…ansform before stopping the migration (#96986) (#99713) * [Saved object migrations] Collect all documents that fail to transform before stopping the migration (#96986) Co-authored-by: Kibana Machine <[email protected]> * Update src/core/server/saved_objects/migrationsv2/integration_tests/corrupt_outdated_docs.test.ts Test relies on an archive with saved objects from version 8.0.0 Co-authored-by: Christiane (Tina) Heiligers <[email protected]>
Resolves #90279
Approach:
assumingnow that we have PIT searchesby thento not search again for outdated docs)Implementation:
migrateRawDocsNonThrowing
similar tomigrateRawDocs
that returns an instance ofEither.left
containing an array oftransformErrors
andcorruptDocumentIds
if there are issues transforming a batch of outdated saved object documents. If there aren't any issues with the current batch, we return an instance ofEither.right
with theprocessedDocs
(transformed saved object documents).(
migrateRawDocs
is used in v1 migrations inkibana_migrator
, hence not refactoring the original)Dependency:
OUTDATED_DOCUMENTS_SEARCH
phase prior to transforming documents (reuse theActions.openPit(...)
andActions.readWithPit(...)
) Use PiT for outdated document search #98797.