Correctly update SLM stats with master shutdown #134152
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
samxbr added :Data Management/ILM+SLM Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes SLM (Snapshot Lifecycle Management) stats accuracy by correctly handling successful snapshots that were not recorded due to master node shutdown. Previously, SLM would incorrectly mark successful snapshots as failures when recovering from master shutdown.
Key changes:
- Pre-load completed snapshot info from repository to determine actual snapshot status (success/failure)
- Update SLM's cluster state update job to use actual snapshot status for accurate stats calculation
- Add comprehensive tests for master shutdown scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SnapshotLifecycleTask.java | Core logic changes to fetch and use actual snapshot status when updating SLM stats |
| SnapshotLifecycleTaskTests.java | Updated test methods to accommodate new snapshot info parameter and added test helper methods |
| SLMSnapshotBlockingIntegTests.java | Added integration tests for master shutdown scenarios with successful and deleted snapshots |
| RegisteredPolicySnapshots.java | Updated comment to reflect new behavior of using actual snapshot status |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Outdated Show resolved Hide resolved
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Show resolved Hide resolved
…apshotLifecycleTask.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
samxbr commented Sep 4, 2025
Comment on lines +217 to +244
| findCompletedRegisteredSnapshotInfo(currentProjectState, policyId, client, new ActionListener<>() { | ||
| @Override | ||
| public void onResponse(List<SnapshotInfo> snapshotInfo) { | ||
| submitUnbatchedTask( | ||
| clusterService, | ||
| "slm-record-success-" + policyId, | ||
| WriteJobStatus.success(projectId, policyId, snapshotId, snapshotStartTime, timestamp, snapshotInfo) | ||
| ); | ||
| } | ||
| | ||
| @Override | ||
| public void onFailure(Exception e) { | ||
| logger.warn(() -> format("failed to retrieve stale registered snapshots for job [%s]", jobId), e); | ||
| // still record the successful snapshot | ||
| submitUnbatchedTask( | ||
| clusterService, | ||
| "slm-record-success-" + policyId, | ||
| WriteJobStatus.success( | ||
| projectId, | ||
| policyId, | ||
| snapshotId, | ||
| snapshotStartTime, | ||
| timestamp, | ||
| Collections.emptyList() | ||
| ) | ||
| ); | ||
| } | ||
| }); |
Contributor Author
There was a problem hiding this comment.
There's a little bit of duplication, maybe there's a more elegant way to do this?
Member
There was a problem hiding this comment.
I don't think the duplication is too bad here, duplicating it once is fine.
elasticsearchmachine added the Team:Data Management 
Collaborator
| Pinging @elastic/es-data-management (Team:Data Management) |
Collaborator
| Hi @samxbr, I've created a changelog YAML for you. |
dakrone approved these changes Sep 11, 2025
Member
dakrone left a comment
dakrone left a comment There was a problem hiding this comment.
LGTM, thanks for working on this Sam! I left a few minor comments, nothing major.
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Outdated Show resolved Hide resolved
Comment on lines +217 to +244
| findCompletedRegisteredSnapshotInfo(currentProjectState, policyId, client, new ActionListener<>() { | ||
| @Override | ||
| public void onResponse(List<SnapshotInfo> snapshotInfo) { | ||
| submitUnbatchedTask( | ||
| clusterService, | ||
| "slm-record-success-" + policyId, | ||
| WriteJobStatus.success(projectId, policyId, snapshotId, snapshotStartTime, timestamp, snapshotInfo) | ||
| ); | ||
| } | ||
| | ||
| @Override | ||
| public void onFailure(Exception e) { | ||
| logger.warn(() -> format("failed to retrieve stale registered snapshots for job [%s]", jobId), e); | ||
| // still record the successful snapshot | ||
| submitUnbatchedTask( | ||
| clusterService, | ||
| "slm-record-success-" + policyId, | ||
| WriteJobStatus.success( | ||
| projectId, | ||
| policyId, | ||
| snapshotId, | ||
| snapshotStartTime, | ||
| timestamp, | ||
| Collections.emptyList() | ||
| ) | ||
| ); | ||
| } | ||
| }); |
Member
There was a problem hiding this comment.
I don't think the duplication is too bad here, duplicating it once is fine.
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Show resolved Hide resolved
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Outdated Show resolved Hide resolved
…apshotLifecycleTask.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
samxbr added auto-merge-without-approval mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Sep 17, 2025
gmjehovich pushed a commit to gmjehovich/elasticsearch that referenced this pull request Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This change makes SLM stats more accurate by pre-loading the completed snapshot info from repository, and pass them to SLM's cluster state update job. The snapshot info is then used to calculate the correct stats for previous SLM snapshots.
Background
SLM runs on master node, it case of the master node being shut down in the middle of a SLM run, the current SLM execution will fail. Depending on the timing, if the master node is shut down after a SLM triggered snapshot was completed but before SLM updates cluster state, the state update will be lost. To recover the lost update, SLM keeps track of a list of registered snapshots so it can detect the above case and retroactively update the SLM state. However SLM did not check the status of the snapshot, it assumes the snapshot failed, resulting SLM incorrectly recording successful snapshots as failure. This change corrects the stats in such case by taking snapshot status (success/failure) into account.