build: nightlies off master and release-23.1 report incorrect version information

[knz]

Describe the problem

On master and release-23.1, the version, or buildinfo tag, as used in output of cockroach version, reported telemetry, debug zips, version information presented on to operators in cockroach status and the db console, etc -- specifically the build tag -- was switched to be backed by the content of a checked in file.

This checked in file is not edited in every commit, and thus two commits, with different content, different bugs and fixes, etc can both describe themselves as the same version. The downstream effects of this on operators needing to know if they need to deploy a fix, or support and field debugging issues, could be significant: it becomes impossible to ascertain which version of the code is actually running when troubleshooting a problem:

• if someone adds some commits to their branch and builds that, the output of cockroach version will not change.
• if someone modifies the code in the branch and rewrites (git commit --amend), the output of cockroach version will also not change.

To Reproduce

Run cockroach version and observe.

For example, a 23.1 nightly, built from 5d5abe7, is accurately described as v23.1.0-alpha.8-219-g5d5abe704b0 (5d5a, which is 219 commits on to of alpha.8), describes itself as alpha.9:

$ ./cockroach-v23.1.0-alpha.8-219-g5d5abe704b0.linux-amd64/cockroach version
Build Tag:        v23.1.0-alpha.9

But a nightly build from 63b683e, described as v23.1.0-alpha.8-223-g63b683eab3e which is not the same code as the one above -- as seen by the fact it has more commits and a different SHA -- also describes itself as alpha.9! if you ask it what version it is, or in reported telemetry/panics, or look at status to figure out if you need to roll out a fix, etc:

$ ./cockroach-v23.1.0-alpha.8-223-g63b683eab3e.linux-amd64/cockroach version
Build Tag:        v23.1.0-alpha.9

This will cause considerable confusion in production.

Expected behavior

The build tag should include character that uniquely identify the commit SHA.

Jira issue: CRDB-26479

[knz]

I think we should revert edc4b47 and then consider an alternate approach later after the 23.1 release.

[knz]

I have tried to investigate how come #97214 was made that way and how this major problem wasn't caught there.
It appears there was miscommunication here: #91306 (comment)

At the time we already knew we need to have the SHA in the build tag and the recommendation was to add (not replace) a field containing a "release series" with a more stable version number.

[rickystewart]

The build tag should include character that uniquely identify the commit SHA.

The commit SHA is still in there. Can we not simply update the Build Tag to append the commit SHA, if that is the desired behavior? I am not seeing the problem here that requires us to perform a very high-friction revert.

[knz]

Can we not simply update the Build Tag to append the commit SHA?

yes we can perhaps do that (we can even reduce to just the subset of the commit SHA that uniquely describes the commit, like git-describe does).

we also need to ensure the call to override the version in many main_test.go files also fixes that field.

[knz]

I am not seeing the problem here that requires us to perform a very high-friction revert.

I agree the revert is only the fall-back solution if we cannot do something more clever.

[rickystewart]

dev-inf cannot singlehandedly make this change (we don't have a way to validate all the necessary invariants are met). I am assigning @renatolabs to this. Unless just updating the Build Tag output from cockroach version is good enough, but then anyone can make that one-line PR.

we also need to ensure the call to override the version in many main_test.go files also fixes that field.

Can you say more about this?

[knz]

We have in many packages a call like this:

        defer build.TestingOverrideVersion("v999.0.0")()

because the tests perform comparisons against the version string and we want these tests to have a stable output.

[rickystewart]

The build tag should have the revision attached to it if the build is not built with release as the build type:

cockroach/pkg/build/info.go

Line 70 in cd933d3

if IsRelease() {

So I think what is actually being requested here is to not tag these nightlies as release (since if we give it a different tag, like nightly, then the build tag will have the revision as expected). Does that seem fine?

[knz]

When I run dev build short on my machine, I don't get the SHA either. does dev build use the release build type by default?

[rickystewart]

dev build short does not stamp the built binary, so i.e. this stuff (build revision, etc.) is not going to be there. No, the build type is not release (it will be an empty string in this case).

Cross-builds (dev build --cross) are stamped and will have the build revision.

[knz]

ok thank you for explaining. i'm going to run a cross build to satisfy myself that all is well and then we can close this.

[dt]

Wait, I’m still getting it: we don’t want all builds sha identified ie we want a build that is actually of 23.1.4 to identify itself as 23.1.4, not via
a sha which we then need to compare to git to find out if it is “23.1.4” or not. How do we fix this without changing the stamping?

[knz]

David, look waht this release stamping gives us:

kena@gceworker-kena ~/cockroach % ./artifacts/cockroach version
Build Tag:        v23.1.0-alpha.8-dev-cc9e0c693cd4186bdef21e62d2cb5af2cf38b589
Build Time:       2023/04/03 23:18:45
Distribution:     CCL
Platform:         linux amd64 (x86_64-pc-linux-gnu)
Go Version:       go1.19.4
C Compiler:       gcc 6.5.0
Build Commit ID:  cc9e0c693cd4186bdef21e62d2cb5af2cf38b589
Build Type:       development

I think this build tag is OK to me. What do you think?

[knz]

So to wrap up here is my understanding:

• we are replacing our previous use of the git branch label by a file version.txt, so that adding and removing branch labels and tags doesn't influence the build tag any more.

• separately, we also stopped including the SHA in dev build rules except in release builds, which makes it possible for bazel to cache/reuse more artifacts across commits in the same branch.

@rickystewart is that understanding correct?

[dt]

not tag these nightlies as release

I think we need something stronger than this, specifically a mechanism that ensures that for a given content of version.txt, we can only ever build at most one SHA with release=true, since otherwise we're back to the really bad behavior shows between the two nightlies in the original issue text.

[dt]

Even if we do this -- figure out a mechanism by which we can ensure we only pass release=true for a specific SHA -- I still think identifying publicly -- in status, in telemetry, in crash reports, etc -- using this version in file is a step backwards: we've gone from a safe-by-default mechanism, where the identifier for the binary, used in all the places where it is critically important that we correctly identify it accurately, was derived from the source of truth (git, i.e the version of the code itself), to unsafe-by-default: now if we build two binaries, from two different SHAs, but happen pass release=true both times, we end up with two different binaries that incorrectly claim to be the same thing.

[dt]

My recollection is that identified the need to shift to using a checked-in file so that tests which needed to determine the predecessor version would change what version they thought they were running at when they went to look up the predecessor for that version? right? Why not just check in "predecessor_version.txt" for that purpose then, versus changing how a binary determines its own version?

[rafiss]

Just want to point out: the old approach was "safe" but I also think it was incorrect and confusing, as I described in #75287

[dt]

I also think it was incorrect and confusing

Confusing, sure, when your tags aren't fresh. But "incorrect"? I'd say it was less capable of being incorrect in that it would never claim to be a SHA that it isn't -- it might say it is SHA foo that is 10000 commits after v1.2, which, like, is less informative than it saying it is v8.9, but that's still more correct -- it accurately and more importantly uniquely identified the running code -- than a scheme that can describe two completely different versions of code as being the same version. Is It confusing to call v8.9 v1.2+1000 ? yes. but still much more correct than to call something that isn't actually 8.9 "8.9".

[rafiss]

I'm not disputing that it's less incorrect. Just saying, if we have any alternative, let's not go back to something which is confusing and incorrect (bearing in mind that the current situation is also incorrect).

[dt]

I think my short-term proposal would be, rather than a bool to SHA-suffix the file content or not, which seems like it has a pretty bad failure mode if we ever mess it up, instead we add back an empty string package var that can then set using ldflags to the git describe output, called something like releasedAsVersion in, and only in, release builds (basically it being non-empty would replace IsRelease).

If releasedAsVersion is non-empty, that's the version we identify as, plain and simple. If it is empty, we do more or less the current: <version.txt-content>-dev-. The one difference from the current code would be that version.txt content is never presented _as the version_ without the added -dev` suffix and stamped SHA if available, preventing, the example seen in the issue text above, where two different SHAs both identify as the same version.

We would pass the ldflag to set releaseAsVersion in all nighties and final release artifact building (everywhere we build "release" binaries today), so they'd use the git-backed same identifiers had from v1 to v22.2, while all tests would not set the ldflag -- just as they don't set IsRelease today -- and thus continue to use the checked in file.

Basically, I think this is similar to the suggestion above to just use IsRelease, but with the slight tweak that when you use it, you provide a unique value directly, vs just hoping the content of version.txt is unique at the time.

[rickystewart]

@knz Yes, you have summarized the situation accurately.

The previous git tag-based approach was not more "correct" in any real way than what we're doing now. That approach was also flawed and sometimes has intuitively "incorrect" behavior (like internal example from today -- keep in mind this is about 22.2 which had the old way of doing this versioning).

I'd say it was less capable of being incorrect in that it would never claim to be a SHA that it isn't

This will not happen in the normal course of use. The Build Commit ID from cockroach version will be the SHA that came from git (or an empty string for development builds).

Of course, anyone can just build their own binary that says it's whatever SHA, but anyone could do that at any point.

[celiala]

I'm trying to confirm what is being proposed -- is it to combine @renatolabs and @dt 's ideas to do something like:

// pkg/build/info.go
var (
    publicReleaseVersion = os.Getenv("OPERATOR_SUPPLIED_TAG","")
)

func computeBinaryVersion() {
    if IsRelease() && publicReleaseVersion == version.txt {
        return version.txt
    }

    return version.txt + "-dev" + SHA if available
}

[rickystewart]

Right now, if you hit build on two different SHAs that have the same version.txt content, they'll identify the same.

You have to explicitly set the build environment to release for that to be the case. Otherwise the build ID will contain a substring like -dev-.

If you build two different SHA's with the same version.txt content that are both explicitly release builds, they will both claim to have the same version, of course. I don't see what is incorrect about this. In fact it seems like what the build should do -- if the code says we're at a version, just trust it instead of trawling through git . In both steps the person conducting the release can forget a step (updating version.txt, or forgetting to push or pushing the wrong git tag). I agree with @renatolabs that in the version.txt case it is at least trivial to look at the state of the repo to see you missed a step, unlike with git tags where it is unclear how to audit the state of the git tags on origin to be sure you have not messed anything up and the git describe will return the value you expect.

In any case, the actual SHA is still there in the same place it has always been.

With regard to solving the nightly build tag problem, I propose all of the following.

1. Update Publish Bleeding Edge to use some non-release value for build.typ (nightly? ci?)
2. In addition to IsRelease, add a function like HasReleaseBehavior: func HasReleaseBehavior() bool { return IsRelease() || typ == "nightly"; }. Now, we have two functions that do what they say on the tin rather than one function IsRelease that lies. (Actually, we already have the function SeemsOfficial, so maybe just use that one.)
3. Update all invocations of IsRelease to perform the correct operation, either IsRelease (if they are trying to determine whether this binary is actually released) or HasReleaseBehavior (if they want to perform a conditional action on whether this is a release-like binary like a nightly). IsRelease is called like <10 times in the entire codebase so this is not prohibitive.

From that point forward people writing new code will have to decide whether IsRelease or HasReleaseBehavior is the appropriate function to call, but of course now that we have two functions with clear contracts, they will be equipped to make that decision properly.

[dt]

Thinking a bit more here:

If we go the publicRelease flag route, and rely on the version.txt file to be the canonical source of truth for how a binary we publish will identify itself, then I think we'd need to adapt the rest of the publication process use it for the source of truth as well -- i.e. use its value in release.MakeRelease, release.PutRelease, etc when deciding the artifact name, the path to upload the artifact, ensure that that publication fails if something already exists there, etc. That would ensure that we never publish two binaries that use the same identifier, because if we did, publication itself would fail.

[dt]

@rickystewart How about this slight adjustment to your suggestion:

Have IsRelease remain true in places it is true today, i.e. keep nightly artifacts as returning true (so IsRelease would be typ == "release" || typ == "nightly"), but then use the logic that cat's version.txt and optionally appends -dev-$sha based on the build type not being "release" to pick canonical "build ID" during make and publish so that that is then used both by the binary itself to identify itself (presumably passed to it via ldflag) but then also by the build and publish pipeline?

[rail]

A few points.

• AFAIK, we don't have nightly builds, only tests. Maybe call them "post-merge" or some other name that reflects their nature?
• If we decide to use publicRelease, maybe rename it as publicBuild?

[dt]

One thing I'll point out: there seems to be a strong desire to not to base the binary's reported version of the unique tag in the git, but in basically all debugging and support interactions, we use the binary's reported version to go look at git and what it says the code as of that tag was. When a customer says "I'm running 22.2.1 and see the job retrying when it does X", we go look at git to see what the code as of tag 22.2.1 did, if a given fix is in its parents, etc. The git tag is our source of truth for what "22.2.1" was.

But, if we're dead-set on not forcing the binary's reported version to match the canonical scm tag, then I'd say we should at least ensure the publication pipeline ensures we cannot publish the same reported version twice: use the version.txt-derived identifier as the "Build ID", fail to upload artifacts if it already exists, etc, so that we ensure we never have two published binaries floating around that claim to be the same thing the way we currently do.

[dt]

I drafted something in #100694 that I think hits both goals: (a) use the version file instead of describe but also (b) ensure that what we publish as a particular version, including from the continuous publication CI, always identifies as the version it was published as, which should also ensure nothing else identifies as the same thing

[jlinder]

AFAIK, we don't have nightly builds, only tests. Maybe call them "post-merge" or some other name that reflects their nature?

We do have nightly builds. The "Make and Publish Build" family of build configs produces them. (Sometimes we forget things.)

On the proposals:

The things I'd like to see in whichever solution we choose (Ricky's, Ricky's + DTs mod, Renato's) are:

• Make API in publish provisional artifacts clear about there being 1) a release level of stamping and 2) a post-merge but not release level of stamping
• Have a parameter to publish-provisional-artifacts specify which part of the release pipeline the binary(ies) are being built in (see below for more on this)

Update Publish Bleeding Edge to use some non-release value for build.typ (nightly? ci?)

This needs to be done for Make and Publish Build as well. Given that need, how about we make the value either more generic (pre-release?) or more specific to the stage in the release process the build is from? For example:

• Publish Bleeding Edge (edge)
• Make and Publish Build (nightly)
• in CI (ci)
• built outside TC (dev)
• a future custom/one-off build build config (custom or one-off) (we are planning to split this from make and publish build)

If the value is more specific, we could identify where in the release process a given binary was generated.

[dt]

I started working on implementing @rickystewart's suggestion above to have a different build.typ value other than release for nightlies, so that computeBinaryVersion could switch on that when deciding to SHA-suffix.

But when I got to hooking that up in make-and-publish-build-artifacts.sh, I'm now realizing something else: even once we implement it, we've introduced a subtle but worth discussing change, perhaps a breaking change, in how nightlies describe themselves.

Previously, for the past few years, a nightly or custom build from a release branch, say from release-22.2 on some SHA after v22.2.6 but before v22.2.7 would report itself as being v22.2.6-8-gSHA, which roughly translates to v22.2.6 plus some stuff where "some stuff" is eight commits, ending in SHA. The most visible/significant thing is that it is based on v22.2.6 but with some patches.

As it now stands, assuming we fix the IsRelease thing, that same build, which is half way between v22.2.6 and v22.2.7, will now report itself as 22.2.7-dev-SHA.

Maybe this is better -- not that we follow semvar, but if we did, it would say this is the correct way to identify a pre-release of 22.2.7 which anything taken from release-22.2 after 22.2.6 is tagged would be -- but it is certainly a change in behavior, and perhaps one on which we should consult with the TSE, L2 and field teams?

In particular, it is a little scary we're changing it in-place: previously a nightly identified as "v22.2.6-blahblah" was greater than 22.2.6, but now it'll be the exact opposite -- if you see v23.1.6-blahblah it means you're actually running something below v23.1.6 now. Again, maybe this is better -- $nextVer-$prereleaseID is how we message alphas or betas of the major after all -- but its a potentially breaking change when it changes the meaning in-place like this, in particular to mean the opposite of what it did but in a subtle, easy-to-miss way. We should check with TSE to make sure it sounds okay to them, and/or make this less subtle (identify as v23.1.7-prerelease-gSHA or something that more loudly announces the "pre" aspect)

[dt]

One option: we could very narrowly, i.e. only for release builds for 23.1, revive the old behavior for now. That is, we could re-open #100581, so that release builds and only release-builds would use use have the $build_name, which is currently picked with the old sam-state-based logic, passed into the release binary to be the value of the version identifier.

I believe doing so this would mean there is no user-visible change between reported version behaviors in 23.1 release builds compared to 22.2 release builds, meaning 23.1 could then be unblocked to proceed while we figure out the longer-term solution (introduce more release build types, figure out how to present them and their derived version identifiers to users).

I don't think using #100581 as in interim measure to unblock 23.1 would bring back too much of the negative effects of SCM-backed version identifiers either -- IIRC most of the acute drawbacks to SCM-picked version identifiers were their effect on dev builds and specifically tests, not release builds, but tests should be unaffected by #100581 given its narrow applicability to only release builds.

[rickystewart]

One option: we could very narrowly, i.e. only for release builds for 23.1, revive the old behavior for now.

What would be the advantage of doing this? So we don't have to notify TSE/L2/field that the build tags for nightly builds of cockroach is changing marginally to become more consistent with standards like semver?

I think is probably sufficient to clarify that "You should not be reading any special meaning into whatever characters we add at the end to disambiguate between alphas of the same number (but just so you know, after 23.1 it means...)", and ASAP before our next major release is as good a time as any to do it. Of course my hunch about that may be wrong, so reaching out to all of these groups is still a good idea.

I believe doing so this would mean there is no user-visible change between reported version behaviors in 23.1 release builds compared to 22.2 release builds, meaning 23.1 could then be unblocked to proceed while we figure out the longer-term solution (introduce more release build types, figure out how to present them and their derived version identifiers to users).

AFAIK the only thing that is actually a blocking issue is that the alphas don't have disambiguated build tags. We can (I think) easily address this specifically without much churn. I think it is highly preferable to fix forward rather than go back and do a bunch of monotonous reverts and backports just to add some regressive logic back into the build system.

[rickystewart]

I can write a PR to implement the fix, but I don't want to collide with @dt who apparently has been working on this.

[dt]

not be reading any special meaning into whatever characters we add at the end

My point is that we're doing the opposite: even if you say to ignore everything on the end, we're changing the meaning of the stuff at the front.

We push make and publish builds to customers all the time; for several weeks last month, all 22.2 CC clusters were running 22.2.6-blahblahblah. 22.2.6-blahblahblah meant they were running "22.2.6 plus some patches" but currently, if we did the same thing with 23.1-style stamping, that same "22.2.6 plus some stuff" would identify as "22.2.7-blahblahblah".

Is that okay? Maybe it is! You can argue that "22.2.6 plus stuff" is by definition a pre-release of 22.2.7, so maybe this is fine, but as far as I know, we never asked CC SREs, TSEs, L2s, etc about this (if there was an RFC or doc, I didn't see it?).

a bunch of monotonous reverts

FWIW, my suggestion above was one commit changing ~20 lines specifically for release builds and not reverting anything else, just to keep status quo for 23.1 release builds while we vet the proposed behavior change.

[celiala]

reaching out to all of these groups is still a good idea.

I don't know if anyone has already created a vehicle to having this conversation?

If not, I've created [DRAFT] Build Tag for Nightly, Custom and Production Release Builds:

• I believe I've captured the latest proposal into "Proposal 1", but would appreciate a review to ensure correctness / completeness.
• Were there other Proposals to consider? If so, feel free to add; otherwise, we can start the conversation with just "Proposal 1".

Once we 👍🏼 on correctness / completeness we can loop in the groups mentioned.

[rickystewart]

I've renamed the title of the bug to reflect the issue which is that the nightly builds report incorrect, non-unique build tags. The PR I will file to close this is targeted toward that one problem specifically. We will follow up with other work that will update the build tags further to add more precision to the edge cases we've described.