SNAPSHOT: Keep SnapshotsInProgress State in Sync with Routing Table

[original-brownbear]

• Keep SnapshotsInProgress state in sync with routing table
• Handle being unable to get shard from indicesService gracefully
• Add assertions about inner consistency of SnapShotsInProgress with routing table

[elasticmachine]

Pinging @elastic/es-distributed

[original-brownbear]

@ywelsch

I think this is good for a review if you have some time.
I organized the signature of updateWithRoutingTable and the assertion on the state a little differently than in your initial approach. I like this better because you get an assertion on the full SnapshotsInProgress state instead of failing right on the first shard entry that is off => you get a little more information out of the assert message. Also, I found updateWithRoutingTable a litlle easier to digest this way, but let me know what you think :)

[original-brownbear]

@ywelsch looks like it's green now :)

I had to relax the overall state assertion a little though and not run it when the master was just elected (here https://github.com/elastic/elasticsearch/pull/35710/files#diff-a0853be4492c052f24917b5c1464003dR662)

This is a result of the cleanup action here https://github.com/elastic/elasticsearch/pull/35710/files#diff-a0853be4492c052f24917b5c1464003dR654 not happening in this case as far as I can tell. You refactored that logic too in your branch and it's fine there but I guess we can leave that for the next round?

(PS: Also ran :server:test :server:integTest in a loop locally for a few hours today without issues with this code :))

[ywelsch]

I've left mostly smaller comments on the PR. Can you also rename the PR to better reflect what's been done here, namely to keep SnapshotsInProgress state in sync with routing table?

[ywelsch]

X != false <==> X

[ywelsch]

Note that we'll have a similar problem when transitioning from a master that did not keep the routing table in sync to a master that now does. We will probably have to schedule a clean-up task that brings these two in sync again. There's no guarantee that that task will run before any other tasks, so the assertion might not only be violated while event.previousState().nodes().isLocalNodeElectedMaster() == false, but also on some follow-up cluster state changes. I'm wondering if we need to introduce some state to SnapshotsService to say whether we've properly cleaned up as a master and then make that assertion dependent on that boolean variable. Something we can look into in a follow-up.

[ywelsch]

I wonder if we can expect there to always exist the corresponding shard routing table?

[original-brownbear]

I don't know but can't a ShardId that we got from the callback org.elasticsearch.cluster.routing.RoutingChangesObserver.AbstractChangedShardObserver#shardFailed lead to a null entry here? (I could be way off here ... not sure about the order of these things yet)

[ywelsch]

we are never adding or removing keys in the shard routing table during the AllocationService.reroute() phase. Can you add an assertion that the routing table is not null, but still keep the extra safety logic around?

[ywelsch]

can you add a comment that we are in the relocating state here

[original-brownbear]

Added the assertion for that below.

[ywelsch]

perhaps move the assertion one level up, i.e.,

} else {
  assert priamryShardRouting.relocating();
  if (currentState == State.INIT || currentStatus.state() == State.ABORTED) { 
    ..
  } else {
    ..
  }
}

[original-brownbear]

@ywelsch all points addressed I think :)

[original-brownbear]

@ywelsch ping (if you have a second) :)

[ywelsch]

2 nits, looks good otherwise.

[original-brownbear]

@ywelsch thanks!, nits handled => merging :)