Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/build/cmd/coordinator: runSubrepoTests (golang.org/x repo tests) should also check maxTestExecErrors constant #36226

Open
dmitshur opened this issue Dec 20, 2019 · 0 comments
Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

In my investigation at #35581 (comment), I wrote:

It is intentional to keep retrying "communications failures" forever, because the expectation is that they should eventually succeed.

I'm seeing now that this isn't quite true. There is a constant defined:

// maxTestExecError is the number of test execution failures at which
// we give up and stop trying and instead permanently fail the test.
// Note that this is not related to whether the test failed remotely,
// but whether we were unable to start or complete watching it run.
// (A communication error)
const maxTestExecErrors = 3

The runTestsOnBuildlet method, which is called by runTests method, has block that checks if ti.numFail has reached maxTestExecErrors:

if err != nil {
	bc.MarkBroken() // prevents reuse
	for _, ti := range tis {
		ti.numFail++
		st.logf("Execution error running %s on %s: %v (numFails = %d)", ti.name, bc, err, ti.numFail)
		if err == buildlet.ErrTimeout {
			ti.failf("Test %q ran over %v limit (%v); saw output:\n%s", ti.name, timeout, execDuration, buf.Bytes())
		} else if ti.numFail >= maxTestExecErrors {
			ti.failf("Failed to schedule %q test after %d tries.\n", ti.name, maxTestExecErrors)
		} else {
			ti.retry()
		}
	}
	return
}

However, the runTests method is only used for the main Go repository, not golang.org/x repos:

if st.IsSubrepo() {
	remoteErr, err = st.runSubrepoTests()
} else {
	remoteErr, err = st.runTests(st.getHelpers())
}

So this bug is about making the golang.org/x repos path also use the maxTestExecErrors constant and give up after some number of tries.

It's low value to fix because we rarely run into a situation where communication errors happen 3 times or more; that happens most often due to other bugs which we need to fix anyway.

/cc @bradfitz @cagedmantis @toothrot

@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done. FeatureRequest labels Dec 20, 2019
@dmitshur dmitshur added this to the Backlog milestone Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

1 participant