Create retention leases file during recovery #39359

DaveCTurner · 2019-02-25T13:10:24Z

Today we load the shard history retention leases from disk whenever opening the
engine, and treat a missing file as an empty set of leases. However in some
cases this is inappropriate: we might be restoring from a snapshot (if the
target index already exists then there may be leases on disk) or
force-allocating a stale primary, and in neither case does it make sense to
restore the retention leases from disk.

With this change we write an empty retention leases file during recovery,
except for the following cases:

During peer recovery the on-disk leases may be accurate and could be needed
if the recovery target is made into a primary.
During recovery from an existing store, as long as we are not
force-allocating a stale primary.

Relates #37165

Today we recover shard history retention leases from disk whenever opening the engine. However in many cases this is inappropriate: we might be restoring from a snapshot (if the target index already exists then there may be leases on disk) or force-allocating a stale primary, and in neither case does it make sense to restore the retention leases. Moreover when recovering a replica during peer recovery there is no need to restore the retention leases since they will be overwritten by the primary soon after. In fact the only time we want to recover the shard history retention leases from disk is if the recovery source is `RecoverySource.ExistingStoreRecoverySource.INSTANCE`. This change does that.

elasticmachine · 2019-02-25T13:10:26Z

Pinging @elastic/es-distributed

DaveCTurner · 2019-02-25T15:29:15Z

@elasticmachine please run elasticsearch-ci/docbldesx

ywelsch · 2019-02-25T18:34:09Z

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -1433,7 +1433,12 @@ private void innerOpenEngineAndTranslog() throws IOException {
        final String translogUUID = store.readLastCommittedSegmentsInfo().getUserData().get(Translog.TRANSLOG_UUID_KEY);
        final long globalCheckpoint = Translog.readGlobalCheckpoint(translogConfig.getTranslogPath(), translogUUID);
        replicationTracker.updateGlobalCheckpointOnReplica(globalCheckpoint, "read from translog checkpoint");
-        updateRetentionLeasesOnReplica(loadRetentionLeases());
+        // only recover retention leases if recovering from an existing store _and_ not bootstrapping a new history UUID.
+        if (recoveryState.getRecoverySource() == RecoverySource.ExistingStoreRecoverySource.INSTANCE) {


This logic should preferably go into StoreRecovery, where we also do the initialization of the lucene commit and translog files. Whenever we call createEmptyTranslog we should also delete the retention leases file.

Ok. By correlating this with createEmptyTranslog do you mean that we should consider a missing retention leases state file as an error when loading it? Today we treat this as if it were empty.

Yeah, I think we should not be lenient if we prepare the lease file before opening an Engine.

Ok, we can't be strict here yet because of BWC: indices created in 7.x do not yet have a retention lease file, and indices created in 6.x (<6.7.0) will never do so. I left the lenience in and will remove it in a followup after backporting. I included an assertion to ensure we follow up.

DaveCTurner · 2019-02-26T22:50:53Z

@elasticmachine run elasticsearch-ci/bwc

ywelsch · 2019-02-28T08:48:29Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java

@@ -414,6 +414,11 @@ public void cleanFiles(int totalTranslogOps, Store.MetadataSnapshot sourceMetaDa
                indexShard.shardPath().resolveTranslog(), SequenceNumbers.UNASSIGNED_SEQ_NO, shardId,
                indexShard.getPendingPrimaryTerm());
            store.associateIndexWithNewTranslog(translogUUID);
+
+            assert indexShard.getRetentionLeases().leases().isEmpty() : indexShard.getRetentionLeases(); // not loaded yet


peer recoveries can be retried if they fail midway through, so I wonder if this can be violated

I tried adding some recovery failures in 0990058 but this didn't trigger this assertion. But then I tried failing recoveries the other way round too in 8391e56 and did indeed get this to trigger, so have adjusted this code to deal with this.

DaveCTurner · 2019-03-07T11:37:30Z

@jasontedor @dnhatn just checking this PR hasn't been lost - it'd be good to hear your thoughts.

jasontedor · 2019-03-11T03:10:18Z

Thanks for the ping @DaveCTurner. I’ll look in my morning. Would you mark this PR as a blocker?

jasontedor

LGTM.

Today we load the shard history retention leases from disk whenever opening the engine, and treat a missing file as an empty set of leases. However in some cases this is inappropriate: we might be restoring from a snapshot (if the target index already exists then there may be leases on disk) or force-allocating a stale primary, and in neither case does it make sense to restore the retention leases from disk. With this change we write an empty retention leases file during recovery, except for the following cases: - During peer recovery the on-disk leases may be accurate and could be needed if the recovery target is made into a primary. - During recovery from an existing store, as long as we are not force-allocating a stale primary. Relates #37165

Today we load the shard history retention leases from disk whenever opening the engine, and treat a missing file as an empty set of leases. However in some cases this is inappropriate: we might be restoring from a snapshot (if the target index already exists then there may be leases on disk) or force-allocating a stale primary, and in neither case does it make sense to restore the retention leases from disk. With this change we write an empty retention leases file during recovery, except for the following cases: - During peer recovery the on-disk leases may be accurate and could be needed if the recovery target is made into a primary. - During recovery from an existing store, as long as we are not force-allocating a stale primary. Relates elastic#37165

Today we load the shard history retention leases from disk whenever opening the engine, and treat a missing file as an empty set of leases. However in some cases this is inappropriate: we might be restoring from a snapshot (if the target index already exists then there may be leases on disk) or force-allocating a stale primary, and in neither case does it make sense to restore the retention leases from disk. With this change we write an empty retention leases file during recovery, except for the following cases: - During peer recovery the on-disk leases may be accurate and could be needed if the recovery target is made into a primary. - During recovery from an existing store, as long as we are not force-allocating a stale primary. Relates #37165

…ate file Since elastic#39359, retention leases should always exist can never be null

`MetadataStateFormat.FORMAT.loadLatestState` can actually return null when the state directory hasn't been initialized yet, so we have to keep the null check when loading retention leases during the initialization of the engine. See #39359

`MetadataStateFormat.FORMAT.loadLatestState` can actually return null when the state directory hasn't been initialized yet, so we have to keep the null check when loading retention leases during the initialization of the engine. See elastic#39359

DaveCTurner added >bug :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v7.0.0 v6.7.0 v8.0.0 v7.2.0 labels Feb 25, 2019

DaveCTurner requested review from ywelsch, jasontedor and dnhatn February 25, 2019 13:10

ywelsch reviewed Feb 25, 2019

View reviewed changes

DaveCTurner added 8 commits February 26, 2019 08:16

Wipe out retention leases in StoreRecovery

eef3eca

May as well have the same comment here

29e5f4c

Push assertion into RecoverySource hierarchy

cf1c78e

Be strict about the existence of the retention leases file

d876843

Imports

28b999c

Defuse timebomb

774690b

TODO followup

ac2d378

TODO followup

e337bf7

DaveCTurner requested a review from ywelsch February 26, 2019 22:52

DaveCTurner changed the title ~~Recover leases only when recovering from store~~ Create retention leases file during recovery Feb 27, 2019

ywelsch reviewed Feb 28, 2019

View reviewed changes

DaveCTurner added 3 commits February 28, 2019 10:44

Cause some recoveries to fail

0990058

Disconnect target from source causing a recovery retry

8391e56

Merge branch 'master' into 2019-02-19-clear-retention-leases-on-restore

ea1e7db

DaveCTurner requested a review from ywelsch February 28, 2019 11:24

DaveCTurner added the blocker label Mar 11, 2019

jasontedor approved these changes Mar 13, 2019

View reviewed changes

DaveCTurner merged commit 9bc332a into elastic:master Mar 15, 2019

DaveCTurner deleted the 2019-02-19-clear-retention-leases-on-restore branch March 15, 2019 07:36

DaveCTurner mentioned this pull request Mar 15, 2019

Create retention leases file during recovery (#39359) #40082

Merged

michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

arteam added a commit to arteam/elasticsearch that referenced this pull request Oct 8, 2024

Throw an exception if we're unable to load retention leases from a st…

d2a9f58

…ate file Since elastic#39359, retention leases should always exist can never be null

arteam mentioned this pull request Oct 8, 2024

Throw an exception if we're unable to load retention leases from a state file #114298

Closed

arteam mentioned this pull request Oct 24, 2024

Clarify the null check for retention leases #114979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create retention leases file during recovery #39359

Create retention leases file during recovery #39359

DaveCTurner commented Feb 25, 2019 •

edited

Loading

elasticmachine commented Feb 25, 2019

DaveCTurner commented Feb 25, 2019

ywelsch Feb 25, 2019

dnhatn Feb 25, 2019

DaveCTurner Feb 26, 2019

dnhatn Feb 26, 2019

DaveCTurner Feb 26, 2019

DaveCTurner commented Feb 26, 2019

ywelsch Feb 28, 2019

DaveCTurner Feb 28, 2019

DaveCTurner commented Mar 7, 2019

jasontedor commented Mar 11, 2019

jasontedor left a comment

Create retention leases file during recovery #39359

Create retention leases file during recovery #39359

Conversation

DaveCTurner commented Feb 25, 2019 • edited Loading

elasticmachine commented Feb 25, 2019

DaveCTurner commented Feb 25, 2019

ywelsch Feb 25, 2019

Choose a reason for hiding this comment

dnhatn Feb 25, 2019

Choose a reason for hiding this comment

DaveCTurner Feb 26, 2019

Choose a reason for hiding this comment

dnhatn Feb 26, 2019

Choose a reason for hiding this comment

DaveCTurner Feb 26, 2019

Choose a reason for hiding this comment

DaveCTurner commented Feb 26, 2019

ywelsch Feb 28, 2019

Choose a reason for hiding this comment

DaveCTurner Feb 28, 2019

Choose a reason for hiding this comment

DaveCTurner commented Mar 7, 2019

jasontedor commented Mar 11, 2019

jasontedor left a comment

Choose a reason for hiding this comment

DaveCTurner commented Feb 25, 2019 •

edited

Loading