-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verify repository before cluster update #108531
Changes from all commits
0474f8c
2d63dc6
bfa15df
66eaa22
58d6852
4892603
8c4e77e
badc639
98bffa9
cd50ec2
ed55c01
d95ecd2
e7b5bb6
fcd79a6
418e167
7d0696f
219ddbc
b09dbf6
89f793f
811233b
d698530
f30c966
ec7a1a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -48,8 +48,6 @@ | |||||||||||||||
import org.elasticsearch.transport.Transport; | ||||||||||||||||
import org.elasticsearch.transport.TransportService; | ||||||||||||||||
import org.elasticsearch.xcontent.NamedXContentRegistry; | ||||||||||||||||
import org.junit.AfterClass; | ||||||||||||||||
import org.junit.BeforeClass; | ||||||||||||||||
|
||||||||||||||||
import java.util.Arrays; | ||||||||||||||||
import java.util.Collection; | ||||||||||||||||
|
@@ -65,28 +63,16 @@ | |||||||||||||||
|
||||||||||||||||
public class RepositoriesServiceTests extends ESTestCase { | ||||||||||||||||
|
||||||||||||||||
private static ThreadPool threadPool; | ||||||||||||||||
|
||||||||||||||||
private ClusterService clusterService; | ||||||||||||||||
private RepositoriesService repositoriesService; | ||||||||||||||||
|
||||||||||||||||
@BeforeClass | ||||||||||||||||
public static void createThreadPool() { | ||||||||||||||||
threadPool = new TestThreadPool(RepositoriesService.class.getName()); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
@AfterClass | ||||||||||||||||
public static void terminateThreadPool() { | ||||||||||||||||
if (threadPool != null) { | ||||||||||||||||
threadPool.shutdownNow(); | ||||||||||||||||
threadPool = null; | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
private ThreadPool threadPool; | ||||||||||||||||
|
||||||||||||||||
@Override | ||||||||||||||||
public void setUp() throws Exception { | ||||||||||||||||
super.setUp(); | ||||||||||||||||
|
||||||||||||||||
threadPool = new TestThreadPool(RepositoriesService.class.getName()); | ||||||||||||||||
|
||||||||||||||||
final TransportService transportService = new TransportService( | ||||||||||||||||
Settings.EMPTY, | ||||||||||||||||
mock(Transport.class), | ||||||||||||||||
|
@@ -113,6 +99,8 @@ public void setUp() throws Exception { | |||||||||||||||
TestRepository::new, | ||||||||||||||||
UnstableRepository.TYPE, | ||||||||||||||||
UnstableRepository::new, | ||||||||||||||||
VerificationFailRepository.TYPE, | ||||||||||||||||
VerificationFailRepository::new, | ||||||||||||||||
MeteredRepositoryTypeA.TYPE, | ||||||||||||||||
metadata -> new MeteredRepositoryTypeA(metadata, clusterService), | ||||||||||||||||
MeteredRepositoryTypeB.TYPE, | ||||||||||||||||
|
@@ -135,6 +123,10 @@ public void setUp() throws Exception { | |||||||||||||||
@Override | ||||||||||||||||
public void tearDown() throws Exception { | ||||||||||||||||
super.tearDown(); | ||||||||||||||||
if (threadPool != null) { | ||||||||||||||||
threadPool.shutdownNow(); | ||||||||||||||||
threadPool = null; | ||||||||||||||||
} | ||||||||||||||||
clusterService.stop(); | ||||||||||||||||
repositoriesService.stop(); | ||||||||||||||||
} | ||||||||||||||||
|
@@ -181,6 +173,44 @@ public void testRegisterRejectsInvalidRepositoryNames() { | |||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
public void testPutRepositoryVerificationFails() { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to also have a test that starts with an existing repository and demostrate a failed update (due to verfication) request does not change the existing repo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added new test |
||||||||||||||||
var repoName = randomAlphaOfLengthBetween(10, 25); | ||||||||||||||||
var request = new PutRepositoryRequest().name(repoName).type(VerificationFailRepository.TYPE).verify(true); | ||||||||||||||||
var resultListener = new SubscribableListener<AcknowledgedResponse>(); | ||||||||||||||||
repositoriesService.registerRepository(request, resultListener); | ||||||||||||||||
var failure = safeAwaitFailure(resultListener); | ||||||||||||||||
assertThat(failure, isA(RepositoryVerificationException.class)); | ||||||||||||||||
// also make sure that cluster state does not include failed repo | ||||||||||||||||
assertThrows(RepositoryMissingException.class, () -> { repositoriesService.repository(repoName); }); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
public void testPutRepositoryVerificationFailsOnExisting() { | ||||||||||||||||
var repoName = randomAlphaOfLengthBetween(10, 25); | ||||||||||||||||
var request = new PutRepositoryRequest().name(repoName).type(TestRepository.TYPE).verify(true); | ||||||||||||||||
var resultListener = new SubscribableListener<AcknowledgedResponse>(); | ||||||||||||||||
repositoriesService.registerRepository(request, resultListener); | ||||||||||||||||
var ackResponse = safeAwait(resultListener); | ||||||||||||||||
assertTrue(ackResponse.isAcknowledged()); | ||||||||||||||||
Comment on lines
+190
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think we can directly use
Suggested change
|
||||||||||||||||
|
||||||||||||||||
// try to update existing repository with faulty repo and make sure it is not applied | ||||||||||||||||
request = new PutRepositoryRequest().name(repoName).type(VerificationFailRepository.TYPE).verify(true); | ||||||||||||||||
resultListener = new SubscribableListener<>(); | ||||||||||||||||
repositoriesService.registerRepository(request, resultListener); | ||||||||||||||||
var failure = safeAwaitFailure(resultListener); | ||||||||||||||||
assertThat(failure, isA(RepositoryVerificationException.class)); | ||||||||||||||||
var repository = repositoriesService.repository(repoName); | ||||||||||||||||
assertEquals(repository.getMetadata().type(), TestRepository.TYPE); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
public void testPutRepositorySkipVerification() { | ||||||||||||||||
var repoName = randomAlphaOfLengthBetween(10, 25); | ||||||||||||||||
var request = new PutRepositoryRequest().name(repoName).type(VerificationFailRepository.TYPE).verify(false); | ||||||||||||||||
var resultListener = new SubscribableListener<AcknowledgedResponse>(); | ||||||||||||||||
repositoriesService.registerRepository(request, resultListener); | ||||||||||||||||
var ackResponse = safeAwait(resultListener); | ||||||||||||||||
assertTrue(ackResponse.isAcknowledged()); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
public void testRepositoriesStatsCanHaveTheSameNameAndDifferentTypeOverTime() { | ||||||||||||||||
String repoName = "name"; | ||||||||||||||||
expectThrows(RepositoryMissingException.class, () -> repositoriesService.repository(repoName)); | ||||||||||||||||
|
@@ -502,6 +532,19 @@ private UnstableRepository(RepositoryMetadata metadata) { | |||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
private static class VerificationFailRepository extends TestRepository { | ||||||||||||||||
public static final String TYPE = "verify-fail"; | ||||||||||||||||
|
||||||||||||||||
private VerificationFailRepository(RepositoryMetadata metadata) { | ||||||||||||||||
super(metadata); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
@Override | ||||||||||||||||
public String startVerification() { | ||||||||||||||||
throw new RepositoryVerificationException(TYPE, "failed to validate repository"); | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
private static class MeteredRepositoryTypeA extends MeteredBlobStoreRepository { | ||||||||||||||||
private static final String TYPE = "type-a"; | ||||||||||||||||
private static final RepositoryStats STATS = new RepositoryStats(Map.of("GET", 10L)); | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,7 +181,9 @@ public final void testSnapshotWithLargeSegmentFiles() throws Exception { | |
} | ||
|
||
public void testRequestStats() throws Exception { | ||
final String repository = createRepository(randomRepositoryName()); | ||
// need to use verify=false, because the verification process on master makes extra calls on placeholder repo | ||
// hence impacting http metrics and failing test | ||
final String repository = createRepository(randomRepositoryName(), false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Additional verification adds few more invocations on master node, failing all related tests in s3/gcp/azure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is not the case. The test failed because the repository master node uses for verification is different from the actual repository that gets created afterwards. The initial repository is closed and not counted towards |
||
final String index = "index-no-merges"; | ||
createIndex(index, 1, 0); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,7 @@ | |
import org.elasticsearch.TransportVersion; | ||
import org.elasticsearch.action.ActionFuture; | ||
import org.elasticsearch.action.RequestBuilder; | ||
import org.elasticsearch.action.support.ActionTestUtils; | ||
import org.elasticsearch.action.support.PlainActionFuture; | ||
import org.elasticsearch.action.support.SubscribableListener; | ||
import org.elasticsearch.bootstrap.BootstrapForTesting; | ||
|
@@ -2187,6 +2188,14 @@ public static <T> T safeGet(Future<T> future) { | |
} | ||
} | ||
|
||
public static Exception safeAwaitFailure(SubscribableListener<?> listener) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think we can have a variant of this method that takes |
||
return safeAwait( | ||
SubscribableListener.newForked( | ||
exceptionListener -> listener.addListener(ActionTestUtils.assertNoSuccessListener(exceptionListener::onResponse)) | ||
) | ||
); | ||
} | ||
|
||
public static void safeSleep(TimeValue timeValue) { | ||
safeSleep(timeValue.millis()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will deadlock on snapshot thread pool, we are running with single thread which is busy at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have this comment in the code please?