skip restoring global state when one is absent in a snapshot #82075
Conversation
Currently it is possible to restore snapshot that was created without global state with `include_global_state: true`. This is silently ignored now. This change warns when this is happening.
idegtiarenko
added
>bug
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
100% on board with the change, I'd like to make it a little simpler mechanically. I could see how that would mess with our testing, but maybe we could instead just add an integration test that verifies that the global state file isn't even read? (happy to help with that on another channel)
| @@ -301,6 +301,9 @@ private void startRestore( | |||
| // Make sure that we can restore from this snapshot | |||
| validateSnapshotRestorable(repository.getMetadata(), snapshotInfo); | |||
|
|
|||
| // Make sure that we can restore cluster state from this snapshot | |||
| validateGlobalStateRestorable(request, snapshot, snapshotInfo); | |||
There was a problem hiding this comment.
This is a little confusing in terms of method name and mechanics (IMO).
Could we maybe just move the check into the conditional below
if (request.includeGlobalState()) {
and not mutate the request object instead?
There was a problem hiding this comment.
I was thinking about this, however that is not the only usage. there is also one in getFeatureStatesToRestore and could be more in future.
| static void validateGlobalStateRestorable(RestoreSnapshotRequest request, Snapshot snapshot, SnapshotInfo snapshotInfo) { | ||
| if (request.includeGlobalState() && snapshotInfo.includeGlobalState() != Boolean.TRUE) { | ||
| request.includeGlobalState(false); | ||
| logger.warn("[{}] the snapshot was created without global state, skipping restoring global sate", snapshot); |
There was a problem hiding this comment.
Maybe word this explicitly and say "[{}] was created without global state but restore request [{}] asks for global state restore explicitly, skipping global state restore"
|
9b62e0f this scares me a little now, are we changing behavior after all? |
|
Yes, this affects |
| static void maybeFixRestoreGlobalStateFlag(RestoreSnapshotRequest request, Snapshot snapshot, SnapshotInfo snapshotInfo) { | ||
| if (request.includeGlobalState() && snapshotInfo.includeGlobalState() != Boolean.TRUE) { | ||
| request.includeGlobalState(false); | ||
| logger.warn( |
There was a problem hiding this comment.
Should we rather fail out here? Seems like an inadvertent use of a snapshot?
There was a problem hiding this comment.
@henningandersen yes me and @idegtiarenko were one the same page on this as well but figured we'd have to ask you here since it's technically an API breaking change? :) If you're good with that kind of change, ++ I think we should just straight opt for failing.
|
Closing in favor of: #82037 |
Currently it is possible to restore with include_global_state: true from a snapshot that does not have a global state.
This behavior will nullify cluster state once #81373 is merged.
This pr adds skips global state restore and logs a warning when this happens.
related to #82019