-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Fleet] catching only mapper errors #167044
Conversation
Pinging @elastic/fleet (Team:Fleet) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
logger.error(`Mappings update for ${dataStreamName} failed due to unexpected error: ${err}`); | ||
throw err; |
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.
My only concern here is that this might surface existing errors that were ignored before, but I think it's better to see the errors early.
@juliaElastic I think it's a great improvement but I am wondering if we should push that changes with a new flag to bypass mappings update/rollover and log the errors, so we have an escape hatch for future SDH if things go wrong. |
@@ -61,6 +61,10 @@ export interface FleetConfigType { | |||
}; | |||
}; | |||
createArtifactsBulkBatchSize?: number; | |||
packageUpgrade?: { |
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 am wondering if it make more sense to have these flags on the install package endpoint, so we can apply this on case per case basis, if an error happen or a package upgrade is stuck, wdyt?
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.
Yeah that could be easier to apply than a kibana config, the only caveat is that we can't stop Fleet setup to re-run the upgrade if a preconfigured package has an issue.
Though if the API call succeeds, the setup should not try to reinstall the package, so it might be enough to have the flags on the API.
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 API flags are ready
8c6b013
to
8f171a9
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.
LGTM 🚀 This will be a great tool for future SDHs
💔 Build FailedFailed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Looks great thank you!
Tests seem to fail due to this issue: #167255 |
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've verified that the 26 TypeScript errors coming from x-pack/test/tsconfig.json
in the "Check Types Commit Diff" check are not related to this PR and so we can safely merge it as long as the other CI checks are green
Summary
Closes #162772
Changed the logic to only rollover on mapper errors, will check in ES what are the possible errors.
I think it would be useful to completely fail on unexpected errors instead of hiding bugs (like the simulate template error).
Added 2 flags in package install API to be able to ignore unexpected mapping errors and skip rollover, this can be useful if we want to bypass throwing an error / doing rollover in a support case.
To verify, run the package install API with the flags to skip rollover:
See in the logs that rollover is skipped:
Checklist