Make the build version qualifier aware by alpar-t · Pull Request #32128 · elastic/elasticsearch

alpar-t · 2018-07-17T13:20:06Z

remove the indirection of writing out a config file with the computed version ( there's already -Dbuild.snapshot so the full version string is computed ). The versions are read from VersionProperties anyhow and never directly.
relax the condition in the build to only check for version by number in Version.java, disregarding the qualifier since it will never be added to Version.java
remove the qualifier from version.properties and Version.java. All the updates in other java files are automated refactoring.

And relax the check in build to only take the number into account

elasticmachine · 2018-07-17T13:20:08Z

Pinging @elastic/es-core-infra

…ware

jasontedor

I left some initial thoughts.

jasontedor · 2018-07-17T13:48:34Z

buildSrc/src/main/java/org/elasticsearch/gradle/VersionProperties.java

+            baseVersion.getMajor(),
+            baseVersion.getMinor(),
+            baseVersion.getRevision(),
+            System.getProperty("build.version_qualifier", "alpha1"),


This should not default to alpha1. It should default to the empty string. In the production of our nightly snapshots we want to set this externally now and we will start with alpha1.

I think that this also means that we need to percolate this through to CI otherwise the builds will be looking for ML 7.0.0-SNAPSHOT artifacts which will not exist.

One reason that I do not like the default of alpha1 and instead prefer this to be completely managed externally is that we can end up with the following sequence:

today we are producing alpha1 snapshots

at some point we will bump to producing alpha2 snapshots

with this default, we would still be running alpha1 in CI and that's confusing and wrong (wrong because we need it to be alpha2 to pick up the latest ML builds)

I think the confusion is that without any qualifier, we'll be producing 7.0.0-SNAPSHOT which is not something we produce now AFAIK, as we go 7.0.0-alpha1-SNAPSHOT -> e.x 7.0.0-rc3-SNAPSHOT -> 7.0.0-rc3 -> 7.0.0. In this context 7.0.0-SNAPSHOT is a pre-alpha1 .
We need to check the Version class in the build for this, since I don't think it compares like that.
It will a bit awkward to start with 7.0.0-SNAPSHOT end with 7.0.0. and have all the qualifiers in between, but I think it's the only way to have a default to no qualifier. That means CI will use this default while we are in development mode, and start using the alpha1 qualifier close to the release only. We don't want to force people building outside of CI to pass this, and knowing this always passing it in CI feels like to big of a difference to me. If we do this, a good way to percolate this to CI is to merge it like this, with alpha1 as default, then start changing it to empty in CI and eventually change the code to default to empty and remove the parameter from CI.

An alternative is to continue having the qualifier in version.properties, but also allow for a property. This way the bump from -alpha1 to alpha2 all the way to rc3 will be a code change,
but if we are happy with rc3 our job to produce the final release will not do a code change but pass in the qualifier using the property to override the one in version.properties. I think this might be a more straight forward approach as it avoids 7.0.0-SNAPSHOT. We can even enforce that an empty qualifier can't be a snapshot. It's also easier to propagate and less disruptive overall.

I am starting to come around to the view that we simply always produce unqualified builds, yet from time to time we will ship a qualified build with a progression of alpha1, alpha2, etc. as we head towards GA. This feels like the simplest approach and I am not seeing any major downsides? We would still need to bake the qualifier into the version ID when a qualified build is produced though.

@jasontedor you mean to have CI always build a snapshot without a qualifier like 7.0.0-SNAPSHOT and then when we want to ship something we would have one with a qualifier only then e.x 7.0.0-alpha1 so we'll have snapshots before and after the qualified build without distinguishing them. Snapshot and qualifier will become mutually exclusive.

I like it how that simplifies things, and I think it is ok since we either don't consider the qualifiers already ( e.x. we remove qualified versions from bwc tests ) or we are making changes to compare without the version qualifier ( e.x. when comparing build with Version.java and when comparing plugin vs es version ). The only risk I can think of is that we will never run those qualified builds that we make time to time trough our testing. Is there already some sanity tests that runs on artifacts produced by the RM ? That being said, this isn't entirely new, it's just one way there could be differences between elasticsearch ci and RM and the version comparison code is easy to unit test to make sure it does the right thing.

jasontedor · 2018-07-17T13:54:50Z

server/src/main/java/org/elasticsearch/Version.java

-    public static final Version V_7_0_0_alpha1 =
-        new Version(V_7_0_0_alpha1_ID, org.apache.lucene.util.Version.LUCENE_7_4_0);
-    public static final Version CURRENT = V_7_0_0_alpha1;
+    public static final int V_7_0_0_ID = 7000001;


This needs to be 7000099.

jasontedor · 2018-07-17T13:55:58Z

server/src/main/java/org/elasticsearch/Version.java

-    public static final Version CURRENT = V_7_0_0_alpha1;
+    public static final int V_7_0_0_ID = 7000001;
+    public static final Version V_7_0_0 = new Version(V_7_0_0_ID, org.apache.lucene.util.Version.LUCENE_7_4_0);
+    public static final Version CURRENT = V_7_0_0;


This requires some thought as this now depends on the build. I think that we might need to dynamically ascertain the value of CURRENT based on the version and the qualifier (dynamically compute the ID).

If we do that, we'll have to model the qualifier here again.
My understanding is that this code will not know about the qualifier,
in this respect I don't think there's any change, we used to have the version and qualifier hard coded here with matching in version.properties, so if we no longer care about the qualifier here,
it pretty much stays the same regardless of the fact that the qualifier can be changed at build time.

@jasontedor re-reading your comment, I think I misinterpreted it. Are you thinking about determining CURRENT based on the version in the jar ? Topically we would change this for an alpha and will not have to update it after that. We could make a change in a separate PR to assign this based on the manifest. let me know.

…ware

alpar-t · 2018-07-18T08:22:16Z

This needs more work around Exception in thread "main" java.lang.IllegalArgumentException: Plugin [analysis-icu] was built for Elasticsearch version 7.0.0-alpha1 but version 7.0.0 is running

should we throw away the qualifier when comparing ?
Or use the version number from the manifest to do this check ?
Maybe reconsider and keep the qualifier in Version but make CURRENT dynamic ? Would imply that one has to add the major version ex 7.0.0 before adding any qualified version to make sure it's forward facing for when we build the GA without a code change.

What are your thoughts @jasontedor ?

jasontedor · 2018-07-18T23:41:33Z

@atorok I left my latest thoughts in a response to our inline discussion. I am eager to hear your thoughts.

alpar-t · 2018-07-23T12:22:40Z

I updated the plugin builder plugin not to include the build qualifier in the plugin description so that it will correctly match at run-time. This is similar to what was done for snapshot and does not modify the server code.

There are further implications of not including qualifiers in Version.java:

VersionCollection and the fact that isReleased uses the qualified versions to determine if the version is released or not, we no longer have this info, so can't make a call based on that, and this is not present in any other form. At a minimum we would have to store this information in the build and remember to update it with releases.
from what I can tell we pull in released or snapshot but never the qualified builds so this doesn't matter right now, but it does leave us without the ability to do so, since we won't know what the latest released qualifier is.

All the caveats makes this logic difficult to grasp, hard and error prone to change, so I am wondering if this would be a good opportunity to try and simplify it. We could pas this PR with a flag and act from there right after it's merged so we don't increase this further.

I'm thinking about having a meta file, that we always back-port everywhere. The bwc could enforce this when checking out older versions to build them. This would have information about every version ever released or consider (just an example does not match reality):

7:
  0: 
    0:
      status: snapshot   
      rolling-upgrade: 6.0.0
      full-cluster-restart: 5.6.0 
6:
   4:
      0: 
         status: alpha1
         full-cluster-restart: 5.6.0           
6:
   3:
     1: 
        status: ga
        full-cluster-restart: 5.6.0

This would tell us that 7.0.0 is not yet released. The last released major is 6 and from which version different features like full cluster restart and rolling upgrade are supported.
We could include things like index compatibility versions and such as well.
There are policies for most of these, so we could include information about supported features, then write code that knows the policies to figure out where these were introduce how much backward compatability to support etc. I have a slight preference towards making this explicit because policies change, might be version specific and there are always exceptions, so the code to implement them will grow in complexity and we might not end up simplifying much.
In essence this would be a config file for what are all the bwc configs we need to run.
Starting from a template for say rolling-upgrade we could generate all the testing needed from this config file.

The first thing I would look at is to have a task that generates a .dot diagram with a collection of DAGs that create a visualization of all the testing that will happen across all the branches drawing the current branch with a different color to highlight it, to make sure that a) it's easy to follow what's being tested and b) can be used as a tool to validate that the configuration is correct. We could even write a gradle integration test to validate that the correct tasks are generated based on the DAG.
Example DAG fragment:

  6.3.0 ---|---- rolling upgrade ----- |---> 7.0.0-SNAPSHOT
              |---- full cluster restart --|

Pinging @hub-cap and @DaveCTurner , the authors of VersionCollection to pitch in.

alpar-t · 2018-07-25T07:02:45Z

@jasontedor this is ready for another round of review with the caveat that VersionCollection.LAST_RELEASED_MAJOR will have to be kept up to date with releases. See my lengthy comment around mitigating that above, which I think could go into a separate discussion and PR. I now consider this to be merge ready. After the merge we need to push -Dbuild.version_qualifier="" to the ML ci and then make make that the default, and add a check to disallow setting a qualifier when the build is a snapshot.

alpar-t · 2018-07-25T07:03:47Z

@elasticmachine test this please

DaveCTurner · 2018-07-30T10:50:04Z

Pinging ... @DaveCTurner , the authors of VersionCollection to pitch in.

I have nothing concrete to add here. I found it fairly mind-bending to try and encode our versioning policies and release history in code that is itself versioned via those policies, and what I originally wrote in VersionCollection was merely an incomplete understanding of how this worked.

There is tension between (a) builds need to know about which versions have really been released vs (b) builds should be as reproducible as possible. I think being explicit is a good idea, along with automation to verify that the explicit description is consistent with reality and ideally to help update the explicit description as reality changes.

rjernst

I don't think this change should be modifying how SNAPSHOT builds work. Snapshots are independent of the version number, except for maven and the resulting artifact names. It was removed a long time ago from the Version code, and a very intentional decision to make the build produce snapshots by default. Why isn't the qualifier just the alpha/beta/rc portion of the version?

alpar-t · 2018-08-06T09:22:02Z

@rjernst snapshots will still be the default, alpha/beta etc is the qualifier, and snapshot or not is not part of that. I was only proposing we don't allow snapshots with a qualifier to make things less confusing,

…ware

droberts195 · 2018-08-06T10:43:00Z

I was only proposing we don't allow snapshots with a qualifier to make things less confusing

Every team would need to do the same for this to work with release manager, and it will make it very hard to keep release manager working if different teams implement this functionality at different times (which they will). I think we need to stick with "alpha1" as a qualifier for 7.0.0 snapshot builds at least until every team has made their build qualifier-aware. Even after that any changes would need to be agreed across dev. For example, Kibana CI tests use Elasticsearch so we'd need consistency over whether snapshots have qualifiers in Kibana CI.

jasontedor · 2018-08-06T12:52:38Z

Indeed, as @droberts195 points out, we need to preserve that snapshots can be qualified for the time being. This is so that this change can be merged, the rest of the stack can proceed iteratively, and with corresponding internal changes to release-manager, we can continue to produce 7.0.0-alpha1-SNAPSHOTs until the remainder of the stack is ready for qualifiers to be controlled by release-manager. At that point, we would then be able to start producing pure 7.0.0-SNAPSHOTs and only produce qualified builds at pre-release times.

alpar-t · 2018-08-20T10:38:52Z

@jasontedor @rjernst this is ready for review

rjernst

I have few questions.

rjernst · 2018-08-20T18:36:40Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

            .tailSet(Version.fromString("${version.major}.0.0"))
            .headSet(Version.fromString("${version.major + 1}.0.0"))
-            .count { it.suffix.isEmpty() }  // count only non suffix'd versions as actual versions that may be released
+            .count { it.qualifier.isEmpty() }  // count only non qualifier'd versions as actual versions that may be released


nit: qualifier'd -> qualified

fixed, thanks

rjernst · 2018-08-20T18:38:00Z

buildSrc/src/main/java/org/elasticsearch/gradle/Version.java

        this.revision = revision;
        this.snapshot = snapshot;
-        this.suffix = suffix == null ? "" : suffix;
+        if (qualifier == null || qualifier.isEmpty()) {


Why do we need this leniency? The semantics should be either no qualifier is null, or empty string, but not either.

I think it's because the instance was constructed with null most of the time but, we mostly get the qualifier to compare against it, so having "" instead is more convenient and readable.
We could disallow nulls in the constructor, I would clean this up as part of #32872

rjernst · 2018-08-20T18:38:40Z

buildSrc/src/main/java/org/elasticsearch/gradle/Version.java

+        } else if (qualifier.startsWith("-")) {
+            this.qualifier = qualifier;
+        } else {
+            this.qualifier = "-" + qualifier;


Do we need to do the formatting here (prefixing with dash), or could this be done when printed?

We used to store it with a dash so existing code expected it, but I didn't want to add have it in the property, so this takes care of that. I think the cleanest way would be to have an enum for the qualifier.

An enum won't work since these are numbered (or it would require separate variable to keep track of the number). But I don't see why the leniency needs to exist here to add the dash if it doesn't exist. I think the qualifier should always have no dash internally, just like we don't have dot prefixes on the numeric parts of the version.

rjernst · 2018-08-20T18:42:00Z

buildSrc/src/main/java/org/elasticsearch/gradle/Version.java

+            revision == other.revision;
+    }
+
+    public Version dropQualifier() {


nit: can we use the "without" terminology? I think this better matches other code like the builders for java time stuff having egwithTimeZone. Drop implies mutating the current object.

rjernst · 2018-08-20T18:49:30Z

buildSrc/src/main/java/org/elasticsearch/gradle/Version.java

-            // no suffix will be considered smaller, uncomment to change that
+        if (this.qualifier.isEmpty()) {
+            // no qualifier will be considered smaller, uncomment to change that
            // suffixOffset = 100;


What is the purpose of having this commented out conditional block? With the new rules, we shouldn't ever be comparing a qualified build against each other or a non qualified build, since qualified and non qualified of the same version have the exact same bwc behavior.

I added this comment some time ago when I discovered that 7.0.0 would be considered smaller than 7.0.0-alpha1 which is incorrect, but changing that caused failures in VersionCollection and I didn't want to dig deeper at that time so just left this comment as a reminder.
The qualifier will not be relevant indeed for bwc behavior, but it's still relevant for the build, e.x. we still want to use an beta over an alpha if available so the build still has to order them.
I'm hoping that with the metadata format in #32872 the proper ordering will be clearer and the implementation will clean up as a result as well.
I think these are simple things, yet hard to get right with harsh consequences if they are not right, I'm reluctant to change them without the visualization described in #32872 to be able to better reason about it.

we still want to use an beta over an alpha

There shouldn't be anything needing to choose a beta over an alpha? There should be nothing using any qualified build to check bwc.

I removed the commented code to avoid confusion.
To make sure my understanding is correct: we will need to consider qualifier in the build, i.e. so we don't BWC test against an alpha if a beta is already available. This is for build only, server will not know of qualifier, so it has no way to decide based on it.

I don't know what you mean by "consider qualifier in the build". There should be no attempt to test bwc of any qualified version, so I don't believe the build needs to know about it in our build Version, since that class is all about which versions we bwc test against.

The version class is now used more broadly, including in figuring out dependencies ( as it's string value is set as project.version which is then used when considering dependencies ).
My understanding was that in BWC testing we will want to pic up alpha, beta etc as these become available, I'm starting to realize that since we don't offer guarantees we won't have to test with those and would just test against the latest snapshot and be ok.

I don't think we should remove the qualifier from the version right now, we should eventually better express requirements for a version used in bwc, perhaps as part of #32872.

The version class is now used more broadly, including in figuring out dependencies ( as it's string value is set as project.version which is then used when considering dependencies ).

There shouldn't be any dependencies on a qualified version, so there should be no need to serialize it into a string value.

I don't think we should remove the qualifier from the version right now, we should eventually better express requirements for a version used in bwc

Why would we keep something around that is unused? I think it only adds to confusion in a class that is already difficult to under (our gradle's Version class).

…ware

alpar-t · 2018-08-22T08:08:02Z

@elasticmachine test this please

droberts195 · 2018-08-28T16:04:17Z

buildSrc/src/main/java/org/elasticsearch/gradle/VersionProperties.java

+            baseVersion.getMinor(),
+            baseVersion.getRevision(),
+            // TODO: Change default to "" after CI "warm-up" by adding -Dbuild.version_qualifier="" to ML jobs to produce
+            //       a snapshot.


We now have the ability to set the ML C++ version qualifier when kicking off a build, so if it makes life easier for you I can create a one-off build of ML C++ 7.0.0-SNAPSHOT artifacts now so that the PR build can pass with a default qualifier of "". Then ping me when you merge this PR and I'll change the value for the periodic ML C++ builds to be no qualifier too.

…ware

alpar-t · 2018-09-06T17:28:02Z

@rjernst thank you! I get it now. The qualifier and snapshot is now completely removed from the version class. We still have to deal with them in a few places because the build will reference them like that in places like dependencies, the distributions will have them etc.
There's still some work I need to do on the tests, the docs check fails, but I wanted to put this up for feedback.

rjernst

Thanks @atorok, this looks better. One minor question: why does VersionProperties.getElasticsearch() exist? I don't see a reason to keep a variant returning Version and another returning String?

alpar-t · 2018-09-06T19:10:59Z

@rjernst The String version has qualifier and snapshot too if they are set so would be something like 7.0.0-alpha1-SNAPSHOT. This is what project.version is set to and what is used in the dependency resolution. We need this so that build-tool knows about it's version and can use it's version when building plugins and project.version might be set to something else. An alternative I considered was to call it getElasticsearchBuild.

alpar-t · 2018-09-07T06:09:28Z

@rjernst The more I look at the tests and the changes required to make this right, the less confident I feel about these changes and that I don't break any of the bwc tests. I think the logic is fairly complex and the implementation is spread across different places which makes it hard to understand and get right. I would rather revert to having snapshot and qualifier in Version and clean that up as part of #32872.

alpar-t · 2018-09-25T14:14:17Z

I'm closing this PR as it has grown too much in favor of multiple smaller changes starting with #34050.

alpar-t added 2 commits July 17, 2018 14:30

Switch build to dynamically configured qualifiers

2bb8616

Remove alpha1 from version qualifier

9dd442c

And relax the check in build to only take the number into account

alpar-t added the :Delivery/Build Build or test infrastructure label Jul 17, 2018

alpar-t requested review from jasontedor and mgreau July 17, 2018 13:20

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

9380817

…ware

jasontedor added the v7.0.0 label Jul 17, 2018

jasontedor requested changes Jul 17, 2018

View reviewed changes

alpar-t added 2 commits July 18, 2018 08:55

PR changes, version ID

08096d2

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

018fafa

…ware

Correct references to 7.0.0-alpha1

76b67b3

alpar-t added 3 commits July 23, 2018 15:48

Don't pass qualifier to plugin metadata

ee2359f

Rename from suffix to qualifier for consistency

54176b6

Temp: hard code information about released major

67c06f3

rjernst reviewed Jul 30, 2018

View reviewed changes

Add comment, reproduction info

cbaa33a

alpar-t added 2 commits August 6, 2018 12:25

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

6071cfd

…ware

fix references to 7.0.0 alpha1

218bbf4

alpar-t added 2 commits August 6, 2018 16:41

fix merge errort

d5ceca9

fix doc tests

6507480

alpar-t added 2 commits August 14, 2018 18:51

update version IDs

e10f818

PR review

1fe9ac4

alpar-t mentioned this pull request Aug 15, 2018

Consolidate release information into single metadata file that's consumed by builds and test #32872

Closed

4 tasks

rjernst reviewed Aug 20, 2018

View reviewed changes

alpar-t added 3 commits August 21, 2018 10:21

PR review: change method names

051539d

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

c9be34e

…ware

replace reference to alpha1 version

4195a55

droberts195 reviewed Aug 28, 2018

View reviewed changes

alpar-t added 11 commits August 29, 2018 16:27

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

377b632

…ware

Don't allow for dash in qualifier

dd5122e

remove qualifeir from version enums as part of merge

d5a3f2e

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

6a47804

…ware

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

53b1fce

…ware

Correct test failure

85d010e

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

6f53bd3

…ware

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

4074bc3

…ware

Merge remote-tracking branch 'origin/master' into version-qualifier-a…

e76a255

…ware

PR review: remove snapshot and qualifier from version

2e7b567

Make clusterformation deal with snapshot and qualifier not in build

dcf67a9

rjernst reviewed Sep 6, 2018

View reviewed changes

alpar-t closed this Sep 25, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Conversation

alpar-t commented Jul 17, 2018 • edited by nik9000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 17, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Jul 18, 2018

Uh oh!

alpar-t commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alpar-t commented Jul 25, 2018

Uh oh!

alpar-t commented Jul 25, 2018

Uh oh!

DaveCTurner commented Jul 30, 2018

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

alpar-t commented Aug 6, 2018

Uh oh!

droberts195 commented Aug 6, 2018

Uh oh!

jasontedor commented Aug 6, 2018

Uh oh!

alpar-t commented Aug 20, 2018

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

alpar-t commented Jul 17, 2018 •

edited by nik9000

Loading

alpar-t commented Jul 18, 2018 •

edited

Loading

alpar-t commented Jul 23, 2018 •

edited

Loading