-
Notifications
You must be signed in to change notification settings - Fork 25.5k
[Test] Replace 0.0.0 version with "detached" property on ElasticsearchDistibution #134584
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
Conversation
93d9153
to
c0bb3d6
Compare
b571c5c
to
18c721b
Compare
doFirst { | ||
if (System.getProperty("tests.bwc.refspec.main") == null) { | ||
throw new GradleException("You must set the `tests.bwc.refspec.main` system property to run bcUpgradeTest") | ||
} | ||
if (System.getProperty("tests.bwc.main.version") == null) { | ||
throw new GradleException("You must set the `tests.bwc.main.version` system property to run bcUpgradeTest") | ||
} | ||
} | ||
if (System.getProperty("tests.bwc.refspec.main") != null && System.getProperty("tests.bwc.main.version") != null) { | ||
usesBwcDistributionFromRef(System.getProperty("tests.bwc.refspec.main"), Version.fromString(System.getProperty("tests.bwc.main.version"))) | ||
systemProperty("tests.old_cluster_version", System.getProperty("tests.bwc.main.version")) | ||
} |
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.
This and usesBwcDistributionFromRef
implementation in RestTestBasePlugin
are the core changes of this PR. Instead of setting the 0.0.0
version, the correct version from required tests.bwc.main.version
is used.
if (version.before(bwcVersions.getMinimumWireCompatibleVersion())) { | ||
// If we are upgrade testing older versions we also need to upgrade to 7.last |
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.
The above usesBwcDistribution
is now simplified, because handling of 0.0.0
(now "detached") has been moved to the usesBwcDistributionFromRef
implementation below.
/** | ||
* Informs if the version is not tied to any Elasticsearch release and is a custom build. | ||
* This is true when the distribution is not from HEAD but also not any known released version. | ||
* In that case the detached source build needs to be prepared by `usedBwcDistributionFromRef(ref, version)`. | ||
*/ | ||
public boolean isDetachedVersion() { | ||
return detachedVersion.get(); | ||
} | ||
|
||
public void setDetachedVersion(boolean detachedVersion) { | ||
this.detachedVersion.set(detachedVersion); | ||
} | ||
|
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 refrained from using "snapshot" and used "detached" instead, because "snapshot" seems to be overloaded term. However, I'm interested in your opinion on that and any suggestions for better naming.
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 like the term detatched
. I certainly can't think of a better one and I agree "snapshot" is both overloaded and not really appropriate here.
qa/full-cluster-restart/build.gradle
Outdated
doFirst { | ||
if (System.getProperty("tests.bwc.refspec.main") == null) { | ||
throw new GradleException("You must set the `tests.bwc.refspec.main` system property to run luceneBwcTest") | ||
} | ||
if (System.getProperty("tests.bwc.main.version") == null) { | ||
throw new GradleException("You must set the `tests.bwc.main.version` system property to run luceneBwcTest") | ||
} | ||
} | ||
if (System.getProperty("tests.bwc.refspec.main") != null && System.getProperty("tests.bwc.main.version") != null) { | ||
usesBwcDistributionFromRef(System.getProperty("tests.bwc.refspec.main"), Version.fromString(System.getProperty("tests.bwc.main.version"))) | ||
systemProperty("tests.old_cluster_version", System.getProperty("tests.bwc.main.version")) | ||
} |
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.
Same logic change as in /elasticsearch.bc-upgrade-test.gradle
Node node = nodes.get(index); | ||
node.stop(false); | ||
LOGGER.info("Upgrading node '{}' to version {}", node.getName(), version); | ||
node.getSpec().setDetachedVersion(false); |
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.
This is needed for the cluster to be upgraded to the actual specified version and not keep using the same detached version as before.
It has a limitation of not being able to upgrade to the detached version, but there is no such test case currently.
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.
Yeah, this separation is what makes this somewhat awkward. Which is why I originally envisioned the notion of "detached" here to be captured by Version
itself. All the test clusters stuff uses it's own Version
class, so we could add a detached
property to it. It would save us having to muck with all these method signatures and API that already accept a Version
while still being able to pass that knowledge down to things that care about that.
So how about we add detached
to Version
. We'll then add some factory methods that take the additional boolean argument. We can do the same on LocalSpecBuilder
by adding a version(String, boolean)
method. If unspecified the default will be to set detached = false
. That way this all "just works". We just look at the detached
property of the passed Version
to figure out what to do. This also allows us in the future to have tests that upgrade to detached versions as well. As this is written that wouldn't be possible.
Pinging @elastic/es-delivery (Team:Delivery) |
ElasticsearchDistibution for tests Introduce a new property to indicate if the version is a custom build not tied to any release. This was previously handled by specifying version as 0.0.0, but caused problems when comparing versions (e.g., to decide on configuration).
Added `.detachedVersion(System.getProperty("tests.bwc.refspec.main") != null)` to all the tests which apply 'elasticsearch.bc-upgrade-test' plugin to make them properly resolve distribution (previously configured by specifying `0.0.0` version).
In the bcUpgradeTests these tests were previously skipped because version was set to `0.0.0` which was outside the supported range. Now, when `tests.old_cluster_version` has correct version they run, but fail just because `9.2.0-SNAPSHOT` was considered unsupported version format with the "SNAPSHOT".
Needs to stay there, because of the serverless tests.
…hDistibution (elastic#134584) Introduce a new `detached` property to indicate if the version is a custom build not tied to any release. This was previously handled by specifying version as 0.0.0, but caused problems when comparing versions (e.g., to decide on configuration).
💔 Backport failed
You can use sqren/backport to manually backport by running |
…hDistibution (elastic#134584) Introduce a new `detached` property to indicate if the version is a custom build not tied to any release. This was previously handled by specifying version as 0.0.0, but caused problems when comparing versions (e.g., to decide on configuration).
…hDistibution (elastic#134584) Introduce a new `detached` property to indicate if the version is a custom build not tied to any release. This was previously handled by specifying version as 0.0.0, but caused problems when comparing versions (e.g., to decide on configuration). (cherry picked from commit 664a604) # Conflicts: # qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/AbstractRollingUpgradeWithSecurityTestCase.java # test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java # x-pack/plugin/logsdb/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/StandardToLogsDbIndexModeRollingUpgradeIT.java
💔 Some backports could not be created
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
This doesn't need to be backported to 8.19 and 8.18 because |
…hDistibution (#134584) (#135924) Introduce a new `detached` property to indicate if the version is a custom build not tied to any release. This was previously handled by specifying version as 0.0.0, but caused problems when comparing versions (e.g., to decide on configuration). (cherry picked from commit 664a604) # Conflicts: # qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/AbstractRollingUpgradeWithSecurityTestCase.java # test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java # x-pack/plugin/logsdb/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/StandardToLogsDbIndexModeRollingUpgradeIT.java
These asserts have been enabled for the lucene compat because `oldVersion` is now correctly set after elastic#134584 which removed `0.0.0` version.
Introduce a new property to indicate if the version is a custom build not tied to any release. This was previously handled by specifying version as 0.0.0, but caused problems when comparing versions (e.g., to decide on configuration).
In the tests, which support the
bcUpgradeTest
task it means that they need to:elasticsearch.bc-upgrade-test
(this was already required),tests.bwc.main.version
ortests.old_cluster_version
since both are the same. Previously, sometimes version had to be applied fromtests.old_cluster_version
which could be0.0.0
to be able to support special cases.)detachedVersion
totrue
using the ElasticsearchCluster JUnit rule when the cluster should resolve to the version not tied to any released version nor the current HEAD (This is new. This setting is currently tied to the existence of thetests.bwc.refspec.main
system property).I haven't found any of the legacy rest/yaml tests supporting
bcUpgradeTest
(none applieselasticsearch.bc-upgrade-test
plugin), so I haven't been implementing any support for detached version for the legacy test framework.