I2I: Maintaining AMP version numbers for cherry-picked releases

[danielrozenberg]

Background

Version numbers

The last significant change to how we determine a release's version number was #16631. In that change we switched from UNIX timestamp (13 digits) to a date-time based solution (format YYmmDDHHMMSS) (12 digits) - to ensure the version numbers are monotonically increasing we added an arbitrary digit X at the end of the version (YYmmDDHHMMSSX).

Previously this digit was used to denote whether the version was built from a "clean workspace" (0 - no uncommitted changes; 1 - there are uncommitted changes) but this proved to not be useful, so we changed the version number to just default X to 0, and have it arbitrarily changeable by passing a digit to gulp dist --custom_version_mark (now --custom_version which sets the entire version number). This feature was used exactly once.

Cherry picks

When a breaking issue (one that meets the cherry pick criteria) is discovered on Stable or other channel with active traffic to them we rollback to a previously known good version, create and merge a fix to master, then cherry pick the fix onto the broken version, and deploy it. (on rare occasions where the fix is already available, we sometimes skip the rollback and deploy directly on top of the breaking release. This is done at the discretion of the TSC)

The version number resulting in this cherry pick uses the time when the PR that fixes the issue is cherry picked onto the existing release. While in line with the method chosen in #16631, this creates difficulties in tracing back which versions are cherry picked versions and on top of which release they were created.

For example, the following is a graph of some recent releases:

Releases marked in blue were built from commits that are on the master branch; commits in black were created with cherry picked commits. NOTE: this graph was modified slightly from the reality! This is due to a combination of changes in the release process and human error during the release, that would have made this solution seem much more complicated than it would be in reality.

While investigating a regression that was discovered on 2004142326360 (the release marked as Stable (rolled back), and was not present in 2004030010070 (marked as Stable - the Stable version previous to the one that was rolled back) it was difficult for AMP developers to perform a bisection of the releases and find the root cause. This convenient graph was not initially available and had to be generated manually. (for those curious about the issue that caused all this trouble, the rabbit-hole starts here: #27790)

Unlike releases built from commits that are directly on master, that correspond to a specific time and date in the revision history of this project, the cherry picked releases do not help trace back where their code lives or what versions came prior. They only indicate when the cherry-pick was performed; this does not expose relevant information.

Proposal

The proposal here is to use the trailing digit as an indicator of the number of cherry picks on top of an existing release. e.g., the first cherry pick on top of 2003241730220 will be called 2003241730221. A second cherry pick will be called 2003241730222, and so on.

If there are more than 9 cherry-picks, a new release will have to be performed. Since we switched to the current versioning schema, there have been 6 releases (on 3 release branches) that required more than 9 cherry-picks, and only 1 such in the past year.

Using this new versioning schema, the above graph would have looked like this:

/cc @ampproject/wg-approvers @ampproject/wg-infra

[rcebulko]

I whole-heartedly support this. I think it would make it much easier to follow discussions about releases

[danielrozenberg]

I personally feel like this is too small of a topic to go through a full design review, but I would still like to solicit feedback from interested parties. @mrjoro is this a legitimate thing to do? What's the process for this?

[rsimha]

I too support this proposal, and agree that tracking versions will be significantly easier if cherry-picks use the original 12 digits (date + time) and modify the 13th digit. This will enable our existing versioning system to trivially indicate when a release is a minor / patch release.

[mdmower]

I think this is probably the right design. With the LTS branch's long life, I suspect there will be numerous releases with 10+ cherry picks. The sudden shift in version numbers will be unfortunate. I considered recommending something like "when any release is cut from master, set seconds to zero in timestamp, allowing for up to 999 cherry-picks", but this breaks from convention. It would be a redefinition of the version number, really, where no two releases are allowed to be cut within the same minute. So, back to my first sentence, I think this is probably the right design.

[danielrozenberg]

The only case where we had more than 9 cherry-picks in the past year was indeed on top of an LTS release (2002191527100, the 13 commits between fe9f4cb and aab4f9b are the commits that have been cherry-picked), and this was pretty early after the creation of LTS releases, so I would say the benefits still outweigh the confusion that can come from resetting the release number for the 10th CP :)

[rsimha]

I considered recommending something like "when any release is cut from master, set seconds to zero in timestamp, allowing for up to 999 cherry-picks", but this breaks from convention.

I gave this topic some additional thought today, and am beginning to see the value of following @mdmower's recommendation to eliminate the two seconds digits from cherry-pick versions only. While it is indeed a deviation from the full 12 digit timestamp used for normal releases, I think it's good enough to have 10 digits in common between cherry-pick versions and their base versions. (It will still eliminate the confusion that came from using a completely different date.)

Also, as you've pointed out, this can come into play in future as we have more LTS versions, and will eliminate the need for an artificial cap on the number of CP releases that follow a given base release. I think it's more likely to have 9+ CPs on a given LTS version than it is to cut two release branches during the exact same minute.

For example:

Name	Version
Base	2002191527100
CP 1	2002191527001
CP 2	2002191527002
...	...
CP 15	2002191527015

WDYT?

[mdmower]

... to eliminate the two seconds digits from cherry-pick versions only.

If you opt for the "reset seconds to zero" idea, I don't think you can reserve it for cherry picks only. I think it has to be done when a release is cut from master. For example, if you only reset the seconds when cherry picks begin, how do you know whether the following version is the original release or the 10th cherry pick?

012004201111010

This is why I was hesitant to push the idea. It effectively changes the version convention across the board.

[estherkim]

+1 Matt's point that it has to be set to zero at the time a release is cut.

Building off that and Raghu's example, why not set the last 3 to zero?
Format: YYMMDDHHmm000

[rcebulko]

I'm supportive of this move, both because it holds useful CP info and because simpler RTV strings will make it easier to visually read them.

[danielrozenberg]

While the seconds don't provide a meaningful enough granularity to insist on keeping them, there is the issue where there might be existing tools that expect digits 3 through 14 to be in the YYmmDDHHMMSS format, especially in the CDN server code - if we go down this route we should triple check that to ensure nothing breaks :)

[mdmower]

there might be existing tools that expect digits 3 through 14 to be in the YYmmDDHHMMSS format

While I'm still not advocating for the idea, strictly from a "is the format invalid" standpoint, YYmmDDHHMMSS won't have any trouble until cherry-pick number 600. I hope this wouldn't ever happen...

[danielrozenberg]

Yeah I thought about that. It'll just be semantically wrong in those systems that parse the RTV but they wouldn't fail :) alright, I'm gonna change my PR to reflect this discussion

[rsimha]

Good catch @mdmower, and thanks everyone else for the follow up!