lazy snapshot repository initialization #31606

vladimirdolzhenko · 2018-06-27T10:09:53Z

Instead of eager initialization of underlying blob store in ctor this PR move actual initialization of underlying blob store to later stage.

It makes possible to register e.g. s3repo with bucket that is not available at the moment.

elasticmachine · 2018-06-27T10:09:55Z

Pinging @elastic/es-distributed

tlrx

I didn't look at the tests but I looked at the main components, it looks great. I think this is the right way to go.

I know we already discussed this but I don't like the fact that settings are sometimes verified in the repository constructor and sometimes in the blobstore creation method. I think we need to check all settings at the same place if they don't require any external access (I think the ctor is the right place for this but Yannick might have a different opinion).

tlrx · 2018-07-02T13:13:32Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

-public abstract class BlobStoreRepository extends AbstractLifecycleComponent implements Repository {
-
-    private BlobContainer snapshotsBlobContainer;
+public abstract class BlobStoreRepository<BS extends BlobStore> extends AbstractLifecycleComponent implements Repository {


Do we really need a generic here? It looks to me that it's useful for innerBlobStore() / blobContainer() methods used in tests but otherwise it's not really required.

tlrx · 2018-07-02T13:13:51Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -225,6 +224,12 @@

    private final ChecksumBlobStoreFormat<BlobStoreIndexShardSnapshots> indexShardSnapshotsFormat;

+    private final Object lock = new Object();
+
+    private volatile BlobContainer snapshotsBlobContainer;


Let's name this blobContainer too?

tlrx · 2018-07-02T13:15:03Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

-            blobStore().close();
-        } catch (Exception t) {
-            logger.warn("cannot close blob store", t);
+        BlobStore store = blobStore;


I'd expect this to be in a synchronized block

I don't see any reason for sync here as blobStore is volatile. It could be reasonable to add opened/closed flag to avoid reinit after repo is closed

BlobStoreRepository is a lifecycle component - no any need for open/close flag there

Lifecycle component is not concurrency-aware, and not all methods that create the blobstore etc. are guarded by "if-not-closed" checks. We need to guard against creating a blobstore / container after the closing has started

tlrx · 2018-07-02T13:48:18Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+    }
+
+    // for test purposes only
+    protected BS innerBlobStore() {


If you need this in test, you can still call it getBlobStore()

vladimirdolzhenko · 2018-07-02T14:33:54Z

thanks @tlrx for your comments - make sense

will move checks (those don't require extra storage call) to ctor

ywelsch

I've left some comments. Overall approach makes sense to me.

ywelsch · 2018-07-09T07:06:41Z

modules/repository-url/src/main/java/org/elasticsearch/repositories/url/URLRepository.java

+
+    @Override
+    protected BlobStore createBlobStore() {
+        return new URLBlobStore(settings, url);


previously this was using the normalizedURL? Why not anymore?

Also, why is this not breaking any tests?

lack of test, adding it

ywelsch · 2018-07-09T07:07:24Z

modules/repository-url/src/main/java/org/elasticsearch/repositories/url/URLRepository.java

        basePath = BlobPath.cleanPath();
+        url = URL_SETTING.exists(metadata.settings())
+            ? URL_SETTING.get(metadata.settings()) : REPOSITORIES_URL_SETTING.get(settings);
+        checkURL(url);


I think this check should be done as part of createBlobStore

ywelsch · 2018-07-09T07:12:01Z

modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java

            new NamedXContentRegistry(Collections.emptyList()));
+
+        assertThat("blob store has to be lazy initialized", repository.getBlobStore(), is(nullValue()));


This looks to be testing the lazy initialization of BlobStoreRepository, not URLRepository? Do we need to expose the methods in URLRepository and test this at this level?

URLRepositoryTests doesn't extend ESBlobStoreRepositoryIntegTestCase (as FsBlobStoreRepositoryIT, S3BlobStoreRepositoryTests and GoogleCloudStorageBlobStoreRepositoryTests) - it looks like more due to read-only nature of url repo - therefore such kind of check is done on this level

ywelsch · 2018-07-09T07:16:08Z

...ins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java

+        if (Repository.READONLY_SETTING.exists(metadata.settings())) {
+            this.readonly = Repository.READONLY_SETTING.get(metadata.settings());
+        } else {
+            this.readonly = blobStore.getLocationMode() == LocationMode.SECONDARY_ONLY;


I think this can all be done in the constructor. The location mode is determined through a setting, just as the read-only setting. There's no need to create the BlobStore to determine the location mode.

ywelsch · 2018-07-09T07:20:53Z

...itory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java


    GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment environment,
                                        NamedXContentRegistry namedXContentRegistry,
-                                        GoogleCloudStorageService storageService) throws Exception {
+                                        GoogleCloudStorageService storageService){


space missing

ywelsch · 2018-07-09T07:50:13Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

     */
-    protected abstract BlobStore blobStore();
+    protected BlobStore blobStore() {
+        BlobStore store = blobStore;


let's add an assertion that this method is only called from the snapshot thread.

it could be done on generic thread e.g. from IndexShard.restoreFromRepository chain:

at org.elasticsearch.repositories.blobstore.BlobStoreRepository.verificationThreadCheck(BlobStoreRepository.java:608) at org.elasticsearch.repositories.blobstore.BlobStoreRepository.blobContainer(BlobStoreRepository.java:301) at org.elasticsearch.repositories.blobstore.BlobStoreRepository.listBlobsToGetLatestIndexId(BlobStoreRepository.java:809) at org.elasticsearch.repositories.blobstore.BlobStoreRepository.latestIndexBlobId(BlobStoreRepository.java:787) at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:650) at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:454) at org.elasticsearch.index.shard.StoreRecovery.lambda$recoverFromRepository$5(StoreRecovery.java:278) at org.elasticsearch.index.shard.StoreRecovery.executeRecovery(StoreRecovery.java:301) at org.elasticsearch.index.shard.StoreRecovery.recoverFromRepository(StoreRecovery.java:276) at org.elasticsearch.index.shard.IndexShard.restoreFromRepository(IndexShard.java:1547) at org.elasticsearch.index.shard.IndexShard.lambda$startRecovery$6(IndexShard.java:2015)

ok, then assert that it's either snapshot or generic threadpool

can you address this comment, i.e. make sure that the blobStore is only accessed through the snapshot / generic thread.

ywelsch · 2018-07-09T07:50:20Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+     * maintains single lazy instance of {@link BlobContainer}
+     */
+    protected BlobContainer blobContainer() {
+        BlobContainer blobContainer = this.blobContainer;


let's add an assertion that this method is only called from the snapshot thread.

Can you address this comment please?

ywelsch · 2018-07-09T07:54:40Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

        try {
            if (isReadOnly()) {
-                // It's readonly - so there is not much we can do here to verify it
+                // It's readonly - so there is not much we can do here to verify it apart try to create blobStore()
+                blobStore();


this only verifies the repo on the master, not the other nodes...

do you mean that comment has to be - try the best - try to create blobStore on master ?

I think we need to extend verification so that it also works on read-only repos (and make it work in BWC-compatible fashion). Also, this should probably call blobStore().blobContainer(basePath()).
We can do that in a follow-up, but for now add a comment that this currently only checks that the repository is accessible on the master. // TODO: add repository verification for read-only repositories

makes sense to me

ywelsch · 2018-07-09T08:00:17Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

-            blobStore().close();
-        } catch (Exception t) {
-            logger.warn("cannot close blob store", t);
+        BlobStore store = blobStore;


Lifecycle component is not concurrency-aware, and not all methods that create the blobstore etc. are guarded by "if-not-closed" checks. We need to guard against creating a blobstore / container after the closing has started

ywelsch · 2018-07-09T08:05:36Z

...c/main/java/org/elasticsearch/repositories/blobstore/ESBlobStoreRepositoryIntegTestCase.java

+        createTestRepository(name, verify);
+
+        final RepositoriesService repositoriesService =
+            internalCluster().getDataOrMasterNodeInstances(RepositoriesService.class).iterator().next();


let's check all of them?

makes sense

…meters are moved to ctors; using SetOnce and lifecycle-aware lazy creation

vladimirdolzhenko · 2018-07-09T11:18:12Z

Thanks @ywelsch for review - I did relevant changes - one comment is still under question (to me) - is to add assertion on snapshot thread (pls check comment)

vladimirdolzhenko · 2018-07-10T07:41:10Z

test this please

vladimirdolzhenko · 2018-07-10T08:32:49Z

test this please

ywelsch · 2018-07-10T12:53:23Z

...ins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java

+
+    // only use for testing
+    @Override
+    protected BlobStore getBlobStore() {


can this be removed now?

nope, it is in use in AzureRepositorySettingsTests

ywelsch · 2018-07-10T12:59:57Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+            synchronized (lock) {
+                store = blobStore.get();
+                if (store == null) {
+                    if (lifecycle.started() == false) {


this is not concurrency-safe. Closing can be happening concurrently to this.

ywelsch · 2018-07-10T13:06:02Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

     */
-    protected abstract BlobStore blobStore();
+    protected BlobStore blobStore() {
+        BlobStore store = blobStore;


ok, then assert that it's either snapshot or generic threadpool

… started during close

ywelsch · 2018-07-12T12:38:59Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+                    if (lifecycle.started() == false) {
+                        throw new RepositoryException(metadata.name(), "repository is not in started state");
+                    }
+                    verificationThreadCheck();


let's check this on every access, not only creation.

i see your point - you'd like to enforce any access to blobStore rather just initialization; there are some non-integration test failures - gonna fix it soon

…neric threads

ywelsch

I've left one more comment. No need for another iteration though. LGTM

ywelsch · 2018-07-13T14:50:35Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -543,11 +613,21 @@ public long getRestoreThrottleTimeInNanos() {
        return restoreRateLimitingTimeInNanos.count();
    }

+    protected void verificationThreadCheck() {


we're not only checking the verification thread here, but all operations that are accessing the blobstore. Can you rename this method?

…d couple comments for assertSnapshotOrGenericThread and the reason of FsLikeRepoPlugin in BlobStoreRepositoryTests.java

vladimirdolzhenko · 2018-07-13T18:10:52Z

thanks @ywelsch for the review

(cherry picked from commit b1bf643)

* 6.x: Watcher: Make settings reloadable (#31746) [Rollup] Histo group config should support scaled_floats (#32048) lazy snapshot repository initialization (#31606) Add secure setting for watcher email password (#31620) Watcher: cleanup ensureWatchExists use (#31926) Add second level of field collapsing (#31808) Added lenient flag for synonym token filter (#31484) (#31970) Test: Fix a second case of bad watch creation [Rollup] Use composite's missing_bucket (#31402) Docs: Restyled cloud link in getting started Docs: Change formatting of Cloud options Re-instate link in StringFunctionUtils javadocs Correct spelling of AnalysisPlugin#requriesAnalysisSettings (#32025) Fix problematic chars in javadoc [ML] Move open job failure explanation out of root cause (#31925) [ML] Switch ML native QA tests to use a 3 node cluster (#32011)

lazy snapshot repository initialization

3dd9d22

vladimirdolzhenko added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 labels Jun 27, 2018

vladimirdolzhenko requested a review from tlrx June 27, 2018 10:09

vladimirdolzhenko added the review label Jun 27, 2018

Vladimir Dolzhenko added 6 commits June 27, 2018 21:00

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

16d9b40

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

848f346

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

79ab701

drop unused import

2eaed57

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

3b03345

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

4b4b837

tlrx reviewed Jul 2, 2018

View reviewed changes

Vladimir Dolzhenko added 4 commits July 2, 2018 17:32

move checks to ctor; rename methods for consistency

c89a103

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

89e2310

fix URLRepositoryTests

3529a7e

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

608f644

ywelsch reviewed Jul 9, 2018

View reviewed changes

changes due to Yannick's review - test code clean up; blob store para…

bc60b22

…meters are moved to ctors; using SetOnce and lifecycle-aware lazy creation

Vladimir Dolzhenko added 3 commits July 9, 2018 13:57

extend comment for r/o repo verification

3a322aa

added missed start

f84ec61

added missed start

da5c07f

ywelsch reviewed Jul 10, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits July 10, 2018 17:38

fix test; close blobStore if a concurrent blobStore initialization is…

d25b248

… started during close

fix thread check for unit test

b2ac9d8

Vladimir Dolzhenko added 2 commits July 11, 2018 15:00

fix thread check for unit test

c6b836a

Merge remote-tracking branch 'remotes/origin/master' into lazy_repo_init

d8c9593

vladimirdolzhenko requested a review from ywelsch July 12, 2018 07:29

ywelsch reviewed Jul 12, 2018

View reviewed changes

enforcing access to blobStore / blobContainer only to snapshot and ge…

8dfa60d

…neric threads

vladimirdolzhenko requested a review from ywelsch July 12, 2018 17:27

ywelsch approved these changes Jul 13, 2018

View reviewed changes

verifyBlobInnersAccess renamed to assertSnapshotOrGenericThread; adde…

ae3ee24

…d couple comments for assertSnapshotOrGenericThread and the reason of FsLikeRepoPlugin in BlobStoreRepositoryTests.java

vladimirdolzhenko added the v6.4.0 label Jul 13, 2018

vladimirdolzhenko merged commit b1bf643 into elastic:master Jul 13, 2018

vladimirdolzhenko deleted the lazy_repo_init branch July 13, 2018 18:07

vladimirdolzhenko added backport pending and removed review labels Jul 13, 2018

vladimirdolzhenko pushed a commit that referenced this pull request Jul 13, 2018

lazy snapshot repository initialization (#31606)

6509c59

(cherry picked from commit b1bf643)

vladimirdolzhenko removed the backport pending label Jul 13, 2018

tlrx mentioned this pull request Aug 22, 2018

[CI] "s3 repository can be registered" test fails intermittently #26240

Closed

original-brownbear mentioned this pull request Dec 3, 2018

Don't create snapshot repositories on the cluster state update thread #9488

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

original-brownbear mentioned this pull request Feb 14, 2019

S3 snapshotting times out requiring Elasticsearch process restart #38272

Closed

		new NamedXContentRegistry(Collections.emptyList()));

		assertThat("blob store has to be lazy initialized", repository.getBlobStore(), is(nullValue()));

lazy snapshot repository initialization #31606

lazy snapshot repository initialization #31606

Conversation

vladimirdolzhenko commented Jun 27, 2018

elasticmachine commented Jun 27, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Jul 2, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Jul 9, 2018

vladimirdolzhenko commented Jul 10, 2018

vladimirdolzhenko commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Jul 13, 2018