From 2d0582f691044a9bcc8199bc4bec0aa6979340a2 Mon Sep 17 00:00:00 2001 From: Space Meyer Date: Sat, 6 May 2023 12:18:30 +0200 Subject: dashboard: allow admins to retry individual bisections Changes to our rootfs, compilers or bisection logic regularly cause regressions in our bisection accuracy. Retrying them currently entails fiddling with the GCP datastore directly or mass deleting all failed bisections. This change will allow us to retry specific bisections with a single click. --- dashboard/app/admin.go | 57 +++++------- dashboard/app/bisect_test.go | 211 ++++++++++++++++++++----------------------- dashboard/app/entities.go | 3 +- dashboard/app/jobs.go | 72 +++++++++++++-- dashboard/app/main.go | 25 ++++- dashboard/app/templates.html | 4 + dashboard/app/util_test.go | 18 ++++ dashboard/dashapi/dashapi.go | 2 + pkg/html/pages/style.css | 2 +- 9 files changed, 231 insertions(+), 163 deletions(-) diff --git a/dashboard/app/admin.go b/dashboard/app/admin.go index e487f9b9e..0b6eef0c6 100644 --- a/dashboard/app/admin.go +++ b/dashboard/app/admin.go @@ -15,6 +15,26 @@ import ( aemail "google.golang.org/appengine/v2/mail" ) +func handleInvalidateBisection(c context.Context, w http.ResponseWriter, r *http.Request) error { + encodedKey := r.FormValue("key") + if encodedKey == "" { + return fmt.Errorf("mandatory parameter key is missing") + } + jobKey, err := db.DecodeKey(encodedKey) + if err != nil { + return fmt.Errorf("failed to decode job key %v: %w", encodedKey, err) + } + + err = invalidateBisection(c, jobKey) + if err != nil { + return fmt.Errorf("failed to invalidate job %v: %w", jobKey, err) + } + + // Sending back to bug page after successful invalidation. + http.Redirect(w, r, r.Header.Get("Referer"), http.StatusFound) + return nil +} + // dropNamespace drops all entities related to a single namespace. // Use with care. There is no undo. // This functionality is intentionally not connected to any handler. @@ -162,40 +182,9 @@ func restartFailedBisections(c context.Context, w http.ResponseWriter, r *http.R return nil } for idx, jobKey := range toReset { - tx := func(c context.Context) error { - // Reset the job. - job := new(Job) - if err := db.Get(c, jobKey, job); err != nil { - return fmt.Errorf("job %v: failed to get in tx: %v", idx, err) - } - job.LastStarted = time.Time{} - job.Finished = time.Time{} - job.Log = 0 - job.Error = 0 - job.CrashLog = 0 - job.Flags = 0 - if _, err := db.Put(c, jobKey, job); err != nil { - return fmt.Errorf("job %v: failed to put: %v", idx, err) - } - // Update the bug. - bug := new(Bug) - bugKey := jobKey.Parent() - if err := db.Get(c, bugKey, bug); err != nil { - return fmt.Errorf("job %v: failed to get bug: %v", idx, err) - } - if job.Type == JobBisectCause { - bug.BisectCause = BisectNot - } else if job.Type == JobBisectFix { - bug.BisectFix = BisectNot - } - if _, err := db.Put(c, bugKey, bug); err != nil { - return fmt.Errorf("job %v: failed to put the bug: %v", idx, err) - } - return nil - } - if err := db.RunInTransaction(c, tx, &db.TransactionOptions{XG: true, Attempts: 10}); err != nil { - fmt.Fprintf(w, "update failed: %s", err) - return nil + err = invalidateBisection(c, jobKey) + if err != nil { + fmt.Fprintf(w, "job %v update failed: %s", idx, err) } } diff --git a/dashboard/app/bisect_test.go b/dashboard/app/bisect_test.go index 7fcdf1e9d..11102a318 100644 --- a/dashboard/app/bisect_test.go +++ b/dashboard/app/bisect_test.go @@ -6,6 +6,7 @@ package main import ( "bytes" "fmt" + "net/http" "strings" "testing" "time" @@ -1040,96 +1041,14 @@ func TestBugBisectionResults(t *testing.T) { c := NewCtx(t) defer c.Close() - // Upload a crash report. - build := testBuild(1) - c.client2.UploadBuild(build) - crash := testCrashWithRepro(build, 1) - c.client2.ReportCrash(crash) - c.client2.pollEmailBug() - - // Receive the JobBisectCause and send cause information. - resp := c.client2.pollJobs(build.Manager) - c.client2.expectNE(resp.ID, "") - c.client2.expectEQ(resp.Type, dashapi.JobBisectCause) - jobID := resp.ID - done := &dashapi.JobDoneReq{ - ID: jobID, - Build: *build, - Log: []byte("bisectfix log 4"), - CrashTitle: "bisectfix crash title 4", - CrashLog: []byte("bisectfix crash log 4"), - CrashReport: []byte("bisectfix crash report 4"), - Commits: []dashapi.Commit{ - { - Hash: "36e65cb4a0448942ec316b24d60446bbd5cc7827", - Title: "kernel: add a bug", - Author: "author@kernel.org", - AuthorName: "Author Kernelov", - CC: []string{ - "reviewer1@kernel.org", "\"Reviewer2\" ", - // These must be filtered out: - "syzbot@testapp.appspotmail.com", - "syzbot+1234@testapp.appspotmail.com", - "\"syzbot\" ", - }, - Date: time.Date(2000, 2, 9, 4, 5, 6, 7, time.UTC), - }, - }, - } - c.expectOK(c.client2.JobDone(done)) - - // Advance time by 30 days and read out any notification emails. - { - c.advanceTime(30 * 24 * time.Hour) - msg := c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "syzbot has bisected this issue to:")) - msg = c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "Sending this report to the next reporting stage.")) - msg = c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "syzbot found the following issue")) - } - - // Receive a JobBisectfix and send fix information. - resp = c.client2.pollJobs(build.Manager) - c.client2.expectNE(resp.ID, "") - c.client2.expectEQ(resp.Type, dashapi.JobBisectFix) - jobID = resp.ID - done = &dashapi.JobDoneReq{ - ID: jobID, - Build: *build, - Log: []byte("bisectfix log 4"), - CrashTitle: "bisectfix crash title 4", - CrashLog: []byte("bisectfix crash log 4"), - CrashReport: []byte("bisectfix crash report 4"), - Commits: []dashapi.Commit{ - { - Hash: "46e65cb4a0448942ec316b24d60446bbd5cc7827", - Title: "kernel: add a fix", - Author: "author@kernel.org", - AuthorName: "Author Kernelov", - CC: []string{ - "reviewer1@kernel.org", "\"Reviewer2\" ", - // These must be filtered out: - "syzbot@testapp.appspotmail.com", - "syzbot+1234@testapp.appspotmail.com", - "\"syzbot\" ", - }, - Date: time.Date(2000, 2, 9, 4, 5, 6, 7, time.UTC), - }, - }, - } - c.expectOK(c.client2.JobDone(done)) - msg := c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "syzbot suspects this issue was fixed by commit:")) + build, _ := addBuildAndCrash(c) + _, bugKey := c.loadSingleBug() - // Fetch bug details. - var bugs []*Bug - keys, err := db.NewQuery("Bug").GetAll(c.ctx, &bugs) - c.expectEQ(err, nil) - c.expectEQ(len(bugs), 1) + addBisectCauseJob(c, build) + addBisectFixJob(c, build) // Ensure expected results show up on web UI - url := fmt.Sprintf("/bug?id=%v", keys[0].StringID()) + url := fmt.Sprintf("/bug?id=%v", bugKey.StringID()) content, err := c.GET(url) c.expectEQ(err, nil) c.expectTrue(bytes.Contains(content, []byte("Cause bisection: introduced by"))) @@ -1144,13 +1063,91 @@ func TestBugBisectionStatus(t *testing.T) { defer c.Close() // Upload a crash report. + build, _ := addBuildAndCrash(c) + + addBisectCauseJob(c, build) + + // Fetch bug, namespace details. + var bugs []*Bug + _, err := db.NewQuery("Bug").GetAll(c.ctx, &bugs) + c.expectEQ(err, nil) + c.expectEQ(len(bugs), 1) + url := fmt.Sprintf("/%v", bugs[0].Namespace) + content, err := c.GET(url) + c.expectEQ(err, nil) + c.expectTrue(bytes.Contains(content, []byte("done"))) + + addBisectFixJob(c, build) + + content, err = c.GET(url) + c.expectEQ(err, nil) + c.expectTrue(bytes.Contains(content, []byte("done"))) +} + +// Test that invalidated bisections are not shown in the UI and marked as invalid. +func TestBugBisectionInvalidation(t *testing.T) { + c := NewCtx(t) + defer c.Close() + + build, _ := addBuildAndCrash(c) + // Receive the JobBisectCause and send cause information. + addBisectCauseJob(c, build) + + // Ensure expected results show up on web UI + bug, bugKey := c.loadSingleBug() + job, jobKey := c.loadSingleJob() + bugURL := fmt.Sprintf("/bug?id=%v", bugKey.StringID()) + content, err := c.GET(bugURL) + c.expectEQ(err, nil) + c.expectEQ(bug.BisectCause, BisectYes) + c.expectTrue(bytes.Contains(content, []byte("Cause bisection: introduced by"))) + c.expectTrue(bytes.Contains(content, []byte("kernel: add a bug"))) + c.expectEQ(job.InvalidatedBy, "") + + // Mark bisection as invalid. + _, err = c.AuthGET(AccessAdmin, "/admin?action=invalidate_bisection&key="+jobKey.Encode()) + httpErr, ok := err.(HTTPError) + c.expectTrue(ok) + c.expectEQ(httpErr.Code, http.StatusFound) + + // The invalidated bisection should have vanished from the web UI + bug, _ = c.loadSingleBug() + job, _ = c.loadSingleJob() + content, err = c.GET(bugURL) + c.expectEQ(err, nil) + c.expectEQ(bug.BisectCause, BisectNot) + c.expectTrue(!bytes.Contains(content, []byte("Cause bisection: introduced by"))) + c.expectTrue(!bytes.Contains(content, []byte("kernel: add a bug"))) + c.expectEQ(job.InvalidatedBy, "user@syzkaller.com") + + // Confirm we wait 7 days before retrying cause bisections. + resp := c.client2.pollJobs(build.Manager) + c.client2.expectNE(resp.ID, "") + c.advanceTime(24 * 7 * time.Hour) + resp = c.client2.pollJobs(build.Manager) + c.client2.expectNE(resp.ID, "") + c.client2.expectEQ(resp.Type, dashapi.JobBisectCause) +} + +// Upload a build, a crash report and poll bug emails. +func addBuildAndCrash(c *Ctx) (*dashapi.Build, *dashapi.Crash) { build := testBuild(1) c.client2.UploadBuild(build) crash := testCrashWithRepro(build, 1) c.client2.ReportCrash(crash) c.client2.pollEmailBug() - // Receive the JobBisectCause and send cause information. + c.advanceTime(30 * 24 * time.Hour) + msg := c.client2.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, "Sending this report to the next reporting stage.")) + msg = c.client2.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, "syzbot found the following issue")) + + return build, crash +} + +// Poll a JobBisectCause and send cause information. +func addBisectCauseJob(c *Ctx, build *dashapi.Build) (*dashapi.JobPollResp, *dashapi.JobDoneReq, string) { resp := c.client2.pollJobs(build.Manager) c.client2.expectNE(resp.ID, "") c.client2.expectEQ(resp.Type, dashapi.JobBisectCause) @@ -1181,33 +1178,20 @@ func TestBugBisectionStatus(t *testing.T) { } c.expectOK(c.client2.JobDone(done)) - // Fetch bug, namespace details. - var bugs []*Bug - _, err := db.NewQuery("Bug").GetAll(c.ctx, &bugs) - c.expectEQ(err, nil) - c.expectEQ(len(bugs), 1) - url := fmt.Sprintf("/%v", bugs[0].Namespace) - content, err := c.GET(url) - c.expectEQ(err, nil) - c.expectTrue(bytes.Contains(content, []byte("done"))) + c.advanceTime(24 * time.Hour) + msg := c.client2.pollEmailBug() + c.expectTrue(strings.Contains(msg.Body, "syzbot has bisected this issue to:")) - // Advance time by 30 days and read out any notification emails. - { - c.advanceTime(30 * 24 * time.Hour) - msg := c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "syzbot has bisected this issue to:")) - msg = c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "Sending this report to the next reporting stage.")) - msg = c.client2.pollEmailBug() - c.expectTrue(strings.Contains(msg.Body, "syzbot found the following issue")) - } + return resp, done, jobID +} - // Receive a JobBisectfix and send fix information. - resp = c.client2.pollJobs(build.Manager) +// Poll a JobBisectfix and send fix information. +func addBisectFixJob(c *Ctx, build *dashapi.Build) (*dashapi.JobPollResp, *dashapi.JobDoneReq, string) { + resp := c.client2.pollJobs(build.Manager) c.client2.expectNE(resp.ID, "") c.client2.expectEQ(resp.Type, dashapi.JobBisectFix) - jobID = resp.ID - done = &dashapi.JobDoneReq{ + jobID := resp.ID + done := &dashapi.JobDoneReq{ ID: jobID, Build: *build, Log: []byte("bisectfix log 4"), @@ -1232,10 +1216,7 @@ func TestBugBisectionStatus(t *testing.T) { }, } c.expectOK(c.client2.JobDone(done)) - content, err = c.GET(url) - c.expectEQ(err, nil) - c.expectTrue(bytes.Contains(content, []byte("done"))) - msg := c.client2.pollEmailBug() c.expectTrue(strings.Contains(msg.Body, "syzbot suspects this issue was fixed by commit:")) + return resp, done, jobID } diff --git a/dashboard/app/entities.go b/dashboard/app/entities.go index 221bb0b31..a4f9dcbe9 100644 --- a/dashboard/app/entities.go +++ b/dashboard/app/entities.go @@ -569,7 +569,8 @@ type Job struct { Error int64 // reference to Error text entity, if set job failed Flags dashapi.JobDoneFlags - Reported bool // have we reported result back to user? + Reported bool // have we reported result back to user? + InvalidatedBy string // user who marked this bug as invalid, empty by default } func (job *Job) IsFinished() bool { diff --git a/dashboard/app/jobs.go b/dashboard/app/jobs.go index f69ea93a2..b7e8a8b0d 100644 --- a/dashboard/app/jobs.go +++ b/dashboard/app/jobs.go @@ -18,6 +18,7 @@ import ( "golang.org/x/net/context" db "google.golang.org/appengine/v2/datastore" "google.golang.org/appengine/v2/log" + "google.golang.org/appengine/v2/user" ) type testReqArgs struct { @@ -225,6 +226,48 @@ func checkTestJob(c context.Context, bug *Bug, bugReporting *BugReporting, crash return "" } +// Mark bisection job as invalid and reset bisection state of the related bug. +func invalidateBisection(c context.Context, jobKey *db.Key) error { + u := user.Current(c) + tx := func(c context.Context) error { + job := new(Job) + if err := db.Get(c, jobKey, job); err != nil { + return fmt.Errorf("failed to get job: %v", err) + } + + if job.Type != JobBisectCause && job.Type != JobBisectFix { + return fmt.Errorf("can only invalidate bisection jobs") + } + + // Update the job. + job.InvalidatedBy = u.Email + if _, err := db.Put(c, jobKey, job); err != nil { + return fmt.Errorf("failed to put job: %v", err) + } + + // Update the bug. + bug := new(Bug) + bugKey := jobKey.Parent() + if err := db.Get(c, bugKey, bug); err != nil { + return fmt.Errorf("failed to get bug: %v", err) + } + if job.Type == JobBisectCause { + bug.BisectCause = BisectNot + } else if job.Type == JobBisectFix { + bug.BisectFix = BisectNot + } + if _, err := db.Put(c, bugKey, bug); err != nil { + return fmt.Errorf("failed to put bug: %v", err) + } + return nil + } + if err := db.RunInTransaction(c, tx, &db.TransactionOptions{XG: true, Attempts: 10}); err != nil { + return fmt.Errorf("update failed: %v", err) + } + + return nil +} + type BadTestRequestError struct { message string } @@ -584,7 +627,7 @@ func findBugsForBisection(c context.Context, managers map[string]bool, return nil, nil, fmt.Errorf("failed to query bugs: %v", err) } for bi, bug := range bugs { - if !shouldBisectBug(bug, managers) { + if !shouldBisectBug(c, bug, managers, jobType) { continue } crash, crashKey, err := bisectCrashForBug(c, bug, keys[bi], managers, jobType) @@ -594,26 +637,33 @@ func findBugsForBisection(c context.Context, managers map[string]bool, if crash == nil { continue } - const fixJobRepeat = 24 * 30 * time.Hour - if jobType == JobBisectFix && timeSince(c, bug.LastTime) < fixJobRepeat { - continue - } - const causeJobRepeat = 24 * 7 * time.Hour - if jobType == JobBisectCause && timeSince(c, bug.LastCauseBisect) < causeJobRepeat { - continue - } return createBisectJobForBug(c, bug, crash, keys[bi], crashKey, jobType) } return nil, nil, nil } -func shouldBisectBug(bug *Bug, managers map[string]bool) bool { +func shouldBisectBug(c context.Context, bug *Bug, managers map[string]bool, jobType JobType) bool { + // We already have a fixing commit, no need to bisect. if len(bug.Commits) != 0 { return false } + if config.Namespaces[bug.Namespace].Decommissioned { return false } + + // There likely is no fix yet, as the bug recently reproduced. + const fixJobRepeat = 24 * 30 * time.Hour + if jobType == JobBisectFix && timeSince(c, bug.LastTime) < fixJobRepeat { + return false + } + // Likely to find the same (invalid) result without admin intervention, don't try too often. + const causeJobRepeat = 24 * 7 * time.Hour + if jobType == JobBisectCause && timeSince(c, bug.LastCauseBisect) < causeJobRepeat { + return false + } + + // Ensure one of the managers the bug reproduced on is taking bisection jobs. for _, mgr := range bug.HappenedOn { if managers[mgr] { return true @@ -1427,6 +1477,7 @@ func makeJobInfo(c context.Context, job *Job, jobKey *db.Key, bug *Bug, build *B kernelRepo, kernelCommit = build.KernelRepo, build.KernelCommit } info := &dashapi.JobInfo{ + JobKey: jobKey.Encode(), Type: dashapi.JobType(job.Type), Flags: job.Flags, Created: job.Created, @@ -1450,6 +1501,7 @@ func makeJobInfo(c context.Context, job *Job, jobKey *db.Key, bug *Bug, build *B LogLink: externalLink(c, textLog, job.Log), ErrorLink: externalLink(c, textError, job.Error), Reported: job.Reported, + InvalidatedBy: job.InvalidatedBy, TreeOrigin: job.TreeOrigin, OnMergeBase: job.MergeBaseRepo != "", } diff --git a/dashboard/app/main.go b/dashboard/app/main.go index 7daa4c1da..4de600fc2 100644 --- a/dashboard/app/main.go +++ b/dashboard/app/main.go @@ -9,6 +9,7 @@ import ( "fmt" "html/template" "net/http" + "net/url" "os" "regexp" "sort" @@ -32,6 +33,7 @@ import ( db "google.golang.org/appengine/v2/datastore" "google.golang.org/appengine/v2/log" "google.golang.org/appengine/v2/memcache" + "google.golang.org/appengine/v2/user" proto "google.golang.org/genproto/googleapis/appengine/logging/v1" ltype "google.golang.org/genproto/googleapis/logging/type" ) @@ -355,7 +357,8 @@ type uiCrashTable struct { type uiJob struct { *dashapi.JobInfo - Crash *uiCrash + Crash *uiCrash + InvalidateJobLink string } type userBugFilter struct { @@ -609,6 +612,8 @@ func handleAdmin(c context.Context, w http.ResponseWriter, r *http.Request) erro if err := memcache.Flush(c); err != nil { return fmt.Errorf("failed to flush memcache: %v", err) } + case "invalidate_bisection": + return handleInvalidateBisection(c, w, r) default: return fmt.Errorf("%w: unknown action %q", ErrClientBadRequest, action) } @@ -1979,7 +1984,8 @@ func loadTestPatchJobs(c context.Context, bug *Bug) ([]*uiJob, error) { func makeUIJob(c context.Context, job *Job, jobKey *db.Key, bug *Bug, crash *Crash, build *Build) *uiJob { ui := &uiJob{ - JobInfo: makeJobInfo(c, job, jobKey, bug, build, crash), + JobInfo: makeJobInfo(c, job, jobKey, bug, build, crash), + InvalidateJobLink: invalidateJobLink(c, job, jobKey), } if crash != nil { ui.Crash = makeUICrash(c, crash, build) @@ -1987,6 +1993,21 @@ func makeUIJob(c context.Context, job *Job, jobKey *db.Key, bug *Bug, crash *Cra return ui } +func invalidateJobLink(c context.Context, job *Job, jobKey *db.Key) string { + if !user.IsAdmin(c) { + return "" + } + + if job.Type != JobBisectCause && job.Type != JobBisectFix { + return "" + } + + params := url.Values{} + params.Add("action", "invalidate_bisection") + params.Add("key", jobKey.Encode()) + return "/admin?" + params.Encode() +} + func formatLogLine(line string) string { const maxLineLen = 1000 diff --git a/dashboard/app/templates.html b/dashboard/app/templates.html index 2c84878d7..9529654e2 100644 --- a/dashboard/app/templates.html +++ b/dashboard/app/templates.html @@ -362,6 +362,7 @@ Use of this source code is governed by Apache 2 LICENSE that can be found in the {{end}} {{if not .Reported}}[report pending]
{{end}} + {{optlink .InvalidateJobLink "❌ retry this bisection"}} {{end}} {{end}} @@ -474,6 +475,9 @@ Use of this source code is governed by Apache 2 LICENSE that can be found in the {{if $job.CrashLogLink}} {{optlink $job.CrashLogLink "log"}} {{end}} + {{ if $job.InvalidatedBy }} +
marked invalid by {{$job.InvalidatedBy}} + {{end}} {{end}} diff --git a/dashboard/app/util_test.go b/dashboard/app/util_test.go index 1158be65d..790984fc6 100644 --- a/dashboard/app/util_test.go +++ b/dashboard/app/util_test.go @@ -357,6 +357,24 @@ func (c *Ctx) loadManager(ns, name string) (*Manager, *Build) { return mgr, build } +func (c *Ctx) loadSingleBug() (*Bug, *db.Key) { + var bugs []*Bug + keys, err := db.NewQuery("Bug").GetAll(c.ctx, &bugs) + c.expectEQ(err, nil) + c.expectEQ(len(bugs), 1) + + return bugs[0], keys[0] +} + +func (c *Ctx) loadSingleJob() (*Job, *db.Key) { + var jobs []*Job + keys, err := db.NewQuery("Job").GetAll(c.ctx, &jobs) + c.expectEQ(err, nil) + c.expectEQ(len(jobs), 1) + + return jobs[0], keys[0] +} + func (c *Ctx) checkURLContents(url string, want []byte) { c.t.Helper() got, err := c.AuthGET(AccessAdmin, url) diff --git a/dashboard/dashapi/dashapi.go b/dashboard/dashapi/dashapi.go index 85ac82b44..d151b1618 100644 --- a/dashboard/dashapi/dashapi.go +++ b/dashboard/dashapi/dashapi.go @@ -866,6 +866,7 @@ const ( ) type JobInfo struct { + JobKey string Type JobType Flags JobDoneFlags Created time.Time @@ -895,6 +896,7 @@ type JobInfo struct { Commit *Commit // for conclusive bisection Commits []*Commit // for inconclusive bisection Reported bool + InvalidatedBy string TreeOrigin bool OnMergeBase bool } diff --git a/pkg/html/pages/style.css b/pkg/html/pages/style.css index 4331f4396..5aed046e6 100644 --- a/pkg/html/pages/style.css +++ b/pkg/html/pages/style.css @@ -151,7 +151,7 @@ table td, table th { .list_table .result { width: 60pt; - max-width: 60pt; + max-width: 200pt; } .list_table .stat { -- cgit mrf-deployment