From c2e837da36a2f3d4407e291e8db889865a53ab92 Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Fri, 25 Oct 2019 15:56:00 +0200 Subject: [PATCH] dashboard/app: flexible rules for bug obsoleting Implement logic described in #1054: - close bugs that happened a lot and then stopped faster - close bugs in non-final reporting with different period - allow closing bugs that happened only on 1 manager with different period Fixes #1054 --- dashboard/app/app_test.go | 12 +++++ dashboard/app/config.go | 56 ++++++++++++++++++++ dashboard/app/notifications_test.go | 81 +++++++++++++++++++++++++++-- dashboard/app/reporting.go | 37 ++++++++++++- 4 files changed, 181 insertions(+), 5 deletions(-) diff --git a/dashboard/app/app_test.go b/dashboard/app/app_test.go index 8b4e45ba..89ef7ff1 100644 --- a/dashboard/app/app_test.go +++ b/dashboard/app/app_test.go @@ -32,6 +32,12 @@ var testConfig = &GlobalConfig{ EmailBlacklist: []string{ "\"Bar\" ", }, + Obsoleting: ObsoletingConfig{ + MinPeriod: 80 * 24 * time.Hour, + MaxPeriod: 100 * 24 * time.Hour, + NonFinalMinPeriod: 40 * 24 * time.Hour, + NonFinalMaxPeriod: 60 * 24 * time.Hour, + }, DefaultNamespace: "test1", Namespaces: map[string]*Config{ "test1": { @@ -54,6 +60,12 @@ var testConfig = &GlobalConfig{ CC: []string{"maintainers@repo10.org", "bugs@repo10.org"}, }, }, + Managers: map[string]ConfigManager{ + "special-obsoleting": { + ObsoletingMinPeriod: 10 * 24 * time.Hour, + ObsoletingMaxPeriod: 20 * 24 * time.Hour, + }, + }, Reporting: []Reporting{ { Name: "reporting1", diff --git a/dashboard/app/config.go b/dashboard/app/config.go index 34314d6e..46a2b959 100644 --- a/dashboard/app/config.go +++ b/dashboard/app/config.go @@ -33,6 +33,8 @@ type GlobalConfig struct { Clients map[string]string // List of emails blacklisted from issuing test requests. EmailBlacklist []string + // Bug obsoleting settings. See ObsoletingConfig for details. + Obsoleting ObsoletingConfig // Namespace that is shown by default (no namespace selected yet). DefaultNamespace string // Per-namespace config. @@ -78,6 +80,22 @@ type Config struct { Repos []KernelRepo } +// ObsoletingConfig describes how bugs without reproducer should be obsoleted. +// First, for each bug we conservatively estimate period since the last crash +// when we consider it stopped happenning. This estimation is based on the first/last time +// and number and rate of crashes. Then this period is capped by MinPeriod/MaxPeriod. +// Then if the period has elapsed since the last crash, we obsolete the bug. +// NonFinalMinPeriod/NonFinalMaxPeriod (if specified) are used to cap bugs in non-final reportings. +// Additionally ConfigManager.ObsoletingMin/MaxPeriod override the cap settings +// for bugs that happen only on that manager. +// If no periods are specified, no bugs are obsoleted. +type ObsoletingConfig struct { + MinPeriod time.Duration + MaxPeriod time.Duration + NonFinalMinPeriod time.Duration + NonFinalMaxPeriod time.Duration +} + // ConfigManager describes a single syz-manager instance. // Dashboard does not generally need to know about all of them, // but in some special cases it needs to know some additional information. @@ -90,6 +108,10 @@ type ConfigManager struct { // and RestrictedTestingReason contains a human readable reason for the restriction. RestrictedTestingRepo string RestrictedTestingReason string + // If a bug happens only on this manager, this overrides global obsoleting settings. + // See ObsoletingConfig for details. + ObsoletingMinPeriod time.Duration + ObsoletingMaxPeriod time.Duration } // One reporting stage. @@ -200,6 +222,7 @@ func checkConfig(cfg *GlobalConfig) { clientNames := make(map[string]bool) checkClients(clientNames, cfg.Clients) checkConfigAccessLevel(&cfg.AccessLevel, AccessPublic, "global") + checkObsoleting(cfg.Obsoleting) if cfg.Namespaces[cfg.DefaultNamespace] == nil { panic(fmt.Sprintf("default namespace %q is not found", cfg.DefaultNamespace)) } @@ -208,6 +231,30 @@ func checkConfig(cfg *GlobalConfig) { } } +func checkObsoleting(o ObsoletingConfig) { + if (o.MinPeriod == 0) != (o.MaxPeriod == 0) { + panic(fmt.Sprintf("obsoleting: both or none of Min/MaxPeriod must be specified")) + } + if o.MinPeriod > o.MaxPeriod { + panic(fmt.Sprintf("obsoleting: Min > MaxPeriod")) + } + if o.MinPeriod != 0 && o.MinPeriod < 24*time.Hour { + panic(fmt.Sprintf("obsoleting: too low MinPeriod")) + } + if (o.NonFinalMinPeriod == 0) != (o.NonFinalMaxPeriod == 0) { + panic(fmt.Sprintf("obsoleting: both or none of NonFinalMin/MaxPeriod must be specified")) + } + if o.NonFinalMinPeriod > o.NonFinalMaxPeriod { + panic(fmt.Sprintf("obsoleting: NonFinalMin > MaxPeriod")) + } + if o.NonFinalMinPeriod != 0 && o.NonFinalMinPeriod < 24*time.Hour { + panic(fmt.Sprintf("obsoleting: too low MinPeriod")) + } + if o.MinPeriod == 0 && o.NonFinalMinPeriod != 0 { + panic(fmt.Sprintf("obsoleting: NonFinalMinPeriod without MinPeriod")) + } +} + func checkNamespace(ns string, cfg *Config, namespaces, clientNames map[string]bool) { if !namespaceNameRe.MatchString(ns) { panic(fmt.Sprintf("bad namespace name: %q", ns)) @@ -324,6 +371,15 @@ func checkManager(ns, name string, mgr ConfigManager) { if mgr.RestrictedTestingRepo == "" && mgr.RestrictedTestingReason != "" { panic(fmt.Sprintf("unrestricted manager %v/%v has restriction reason", ns, name)) } + if (mgr.ObsoletingMinPeriod == 0) != (mgr.ObsoletingMaxPeriod == 0) { + panic(fmt.Sprintf("manager %v/%v obsoleting: both or none of Min/MaxPeriod must be specified", ns, name)) + } + if mgr.ObsoletingMinPeriod > mgr.ObsoletingMaxPeriod { + panic(fmt.Sprintf("manager %v/%v obsoleting: Min > MaxPeriod", ns, name)) + } + if mgr.ObsoletingMinPeriod != 0 && mgr.ObsoletingMinPeriod < 24*time.Hour { + panic(fmt.Sprintf("manager %v/%v obsoleting: too low MinPeriod", ns, name)) + } } func checkConfigAccessLevel(current *AccessLevel, parent AccessLevel, what string) { diff --git a/dashboard/app/notifications_test.go b/dashboard/app/notifications_test.go index bbd5d7cf..3437576f 100644 --- a/dashboard/app/notifications_test.go +++ b/dashboard/app/notifications_test.go @@ -102,6 +102,81 @@ func TestEmailNotifBadFix(t *testing.T) { } } +func TestBugObsoleting(t *testing.T) { + // To simplify test we specify all dates in days from a fixed point in time. + const day = 24 * time.Hour + days := func(n int) time.Time { + t := time.Date(2000, 0, 0, 0, 0, 0, 0, time.UTC) + t.Add(time.Duration(n+1) * day) + return t + } + tests := []struct { + bug *Bug + period time.Duration + }{ + // Final bug with just 1 crash: max final period. + { + bug: &Bug{ + FirstTime: days(0), + LastTime: days(0), + NumCrashes: 1, + Reporting: []BugReporting{{Reported: days(0)}}, + }, + period: 100 * day, + }, + // Non-final bug with just 1 crash: max non-final period. + { + bug: &Bug{ + FirstTime: days(0), + LastTime: days(0), + NumCrashes: 1, + Reporting: []BugReporting{{Reported: days(0)}, {}}, + }, + period: 60 * day, + }, + // Special manger: max period that that manager. + { + bug: &Bug{ + FirstTime: days(0), + LastTime: days(0), + NumCrashes: 1, + HappenedOn: []string{"special-obsoleting"}, + Reporting: []BugReporting{{Reported: days(0)}, {}}, + }, + period: 20 * day, + }, + // Special manger and a non-special: normal rules. + { + bug: &Bug{ + FirstTime: days(0), + LastTime: days(0), + NumCrashes: 1, + HappenedOn: []string{"special-obsoleting", "non-special-manager"}, + Reporting: []BugReporting{{Reported: days(0)}}, + }, + period: 100 * day, + }, + // Happened a lot: min period. + { + bug: &Bug{ + FirstTime: days(0), + LastTime: days(1), + NumCrashes: 1000, + Reporting: []BugReporting{{Reported: days(0)}}, + }, + period: 80 * day, + }, + } + for i, test := range tests { + test.bug.Namespace = "test1" + got := test.bug.obsoletePeriod() + if got != test.period { + t.Errorf("test #%v: got: %.2f, want %.2f", + i, float64(got/time.Hour)/24, float64(test.period/time.Hour)/24) + } + } +} + func TestEmailNotifObsoleted(t *testing.T) { c := NewCtx(t) defer c.Close() @@ -124,7 +199,7 @@ func TestEmailNotifObsoleted(t *testing.T) { c.expectNoEmail() // Not yet. - c.advanceTime(119 * 24 * time.Hour) + c.advanceTime(59 * 24 * time.Hour) c.expectNoEmail() // Now! @@ -145,7 +220,7 @@ func TestEmailNotifObsoleted(t *testing.T) { c.incomingEmail(report.Sender, "#syz upstream") report = c.pollEmailBug() - c.advanceTime(121 * 24 * time.Hour) + c.advanceTime(101 * 24 * time.Hour) notif = c.pollEmailBug() if !strings.Contains(notif.Body, "Auto-closing this bug as obsolete") { t.Fatalf("bad notification text: %q", notif.Body) @@ -190,7 +265,7 @@ func TestEmailNotifNotObsoleted(t *testing.T) { c.incomingEmail(report4.Sender, "#syz upstream") report4 = c.pollEmailBug() - c.advanceTime(119 * 24 * time.Hour) + c.advanceTime(59 * 24 * time.Hour) c.expectNoEmail() c.client2.ReportCrash(crash2) diff --git a/dashboard/app/reporting.go b/dashboard/app/reporting.go index 99adb19a..cb3f52cd 100644 --- a/dashboard/app/reporting.go +++ b/dashboard/app/reporting.go @@ -31,7 +31,7 @@ const ( maxInlineError = 16 << 10 notifyResendPeriod = 14 * 24 * time.Hour notifyAboutBadCommitPeriod = 90 * 24 * time.Hour - autoObsoletePeriod = 120 * 24 * time.Hour + never = 100 * 365 * 24 * time.Hour internalError = "internal error" // This is embedded as first line of syzkaller reproducer files. syzReproPrefix = "# See https://goo.gl/kgGztJ for information about syzkaller reproducers.\n" @@ -207,7 +207,7 @@ func handleReportNotif(c context.Context, typ string, bug *Bug) (*dashapi.BugNot if len(bug.Commits) == 0 && bug.ReproLevel == ReproLevelNone && timeSince(c, bug.LastActivity) > notifyResendPeriod && - timeSince(c, bug.LastTime) > autoObsoletePeriod { + timeSince(c, bug.LastTime) > bug.obsoletePeriod() { log.Infof(c, "%v: obsoleting: %v", bug.Namespace, bug.Title) return createNotification(c, dashapi.BugNotifObsoleted, false, "", bug, reporting, bugReporting) } @@ -222,6 +222,39 @@ func handleReportNotif(c context.Context, typ string, bug *Bug) (*dashapi.BugNot return nil, nil } +func (bug *Bug) obsoletePeriod() time.Duration { + period := never + if config.Obsoleting.MinPeriod == 0 { + return period + } + // Before we have at least 10 crashes, any estimation of frequency is too imprecise. + // In such case we conservatively assume it still happens. + if bug.NumCrashes >= 10 { + // This is linear extrapolation for when the next crash should happen. + period = bug.LastTime.Sub(bug.FirstTime) / time.Duration(bug.NumCrashes-1) + // Let's be conservative with obsoleting too early. + period *= 100 + } + min, max := config.Obsoleting.MinPeriod, config.Obsoleting.MaxPeriod + if config.Obsoleting.NonFinalMinPeriod != 0 && + bug.Reporting[len(bug.Reporting)-1].Reported.IsZero() { + min, max = config.Obsoleting.NonFinalMinPeriod, config.Obsoleting.NonFinalMaxPeriod + } + if len(bug.HappenedOn) == 1 { + mgr := config.Namespaces[bug.Namespace].Managers[bug.HappenedOn[0]] + if mgr.ObsoletingMinPeriod != 0 { + min, max = mgr.ObsoletingMinPeriod, mgr.ObsoletingMaxPeriod + } + } + if period < min { + period = min + } + if period > max { + period = max + } + return period +} + func createNotification(c context.Context, typ dashapi.BugNotif, public bool, text string, bug *Bug, reporting *Reporting, bugReporting *BugReporting) (*dashapi.BugNotification, error) { reportingConfig, err := json.Marshal(reporting.Config)