[7.10][Ingest Manager][EPM] Moving reinstall function outside of promise.all to avoid race#82664
Conversation
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
|
I was able to consistently reproduce the saved object conflict locally by having only one required package that |
jfsiii
left a comment
There was a problem hiding this comment.
Approving since a) worst case "should" be longer install time b) @neptunian can repro error & fix.
Can we open a ticket to create a test based on @neptunian instructions? e.g. mock ensure* to throw and/or put saved object in a certain state?
We could move the function back into Promise.all, create the failure case which causes the failure, then move it back to this position and change the expectation.
|
Issue for adding tests: #82670 |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
This is to address this issue: #82614
There is a potential for a race condition during the
/setupAPI call if one of the required packages is in theinstallingstate. A required package can get in this state if a failure occurred to install/upgrade that package and we were unable to rollback the package for some reason.Upon the next call to
/setup, if the package can be upgraded, the upgrade code will be initiated in addition to attempting to reinstall the failed package version because both theensureInstalledDefaultPackagesand theensurePackagesCompletedInstallcalls happen in parallel in thePromise.all.This PR tries to mitigate that by moving the reinstall functionality outside of the
Promise.allso that it is done after the upgrade attempt.I'm not sure how we would test this with integration tests 🤔 or if it's worth writing unit tests for this. Let me know what you think.