diff options
| author | Aleksandr Nogikh <nogikh@google.com> | 2022-12-08 15:44:44 +0100 |
|---|---|---|
| committer | Aleksandr Nogikh <wp32pw@gmail.com> | 2022-12-09 13:55:42 +0100 |
| commit | a098c49a0e460e4ec2f2d3f601e499b2bd455d2f (patch) | |
| tree | 41883b9de39e95161aeebeecc659ec7e8b83ee62 | |
| parent | 23fb7f227f81e6be6bf41d4a78b11a78a061ff06 (diff) | |
dashboard: support reporting and unreporting of multiple crashes
Previously we could only report a single crash ID for each reporting
stage. That is enough for email reporting, but it unnecessarily
restricts the interface available to external reporting methods.
Instead of relying just on the Reported field of the Crash entity,
introduce the list of other entities that reference the Crash. It lets
us track the moment when the crash has been released by everyone else.
| -rw-r--r-- | dashboard/app/app_test.go | 38 | ||||
| -rw-r--r-- | dashboard/app/entities.go | 83 | ||||
| -rw-r--r-- | dashboard/app/jobs.go | 8 | ||||
| -rw-r--r-- | dashboard/app/reporting.go | 17 | ||||
| -rw-r--r-- | dashboard/dashapi/dashapi.go | 8 |
5 files changed, 145 insertions, 9 deletions
diff --git a/dashboard/app/app_test.go b/dashboard/app/app_test.go index f6e4b869a..a67506e5b 100644 --- a/dashboard/app/app_test.go +++ b/dashboard/app/app_test.go @@ -646,6 +646,44 @@ func TestPurgeOldCrashes(t *testing.T) { c.t.Errorf("preserved bad crash repro=%v: %v", crash.ReproC != 0, idx) } } + + firstCrashExists := func() bool { + _, crashKeys, err := queryCrashesForBug(c.ctx, bug.key(c.ctx), 10*totalReported) + c.expectOK(err) + for _, key := range crashKeys { + if key.IntID() == rep.CrashID { + return true + } + } + return false + } + + // A sanity check for the test itself. + if !firstCrashExists() { + t.Fatalf("The first reported crash should be present") + } + + // Unreport the first crash. + reply, _ := c.client.ReportingUpdate(&dashapi.BugUpdate{ + ID: rep.ID, + Status: dashapi.BugStatusUpdate, + ReproLevel: dashapi.ReproLevelC, + UnreportCrashIDs: []int64{rep.CrashID}, + }) + c.expectEQ(reply.OK, true) + + // Trigger more purge events. + for i := 0; i < maxCrashes; i++ { + c.advanceTime(2 * time.Hour) // This ensures that crashes are saved. + crash.ReproSyz = nil + crash.ReproC = nil + crash.ReproOpts = []byte(fmt.Sprintf("%v", i)) + c.client.ReportCrash(crash) + } + // Check that the unreported crash was purged. + if firstCrashExists() { + t.Fatalf("The unreported crash should have been purged.") + } } func TestManagerFailedBuild(t *testing.T) { diff --git a/dashboard/app/entities.go b/dashboard/app/entities.go index 5ff38e9d9..a27c3f75c 100644 --- a/dashboard/app/entities.go +++ b/dashboard/app/entities.go @@ -180,6 +180,7 @@ type Crash struct { BuildID string Time time.Time Reported time.Time // set if this crash was ever reported + References []CrashReference Maintainers []string `datastore:",noindex"` Log int64 // reference to CrashLog text entity Flags int64 // properties of the Crash @@ -198,6 +199,70 @@ type Crash struct { AssetsLastCheck time.Time // the last time we checked the assets for deprecation } +type CrashReferenceType string + +const ( + CrashReferenceReporting = "reporting" + CrashReferenceJob = "job" + // This one is needed for backward compatibility. + crashReferenceUnknown = "unknown" +) + +type CrashReference struct { + Type CrashReferenceType + // For CrashReferenceReporting, it refers to Reporting.Name + // For CrashReferenceJob, it refers to extJobID(jobKey) + Key string + Time time.Time +} + +func (crash *Crash) AddReference(newRef CrashReference) { + crash.Reported = newRef.Time + for i, ref := range crash.References { + if ref.Type != newRef.Type || ref.Key != newRef.Key { + continue + } + crash.References[i].Time = newRef.Time + return + } + crash.References = append(crash.References, newRef) +} + +func (crash *Crash) ClearReference(t CrashReferenceType, key string) { + newRefs := []CrashReference{} + crash.Reported = time.Time{} + for _, ref := range crash.References { + if ref.Type == t && ref.Key == key { + continue + } + if ref.Time.After(crash.Reported) { + crash.Reported = ref.Time + } + newRefs = append(newRefs, ref) + } + crash.References = newRefs +} + +func (crash *Crash) Load(ps []db.Property) error { + if err := db.LoadStruct(crash, ps); err != nil { + return err + } + // Earlier we only relied on Reported, which does not let us reliably unreport a crash. + // We need some means of ref counting, so let's create a dummy reference to keep the + // crash from being purged. + if !crash.Reported.IsZero() && len(crash.References) == 0 { + crash.References = append(crash.References, CrashReference{ + Type: crashReferenceUnknown, + Time: crash.Reported, + }) + } + return nil +} + +func (crash *Crash) Save() ([]db.Property, error) { + return db.SaveStruct(crash) +} + // ReportingState holds dynamic info associated with reporting. type ReportingState struct { Entries []ReportingStateEntry @@ -659,13 +724,27 @@ func (bug *Bug) dashapiStatus() (dashapi.BugStatus, error) { return status, nil } -func markCrashReported(c context.Context, crashID int64, bugKey *db.Key, now time.Time) error { +func addCrashReference(c context.Context, crashID int64, bugKey *db.Key, ref CrashReference) error { + crash := new(Crash) + crashKey := db.NewKey(c, "Crash", "", crashID, bugKey) + if err := db.Get(c, crashKey, crash); err != nil { + return fmt.Errorf("failed to get reported crash %v: %v", crashID, err) + } + crash.AddReference(ref) + if _, err := db.Put(c, crashKey, crash); err != nil { + return fmt.Errorf("failed to put reported crash %v: %v", crashID, err) + } + return nil +} + +func removeCrashReference(c context.Context, crashID int64, bugKey *db.Key, + t CrashReferenceType, key string) error { crash := new(Crash) crashKey := db.NewKey(c, "Crash", "", crashID, bugKey) if err := db.Get(c, crashKey, crash); err != nil { return fmt.Errorf("failed to get reported crash %v: %v", crashID, err) } - crash.Reported = now + crash.ClearReference(t, key) if _, err := db.Put(c, crashKey, crash); err != nil { return fmt.Errorf("failed to put reported crash %v: %v", crashID, err) } diff --git a/dashboard/app/jobs.go b/dashboard/app/jobs.go index fd1291e48..d2320e837 100644 --- a/dashboard/app/jobs.go +++ b/dashboard/app/jobs.go @@ -168,7 +168,8 @@ func addTestJob(c context.Context, args *testJobArgs, now time.Time) error { if _, err := db.Put(c, jobKey, job); err != nil { return fmt.Errorf("failed to put job: %v", err) } - return markCrashReported(c, job.CrashID, args.bugKey, now) + return addCrashReference(c, job.CrashID, args.bugKey, + CrashReference{CrashReferenceJob, extJobID(jobKey), now}) } err = db.RunInTransaction(c, tx, &db.TransactionOptions{XG: true, Attempts: 30}) if patchID != 0 && deletePatch || err != nil { @@ -480,10 +481,11 @@ func createBisectJobForBug(c context.Context, bug0 *Bug, crash *Crash, bugKey, c if _, err := db.Put(c, bugKey, bug); err != nil { return fmt.Errorf("failed to put bug: %v", err) } - return markCrashReported(c, job.CrashID, bugKey, now) + return addCrashReference(c, job.CrashID, bugKey, + CrashReference{CrashReferenceJob, extJobID(jobKey), now}) } if err := db.RunInTransaction(c, tx, &db.TransactionOptions{ - // We're accessing two different kinds in markCrashReported. + // We're accessing two different kinds in addCrashReference. XG: true, }); err != nil { return nil, nil, fmt.Errorf("create bisect job tx failed: %v", err) diff --git a/dashboard/app/reporting.go b/dashboard/app/reporting.go index e4af28fe4..c3d5c9683 100644 --- a/dashboard/app/reporting.go +++ b/dashboard/app/reporting.go @@ -931,12 +931,23 @@ func incomingCommandUpdate(c context.Context, now time.Time, cmd *dashapi.BugUpd bug.updateCommits(cmd.FixCommits, now) } } + toReport := append([]int64{}, cmd.ReportCrashIDs...) if cmd.CrashID != 0 { - // Rememeber that we've reported this crash. - if err := markCrashReported(c, cmd.CrashID, bugKey, now); err != nil { + bugReporting.CrashID = cmd.CrashID + toReport = append(toReport, cmd.CrashID) + } + newRef := CrashReference{CrashReferenceReporting, bugReporting.Name, now} + for _, crashID := range toReport { + err := addCrashReference(c, crashID, bugKey, newRef) + if err != nil { + return false, internalError, err + } + } + for _, crashID := range cmd.UnreportCrashIDs { + err := removeCrashReference(c, crashID, bugKey, CrashReferenceReporting, bugReporting.Name) + if err != nil { return false, internalError, err } - bugReporting.CrashID = cmd.CrashID } if bugReporting.ExtID == "" { bugReporting.ExtID = cmd.ExtID diff --git a/dashboard/dashapi/dashapi.go b/dashboard/dashapi/dashapi.go index afdbacb7a..0c2db1ac7 100644 --- a/dashboard/dashapi/dashapi.go +++ b/dashboard/dashapi/dashapi.go @@ -444,7 +444,13 @@ type BugUpdate struct { ResetFixCommits bool // Remove all commits (empty FixCommits means leave intact). FixCommits []string // Titles of commits that fix this bug. CC []string // Additional emails to add to CC list in future emails. - CrashID int64 + + CrashID int64 // This is a deprecated field, left here for backward compatibility. + + // The new interface that allows to report and unreport several crashes at the same time. + // This is not relevant for emails, but may be important for external reportings. + ReportCrashIDs []int64 + UnreportCrashIDs []int64 } type BugUpdateReply struct { |
