-
Notifications
You must be signed in to change notification settings - Fork 254
CLDSRV-836 Fix deep healthcheck fail status #6055
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
Conversation
Hello bourgoismickael,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.2 #6055 +/- ##
===================================================
- Coverage 84.41% 84.39% -0.03%
===================================================
Files 206 206
Lines 13016 13018 +2
===================================================
- Hits 10987 10986 -1
- Misses 2029 2032 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR fixes the deep healthcheck logic to fail when ANY client has ALL its backends/locations failing, rather than only failing when ALL backends across ALL clients fail. This ensures better detection of client-specific failures, particularly for multi-location data backends.
Changes:
- Modified the failure detection logic from checking all backends globally to checking each client independently
- Added empty client handling to skip clients with no backends
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
This behaviour needs to be documented in official documentation so customers and CS know what to expect + release notes are a must for this change in behaviour
|
| done(); | ||
| }); | ||
| }); | ||
|
|
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 completeness, should we have a test as well when metadata or vault fails?
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 don't think so, all client are handled generically, there is no more cases to test for specific clients
Now fail if ALL backends of only one client fails. Previously, the deep healthcheck would fail if ALL backends/locations failed globally across all clients (data, metadata, vault, kms). This change modifies the logic to fail if ANY client has ALL its backends/locations failing. This ensures: 1. For data backend with multiple sproxyd location constraints: - Returns HTTP 200 if at least ONE location is healthy - Returns HTTP 500 only if ALL locations fail 2. Each client (data, metadata, vault, kms) is evaluated independently - If ALL locations of the data client fail, overall check fails - If ALL locations of metadata fail, overall check fails - etc. The new logic uses: - `results.some()` to check across clients - `keys.every()` within each client to check all its locations
ab3b7be to
6dd43a5
Compare
|
/approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
Build failedThe build for commit did not succeed in branch w/9.3/bugfix/CLDSRV-836-deep-healthcheck The following options are set: approve |
|
ping |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-836. Goodbye bourgoismickael. |
Now fail if ALL backends of only one client fails.
Previously, the deep healthcheck would fail if ALL backends/locations
failed globally across all clients (data, metadata, vault, kms).
This change modifies the logic to fail if ANY client has ALL its
backends/locations failing. This ensures:
For data backend with multiple sproxyd location constraints:
Each client (data, metadata, vault, kms) is evaluated independently
The new logic uses:
results.some()to check across clientskeys.every()within each client to check all its locations