dashboard/app: properly handle build failures

Separate kernel and syzkaller build failures.
Fix logic to understand when a build is fixed:
look if kernel/syzkaller commit changes to understand
if it's a new good build or re-upload of an old build.

Fixes #1014

Makefile

@@ -55,12 +55,14 @@ ifeq ("$(shell git diff --shortstat)", "")
 else
 	REV=$(GITREV)+
 endif
+GITREVDATE=$(shell git log -n 1 --format="%ad")
 
 # Don't generate symbol table and DWARF debug info.
 # Reduces build time and binary sizes considerably.
 # That's only needed if you use gdb or nm.
 # If you need that, build manually without these flags.
-GOFLAGS := "-ldflags=-s -w -X github.com/google/syzkaller/sys.GitRevision=$(REV)"
+GOFLAGS := "-ldflags=-s -w -X github.com/google/syzkaller/sys.GitRevision=$(REV) -X 'github.com/google/syzkaller/sys.gitRevisionDate=$(GITREVDATE)'"
+
 GOHOSTFLAGS := $(GOFLAGS)
 GOTARGETFLAGS := $(GOFLAGS)
 ifneq ("$(GOTAGS)", "")

dashboard/app/api.go

@@ -382,9 +382,26 @@ func apiUploadBuild(c context.Context, ns string, r *http.Request, payload []byt
 		return nil, err
 	}
 	if isNewBuild {
-		err := updateManager(c, ns, req.Manager, func(mgr *Manager, stats *ManagerStats) {
+		err := updateManager(c, ns, req.Manager, func(mgr *Manager, stats *ManagerStats) error {
+			prevKernel, prevSyzkaller := "", ""
+			if mgr.CurrentBuild != "" {
+				prevBuild, err := loadBuild(c, ns, mgr.CurrentBuild)
+				if err != nil {
+					return err
+				}
+				prevKernel = prevBuild.KernelCommit
+				prevSyzkaller = prevBuild.SyzkallerCommit
+			}
+			log.Infof(c, "new build on %v: kernel %v->%v syzkaller %v->%v",
+				req.Manager, prevKernel, req.KernelCommit, prevSyzkaller, req.SyzkallerCommit)
 			mgr.CurrentBuild = req.ID
-			mgr.FailedBuildBug = ""
+			if req.KernelCommit != prevKernel {
+				mgr.FailedBuildBug = ""
+			}
+			if req.SyzkallerCommit != prevSyzkaller {
+				mgr.FailedSyzBuildBug = ""
+			}
+			return nil
 		})
 		if err != nil {
 			return nil, err
@@ -439,30 +456,31 @@ func uploadBuild(c context.Context, now time.Time, ns string, req *dashapi.Build
 	if len(req.CompilerID) > MaxStringLen {
 		return nil, false, fmt.Errorf("Build.CompilerID is too long (%v)", len(req.CompilerID))
 	}
-	if err := checkStrLen(req.KernelCommit, "Build.KernelCommit", MaxStringLen); err != nil {
-		return nil, false, err
+	if len(req.KernelCommit) > MaxStringLen {
+		return nil, false, fmt.Errorf("Build.KernelCommit is too long (%v)", len(req.KernelCommit))
 	}
 	configID, err := putText(c, ns, textKernelConfig, req.KernelConfig, true)
 	if err != nil {
 		return nil, false, err
 	}
 	build := &Build{
-		Namespace:         ns,
-		Manager:           req.Manager,
-		ID:                req.ID,
-		Type:              typ,
-		Time:              now,
-		OS:                req.OS,
-		Arch:              req.Arch,
-		VMArch:            req.VMArch,
-		SyzkallerCommit:   req.SyzkallerCommit,
-		CompilerID:        req.CompilerID,
-		KernelRepo:        req.KernelRepo,
-		KernelBranch:      req.KernelBranch,
-		KernelCommit:      req.KernelCommit,
-		KernelCommitTitle: req.KernelCommitTitle,
-		KernelCommitDate:  req.KernelCommitDate,
-		KernelConfig:      configID,
+		Namespace:           ns,
+		Manager:             req.Manager,
+		ID:                  req.ID,
+		Type:                typ,
+		Time:                now,
+		OS:                  req.OS,
+		Arch:                req.Arch,
+		VMArch:              req.VMArch,
+		SyzkallerCommit:     req.SyzkallerCommit,
+		SyzkallerCommitDate: req.SyzkallerCommitDate,
+		CompilerID:          req.CompilerID,
+		KernelRepo:          req.KernelRepo,
+		KernelBranch:        req.KernelBranch,
+		KernelCommit:        req.KernelCommit,
+		KernelCommitTitle:   req.KernelCommitTitle,
+		KernelCommitDate:    req.KernelCommitDate,
+		KernelConfig:        configID,
 	}
 	if _, err := db.Put(c, buildKey(c, ns, req.ID), build); err != nil {
 		return nil, false, err
@@ -640,8 +658,14 @@ func apiReportBuildError(c context.Context, ns string, r *http.Request, payload
 	if err != nil {
 		return nil, err
 	}
-	if err := updateManager(c, ns, req.Build.Manager, func(mgr *Manager, stats *ManagerStats) {
-		mgr.FailedBuildBug = bug.keyHash()
+	if err := updateManager(c, ns, req.Build.Manager, func(mgr *Manager, stats *ManagerStats) error {
+		log.Infof(c, "failed build on %v: kernel=%v", req.Build.Manager, req.Build.KernelCommit)
+		if req.Build.KernelCommit != "" {
+			mgr.FailedBuildBug = bug.keyHash()
+		} else {
+			mgr.FailedSyzBuildBug = bug.keyHash()
+		}
+		return nil
 	}); err != nil {
 		return nil, err
 	}
@@ -929,7 +953,7 @@ func apiManagerStats(c context.Context, ns string, r *http.Request, payload []by
 		return nil, fmt.Errorf("failed to unmarshal request: %v", err)
 	}
 	now := timeNow(c)
-	err := updateManager(c, ns, req.Name, func(mgr *Manager, stats *ManagerStats) {
+	err := updateManager(c, ns, req.Name, func(mgr *Manager, stats *ManagerStats) error {
 		mgr.Link = req.Addr
 		mgr.LastAlive = now
 		mgr.CurrentUpTime = req.UpTime
@@ -942,6 +966,7 @@ func apiManagerStats(c context.Context, ns string, r *http.Request, payload []by
 		stats.TotalFuzzingTime += req.FuzzingTime
 		stats.TotalCrashes += int64(req.Crashes)
 		stats.TotalExecs += int64(req.Execs)
+		return nil
 	})
 	return nil, err
 }

dashboard/app/app_test.go

@@ -416,3 +416,108 @@ func TestPurgeOldCrashes(t *testing.T) {
 		}
 	}
 }
+
+func TestManagerFailedBuild(t *testing.T) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	// Upload and check first build.
+	build := testBuild(1)
+	c.client.UploadBuild(build)
+	checkManagerBuild(c, build, nil, nil)
+
+	// Upload and check second build.
+	build.ID = "id1"
+	build.KernelCommit = "kern1"
+	build.SyzkallerCommit = "syz1"
+	c.client.UploadBuild(build)
+	checkManagerBuild(c, build, nil, nil)
+
+	// Upload failed kernel build.
+	failedBuild := new(dashapi.Build)
+	*failedBuild = *build
+	failedBuild.ID = "id2"
+	failedBuild.KernelCommit = "kern2"
+	failedBuild.KernelCommitTitle = "failed build 1"
+	failedBuild.SyzkallerCommit = "syz2"
+	c.expectOK(c.client.ReportBuildError(&dashapi.BuildErrorReq{
+		Build: *failedBuild,
+		Crash: dashapi.Crash{
+			Title: "failed build 1",
+		},
+	}))
+	checkManagerBuild(c, build, failedBuild, nil)
+
+	// Now the old good build again, nothing should change.
+	c.client.UploadBuild(build)
+	checkManagerBuild(c, build, failedBuild, nil)
+
+	// New good kernel build, failed build must reset.
+	build.ID = "id3"
+	build.KernelCommit = "kern3"
+	c.client.UploadBuild(build)
+	checkManagerBuild(c, build, nil, nil)
+
+	// Now more complex scenario: OK -> failed kernel -> failed kernel+syzkaller -> failed syzkaller -> OK.
+	failedBuild.ID = "id4"
+	failedBuild.KernelCommit = "kern4"
+	failedBuild.KernelCommitTitle = "failed build 4"
+	failedBuild.SyzkallerCommit = "syz4"
+	c.expectOK(c.client.ReportBuildError(&dashapi.BuildErrorReq{
+		Build: *failedBuild,
+		Crash: dashapi.Crash{
+			Title: "failed build 4",
+		},
+	}))
+	checkManagerBuild(c, build, failedBuild, nil)
+
+	failedBuild2 := new(dashapi.Build)
+	*failedBuild2 = *failedBuild
+	failedBuild2.ID = "id5"
+	failedBuild2.KernelCommit = ""
+	failedBuild2.KernelCommitTitle = "failed build 5"
+	failedBuild2.SyzkallerCommit = "syz5"
+	c.expectOK(c.client.ReportBuildError(&dashapi.BuildErrorReq{
+		Build: *failedBuild2,
+		Crash: dashapi.Crash{
+			Title: "failed build 5",
+		},
+	}))
+	checkManagerBuild(c, build, failedBuild, failedBuild2)
+
+	build.ID = "id6"
+	build.KernelCommit = "kern6"
+	c.client.UploadBuild(build)
+	checkManagerBuild(c, build, nil, failedBuild2)
+
+	build.ID = "id7"
+	build.KernelCommit = "kern6"
+	build.SyzkallerCommit = "syz7"
+	c.client.UploadBuild(build)
+	checkManagerBuild(c, build, nil, nil)
+}
+
+func checkManagerBuild(c *Ctx, build, failedKernelBuild, failedSyzBuild *dashapi.Build) {
+	mgr, dbBuild := c.loadManager("test1", build.Manager)
+	c.expectEQ(mgr.CurrentBuild, build.ID)
+	compareBuilds(c, dbBuild, build)
+	checkBuildBug(c, mgr.FailedBuildBug, failedKernelBuild)
+	checkBuildBug(c, mgr.FailedSyzBuildBug, failedSyzBuild)
+}
+
+func checkBuildBug(c *Ctx, hash string, build *dashapi.Build) {
+	if build == nil {
+		c.expectEQ(hash, "")
+		return
+	}
+	c.expectNE(hash, "")
+	bug, _, dbBuild := c.loadBugByHash(hash)
+	c.expectEQ(bug.Title, build.KernelCommitTitle)
+	compareBuilds(c, dbBuild, build)
+}
+
+func compareBuilds(c *Ctx, dbBuild *Build, build *dashapi.Build) {
+	c.expectEQ(dbBuild.ID, build.ID)
+	c.expectEQ(dbBuild.KernelCommit, build.KernelCommit)
+	c.expectEQ(dbBuild.SyzkallerCommit, build.SyzkallerCommit)
+}

dashboard/app/entities.go

@@ -25,13 +25,14 @@ const (
 )
 
 type Manager struct {
-	Namespace      string
-	Name           string
-	Link           string
-	CurrentBuild   string
-	FailedBuildBug string
-	LastAlive      time.Time
-	CurrentUpTime  time.Duration
+	Namespace         string
+	Name              string
+	Link              string
+	CurrentBuild      string
+	FailedBuildBug    string
+	FailedSyzBuildBug string
+	LastAlive         time.Time
+	CurrentUpTime     time.Duration
 }
 
 // ManagerStats holds per-day manager runtime stats.
@@ -46,22 +47,23 @@ type ManagerStats struct {
 }
 
 type Build struct {
-	Namespace         string
-	Manager           string
-	ID                string // unique ID generated by syz-ci
-	Type              BuildType
-	Time              time.Time
-	OS                string
-	Arch              string
-	VMArch            string
-	SyzkallerCommit   string
-	CompilerID        string
-	KernelRepo        string
-	KernelBranch      string
-	KernelCommit      string
-	KernelCommitTitle string    `datastore:",noindex"`
-	KernelCommitDate  time.Time `datastore:",noindex"`
-	KernelConfig      int64     // reference to KernelConfig text entity
+	Namespace           string
+	Manager             string
+	ID                  string // unique ID generated by syz-ci
+	Type                BuildType
+	Time                time.Time
+	OS                  string
+	Arch                string
+	VMArch              string
+	SyzkallerCommit     string
+	SyzkallerCommitDate time.Time
+	CompilerID          string
+	KernelRepo          string
+	KernelBranch        string
+	KernelCommit        string
+	KernelCommitTitle   string    `datastore:",noindex"`
+	KernelCommitDate    time.Time `datastore:",noindex"`
+	KernelConfig        int64     // reference to KernelConfig text entity
 }
 
 type Bug struct {
@@ -243,21 +245,37 @@ const (
 	BisectYes
 )
 
+func mgrKey(c context.Context, ns, name string) *db.Key {
+	return db.NewKey(c, "Manager", fmt.Sprintf("%v-%v", ns, name), 0, nil)
+}
+
+func (mgr *Manager) key(c context.Context) *db.Key {
+	return mgrKey(c, mgr.Namespace, mgr.Name)
+}
+
+func loadManager(c context.Context, ns, name string) (*Manager, error) {
+	mgr := new(Manager)
+	if err := db.Get(c, mgrKey(c, ns, name), mgr); err != nil {
+		if err != db.ErrNoSuchEntity {
+			return nil, fmt.Errorf("failed to get manager %v/%v: %v", ns, name, err)
+		}
+		mgr = &Manager{
+			Namespace: ns,
+			Name:      name,
+		}
+	}
+	return mgr, nil
+}
+
 // updateManager does transactional compare-and-swap on the manager and its current stats.
-func updateManager(c context.Context, ns, name string, fn func(mgr *Manager, stats *ManagerStats)) error {
+func updateManager(c context.Context, ns, name string, fn func(mgr *Manager, stats *ManagerStats) error) error {
 	date := timeDate(timeNow(c))
 	tx := func(c context.Context) error {
-		mgr := new(Manager)
-		mgrKey := db.NewKey(c, "Manager", fmt.Sprintf("%v-%v", ns, name), 0, nil)
-		if err := db.Get(c, mgrKey, mgr); err != nil {
-			if err != db.ErrNoSuchEntity {
-				return fmt.Errorf("failed to get manager %v/%v: %v", ns, name, err)
-			}
-			mgr = &Manager{
-				Namespace: ns,
-				Name:      name,
-			}
+		mgr, err := loadManager(c, ns, name)
+		if err != nil {
+			return err
 		}
+		mgrKey := mgr.key(c)
 		stats := new(ManagerStats)
 		statsKey := db.NewKey(c, "ManagerStats", "", int64(date), mgrKey)
 		if err := db.Get(c, statsKey, stats); err != nil {
@@ -269,7 +287,9 @@ func updateManager(c context.Context, ns, name string, fn func(mgr *Manager, sta
 			}
 		}
 
-		fn(mgr, stats)
+		if err := fn(mgr, stats); err != nil {
+			return err
+		}
 
 		if _, err := db.Put(c, mgrKey, mgr); err != nil {
 			return fmt.Errorf("failed to put manager: %v", err)