From baccc13e017dfaae5e676514b3b3bcd0a17c5588 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Fri, 22 Jul 2022 14:05:13 -0400 Subject: [PATCH] internal/relui: run "advisory" TryBots We don't have the ability to run security fixes through the real TrybBts before we commit them to the private repo. To avoid surprises like we just had with the wasm tests, run secondary TryBots, but don't treat them as release-blocking. If any fail, the coordinator will be asked to approve the results. For golang/go#53799. Change-Id: Icad4ece6e32f47dc81f4a8d850f56cf488c7a030 Reviewed-on: https://go-review.googlesource.com/c/build/+/419415 TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov Run-TryBot: Heschi Kreinick Auto-Submit: Heschi Kreinick --- internal/relui/buildrelease_test.go | 57 +++++++++++++- internal/relui/workflows.go | 116 +++++++++++++++++++++------- internal/task/buildrelease.go | 21 +++++ 3 files changed, 160 insertions(+), 34 deletions(-) diff --git a/internal/relui/buildrelease_test.go b/internal/relui/buildrelease_test.go index 9df78cc9b1..76aeaa04f6 100644 --- a/internal/relui/buildrelease_test.go +++ b/internal/relui/buildrelease_test.go @@ -143,8 +143,11 @@ func newReleaseTestDeps(t *testing.T, wantVersion string) *releaseTestDeps { CreateBuildlet: fakeBuildlets.createBuildlet, DownloadURL: dlServer.URL, PublishFile: publishFile, - ApproveAction: func(*workflow.TaskContext, interface{}) error { - return nil + ApproveAction: func(ctx *workflow.TaskContext, _ interface{}) error { + if strings.Contains(ctx.TaskName, "Release Coordinator Approval") { + return nil + } + return fmt.Errorf("unexpected approval for %q", ctx.TaskName) }, } // Cleanups are called in reverse order, and we need to cancel the context @@ -172,7 +175,7 @@ func testRelease(t *testing.T, wantVersion string, kind task.ReleaseKind) { wd.Output("Published Go version", v) w, err := workflow.Start(wd, map[string]interface{}{ - "Targets to skip testing (or 'all') (optional)": []string(nil), + "Targets to skip testing (or 'all') (optional)": []string{"js-wasm"}, "Ref from the private repository to build from (optional)": "", }) if err != nil { @@ -291,7 +294,7 @@ func testSecurity(t *testing.T, mergeFixes bool) { wd.Output("Published Go version", v) w, err := workflow.Start(wd, map[string]interface{}{ - "Targets to skip testing (or 'all') (optional)": []string(nil), + "Targets to skip testing (or 'all') (optional)": []string{"js-wasm"}, "Ref from the private repository to build from (optional)": "security-ref", }) if err != nil { @@ -316,6 +319,42 @@ func testSecurity(t *testing.T, mergeFixes bool) { }) } +func TestAdvisoryTrybotFail(t *testing.T) { + deps := newReleaseTestDeps(t, "go1.18rc1") + defaultApprove := deps.buildTasks.ApproveAction + approvedTrybots := false + deps.buildTasks.ApproveAction = func(ctx *workflow.TaskContext, i interface{}) error { + if strings.Contains(ctx.TaskName, "TryBot failures") { + approvedTrybots = true + return nil + } + return defaultApprove(ctx, i) + } + + // Run the release. + wd := workflow.New() + v, err := addSingleReleaseWorkflow(deps.buildTasks, deps.milestoneTasks, deps.versionTasks, wd, 18, task.KindRC) + if err != nil { + t.Fatal(err) + } + wd.Output("Published Go version", v) + + w, err := workflow.Start(wd, map[string]interface{}{ + "Targets to skip testing (or 'all') (optional)": []string(nil), + "Ref from the private repository to build from (optional)": "", + }) + if err != nil { + t.Fatal(err) + } + if _, err := w.Run(deps.ctx, &verboseListener{t, deps.outputListener}); err != nil { + t.Fatal(err) + } + if !approvedTrybots { + t.Errorf("advisory trybots didn't need approval") + } + +} + // makeScript pretends to be make.bash. It creates a fake go command that // knows how to fake the commands the release process runs. const makeScript = `#!/bin/bash @@ -356,6 +395,12 @@ touch $GO/tool/something_orother/compile const allScript = `#!/bin/bash -eu echo "I'm a test! :D" + +if [[ $GO_BUILDER_NAME =~ "js-wasm" ]]; then + echo "Oh no, WASM is broken" + exit 1 +fi + exit 0 ` @@ -371,6 +416,8 @@ func serveSnapshot(w http.ResponseWriter, r *http.Request) { "src/make.bat": makeScript, "src/all.bash": allScript, "src/all.bat": allScript, + "src/race.bash": allScript, + "src/race.bat": allScript, }, w, r) } @@ -380,6 +427,8 @@ func serveSecureSnapshot(w http.ResponseWriter, r *http.Request) { "src/make.bat": makeScript, "src/all.bash": allScript, "src/all.bat": allScript, + "src/race.bash": allScript, + "src/race.bat": allScript, "security.txt": "This file makes us secure", }, w, r) } diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go index 61bfb63424..e47d4e5b29 100644 --- a/internal/relui/workflows.go +++ b/internal/relui/workflows.go @@ -16,6 +16,7 @@ import ( "math/rand" "net/http" "path" + "strings" "sync" "time" @@ -772,13 +773,19 @@ func (tasks *BuildReleaseTasks) addBuildTasks(wd *workflow.Definition, major int if target.BuildOnly { continue } - short := wd.Action("Run short tests", tasks.runTests, targetVal, wd.Constant(target.Builder), skipTests, bin) + short := wd.Action("Run short tests", tasks.runTests, targetVal, wd.Constant(dashboard.Builders[target.Builder]), skipTests, bin) testsPassed = append(testsPassed, short) if target.LongTestBuilder != "" { - long := wd.Action("Run long tests", tasks.runTests, targetVal, wd.Constant(target.LongTestBuilder), skipTests, bin) + long := wd.Action("Run long tests", tasks.runTests, targetVal, wd.Constant(dashboard.Builders[target.Builder]), skipTests, bin) testsPassed = append(testsPassed, long) } } + var advisoryResults []workflow.Value + for _, bc := range advisoryTryBots(major) { + result := wd.Task("Run advisory TryBot "+bc.Name, tasks.runAdvisoryTryBot, wd.Constant(bc), skipTests, source) + advisoryResults = append(advisoryResults, result) + } + tryBotsApproved := wd.Action("Approve any TryBot failures", tasks.checkAdvisoryTrybots, wd.Slice(advisoryResults...)) if skipSigning { builtAndTested := wd.Task("Wait for artifacts and tests", func(ctx *workflow.TaskContext, artifacts []artifact) ([]artifact, error) { return artifacts, nil @@ -789,10 +796,33 @@ func (tasks *BuildReleaseTasks) addBuildTasks(wd *workflow.Definition, major int signedArtifacts := wd.Task("Wait for signed artifacts", tasks.awaitSigned, version, wd.Constant(darwinTargets), stagedArtifacts) signedAndTested := wd.Task("Wait for signing and tests", func(ctx *workflow.TaskContext, artifacts []artifact) ([]artifact, error) { return artifacts, nil - }, append([]workflow.TaskInput{signedArtifacts}, testsPassed...)...) + }, append([]workflow.TaskInput{signedArtifacts, tryBotsApproved}, testsPassed...)...) return signedAndTested, nil } +func advisoryTryBots(major int) []*dashboard.BuildConfig { + usedBuilders := map[string]bool{} + for _, t := range releasetargets.TargetsForGo1Point(major) { + usedBuilders[t.Builder] = true + usedBuilders[t.LongTestBuilder] = true + } + + var extras []*dashboard.BuildConfig + for name, bc := range dashboard.Builders { + if usedBuilders[name] { + continue + } + if !bc.BuildsRepoPostSubmit("go", fmt.Sprintf("release-branch.go1.%d", major), "") { + continue + } + if !bc.IsVM() && !bc.IsContainer() { + continue + } + extras = append(extras, bc) + } + return extras +} + // BuildReleaseTasks serves as an adapter to the various build tasks in the task package. type BuildReleaseTasks struct { GerritHTTPClient *http.Client @@ -807,7 +837,7 @@ type BuildReleaseTasks struct { } func (b *BuildReleaseTasks) buildSource(ctx *workflow.TaskContext, revision, securityRevision, version string) (artifact, error) { - return b.runBuildStep(ctx, nil, "", artifact{}, "src.tar.gz", func(_ *task.BuildletStep, _ io.Reader, w io.Writer) error { + return b.runBuildStep(ctx, nil, nil, artifact{}, "src.tar.gz", func(_ *task.BuildletStep, _ io.Reader, w io.Writer) error { if securityRevision != "" { return task.WriteSourceArchive(ctx, b.GerritHTTPClient, b.PrivateGerritURL, securityRevision, version, w) } @@ -816,7 +846,7 @@ func (b *BuildReleaseTasks) buildSource(ctx *workflow.TaskContext, revision, sec } func (b *BuildReleaseTasks) checkSourceMatch(ctx *workflow.TaskContext, head, version string, source artifact) error { - _, err := b.runBuildStep(ctx, nil, "", source, "", func(_ *task.BuildletStep, r io.Reader, _ io.Writer) error { + _, err := b.runBuildStep(ctx, nil, nil, source, "", func(_ *task.BuildletStep, r io.Reader, _ io.Writer) error { branchArchive := &bytes.Buffer{} if err := task.WriteSourceArchive(ctx, b.GerritHTTPClient, b.GerritURL, head, version, branchArchive); err != nil { return err @@ -862,43 +892,76 @@ func tarballHashes(r io.Reader) (map[string]string, error) { } func (b *BuildReleaseTasks) buildBinary(ctx *workflow.TaskContext, target *releasetargets.Target, source artifact) (artifact, error) { - return b.runBuildStep(ctx, target, target.Builder, source, "tar.gz", func(bs *task.BuildletStep, r io.Reader, w io.Writer) error { + build := dashboard.Builders[target.Builder] + return b.runBuildStep(ctx, target, build, source, "tar.gz", func(bs *task.BuildletStep, r io.Reader, w io.Writer) error { return bs.BuildBinary(ctx, r, w) }) } func (b *BuildReleaseTasks) buildMSI(ctx *workflow.TaskContext, target *releasetargets.Target, binary artifact) (artifact, error) { - return b.runBuildStep(ctx, target, target.Builder, binary, "msi", func(bs *task.BuildletStep, r io.Reader, w io.Writer) error { + build := dashboard.Builders[target.Builder] + return b.runBuildStep(ctx, target, build, binary, "msi", func(bs *task.BuildletStep, r io.Reader, w io.Writer) error { return bs.BuildMSI(ctx, r, w) }) } func (b *BuildReleaseTasks) convertToZip(ctx *workflow.TaskContext, target *releasetargets.Target, binary artifact) (artifact, error) { - return b.runBuildStep(ctx, target, "", binary, "zip", func(_ *task.BuildletStep, r io.Reader, w io.Writer) error { + return b.runBuildStep(ctx, target, nil, binary, "zip", func(_ *task.BuildletStep, r io.Reader, w io.Writer) error { return task.ConvertTGZToZIP(r, w) }) } -func (b *BuildReleaseTasks) runTests(ctx *workflow.TaskContext, target *releasetargets.Target, buildlet string, skipTests []string, binary artifact) error { - skipped := false +func (b *BuildReleaseTasks) runTests(ctx *workflow.TaskContext, target *releasetargets.Target, build *dashboard.BuildConfig, skipTests []string, binary artifact) error { for _, skip := range skipTests { if skip == "all" || target.Name == skip { - skipped = true - break + ctx.Printf("Skipping test") + return nil } } - if skipped { - ctx.Printf("Skipping test") - return nil - } - _, err := b.runBuildStep(ctx, target, buildlet, binary, "", func(bs *task.BuildletStep, r io.Reader, _ io.Writer) error { + _, err := b.runBuildStep(ctx, target, build, binary, "", func(bs *task.BuildletStep, r io.Reader, _ io.Writer) error { return bs.TestTarget(ctx, r) }) return err } +type tryBotResult struct { + Name string + Passed bool +} + +func (b *BuildReleaseTasks) runAdvisoryTryBot(ctx *workflow.TaskContext, bc *dashboard.BuildConfig, skipTests []string, source artifact) (tryBotResult, error) { + for _, skip := range skipTests { + if skip == "all" || bc.Name == skip { + ctx.Printf("Skipping test") + return tryBotResult{bc.Name, true}, nil + } + } + + passed := false + _, err := b.runBuildStep(ctx, nil, bc, source, "", func(bs *task.BuildletStep, r io.Reader, w io.Writer) error { + var err error + passed, err = bs.RunTryBot(ctx, r) + return err + }) + return tryBotResult{bc.Name, passed}, err +} + +func (b *BuildReleaseTasks) checkAdvisoryTrybots(ctx *workflow.TaskContext, results []tryBotResult) error { + var fails []string + for _, r := range results { + if !r.Passed { + fails = append(fails, r.Name) + } + } + if len(fails) == 0 { + return nil + } + ctx.Printf("Some advisory TryBots failed. Check their logs and approve this task if it's okay:\n%v", strings.Join(fails, "\n")) + return b.ApproveAction(ctx, "") +} + // runBuildStep is a convenience function that manages resources a build step might need. -// If target and buildlet name are specified, a BuildletStep will be passed to f. +// If target and build config are specified, a BuildletStep will be passed to f. // If inputName is specified, it will be opened and passed as a Reader to f. // If outputSuffix is specified, a unique filename will be generated based off // it (and the target name, if any), the file will be opened and passed as a @@ -906,30 +969,23 @@ func (b *BuildReleaseTasks) runTests(ctx *workflow.TaskContext, target *releaset func (b *BuildReleaseTasks) runBuildStep( ctx *workflow.TaskContext, target *releasetargets.Target, - buildletName string, + build *dashboard.BuildConfig, input artifact, outputSuffix string, f func(*task.BuildletStep, io.Reader, io.Writer) error, ) (artifact, error) { var step *task.BuildletStep - if buildletName != "" { - if target == nil { - return artifact{}, fmt.Errorf("target must be specified to use a buildlet") - } - ctx.Printf("Creating buildlet %v.", buildletName) - client, err := b.CreateBuildlet(buildletName) + if build != nil { + ctx.Printf("Creating buildlet %v.", build.Name) + client, err := b.CreateBuildlet(build.Name) if err != nil { return artifact{}, err } defer client.Close() - buildConfig, ok := dashboard.Builders[buildletName] - if !ok { - return artifact{}, fmt.Errorf("unknown builder: %v", buildConfig) - } step = &task.BuildletStep{ Target: target, Buildlet: client, - BuildConfig: buildConfig, + BuildConfig: build, Watch: true, } ctx.Printf("Buildlet ready.") diff --git a/internal/task/buildrelease.go b/internal/task/buildrelease.go index 4a55e044ed..355ab3ee1c 100644 --- a/internal/task/buildrelease.go +++ b/internal/task/buildrelease.go @@ -361,6 +361,27 @@ func (b *BuildletStep) TestTarget(ctx *workflow.TaskContext, binaryArchive io.Re }) } +func (b *BuildletStep) RunTryBot(ctx *workflow.TaskContext, sourceArchive io.Reader) (bool, error) { + ctx.Printf("Pushing source to buildlet.") + if err := b.Buildlet.PutTar(ctx, sourceArchive, ""); err != nil { + return false, fmt.Errorf("failed to put generated source tarball: %v", err) + } + + if u := b.BuildConfig.GoBootstrapURL(buildenv.Production); u != "" { + ctx.Printf("Installing go1.4.") + if err := b.Buildlet.PutTarFromURL(ctx, u, go14); err != nil { + return false, err + } + } + + ctx.Printf("Testing") + err := b.exec(ctx, goDir+"/"+b.BuildConfig.AllScript(), b.BuildConfig.AllScriptArgs(), buildlet.ExecOpts{}) + if err != nil { + ctx.Printf("testing failed: %v", err) + } + return err == nil, nil +} + // exec runs cmd with args. Its working dir is opts.Dir, or the directory of cmd. // Its environment is the buildlet's environment, plus a GOPATH setting, plus opts.ExtraEnv. // If the command fails, its output is included in the returned error.