Fix gradle stack version selection when a release is not yet published #13051

andsel · 2021-07-05T14:52:05Z

Release notes

[rn:skip]

What does this PR do?

Modifies the logic of stack version's selection. From the "exact version match" passes to the "closest version", this change is useful when new Logstash versions are inserted into versions.yml and those versions are not yet published as artifacts.
During the build Elasticsearch and Filebeat versions are downloaded, when the bump of a new version is happening and those artifacts aren't yet published the build would fail. To avoid the fail this PR implements a logic of best match, so for example is last published version is 7.14.2 and the versions.yml is update to build the version 7.15.0 which doesn't exists, the build script force the stack version to 7.14.2 in this way the integration tests are able to succeed.

In particular this PR adds in buildSrc a couple of support classes (StackVersionSelector and StackVersionSelector.StackVersion) to implement the version selection logic described above.

Fixes #13030

Why is it important/What is the impact to the user?

From the developer that's used to build Logstash from sources (and CI) this change fix a problem happening during versions bumps.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files (and/or docker env variables)
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

try a full build changing versions.yml to a future not yet published version

How to test this PR locally

edit locally versions.yml changing the stack version to something next to be released, for example 7.15.0 something not present in the artifacts registry

logstash: 7.15.0
logstash-core: 7.15.0

try this on master and it fails, try it in this branch and success, logging the fact the version used is 7.14.0, the previous closest version.

Related issues

Relates to configureArtifactInfo gradle task needs build version to exist #13030

Use cases

Logs

…ildSrc utility class

… the one selected' when exact stack version match is not yet available

…user with the introduction of StackVersion as comparable instances

…atisfy the case 7.9.0<7.10.0

roaksoax

Not a full review, just a couple suggestions on wording.

roaksoax · 2021-07-06T13:32:34Z

build.gradle

+    def versionSelector = new StackVersionSelector(artifactVersionsApi)
+    String qualifiedVersion = versionSelector.selectClosestVersion(version)
+    if (qualifiedVersion != version) {
+        println "WARN version $version not yet published, swith automatically to $qualifiedVersion"


There's a typo: swith

Also, what about this instead:

"WARN version $version does not yet exist or has not been published, switching to $qualifiedVersion "

Thanks @roaksoax I like you wording, I go for it

roaksoax · 2021-07-06T13:37:12Z

buildSrc/src/main/groovy/org/logstash/gradle/tooling/StackVersionSelector.groovy

+
+        String qualifiedVersion = selectClosestInList(StackVersion.asVersion(isReleaseBuild ? version : "${version}-SNAPSHOT"), versions)
+        if (qualifiedVersion == null) {
+            throw new GradleException("could not find the current artifact from the artifact-api ${artifactVersionsApi} for ${version}")


"the current" artifact not a good way to describe an artifact, we should specific why one it is. e.g.:

cloud not find version ${version} in the artifact-api (${artifactVersionApi})

So I guess it would read:

could not find artifact Elasticsearch 8.0.0-alpha1 in the arfifact-api (https://xzy)/

or something like:

could not find Elasticsearch (elasticsearch-8.0.0-alpha1) in the artifact api (https://xxxx)

Good point, I would change the error to:

could not find Elastic Stack version (8.0.0-alpha1) in the artifact api (https://xxxx)

because at this point the StackVersionSelector is only used to select the version of the global stack and not for a specific artifact, the StackVersionSelector hasn't got that information

jsvd

My concern with this approach of choosing a fallback version is that we should actually fail in many scenarios such as:

❯ VERSION_QUALIFIER=alpha1 ./gradlew clean runIntegrationTests -PrubyIntegrationSpecs=qa/integration/specs/beats_input_spec.rb --console=plain
...
WARN version 8.0.0-alpha1 does not yet exist or has not been published, switching to 8.0.0-SNAPSHOT
...

When running the integration test task with 8.0.0-alpha1, it's dangerous to fail silently to 8.0.0-SNAPSHOT. While we see the warn when running locally in the console, in CI it can easily gives us a false sense of security that the tests are passing.

andsel · 2021-07-06T15:34:45Z

Without that "dowgrading" of the version the first time the 8.0.0-alpha1 is put in the versions.yml and it's not yet present in https://artifacts-api.elastic.co/v1/versions , the build would fail and also block the publishing of all artifacts, like a chicken-egg.
With this PR, the first time it fallback to the snapshot, the following build instead uses effectively the alpha1,
WDYT?

jsvd · 2021-07-06T15:42:51Z

the build would fail and also block the publishing of all artifacts

It doesn't have to fail. Building logstash artifacts should not require integration tests. In fact if we had a jenkins pipeline, we could do:

build the code
run unit tests
build the artifact
run integration tests
build packages
run package acceptance tests

jsvd · 2021-07-06T15:44:31Z

so, for example, doing VERSION_QUALIFIER=alpha1 ./gradlew assembleTarDistribution should not ping Elastic's artifacts API at all

andsel · 2021-07-06T16:16:55Z

Now I understand what you mean. That problem originate by the fact the gradle script does the download in configuration phase instead of execution phase; I tried to put down a PR for that in #13042. However also with that move the VERSION_QUALIFIER=alpha1 ./gradlew clean runIntegrationTests would fail if the stack version alpha1 is not published

jsvd · 2021-07-07T09:56:25Z

However also with that move the VERSION_QUALIFIER=alpha1 ./gradlew clean runIntegrationTests would fail if the stack version alpha1 is not published

It makes sense for the test to fail, and it is the least surprising course of action. Having a build green just for the sake of being green doesn't help. What's important is that our artifact creation and unit testing tasks aren't blocked by the lack of builds in other products. If there comes a time where Logstash requires another Elastic product to be built, this logic needs to be implemented in our internal build manager.

I tried to put down a PR for that in #13042.

So as long as we safeguard that tasks like rake artifact:all and unit tests don't trigger elasticseach/beats downloads, I'm good with moving these download tasks out of configuration to the execution stages, and accepting that we'll temporarily have failing integration tests during release transitions.

roaksoax · 2021-10-13T16:10:08Z

@andsel is this still something that we need to move forward?

andsel · 2021-10-14T06:43:17Z

I've a mixed feeling about this. It's useful only during version bumps, when some artifacts of stack that we use are not yet published, but as @jsvd pointed out it could drive to some surprise during build. So I vote to close it without merging.

jsvd · 2021-10-14T09:41:04Z

++ on not moving forward with the proposal as is, but we should reopen #13030 if the issue still exists, and it seems it does:

~/elastic/logstash master*
❯ git log origin/master | head -n3
commit 096eb7ac4822426091312b38969d88f371d33798
Author: João Duarte <[email protected]>
Date:   Wed Oct 13 11:17:49 2021 +0100

~/elastic/logstash master*
❯ VERSION_QUALIFIER=somethingnew ./gradlew configureArtifactInfo 
To honour the JVM settings for this build a single-use Daemon process will be forked. See https://docs.gradle.org/7.2/userguide/gradle_daemon.html#sec:disabling_the_daemon.
Daemon will be stopped at the end of the build 
> Task :configureArtifactInfo FAILED

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/joaoduarte/elastic/logstash/build.gradle' line: 169

* What went wrong:
Execution failed for task ':configureArtifactInfo'.
> could not find the current artifact from the artifact-api https://artifacts-api.elastic.co/v1/versions for 8.0.0-somethingnew

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 6s
5 actionable tasks: 1 executed, 4 up-to-date

andsel · 2021-10-15T08:05:17Z

In the original issue #13030 we had the problem that the task configureArtifactInfo can't be configured during Gradle's configuration phase. That means that also a bootstrap task failed, like in #12112.
After that the retrieval of Elastic stack version has been moved in execution phase with #13042 part of the problem has been removed. So now the Gradle tasks are executable but in the cases that require the download of Elasticsearch they could fail if the requested artifacts version are not yet published.

andsel added 3 commits July 5, 2021 14:33

Moved the code to select the stack version from build to script to bu…

7dd089a

…ildSrc utility class

Minor, extracted common code

968b39f

Implemented the logic to switch to 'latest published version close to…

c371a7a

… the one selected' when exact stack version match is not yet available

roaksoax added the status:work-in-progress label Jul 5, 2021

andsel added 2 commits July 5, 2021 18:51

Chaged comparator to a composition of comparators and simplified the …

76ec19d

…user with the introduction of StackVersion as comparable instances

Added 'buildSrc' to the list of directories mounted into Dcokerfile

63ff439

andsel marked this pull request as ready for review July 6, 2021 09:09

jsvd self-requested a review July 6, 2021 09:14

roaksoax added status:needs-review and removed status:work-in-progress labels Jul 6, 2021

andsel added 3 commits July 6, 2021 14:03

Improved readability in tests

57420db

Switched version comparison from String to a Version class, this to s…

c181cbb

…atisfy the case 7.9.0<7.10.0

Minor, streamlined if-s sequence

5ccc82a

roaksoax suggested changes Jul 6, 2021

View reviewed changes

roaksoax added status:changes-requested and removed status:needs-review labels Jul 6, 2021

Fixed wording for error and warn messages

303ac8b

andsel requested a review from roaksoax July 6, 2021 14:05

andsel changed the title ~~Fix/gradle stack version selection release not yet published~~ Fix gradle stack version selection when a release is not yet published Jul 6, 2021

andsel mentioned this pull request Jul 6, 2021

[backport 7.x] Fix LS benchmarking tool to work with releases >= 7.10.0 (#13052) #13053

Merged

jsvd reviewed Jul 6, 2021

View reviewed changes

andsel mentioned this pull request Jul 7, 2021

Move retrieval of Stack version from Gradle's configuration to execution #13042

Merged

7 tasks

roaksoax assigned andsel Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gradle stack version selection when a release is not yet published #13051

Fix gradle stack version selection when a release is not yet published #13051

andsel commented Jul 5, 2021 •

edited

Loading

roaksoax left a comment

roaksoax Jul 6, 2021

andsel Jul 6, 2021

roaksoax Jul 6, 2021

andsel Jul 6, 2021

jsvd left a comment •

edited

Loading

andsel commented Jul 6, 2021

jsvd commented Jul 6, 2021 •

edited

Loading

jsvd commented Jul 6, 2021 •

edited

Loading

andsel commented Jul 6, 2021

jsvd commented Jul 7, 2021 •

edited

Loading

roaksoax commented Oct 13, 2021

andsel commented Oct 14, 2021

jsvd commented Oct 14, 2021

andsel commented Oct 15, 2021

Fix gradle stack version selection when a release is not yet published #13051

Are you sure you want to change the base?

Fix gradle stack version selection when a release is not yet published #13051

Conversation

andsel commented Jul 5, 2021 • edited Loading

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Logs

roaksoax left a comment

Choose a reason for hiding this comment

roaksoax Jul 6, 2021

Choose a reason for hiding this comment

andsel Jul 6, 2021

Choose a reason for hiding this comment

roaksoax Jul 6, 2021

Choose a reason for hiding this comment

andsel Jul 6, 2021

Choose a reason for hiding this comment

jsvd left a comment • edited Loading

Choose a reason for hiding this comment

andsel commented Jul 6, 2021

jsvd commented Jul 6, 2021 • edited Loading

jsvd commented Jul 6, 2021 • edited Loading

andsel commented Jul 6, 2021

jsvd commented Jul 7, 2021 • edited Loading

roaksoax commented Oct 13, 2021

andsel commented Oct 14, 2021

jsvd commented Oct 14, 2021

andsel commented Oct 15, 2021

andsel commented Jul 5, 2021 •

edited

Loading

jsvd left a comment •

edited

Loading

jsvd commented Jul 6, 2021 •

edited

Loading

jsvd commented Jul 6, 2021 •

edited

Loading

jsvd commented Jul 7, 2021 •

edited

Loading