-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Archive-Index upgrade compatibility #118599
Changes from 15 commits
da8f1e2
713384f
f69c5ff
f311c3d
78083bc
0b80ca6
387bf82
5cfa128
6a3cceb
6c54c50
9a63102
476328f
884a9e0
79960b3
2fcb1ca
e8d6fee
acec518
b566c30
60ecd0b
cbd9a4a
df26c76
cd15012
10a227b
146334d
ece34cc
c395ddf
1362eae
07f0553
817afbb
81333dd
8b6068c
57384ae
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 |
---|---|---|
@@ -0,0 +1,25 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
apply plugin: 'elasticsearch.internal-java-rest-test' | ||
apply plugin: 'elasticsearch.internal-test-artifact' | ||
apply plugin: 'elasticsearch.bwc-test' | ||
|
||
buildParams.bwcVersions.withLatestReadOnlyIndexCompatible { bwcVersion -> | ||
tasks.named("javaRestTest").configure { | ||
systemProperty("tests.minimum.index.compatible", bwcVersion) | ||
usesBwcDistribution(bwcVersion) | ||
enabled = true | ||
} | ||
} | ||
|
||
tasks.withType(Test).configureEach { | ||
// CI doesn't like it when there's multiple clusters running at once | ||
maxParallelForks = 1 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,211 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.oldrepos; | ||
|
||
import com.carrotsearch.randomizedtesting.TestMethodAndParams; | ||
import com.carrotsearch.randomizedtesting.annotations.Name; | ||
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; | ||
import com.carrotsearch.randomizedtesting.annotations.TestCaseOrdering; | ||
|
||
import org.apache.http.util.EntityUtils; | ||
import org.elasticsearch.client.Request; | ||
import org.elasticsearch.client.Response; | ||
import org.elasticsearch.client.RestClient; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.repositories.fs.FsRepository; | ||
import org.elasticsearch.test.cluster.ElasticsearchCluster; | ||
import org.elasticsearch.test.cluster.local.LocalClusterConfigProvider; | ||
import org.elasticsearch.test.cluster.local.distribution.DistributionType; | ||
import org.elasticsearch.test.cluster.util.Version; | ||
import org.elasticsearch.test.rest.ESRestTestCase; | ||
import org.junit.Before; | ||
import org.junit.ClassRule; | ||
import org.junit.rules.RuleChain; | ||
import org.junit.rules.TemporaryFolder; | ||
import org.junit.rules.TestRule; | ||
|
||
import java.io.IOException; | ||
import java.io.OutputStream; | ||
import java.net.URISyntaxException; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
import java.util.Comparator; | ||
import java.util.Objects; | ||
import java.util.stream.Stream; | ||
import java.util.zip.ZipEntry; | ||
import java.util.zip.ZipInputStream; | ||
|
||
import static org.elasticsearch.test.cluster.util.Version.CURRENT; | ||
import static org.elasticsearch.test.cluster.util.Version.fromString; | ||
import static org.elasticsearch.test.rest.ObjectPath.createFromResponse; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.notNullValue; | ||
|
||
@TestCaseOrdering(AbstractUpgradeCompatibilityTestCase.TestCaseOrdering.class) | ||
public abstract class AbstractUpgradeCompatibilityTestCase extends ESRestTestCase { | ||
|
||
protected static final Version VERSION_MINUS_2 = fromString(System.getProperty("tests.minimum.index.compatible")); | ||
protected static final Version VERSION_MINUS_1 = fromString(System.getProperty("tests.minimum.wire.compatible")); | ||
protected static final Version VERSION_CURRENT = CURRENT; | ||
|
||
protected static TemporaryFolder REPOSITORY_PATH = new TemporaryFolder(); | ||
|
||
protected static LocalClusterConfigProvider clusterConfig = c -> {}; | ||
private static ElasticsearchCluster cluster = ElasticsearchCluster.local() | ||
.distribution(DistributionType.DEFAULT) | ||
.version(VERSION_MINUS_1) | ||
.nodes(2) | ||
.setting("xpack.security.enabled", "false") | ||
.setting("xpack.ml.enabled", "false") | ||
.setting("path.repo", () -> REPOSITORY_PATH.getRoot().getPath()) | ||
.apply(() -> clusterConfig) | ||
.build(); | ||
|
||
@ClassRule | ||
public static TestRule ruleChain = RuleChain.outerRule(REPOSITORY_PATH).around(cluster); | ||
|
||
private static boolean upgradeFailed = false; | ||
|
||
private final Version clusterVersion; | ||
|
||
public AbstractUpgradeCompatibilityTestCase(@Name("cluster") Version clusterVersion) { | ||
this.clusterVersion = clusterVersion; | ||
} | ||
|
||
@ParametersFactory | ||
public static Iterable<Object[]> parameters() { | ||
return Stream.of(VERSION_MINUS_1, CURRENT).map(v -> new Object[] { v }).toList(); | ||
} | ||
|
||
@Override | ||
protected String getTestRestCluster() { | ||
return cluster.getHttpAddresses(); | ||
} | ||
|
||
@Override | ||
protected boolean preserveClusterUponCompletion() { | ||
return true; | ||
} | ||
|
||
/** | ||
* This method verifies the currentVersion against the clusterVersion and performs a "full cluster restart" upgrade if the current | ||
* is before clusterVersion. The cluster version is fetched externally and is controlled by the gradle setup. | ||
* | ||
* @throws Exception | ||
*/ | ||
@Before | ||
public void maybeUpgrade() throws Exception { | ||
// We want to use this test suite for the V9 upgrade, but we are not fully committed to necessarily having N-2 support | ||
// in V10, so we add a check here to ensure we'll revisit this decision once V10 exists. | ||
assertThat("Explicit check that N-2 version is Elasticsearch 7", VERSION_MINUS_2.getMajor(), equalTo(7)); | ||
|
||
var currentVersion = clusterVersion(); | ||
if (currentVersion.before(clusterVersion)) { | ||
try { | ||
cluster.upgradeToVersion(clusterVersion); | ||
closeClients(); | ||
initClient(); | ||
} catch (Exception e) { | ||
upgradeFailed = true; | ||
throw e; | ||
} | ||
} | ||
|
||
// Skip remaining tests if upgrade failed | ||
assumeFalse("Cluster upgrade failed", upgradeFailed); | ||
} | ||
|
||
protected static Version clusterVersion() throws Exception { | ||
var response = assertOK(client().performRequest(new Request("GET", "/"))); | ||
var responseBody = createFromResponse(response); | ||
var version = Version.fromString(responseBody.evaluate("version.number").toString()); | ||
assertThat("Failed to retrieve cluster version", version, notNullValue()); | ||
return version; | ||
} | ||
|
||
/** | ||
* Execute the test suite with the parameters provided by the {@link #parameters()} in version order. | ||
*/ | ||
public static class TestCaseOrdering implements Comparator<TestMethodAndParams> { | ||
@Override | ||
public int compare(TestMethodAndParams o1, TestMethodAndParams o2) { | ||
var version1 = (Version) o1.getInstanceArguments().get(0); | ||
var version2 = (Version) o2.getInstanceArguments().get(0); | ||
return version1.compareTo(version2); | ||
} | ||
} | ||
|
||
public final void verifyArchiveIndexCompatibility(String version) throws Exception { | ||
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. The |
||
final String repository = "repository"; | ||
final String snapshot = "snapshot"; | ||
final String index = "index"; | ||
final int numDocs = 5; | ||
|
||
String repositoryPath = REPOSITORY_PATH.getRoot().getPath(); | ||
|
||
if (VERSION_MINUS_1.equals(clusterVersion())) { | ||
assertEquals(VERSION_MINUS_1, clusterVersion()); | ||
assertTrue(getIndices(client()).isEmpty()); | ||
|
||
// Copy a snapshot of an index with 5 documents | ||
copySnapshotFromResources(repositoryPath, version); | ||
registerRepository(client(), repository, FsRepository.TYPE, true, Settings.builder().put("location", repositoryPath).build()); | ||
recover(client(), repository, snapshot, index); | ||
|
||
assertTrue(getIndices(client()).contains(index)); | ||
assertDocCount(client(), index, numDocs); | ||
|
||
return; | ||
} | ||
|
||
if (VERSION_CURRENT.equals(clusterVersion())) { | ||
assertEquals(VERSION_CURRENT, clusterVersion()); | ||
assertTrue(getIndices(client()).contains(index)); | ||
assertDocCount(client(), index, numDocs); | ||
} | ||
} | ||
|
||
public abstract void recover(RestClient restClient, String repository, String snapshot, String index) throws Exception; | ||
|
||
private static String getIndices(RestClient client) throws IOException { | ||
final Request request = new Request("GET", "_cat/indices"); | ||
Response response = client.performRequest(request); | ||
return EntityUtils.toString(response.getEntity()); | ||
} | ||
|
||
private static void copySnapshotFromResources(String repositoryPath, String version) throws IOException, URISyntaxException { | ||
Path zipFilePath = Paths.get( | ||
Objects.requireNonNull(AbstractUpgradeCompatibilityTestCase.class.getClassLoader().getResource("snapshot_v" + version + ".zip")) | ||
.toURI() | ||
); | ||
unzip(zipFilePath, Paths.get(repositoryPath)); | ||
} | ||
|
||
private static void unzip(Path zipFilePath, Path outputDir) throws IOException { | ||
try (ZipInputStream zipIn = new ZipInputStream(Files.newInputStream(zipFilePath))) { | ||
ZipEntry entry; | ||
while ((entry = zipIn.getNextEntry()) != null) { | ||
Path outputPath = outputDir.resolve(entry.getName()); | ||
if (entry.isDirectory()) { | ||
Files.createDirectories(outputPath); | ||
} else { | ||
Files.createDirectories(outputPath.getParent()); | ||
try (OutputStream out = Files.newOutputStream(outputPath)) { | ||
byte[] buffer = new byte[1024]; | ||
int len; | ||
while ((len = zipIn.read(buffer)) > 0) { | ||
out.write(buffer, 0, len); | ||
} | ||
} | ||
} | ||
zipIn.closeEntry(); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.oldrepos.archiveindex; | ||
|
||
import org.elasticsearch.client.Request; | ||
import org.elasticsearch.client.RestClient; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.oldrepos.AbstractUpgradeCompatibilityTestCase; | ||
import org.elasticsearch.test.cluster.util.Version; | ||
|
||
import static org.elasticsearch.test.rest.ObjectPath.createFromResponse; | ||
|
||
/** | ||
* Test suite for Archive indices backward compatibility with N-2 versions. | ||
* The test suite creates a cluster in the N-1 version, where N is the current version. | ||
* Restores snapshots from old-clusters (version 5/6) and upgrades it to the current version. | ||
* Test methods are executed after each upgrade. | ||
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. I think it would help to provide an example with concrete versions as well., for clarity 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. I'll do so, ty! |
||
*/ | ||
public class ArchiveIndexTestCase extends AbstractUpgradeCompatibilityTestCase { | ||
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. I think I could use some upgrade mention in this class name as well given it's what it tests? You probably wanted to make a distinction between archive indices restored from snapshot and those mounted as searchable snapshots? 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. Correct, that was my intention. I considered that the |
||
|
||
static { | ||
clusterConfig = config -> config.setting("xpack.license.self_generated.type", "trial"); | ||
} | ||
|
||
public ArchiveIndexTestCase(Version version) { | ||
super(version); | ||
} | ||
|
||
/** | ||
* Overrides the snapshot-restore operation for archive-indices scenario. | ||
* | ||
* @param client | ||
* @param repository | ||
* @param snapshot | ||
* @param index | ||
* @throws Exception | ||
*/ | ||
@Override | ||
public void recover(RestClient client, String repository, String snapshot, String index) throws Exception { | ||
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. I would call this method importArchiveIndex or something along those lines. 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. Using the |
||
var request = new Request("POST", "/_snapshot/" + repository + "/" + snapshot + "/_restore"); | ||
request.addParameter("wait_for_completion", "true"); | ||
request.setJsonEntity(Strings.format(""" | ||
{ | ||
"indices": "%s", | ||
"include_global_state": false, | ||
"rename_pattern": "(.+)", | ||
"include_aliases": false | ||
}""", index)); | ||
createFromResponse(client.performRequest(request)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.oldrepos.archiveindex; | ||
|
||
import org.elasticsearch.test.cluster.util.Version; | ||
|
||
public class RestoreFromVersion5IT extends ArchiveIndexTestCase { | ||
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. Because the Gradle setup is not flexible, I needed to divide the different base versions into separate classes. For each case, we need a fresh cluster with version Current-1. |
||
|
||
public RestoreFromVersion5IT(Version version) { | ||
super(version); | ||
} | ||
|
||
public void testArchiveIndicesV6() throws Exception { | ||
verifyArchiveIndexCompatibility("6"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.oldrepos.archiveindex; | ||
|
||
import org.elasticsearch.test.cluster.util.Version; | ||
|
||
public class RestoreFromVersion6IT extends ArchiveIndexTestCase { | ||
|
||
public RestoreFromVersion6IT(Version version) { | ||
super(version); | ||
} | ||
|
||
public void testArchiveIndicesV5() throws Exception { | ||
verifyArchiveIndexCompatibility("5"); | ||
} | ||
} |
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.
@tlrx are you good with this special case here and the one above that targets archive indices? We are addressing the need to upgrade the compatibility version for archive indices imported first in 8.x, that don't allow to upgrade to 9.x otherwise.
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.
I don't think this is going to work during rolling upgrades, because the master may still be a 8.x node and therefore will reject join requests from 9.x, because a 8.x master will still think that 9.x nodes cannot support indices with a 7.x compatibility version.
I think it does work in the case of a full cluster restart though, since the node will be directly be 9.x with the new logic here.
We have to change that logic to also support searchable snapshot N-2 indices, maybe I can do it for both types of indices at once.
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.
I'm about to merge #118744 that is a prerequisite of #118785 which contains similar logic than the one added here but for searchable snapshots.
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.
good point Tanguy, thanks for raising this. I guess we are better off waiting for your changes then.
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.
I replicated the same test under the
qa/rolling-upgrade
project to verify the behavior. It fails in the line,when upgrading any of the nodes.
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.
thanks for doing that!
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.
I also got stuck in this area (IndexMetadataVerifier#checkSupportedVersion) when trying to revert one of the deleted FullClusterRestartIT tests. My quick fix of changing IndexVersions.MINIMUM_COMPATIBLE to IndexVersions.MINIMUM_READ_ONLY_COMPATIBLE in
elasticsearch/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java
Line 298 in 5ec0a84
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.
Sounds like we may need distrib help with some of these issues @cbuescher , or at least some of the changes they are working on.
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.
I opened #118941 (with #118923 for
8.18
) that should allow archive indices in 7.x to exist on a 9.x cluster. I will try to test the change for archive too but in the case you're interested here is the change for IndexMetadataVerifier.