From fff6e1803a4540558e92ee1a4f2c805040ef892a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 30 Jul 2017 00:42:10 +0000 Subject: [PATCH] maintner/maintnerd: use Gerrit to calculate TryBot/Result votes In https://golang.org/cl/51590 I tried to calculate the Run-TryBot and TryBot-Result vote sums using the Gerrit NoteDB mirror we have in maintner, but after two days of fighting it now, I've concluded it's tricky. While I work on the tricky bits in parallel, this CL partially reverts CL 51590, which had enough other cleanup and progress that it wasn't worth reverting in its entirety. Instead, move the Gerrit querying to maintnerd (instead of coordinator). This isn't bad because maintnerd already queries Gerrit. And coordinator will still be polling every second (introduced in 51590) instead of the old 60 seconds, but this CL now adds caching in the RPC handler. Change-Id: I80a519d9026a0981e3abf43d54a32b4684bda0e5 Reviewed-on: https://go-review.googlesource.com/51970 Reviewed-by: Kevin Burke --- maintner/gerrit.go | 24 +++++++--- maintner/maintnerd/api.go | 86 ++++++++++++++++++++++++++-------- maintner/maintnerd/api_test.go | 19 ++++++++ 3 files changed, 103 insertions(+), 26 deletions(-) diff --git a/maintner/gerrit.go b/maintner/gerrit.go index df85a192..57f19cf5 100644 --- a/maintner/gerrit.go +++ b/maintner/gerrit.go @@ -90,12 +90,13 @@ func (g *Gerrit) ForeachProjectUnsorted(fn func(*GerritProject) error) error { // GerritProject represents a single Gerrit project. type GerritProject struct { - gerrit *Gerrit - proj string // "go.googlesource.com/net" - cls map[int32]*GerritCL - remote map[gerritCLVersion]GitHash - need map[GitHash]bool - commit map[GitHash]*GitCommit + gerrit *Gerrit + proj string // "go.googlesource.com/net" + cls map[int32]*GerritCL + remote map[gerritCLVersion]GitHash + need map[GitHash]bool + commit map[GitHash]*GitCommit + numLabelChanges int // meta commits with "Label:" updates // ref are the non-change refs with keys like "HEAD", // "refs/heads/master", "refs/tags/v0.8.0", etc. @@ -118,6 +119,14 @@ func (gp *GerritProject) gitDir() string { return filepath.Join(gp.gerrit.c.getDataDir(), url.PathEscape(gp.proj)) } +// NumLabelChanges returns the number of times vote labels have +// changed in this project. This number is monotonically increasing. +// This is not guaranteed to be accurate; it might overcount. +// It will not undercount. +func (gp *GerritProject) NumLabelChanges() int { + return gp.numLabelChanges +} + // ServerSlashProject returns the server and project together, such as // "go.googlesource.com/build". func (gp *GerritProject) ServerSlashProject() string { return gp.proj } @@ -620,6 +629,9 @@ func (gp *GerritProject) processMutation(gm *maintpb.GerritMutation) { // GerritCL object. var backwardMessages []*GerritMessage gp.foreachCommitParent(cl.Meta.Hash, func(gc *GitCommit) error { + if strings.Contains(gc.Msg, "\nLabel: ") { + gp.numLabelChanges++ + } if oldMeta != nil && gc.Hash == oldMeta.Hash { return errStopIteration } diff --git a/maintner/maintnerd/api.go b/maintner/maintnerd/api.go index e8a8e666..dfc70fa1 100644 --- a/maintner/maintnerd/api.go +++ b/maintner/maintnerd/api.go @@ -9,7 +9,10 @@ import ( "errors" "sort" "strings" + "sync" + "time" + "golang.org/x/build/gerrit" "golang.org/x/build/maintner" "golang.org/x/build/maintner/maintnerd/apipb" ) @@ -107,35 +110,77 @@ func (s apiService) GetRef(ctx context.Context, req *apipb.GetRefRequest) (*apip return res, nil } +var tryCache struct { + sync.Mutex + forNumChanges int // number of label changes in project val is valid for + lastPoll time.Time // of gerrit + val *apipb.GoFindTryWorkResponse +} + +var tryBotGerrit = gerrit.NewClient("https://go-review.googlesource.com/", gerrit.NoAuth) + func (s apiService) GoFindTryWork(ctx context.Context, req *apipb.GoFindTryWorkRequest) (*apipb.GoFindTryWorkResponse, error) { + tryCache.Lock() + defer tryCache.Unlock() + s.c.RLock() defer s.c.RUnlock() - res := new(apipb.GoFindTryWorkResponse) - - goProj := s.c.Gerrit().Project("go.googlesource.com", "go") + // Count the number of vote label changes over time. If it's + // the same as the last query, return a cached result without + // hitting Gerrit. + var sumChanges int s.c.Gerrit().ForeachProjectUnsorted(func(gp *maintner.GerritProject) error { - gp.ForeachOpenCL(func(cl *maintner.GerritCL) error { - try, done := tryBotStatus(cl, req.ForStaging) - if !try || done { - return nil - } - work := tryWorkItem(cl) - if work.Project != "go" { - // Trybot on a subrepo. - // - // TODO: for Issue 17626, we need to append - // master and the past two releases, but for - // now we'll just do master. - work.GoBranch = append(work.GoBranch, "master") - work.GoCommit = append(work.GoCommit, goProj.Ref("refs/heads/master").String()) - } - res.Waiting = append(res.Waiting, work) + if gp.Server() != "go.googlesource.com" { return nil - }) + } + sumChanges += gp.NumLabelChanges() return nil }) + now := time.Now() + const maxPollInterval = 15 * time.Second + if tryCache.val != nil && + (tryCache.forNumChanges == sumChanges || + tryCache.lastPoll.After(now.Add(-maxPollInterval))) { + return tryCache.val, nil + } + + tryCache.lastPoll = now + + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + const query = "label:Run-TryBot=1 label:TryBot-Result=0 status:open" + cis, err := tryBotGerrit.QueryChanges(ctx, query, gerrit.QueryChangesOpt{ + Fields: []string{"CURRENT_REVISION", "CURRENT_COMMIT"}, + }) + if err != nil { + return nil, err + } + tryCache.forNumChanges = sumChanges + + res := new(apipb.GoFindTryWorkResponse) + goProj := s.c.Gerrit().Project("go.googlesource.com", "go") + + for _, ci := range cis { + cl := s.c.Gerrit().Project("go.googlesource.com", ci.Project).CL(int32(ci.ChangeNumber)) + work := tryWorkItem(cl) + if ci.CurrentRevision != "" { + // In case maintner is behind. + work.Commit = ci.CurrentRevision + } + if work.Project != "go" { + // Trybot on a subrepo. + // + // TODO: for Issue 17626, we need to append + // master and the past two releases, but for + // now we'll just do master. + work.GoBranch = append(work.GoBranch, "master") + work.GoCommit = append(work.GoCommit, goProj.Ref("refs/heads/master").String()) + } + res.Waiting = append(res.Waiting, work) + } + // Sort in some stable order. // // TODO: better would be sorting by time the trybot was @@ -147,5 +192,6 @@ func (s apiService) GoFindTryWork(ctx context.Context, req *apipb.GoFindTryWorkR sort.Slice(res.Waiting, func(i, j int) bool { return res.Waiting[i].Commit < res.Waiting[j].Commit }) + tryCache.val = res return res, nil } diff --git a/maintner/maintnerd/api_test.go b/maintner/maintnerd/api_test.go index 6bc2ea16..bd6b12dd 100644 --- a/maintner/maintnerd/api_test.go +++ b/maintner/maintnerd/api_test.go @@ -6,9 +6,11 @@ package main import ( "context" + "flag" "fmt" "sync" "testing" + "time" "golang.org/x/build/maintner" "golang.org/x/build/maintner/godata" @@ -49,17 +51,34 @@ func TestGetRef(t *testing.T) { } } +var hitGerrit = flag.Bool("hit_gerrit", false, "query production Gerrit in TestFindTryWork") + func TestFindTryWork(t *testing.T) { + if testing.Short() { + t.Skip("skipping in short mode") + } + if !*hitGerrit { + t.Skip("skipping without flag --hit_gerrit") + } c := getGoData(t) s := apiService{c} req := &apipb.GoFindTryWorkRequest{} + t0 := time.Now() res, err := s.GoFindTryWork(context.Background(), req) + d0 := time.Since(t0) if err != nil { t.Fatal(err) } + // Just for interactive debugging. This is using live data. // The stable tests are in TestTryWorkItem and TestTryBotStatus. t.Logf("Current: %v", res) + + t1 := time.Now() + res, err = s.GoFindTryWork(context.Background(), req) + d1 := time.Since(t1) + t.Logf("Latency: %v, then %v", d0, d1) + t.Logf("Cached: %v, %v", res, err) } func TestTryBotStatus(t *testing.T) {