Make snapshots more resilient to node departure

[DaveCTurner]

If a node holding a primary shard leaves the cluster then one of the replica shards is immediately promoted to primary to replace the failed copy. Today if there was a snapshot ongoing when the promotion happens then the corresponding shard-level snapshot fails and the overall snapshot status is at best PARTIAL. This is a problem for graceful shutdowns (#70338), which ideally would not result in any such failures. In cases where a replica is promoted to replace a failed primary it would be better instead to retry the shard-level snapshot on the new primary.

This isn't the first time this idea has come up:

elasticsearch/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

Line 1091 in 9addf0b

// TODO: Restart snapshot on another node?

[elasticmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[original-brownbear]

I think theoretically there aren't many issues with this for repositories of meta version 7.6+ (that use uuid shard generations and thus don't have any troublesome races if two data nodes write to the same shard).

There's definitely some practical complexities to this though (e.g. SLM running snapshots at a high frequency and this now causing snapshots to slow down significantly if a hot index fails over where the snapshot may not be incremental at all due to the changed file structure). I guess we can discuss those elsewhere, but in general we should definitely try to do this IMO. It would make the behavior of snapshots with large shard counts a lot nicer.

[DaveCTurner]

There's definitely some practical complexities to this though

Aren't there always? 😁

Doesn't the hot-index-failing-over case already affect us today? Its impact is currently on the next snapshot to be taken so this would just move it forward a bit. IMO a bigger concern is a snapshot that simply never ends because one of the primaries keeps on moving around. Certainly solvable, but not something that we need worry about today.

[original-brownbear]

a bigger concern is a snapshot that simply never ends because one of the primaries keeps on moving around

That's the even worse version of this for sure (worst case the snapshot would be the reason for the shard failure I guess ...).

Doesn't the hot-index-failing-over case already affect us today? Its impact is currently on the next snapshot to be taken so this would just move it forward a bit.

It certainly does. I think the "moving forward" is a good thing though. If anything it at least takes away the snapshot load from the cluster when a data node fails and shards may get relocated putting extra pressure on things. If we start retrying a primary failover will trigger more work to be done right away on the data node and (if SLM is running in a tight loop) additional snapshots piling up. Hard to quantify the impact of this but it seems like it would make things worse for a lot of clusters that are already on the edge. As always ... all of this isn't blocking. We could certainly make big improvements to the way we track state for multiple snapshots and how we execute them to make this much less of a concern.

[DaveCTurner]

We discussed this in the @elastic/es-distributed team sync today.

We hadn't implemented this already because it risked corrupting the repo, but in newer versions that's no longer true so we can safely implement it. We did not reach a conclusion on exactly when to retry and when to give up. There is a trade-off between bounding the duration of the snapshot vs avoiding failure states in a graceful shutdown.

One possibility would be have the retry behaviour be sensitive to which nodes are gracefully shutting down.

[DaveCTurner]

We (the @elastic/es-distributed team) discussed this again today. We think retrying finitely often (once by default) per node (identified by persistent node ID) would be sufficient protection against snapshots that just never finish.

In addition we discussed a way to abort a snapshot without losing all the work it already did and opened #75340 to pursue that idea further.

[DaveCTurner]

This plan turns out to be pretty tricky to fit into the snapshots state machine as it currently stands today. The assumption that shards don't move while being snapshotted is implicit in several places deep within the convoluted logic of the SnapshotsService, and I am not confident we will find them all. I'm going to try a different approach, integrating things more closely with graceful node shutdown and avoiding the need to handle other kinds of node departure:

---

Abort shard snapshots on shutting-down nodes

Adjust the SnapshotShardsService to notice that the local node has a shutdown marker and treat that as if all its ongoing shard snapshots are aborted. When each one completes, respond indicating it should move back to state WAITING rather than going to FAILED.

I suspect this won't work (or at least it will break some invariants) in older versions, so if the min cluster version is too old then skip this logic.

Stop moving shards from WAITING/QUEUED to INIT state on shutting-down nodes

Adjust the SnapshotsService to treat primaries assigned to shutting-down nodes as if they were unassigned.

---

I think with these two changes we will react well to graceful shutdowns, stopping shard snapshot work ASAP without causing a PARTIAL snapshot, permitting the shards to relocate elsewhere, and resuming the shard snapshot work on more appropriate nodes.

I don't think we need to put extra effort into limiting the retries with this plan, as suggested in an earlier comment. As long as the node shutdown markers aren't flapping and the shards eventually relocate or the node restarts, each snapshot will complete.

If we cannot relocate a shard onto a new node then the snapshot will also never complete. However I believe that we treat this as a failure case in other ways too, eventually timing out and either stopping the node or else removing the shutdown marker, either of which permits the snapshot to proceed.

---

ETA: I've put this comment on the wrong issue. This is a solution for #101171 which avoids the need to solve #71333 at the same time.