Skip to content
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

Update archival indices logic to support ES 7 indices #116565

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented Nov 11, 2024

This PR adds archival support for Elasticsearch 7 indices to ES 9. While we support reading N-1 index versions
directly, support for read-only access to older versions was added via #81210 in 8.3. So far this covered only indices from Elasticsearch 5 and 6 that were written using
Lucene 6 and 7 Codecs.

This change so far adds bwc treatment to two of the existing Lucene 8 Codecs (Lucene80Codec and Lucene87Codec)
in a similar way that we used for the Lucene70Codec already, by introducing own copies of these Codecs (prefixed with "BWC*") that essentially wrap SegmentInfoFormat and FieldInfoFormat in form that formats read logic to modified methods in the BWCCodec class.

Existing tests from :x-pack:qa:repository-old-versions are ported to a new sister project :x-pack:qa:repository-old-versions-7x to use the new test cluster infrastructure going forward.

@cbuescher cbuescher added the WIP label Nov 11, 2024
@cbuescher cbuescher force-pushed the add-bwcLucene87Codec branch 7 times, most recently from eac72e2 to fd4dc0f Compare November 14, 2024 14:58
@cbuescher cbuescher force-pushed the add-bwcLucene87Codec branch 2 times, most recently from 0438df5 to 6134977 Compare November 22, 2024 11:27
@cbuescher cbuescher force-pushed the add-bwcLucene87Codec branch from 6134977 to 85b61cd Compare November 22, 2024 12:02
Copy link
Member Author

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breskeby thanks for your help with the build setup for this, I left a few questions I currently have here in the hope that they are relatively quick to answer


@ClassRule
public static TestRule ruleChain = RuleChain.outerRule(repoDirectory).around(oldCluster).around(currentCluster);
private static boolean repoSetup = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breskeby a few questions here to check if my understanding of the setup is right.
I somehow ended up to have to chain the creation of the directory for the snapshot repo into this RuleChain which I saw used elsewhere. Does this make sure the directory is created before the clusters and are they sure to share the same path?


tasks.named("javaRestTest").configure {
usesDefaultDistribution()
usesBwcDistribution(Version.fromString("7.17.25"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breskeby that looks simple enough, but going forward I would need several "old" clusters (i.e. 7.0.0, maybe even some other minor 7x versions where the Lucene codecs changed) for the snapshot data setup and then they would need to run against a clean version of the "current" cluster version for the tests.

I'm wondering if I should use something like

buildParams.bwcVersions.withWireCompatible { bwcVersion, baseName ->
where we seem to use the bwcVersion probably coming from the elasticsearch.bwc-test plugin or if we can supply a "fixed" list of versions we want to run as "old" versions here. Would I need to register individual tasks for each "old" version?

Also, does this definition here already "start" the clusters or is that happening later with ElasticsearchCluster.local()[...].build() in the test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go ahead with a fixed list as we did for other "old elasticsearch" tests.
I think the versions you wanna use are listed in that task. That probably needs some tweaking of the RestTestBasePlugin to be supported.

This definition basically only downloads or builds the distribution you wanna use in your tests. they are only started in ElasticsearchCluster.local()[...].build(). Given that. I think we should only have one test task here and do the parameterisation of the versions we test in the test code. e.g. having a parameterised junit test per version under test. the lifecycle of the "current" cluster for that would also live in the test code then.

One problem we might see (haven't completely checked) is that those older cluster only support a limited range of java versions? we probably need to ensure those clusters are wired to the correct java version when starting up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that. I think we should only have one test task here and do the parameterisation of the versions we test in the test code. e.g. having a parameterised junit test per version under test.

I'm not sure I understand. Do you propose to start all "old version" clusters consecutively from only one test suite? That would make it hard to e.g. from gradle only run the tests for e.g 7.0.0 vs. CURRENT, or am I missunderstanding something? Or do you mean we pass in the "old version" we are running for the data setup to the test e.g. via a system property and loop over it somewhere in the build.gradle definition? I would like to avoid having to run all old version tests at once every time, at the same time having the flexibility to use more than one test suite with basically the same fixed setup (ideally the old data setup is just done once).

@cbuescher cbuescher force-pushed the add-bwcLucene87Codec branch from f2e8730 to 8aae226 Compare November 25, 2024 22:10

tasks.named("javaRestTest").configure {
def versionString = "7.17.25"
// def versionString = "7.9.0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breskeby I have ported two existing tests extending ESRestTestCase from the old qa project now. I cannot get the loop over versions to work though. I tried something like this:

import org.elasticsearch.gradle.internal.test.RestIntegTestTask
import org.elasticsearch.gradle.Version
import org.elasticsearch.gradle.testclusters.StandaloneRestIntegTestTask

apply plugin: 'elasticsearch.internal-java-rest-test'

for (String versionString : ['7.9.0', '7.17.25']) {
  String versionNoDots = versionString.replace('.', '_')

  tasks.register("javaRestTest#${versionNoDots}", StandaloneRestIntegTestTask) {
    systemProperty 'tests.old_cluster_version', versionString
    usesDefaultDistribution()
    usesBwcDistribution(Version.fromString(versionString))
  }

  tasks.named("check").configure {
    dependsOn "javaRestTest#${versionNoDots}"
  }
}

But for some ran into trouble finding the test classes directory, so something is still off:
https://gradle-enterprise.elastic.co/s/c2iiwoowzbhlg

Any pointers?

Copy link
Contributor

@breskeby breskeby Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in your task registration you need to set the class path and the test classes folder in order to get this working with custom tasks:

tasks.register("javaRestTest#${versionNoDots}", StandaloneRestIntegTestTask) {
    systemProperty 'tests.old_cluster_version', versionString
    usesDefaultDistribution()
    testClassesDirs = sourceSets.javaRestTest.output.classesDirs
    classpath = sourceSets.javaRestTest.runtimeClasspath
    usesBwcDistribution(Version.fromString(versionString))
  }

Yeah my original suggestion was to have one test suite containing all tests for all versions. you might be right breaking those up is a more convenient way. though I think we usually would not trigger them individually by version. I could be wrong here and haven't thought too much about that yet. Lets go ahead with your approach for now and have one test task per version.

include '_common', 'search'
}
restTests {
includeCore 'search/390_doc_values_search.yml'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breskeby next iteration: this yml test shows up now in the "build" directory but isn't executed on ":check"
Here's a local run: https://gradle-enterprise.elastic.co/s/cshioucnucegw
I copied over and modified the "DocValueOnlyFieldsIT" that should run this but I'm not sure how to wire it to the "check" task (or any other task for that matter). The setup might need adjustment still which I can do as long as the gradle wiring works. Thanks for your help.

@cbuescher cbuescher requested review from a team as code owners November 27, 2024 16:14
@cbuescher cbuescher force-pushed the add-bwcLucene87Codec branch from 7379b72 to 6cabe33 Compare November 27, 2024 16:15
@cbuescher
Copy link
Member Author

@breskeby thanks for your help, the testing side looks good for now I think, probably worth some cleanup at some later point but I think I have the three existing test suites from "repository-old-versions" ported to the twin project for 7x now.
@javanna I believe this covers at least the existing tests, CI looks green on local runs at least for versions "7.9.0" (first version using Lucene 8.6.0 and therefore the Lucene86 Codec) and the latest minor "7.17.25" (using Lucene 8.11.3 with Lucene87Codec).
I will convert this from a draft to a regular PR so we can discuss the approach for adding the BWCCodecs in a proper review.

@cbuescher cbuescher changed the title WIP Update archival indices logic to support Lucene 8 indices Nov 28, 2024
@cbuescher cbuescher changed the title Update archival indices logic to support Lucene 8 indices Update archival indices logic to support ES 7 indices Nov 28, 2024
// restore / mount again
restoreMountAndVerify(numDocs, expectedIds, numberOfShards, sourceOnlyRepository, indexName, repoName, snapshotName);

// TODO restart current cluster
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original test this is copied from used to check restored indices are there after a restart of the target cluster (the one with the current version).
Orchestration of this was done through special gradle tasks probably coming from the bwc-test plugin that we haven't introduced here to simplify things.

Doing this with the new testcluster framework should be possible by restarting the "currentCluster", the problem I can into when trying this though is that the existing client/adminClient from ESRestTestCase are loosing their connection and closing / restarting them wasn't as straight forward when I tried. Probably possible though with some more investigation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanna since we talked about this last I've added restarts to this PR.

logger.info(searchResponse);
assertEquals(0, searchResponse.getHits().getTotalHits().value());
assertEquals(numberOfShards, searchResponse.getSuccessfulShards());
// TODO the following is a failure tracked in https://github.com/elastic/elasticsearch/issues/115631
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires adaption once #115631 or the corresponding PR (Fix OldRepositoryAccessIT testOldRepoAccess #117649) is closed.

@cbuescher
Copy link
Member Author

@elasticmachine run elasticsearch-ci/part-4

@cbuescher
Copy link
Member Author

fyi I just pushed the test changes from fixing OldRepositoryIT also here with 050d195

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants