Skip to content

[BC Upgrage] Fix incorrect version parsing in tests #129243

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

Merged
merged 7 commits into from
Jun 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,11 @@ public boolean isRunningAgainstOldCluster() {
return requestedUpgradeStatus == OLD;
}

public static String getOldClusterVersion() {
/**
* The version of the "old" (initial) cluster. It is an opaque string, do not even think about parsing it for version
* comparison. Use (test) cluster features and {@link ParameterizedFullClusterRestartTestCase#oldClusterHasFeature} instead.
*/
protected static String getOldClusterVersion() {
return System.getProperty("tests.bwc.main.version", OLD_CLUSTER_VERSION);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ public abstract class AbstractRollingUpgradeTestCase extends ParameterizedRollin
private static final ElasticsearchCluster cluster = buildCluster();

private static ElasticsearchCluster buildCluster() {
Version oldVersion = Version.fromString(OLD_CLUSTER_VERSION);
// Note we need to use OLD_CLUSTER_VERSION directly here, as it may contain special values (e.g. 0.0.0) the ElasticsearchCluster
// builder uses to lookup a particular distribution
var cluster = ElasticsearchCluster.local()
.distribution(DistributionType.DEFAULT)
.version(getOldClusterTestVersion())
.version(OLD_CLUSTER_VERSION)
.nodes(NODE_NUM)
.setting("path.repo", new Supplier<>() {
@Override
Expand All @@ -46,8 +47,9 @@ public String get() {
.feature(FeatureFlag.TIME_SERIES_MODE);

// Avoid triggering bogus assertion when serialized parsed mappings don't match with original mappings, because _source key is
// inconsistent
if (oldVersion.before(Version.fromString("8.18.0"))) {
// inconsistent. As usual, we operate under the premise that "versionless" clusters (serverless) are on the latest code and
// do not need this.
if (Version.tryParse(getOldClusterVersion()).map(v -> v.before(Version.fromString("8.18.0"))).orElse(false)) {
cluster.jvmArg("-da:org.elasticsearch.index.mapper.DocumentMapper");
cluster.jvmArg("-da:org.elasticsearch.index.mapper.MapperService");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@ public abstract class AbstractRollingUpgradeWithSecurityTestCase extends Paramet
private static final ElasticsearchCluster cluster = buildCluster();

private static ElasticsearchCluster buildCluster() {
Version oldVersion = Version.fromString(OLD_CLUSTER_VERSION);
// Note we need to use OLD_CLUSTER_VERSION directly here, as it may contain special values (e.g. 0.0.0) the ElasticsearchCluster
// builder uses to lookup a particular distribution
var cluster = ElasticsearchCluster.local()
.distribution(DistributionType.DEFAULT)
.version(getOldClusterTestVersion())
.version(OLD_CLUSTER_VERSION)
.nodes(NODE_NUM)
.user(USER, PASS)
.setting("xpack.security.autoconfiguration.enabled", "false")
Expand All @@ -51,8 +52,8 @@ public String get() {
});

// Avoid triggering bogus assertion when serialized parsed mappings don't match with original mappings, because _source key is
// inconsistent
if (oldVersion.before(Version.fromString("8.18.0"))) {
// inconsistent. Assume non-parseable versions (serverless) do not need this.
if (Version.tryParse(getOldClusterVersion()).map(v -> v.before(Version.fromString("8.18.0"))).orElse(false)) {
cluster.jvmArg("-da:org.elasticsearch.index.mapper.DocumentMapper");
cluster.jvmArg("-da:org.elasticsearch.index.mapper.MapperService");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public DenseVectorMappingUpdateIT(@Name("upgradedNodes") int upgradedNodes) {
}

public void testDenseVectorMappingUpdateOnOldCluster() throws IOException {
if (getOldClusterTestVersion().after(Version.V_8_7_0.toString())) {
if (oldClusterHasFeature("gte_v8.7.1")) {
String indexName = "test_index";
if (isOldCluster()) {
Request createIndex = new Request("PUT", "/" + indexName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.client.Request;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.UpdateForV10;
import org.elasticsearch.test.cluster.ElasticsearchCluster;
import org.elasticsearch.test.cluster.FeatureFlag;
import org.elasticsearch.test.cluster.local.distribution.DistributionType;
Expand All @@ -33,8 +34,12 @@

public class FileSettingsUpgradeIT extends ParameterizedRollingUpgradeTestCase {

@UpdateForV10(owner = UpdateForV10.Owner.CORE_INFRA) // Remove this rule entirely
private static final RunnableTestRuleAdapter versionLimit = new RunnableTestRuleAdapter(
() -> assumeTrue("Only valid when upgrading from pre-file settings", getOldClusterTestVersion().before(new Version(8, 4, 0)))
() -> assumeTrue(
"Only valid when upgrading from pre-file settings",
Version.tryParse(getOldClusterVersion()).map(v -> v.before(new Version(8, 4, 0))).orElse(false)
)
);

private static final String settingsJSON = """
Expand All @@ -52,9 +57,11 @@ public class FileSettingsUpgradeIT extends ParameterizedRollingUpgradeTestCase {

private static final TemporaryFolder repoDirectory = new TemporaryFolder();

// Note we need to use OLD_CLUSTER_VERSION directly here, as it may contain special values (e.g. 0.0.0) the ElasticsearchCluster
// builder uses to lookup a particular distribution
private static final ElasticsearchCluster cluster = ElasticsearchCluster.local()
.distribution(DistributionType.DEFAULT)
.version(getOldClusterTestVersion())
.version(OLD_CLUSTER_VERSION)
.nodes(NODE_NUM)
.setting("path.repo", new Supplier<>() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public LogsUsageRollingUpgradeIT(@Name("upgradedNodes") int upgradedNodes) {
}

public void testUsage() throws Exception {
assumeTrue("logsdb.prior_logs_usage only gets set in 8.x", getOldClusterTestVersion().before("9.0.0"));
assumeFalse("logsdb.prior_logs_usage only gets set in 8.x", oldClusterHasFeature("gte_v9.0.0"));
String dataStreamName = "logs-mysql-error";
if (isOldCluster()) {
bulkIndex(dataStreamName, 4, 256, Instant.now());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,6 @@ public static void resetNodes() {
upgradeFailed = false;
}

@Deprecated // Use the new testing framework and oldClusterHasFeature(feature) instead
protected static String getOldClusterVersion() {
return OLD_CLUSTER_VERSION;
}

protected static boolean oldClusterHasFeature(String featureId) {
assert oldClusterTestFeatureService != null;
return oldClusterTestFeatureService.clusterHasFeature(featureId);
Expand All @@ -146,12 +141,20 @@ protected static IndexVersion getOldClusterIndexVersion() {
return oldIndexVersion;
}

protected static Version getOldClusterTestVersion() {
return Version.fromString(OLD_CLUSTER_VERSION);
/**
* The version of the "old" (initial) cluster. It is an opaque string, do not even think about parsing it for version
* comparison. Use (test) cluster features and {@link ParameterizedRollingUpgradeTestCase#oldClusterHasFeature} instead.
*/
protected static String getOldClusterVersion() {
return System.getProperty("tests.bwc.main.version", OLD_CLUSTER_VERSION);
}

protected static boolean isOldClusterVersion(String nodeVersion) {
return OLD_CLUSTER_VERSION.equals(nodeVersion);
protected static boolean isOldClusterVersion(String nodeVersion, String buildHash) {
String bwcRefSpec = System.getProperty("tests.bwc.refspec.main");
if (bwcRefSpec != null) {
return bwcRefSpec.equals(buildHash);
}
return getOldClusterVersion().equals(nodeVersion);
}

protected static boolean isOldCluster() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,21 +88,22 @@ public void testSnapshotBasedRecovery() throws Exception {
}

String primaryNodeId = getPrimaryNodeIdOfShard(indexName, 0);
String primaryNodeVersion = getNodeVersion(primaryNodeId);
var primaryNodeVersion = getNodeVersion(primaryNodeId);

// Sometimes the primary shard ends on the upgraded node (i.e. after a rebalance)
// This causes issues when removing and adding replicas, since then we cannot allocate to any of the old nodes.
// That is an issue only for the first mixed round.
// In that case we exclude the upgraded node from the shard allocation and cancel the shard to force moving
// the primary to a node in the old version, this allows adding replicas in the first mixed round.
logger.info("--> Primary node in first mixed round {} / {}", primaryNodeId, primaryNodeVersion);
if (isOldClusterVersion(primaryNodeVersion) == false) {
if (isOldClusterVersion(primaryNodeVersion.version(), primaryNodeVersion.buildHash()) == false) {
logger.info("--> cancelling primary shard on node [{}]", primaryNodeId);
cancelShard(indexName, 0, primaryNodeId);
logger.info("--> done cancelling primary shard on node [{}]", primaryNodeId);

String currentPrimaryNodeId = getPrimaryNodeIdOfShard(indexName, 0);
assertTrue(isOldClusterVersion(getNodeVersion(currentPrimaryNodeId)));
var currentPrimaryNodeVersion = getNodeVersion(currentPrimaryNodeId);
assertTrue(isOldClusterVersion(currentPrimaryNodeVersion.version(), currentPrimaryNodeVersion.buildHash()));
}
} else {
logger.info("--> not in first upgrade round, removing exclusions for [{}]", indexName);
Expand Down Expand Up @@ -137,17 +138,24 @@ private List<String> getUpgradedNodeIds() throws IOException {
List<String> upgradedNodes = new ArrayList<>();
for (Map.Entry<String, Map<String, Object>> nodeInfoEntry : nodes.entrySet()) {
String nodeVersion = extractValue(nodeInfoEntry.getValue(), "version");
if (isOldClusterVersion(nodeVersion) == false) {
String nodeBuildHash = extractValue(nodeInfoEntry.getValue(), "build_hash");
if (isOldClusterVersion(nodeVersion, nodeBuildHash) == false) {
upgradedNodes.add(nodeInfoEntry.getKey());
}
}
return upgradedNodes;
}

private String getNodeVersion(String primaryNodeId) throws IOException {
private record NodeVersion(String version, String buildHash) {}

private NodeVersion getNodeVersion(String primaryNodeId) throws IOException {
Request request = new Request(HttpGet.METHOD_NAME, "_nodes/" + primaryNodeId);
Response response = client().performRequest(request);
return extractValue(responseAsMap(response), "nodes." + primaryNodeId + ".version");
Map<String, Object> responseAsMap = responseAsMap(response);
return new NodeVersion(
extractValue(responseAsMap, "nodes." + primaryNodeId + ".version"),
extractValue(responseAsMap, "nodes." + primaryNodeId + ".build_hash")
);
}

private String getPrimaryNodeIdOfShard(String indexName, int shard) throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public SourceModeRollingUpgradeIT(@Name("upgradedNodes") int upgradedNodes) {
}

public void testConfigureStoredSourceBeforeIndexCreationLegacy() throws IOException {
assumeTrue("testing deprecation warnings and deprecation migrations", getOldClusterTestVersion().before("9.0.0"));
assumeFalse("testing deprecation warnings and deprecation migrations", oldClusterHasFeature("gte_v9.0.0"));
String templateName = "logs@custom";
if (isOldCluster()) {
var storedSourceMapping = """
Expand Down Expand Up @@ -56,7 +56,7 @@ public void testConfigureStoredSourceBeforeIndexCreationLegacy() throws IOExcept
}

public void testConfigureStoredSourceWhenIndexIsCreatedLegacy() throws IOException {
assumeTrue("testing deprecation warnings and deprecation migrations", getOldClusterTestVersion().before("9.0.0"));
assumeFalse("testing deprecation warnings and deprecation migrations", oldClusterHasFeature("gte_v9.0.0"));
String templateName = "logs@custom";
if (isOldCluster()) {
var storedSourceMapping = """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,16 @@ public VectorSearchIT(@Name("upgradedNodes") int upgradedNodes) {
private static final String BBQ_INDEX_NAME = "bbq_vector_index";
private static final String FLAT_QUANTIZED_INDEX_NAME = "flat_quantized_vector_index";
private static final String FLAT_BBQ_INDEX_NAME = "flat_bbq_vector_index";
private static final String FLOAT_VECTOR_SEARCH_VERSION = "8.4.0";
private static final String BYTE_VECTOR_SEARCH_VERSION = "8.6.0";
private static final String QUANTIZED_VECTOR_SEARCH_VERSION = "8.12.1";
private static final String FLAT_QUANTIZED_VECTOR_SEARCH_VERSION = "8.13.0";
private static final String BBQ_VECTOR_SEARCH_VERSION = "8.18.0";

// TODO: replace these with proper test features
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to open a GitHub issue or Jira ticket describing how this would be done? I'm thinking of capturing your intent and knowledge here to benefit whoever ends up doing this work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH it is fine to leave these as-is, there is no value in retrofitting test features. I want it to be more of a warning for whoever will work on these tests in the future (and "nudge" them towards using a test feature)

private static final String FLOAT_VECTOR_SEARCH_TEST_FEATURE = "gte_v8.4.0";
private static final String BYTE_VECTOR_SEARCH_TEST_FEATURE = "gte_v8.6.0";
private static final String QUANTIZED_VECTOR_SEARCH_TEST_FEATURE = "gte_v8.12.1";
private static final String FLAT_QUANTIZED_VECTOR_SEARCH_TEST_FEATURE = "gte_v8.13.0";
private static final String BBQ_VECTOR_SEARCH_TEST_FEATURE = "gte_v8.18.0";

public void testScriptByteVectorSearch() throws Exception {
assumeTrue("byte vector search is not supported on this version", getOldClusterTestVersion().onOrAfter(BYTE_VECTOR_SEARCH_VERSION));
assumeTrue("byte vector search is not supported on this version", oldClusterHasFeature(BYTE_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
// create index and index 10 random floating point vectors
String mapping = """
Expand Down Expand Up @@ -91,10 +93,7 @@ public void testScriptByteVectorSearch() throws Exception {
}

public void testScriptVectorSearch() throws Exception {
assumeTrue(
"Float vector search is not supported on this version",
getOldClusterTestVersion().onOrAfter(FLOAT_VECTOR_SEARCH_VERSION)
);
assumeTrue("Float vector search is not supported on this version", oldClusterHasFeature(FLOAT_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
// create index and index 10 random floating point vectors
String mapping = """
Expand Down Expand Up @@ -140,10 +139,7 @@ public void testScriptVectorSearch() throws Exception {
}

public void testFloatVectorSearch() throws Exception {
assumeTrue(
"Float vector search is not supported on this version",
getOldClusterTestVersion().onOrAfter(FLOAT_VECTOR_SEARCH_VERSION)
);
assumeTrue("Float vector search is not supported on this version", oldClusterHasFeature(FLOAT_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
String mapping = """
{
Expand Down Expand Up @@ -215,7 +211,7 @@ public void testFloatVectorSearch() throws Exception {
}

public void testByteVectorSearch() throws Exception {
assumeTrue("Byte vector search is not supported on this version", getOldClusterTestVersion().onOrAfter(BYTE_VECTOR_SEARCH_VERSION));
assumeTrue("Byte vector search is not supported on this version", oldClusterHasFeature(BYTE_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
String mapping = """
{
Expand Down Expand Up @@ -288,10 +284,7 @@ public void testByteVectorSearch() throws Exception {
}

public void testQuantizedVectorSearch() throws Exception {
assumeTrue(
"Quantized vector search is not supported on this version",
getOldClusterTestVersion().onOrAfter(QUANTIZED_VECTOR_SEARCH_VERSION)
);
assumeTrue("Quantized vector search is not supported on this version", oldClusterHasFeature(QUANTIZED_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
String mapping = """
{
Expand Down Expand Up @@ -364,7 +357,7 @@ public void testQuantizedVectorSearch() throws Exception {
public void testFlatQuantizedVectorSearch() throws Exception {
assumeTrue(
"Quantized vector search is not supported on this version",
getOldClusterTestVersion().onOrAfter(FLAT_QUANTIZED_VECTOR_SEARCH_VERSION)
oldClusterHasFeature(FLAT_QUANTIZED_VECTOR_SEARCH_TEST_FEATURE)
);
if (isOldCluster()) {
String mapping = """
Expand Down Expand Up @@ -434,10 +427,7 @@ public void testFlatQuantizedVectorSearch() throws Exception {
}

public void testBBQVectorSearch() throws Exception {
assumeTrue(
"Quantized vector search is not supported on this version",
getOldClusterTestVersion().onOrAfter(BBQ_VECTOR_SEARCH_VERSION)
);
assumeTrue("Quantized vector search is not supported on this version", oldClusterHasFeature(BBQ_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
String mapping = """
{
Expand Down Expand Up @@ -518,10 +508,7 @@ public void testBBQVectorSearch() throws Exception {
}

public void testFlatBBQVectorSearch() throws Exception {
assumeTrue(
"Quantized vector search is not supported on this version",
getOldClusterTestVersion().onOrAfter(BBQ_VECTOR_SEARCH_VERSION)
);
assumeTrue("Quantized vector search is not supported on this version", oldClusterHasFeature(BBQ_VECTOR_SEARCH_TEST_FEATURE));
if (isOldCluster()) {
String mapping = """
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.io.Serializable;
import java.io.UncheckedIOException;
import java.util.Objects;
import java.util.Optional;
import java.util.Properties;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -96,6 +97,23 @@ public static Version fromString(final String s, final Mode mode) {
return new Version(Integer.parseInt(major), Integer.parseInt(minor), revision == null ? 0 : Integer.parseInt(revision), qualifier);
}

public static Optional<Version> tryParse(final String s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only ever used in .map(v -> v.before(Version.fromString("SOME_VERSION"))).orElse(false). I wonder if it would make the callers more readable if we offer a helper function that does all of this? I don't feel strongly about it though.

Objects.requireNonNull(s);
Matcher matcher = pattern.matcher(s);
if (matcher.matches() == false) {
return Optional.empty();
}

String major = matcher.group(1);
String minor = matcher.group(2);
String revision = matcher.group(3);
String qualifier = matcher.group(4);

return Optional.of(
new Version(Integer.parseInt(major), Integer.parseInt(minor), revision == null ? 0 : Integer.parseInt(revision), qualifier)
);
}

@Override
public String toString() {
return getMajor() + "." + getMinor() + "." + getRevision();
Expand Down
Loading