Distinguish missing and invalid repositories #85551

mushao999 · 2022-03-31T13:36:44Z

The RepositoryService on each node maintains a repository instance for
every repository defined in the cluster state. The master validates each
repository definition before inserting it into the cluster state, but in
some cases this validation is incomplete. For instance, there may be
node-local configuration of which the master is unaware which prevents
the repository from being instantiated on some other node in the
cluster.

Today if a repository cannot be instantiated then the node will log a
warning and continue as if the repository doesn't exist. This results in
a confusing RepositoryMissingException when trying to use the
repository, and various other surprises (e.g. #85550). With this commit
we create a placeholder InvalidRepository which reports a more
accurate exception when it is used.

Relates #82457 which did the same sort of thing for unknown plugins.
Closes #85550 since the repository in question is no longer null.

DaveCTurner · 2022-03-31T13:43:49Z

...nternalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java

+            if (unstableNodes.contains(clusterService.getNodeName())) {
+                throw new RepositoryException(metadata.name(), "Failed to create repository: current node is not stable");


Can you give a more realistic example of how this might happen? Perhaps a better fix would be to avoid throwing an exception when creating the repository altogether.

In our case ,we use alibaba OSS(an Object Storage service) to store snapshot, but some time the network may not be good enough to connect, which may make the reppository creatation failed

And more general, repository creation logic is open to be implement in repository plugin which is uncontrolled, so if the implement is not good enough creation failed can happen.

This sounds like a bug in your plugin, it should not require a healthy network to create the repository. Compare for instance S3Repository which merely validates some settings in its constructor.

This sounds like a bug in your plugin, it should not require a healthy network to create the repository. Compare for instance S3Repository which merely validates some settings in its constructor.

I‘ve discuss with the plugin owner. It's true there are some network action during repository creation which can cause creation failure.
However, I still think elasticsearch core should not lay it's robustness on plugin implement.

I still think elasticsearch core should not lay it's robustness on plugin implement.

Yes I agree, I just want to be clear that protecting against this NPE will not really address the problem in the plugin. If transient network issues can prevent the plugin repository from even being created on some nodes then you will need to fix this by retrying the put-repository request once the transient issues are resolved. I don't see a great way to detect that a retry is needed here.

server/src/main/java/org/elasticsearch/cluster/service/MasterService.java

DaveCTurner

I would rather fix this using an approach closer to the one in #82457 which avoids the null by creating a dummy repository that returns a more specific exception message than the RepositoryMissingException we get today. The repository isn't really missing, it's invalid on some nodes, and we should distinguish these cases.

elasticmachine · 2022-04-01T07:42:06Z

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

mushao999 · 2022-04-01T09:10:05Z

Thanks, I will update in that way.

mushao999 · 2022-04-01T14:13:41Z

@DaveCTurner hi, I've update this PR by adding a new DummyRepository, please help to reivew it.

mushao999 · 2022-04-01T14:30:53Z

@DaveCTurner By the way , there is a small PR of mine which has been there for a long time , could you please help to review it as well: #83706

DaveCTurner

I like it, thanks @mushao999. I left a few small comments.

server/src/main/java/org/elasticsearch/repositories/DummyRepository.java

server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java

...nternalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java

mushao999 · 2022-04-01T16:04:07Z

@DaveCTurner Thanks for your suggestion, I've updated the code.

mushao999 · 2022-04-01T16:13:25Z

Should we add fail("xxx"); after code.run();?

elasticsearch/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java

Lines 21 to 31 in caa0b61

    
           public static void assertThatThrows( 
        
               LuceneTestCase.ThrowingRunnable code, 
        
               Class<? extends Exception> exceptionType, 
        
               Matcher<String> messageMatcher 
        
           ) { 
        
               try { 
        
                   code.run(); 
        
               } catch (Throwable e) { 
        
                   assertThatException(e, exceptionType, messageMatcher); 
        
               } 
        
           }

DaveCTurner · 2022-04-01T16:22:06Z

Should we add fail("xxx"); after code.run();?

Yes, I think so.

mushao999 · 2022-04-01T16:30:01Z

Should we add fail("xxx"); after code.run();?

Yes, I think so.

so we add it in this PR or maybe open a new one to add it?

DaveCTurner · 2022-04-01T16:31:03Z

Here would be fine with me.

DaveCTurner · 2022-04-01T16:32:37Z

@elasticmachine ok to test

mushao999 · 2022-04-01T16:38:52Z

Here would be fine with me.

code added

DaveCTurner · 2022-04-01T17:10:46Z

Looks like your branch is based off an old commit, you will need to merge latest master before CI will pass.

DaveCTurner

Looks good, I left a handful of comments and suggested changes but structurally this is good.

server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java

test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java

...rTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java

...nternalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java

DaveCTurner · 2022-04-02T11:39:24Z

...nternalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java

+        );
+        // verification should fail with some node has InvalidRepository
+        try {
+            client().admin().cluster().prepareVerifyRepository(repositoryName).get();


Suggest using expectThrows here:

Suggested change

client().admin().cluster().prepareVerifyRepository(repositoryName).get();

final var verificationException = expectThrows(RepositoryVerificationException.class,

() -> client().admin().cluster().prepareVerifyRepository(repositoryName).get());

We can not use expectThrows if we want to assert inner exception.

...nternalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java

DaveCTurner

I think you missed that expectThrows() returns the exception so you can assert more things about it. I think we can use expectThrows rather than the utilities in ThrowableAssertions here, and I'll open a follow-up to remove ThrowableAssertions entirely.

Edit: see #85671

DaveCTurner · 2022-04-04T10:27:29Z

server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java

+        // verification should fail with some node has InvalidRepository
+        boolean verifyPass = false;
+        try {
+            client().admin().cluster().prepareVerifyRepository(repositoryName).get();


Mentioned before, but this should be final var repositoryVerificationException = expectThrows(RepositoryVerificationException.class, () -> client().admin().cluster().prepareVerifyRepository(repositoryName).get());

Note that expectThrows returns the exception it caught so you can assert things about it such as its causes and suppressed exceptions.

DaveCTurner · 2022-04-04T10:48:56Z

See #85671 for the PR to remove ThrowableAssertions in favour of using expectThrows.

mushao999 · 2022-04-04T14:35:41Z

@DaveCTurner Thanks for your suggestion , I've replaced all the ThrowableAssertions with expectedThrows.

DaveCTurner

LGTM, thanks for the extra iterations @mushao999

mushao999 added 2 commits March 31, 2022 20:45

add RepositoriesDuplicatedCreationIT

20a051b

fix NPE in RepositoriesService

5660a78

elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.3.0 labels Mar 31, 2022

mushao999 mentioned this pull request Mar 31, 2022

NPE in RepositoriesService #85550

Closed

DaveCTurner reviewed Mar 31, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/service/MasterService.java Outdated Show resolved Hide resolved

DaveCTurner requested changes Apr 1, 2022

View reviewed changes

DaveCTurner added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Apr 1, 2022

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Apr 1, 2022

DaveCTurner self-assigned this Apr 1, 2022

mushao999 added 3 commits April 1, 2022 17:21

revert MasterService change

6fe56a1

create DummyRepository when repository creation is failed

076f790

add changelog file

8ca6af5

DaveCTurner reviewed Apr 1, 2022

View reviewed changes

optimize code

db544c1

add fail after code.run in ThrowableAssertions#assertThatThrows

c9759e3

mushao999 added 2 commits April 2, 2022 08:30

Merge remote-tracking branch 'origin/master' into fix/put_snapshot_npe

121240a

fix shard failure reason in EncryptedRepositorySecretIntegTests

87011b7

DaveCTurner changed the title ~~Fix NPE in RepositoriesService~~ Distinguish missing and invalid repositories Apr 2, 2022

DaveCTurner requested changes Apr 2, 2022

View reviewed changes

mushao999 added 4 commits April 2, 2022 22:53

update

323aa3e

remove cluster Scope settings in InvalidRepositoryIT

d3f881a

we should ensure at least two node in cluster

a6527cb

fix typo in UnkonwnTypeRepository

59b8faa

DaveCTurner reviewed Apr 4, 2022

View reviewed changes

Use expectThrows instead of ThrowableAssertions

f2b5575

DaveCTurner approved these changes Apr 4, 2022

View reviewed changes

DaveCTurner merged commit 133e34d into elastic:master Apr 4, 2022

		if (unstableNodes.contains(clusterService.getNodeName())) {
		throw new RepositoryException(metadata.name(), "Failed to create repository: current node is not stable");

	client().admin().cluster().prepareVerifyRepository(repositoryName).get();
	final var verificationException = expectThrows(RepositoryVerificationException.class,
	() -> client().admin().cluster().prepareVerifyRepository(repositoryName).get());

Distinguish missing and invalid repositories #85551

Distinguish missing and invalid repositories #85551

Uh oh!

Conversation

mushao999 commented Mar 31, 2022 • edited by DaveCTurner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

mushao999 Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mushao999 Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

mushao999 Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 1, 2022

Uh oh!

mushao999 commented Apr 1, 2022

Uh oh!

mushao999 commented Apr 1, 2022

Uh oh!

mushao999 commented Apr 1, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mushao999 commented Apr 1, 2022

Uh oh!

mushao999 commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner commented Apr 1, 2022

Uh oh!

mushao999 commented Apr 1, 2022

Uh oh!

DaveCTurner commented Apr 1, 2022

Uh oh!

DaveCTurner commented Apr 1, 2022

Uh oh!

mushao999 commented Apr 1, 2022

Uh oh!

DaveCTurner commented Apr 1, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

mushao999 Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

mushao999 commented Mar 31, 2022 •

edited by DaveCTurner

Loading

mushao999 Mar 31, 2022 •

edited

Loading

DaveCTurner Apr 1, 2022 •

edited

Loading

mushao999 commented Apr 1, 2022 •

edited

Loading

DaveCTurner left a comment •

edited

Loading