x/build/dashboard: atLeastGo1 helper incorrectly reports dev.* branches as not meeting the minimum Go version

[thanm]

Filing an issue here as a starting point for a discussion about how the builder/farmer and dashboard handle dev.* branches (as opposed to master or release branches).

Current design uses a full set of builder configurations for master, then smaller sets for less interesting or less actively worked on branches (ex: dev.boringcrypto, older release branches, etc).

It would be useful to have a way to tell the builder configuration that a given branch is "hot" and should have a full set of builders. Cherry created a preliminary CL 223381
to do this, but there were concerns that this might not be a way to go.

Hoping folks will comment on what might be the right path forward.

@toothrot @cherrymui @thanm @jeremyfaller @aclements

Please add in any other participates who might be interested.

[ianlancetaylor]

Do slowbots work on dev branches?

[toothrot]

I am working on #34744 to move the dashboard into the coordinator. Among other things, this will add unit tests that should make the dashboard easier to work on.

I'm trying to clean up the data structure a bit as part of the move, but it already improves on how we render and hide builders. There's a good amount of work to do still, but this feature seams reasonable.

/cc @dmitshur who commented on the CL about builder capacity.

[thanm]

@ianlancetaylor I believe slowbots do work on branches, the issue though is post-commit testing (since a trybot run is not the same as a post-commit builder run).

[dmitshur]

Thanks for filing the issue.

Cherry created a preliminary CL 223381 to do this, but there were concerns that this might not be a way to go.

To clarify, the concerns were only with the current implementation, which we can resolve after clarifying the high-level goal in this issue.

I understand the goal of this issue is to start running (some or all?) post-submit builders on active dev.* branches, at least on recent commits. We still need to work out some specific details, but at a high level I'm convinced it's a reasonable request and we should probably do it (but I welcome feedback from others who are maintaining x/build). /cc @toothrot @cagedmantis

@thanm @cherrymui I have a question about the number of builders you'd want or expect to be used. Do you think all builders that are currently testing the master branch should also start testing new commits on dev.* branches? Or should it be some subset?

[thanm]

Each of the dev.* branches is very different. For starters there's quite a few that are effectively dead (ex: dev.garbage).Then there are the dev.boringcyrpto's, which are all pretty low traffic. Then there is dev.link, which is super active ... and that's pretty much it. For dev.link, we'd really like to see the same set of builders as master.

[dmitshur]

Looking at our current builder configuration more, I see that we already build most dev.* branches on most builders. The default build policy does not reject non-master branches as I previously thought. For example, the dev.link branch specifically has many post-submit builders:

https://build.golang.org/?branch=dev.link

The "aix-ppc64" builder specifically has a custom build repo configuration:

buildsRepo: func(repo, branch, goBranch string) bool {
	switch repo {
	case "net":
		// The x/net package wasn't working in Go 1.12; AIX folk plan to have
		// it ready by Go 1.13. See https://golang.org/issue/31564#issuecomment-484786144
		return atLeastGo1(branch, 13) && atLeastGo1(goBranch, 13)
	case "tools", "tour", "website":
		// The PATH on this builder is misconfigured in a way that causes
		// any test that executes a 'go' command as a subprocess to fail.
		// (https://golang.org/issue/31567).
		// Skip affected repos until the builder is fixed.
		return false
	}
	return atLeastGo1(branch, 12) && atLeastGo1(goBranch, 12) && defaultBuildsRepoPolicy(repo, branch, goBranch)
},

The atLeastGo1(branch, 12) && atLeastGo1(goBranch, 12) && defaultBuildsRepoPolicy(repo, branch, goBranch) condition makes it not work for non-master branches. I suspect that wasn't intentional: the goal of atLeastGo1 is to require a specific Go version to be met, not to filter out non-master branches.

If my understanding above is right, this is a bug for the "aix-ppc64" builder configuration that we should fix. (We should also see if other builders are misconfigured in a similar way.)

Does this match your understanding?

[thanm]

I agree that

return atLeastGo1(branch, 12) && atLeastGo1(goBranch, 12) &&
  defaultBuildsRepoPolicy(repo, branch, goBranch)

has the effect of disabling the builder for dev.* branches.

I also agree that rewriting the rules for this builder would help in this specific instance, but it seems to me that this is a bit of a bandaid, in that it wouldn't prevent similar problems in the future.

When someone else adds a new builder at some point in the future, they will most likely cut & paste the rules for some existing newish builder, which will probably look something like

addBuilder(BuildConfig{
	Name:     "mynewbuilder",
	HostType: "host-mumble-newchip-something",
	buildsRepo: func(repo, branch, goBranch string) bool {
		return atLeastGo1(goBranch, XX) && 
                      defaultBuildsRepoPolicy(repo, branch, goBranch)
	},
})

where XX is the current Go release (so as to not have the builder run on release branches that don't support the new config). That would put us back in the same situation at some point in the future when another "hot" dev branch comes along (very easy to imagine "dev.generics", for example).

Thinking out loud, perhaps the right thing to do would be to write a helper function

includeDevBranches(branch string, version int)

that would examine the specific dev.* branch and return true/false based on what the era of the specific branch, then call that helper before checking the Go release, e.g.

return includeDevBranches(goBranch, 13) && atLeastGo1(goBranch, XX) && defaultBuildsRepoPolicy(repo, branch, goBranch)

This seems functionally similar to changing "atLeastGo1" but it would have the advantage of reminding folks to take a moment and consider dev branches when they add new builders, maybe.

[cherrymui]

Do you think all builders that are currently testing the master branch should also start testing new commits on dev.* branches? Or should it be some subset?

As Than said, it depends. I think the common/default case is to test on all builders, in the sense that the dev branch will eventually be merged back to master, which should not break anything. There are special cases, e.g. boringcrypto only works on x86, which can be addressed case by case.

the goal of atLeastGo1 is to require a specific Go version to be met, not to filter out non-master branches.

Yes, that was the intent of my CL.

[dmitshur]

Thanks for providing more information and having the discussion.

It has helped me understand the current situation better, and I think CL 223381 is a safe change to make now. We're already building commits on other branches, so there are no concerns with too many new commits needing to be built. I also understand that the low-level atLeastGo1 helper is where the problem was, so changing it to implement this high level behavior is actually the right place. The added test cases will help prevent regressions if/when this code is refactored in the future.

As a future followup, I want to re-work the atLeastGo1 helper to be more precise. It should be concerned only with the Go version, not other dimensions. It has problems right now, such as atLeastGo1(goBranch, 16) incorrectly reporting that "master" branch (and soon, "dev.*") is Go 1.16 already (it isn't yet!). We have very precise information about the Go releases in cmd/coordinator, and can do better. But we don't need to wait on that to do better today. (Edit: Opened #37965 for this.)

I'll update my review on the CL.

[gopherbot]

Change https://golang.org/cl/223381 mentions this issue: dashboard: treat dev branches as current