Milo: Console view prototype

appengine/cmd/milo/buildbot/console.go

+// Copyright 2016 The LUCI Authors. All rights reserved.
+// Use of this source code is governed under the Apache License, Version 2.0
+// that can be found in the LICENSE file.
+
+package buildbot
+
+import (
+        "fmt"
+        "strings"
+        "time"
+
+        "github.com/luci/gae/service/datastore"
+        "github.com/luci/luci-go/appengine/cmd/milo/resp"
+        "github.com/luci/luci-go/common/clock"
+        log "github.com/luci/luci-go/common/logging"
+        "github.com/luci/luci-go/common/sync/parallel"
+        "golang.org/x/net/context"
+)
+
+// getBuilds fetches all of the recent builds from the datastore.
+func getFullBuilds(c context.Context, masterName, builderName string, finished bool) ([]*buildbotBuild, error) {

[nodir, 2016/08/03 20:26:16]

why "Full"?

[hinoka, 2016/08/03 21:55:39]

Not sure, I think it was arbitrary.  I guess full vs. summary. 

+        // TODO(hinoka): Builder specific structs.
+        ds := datastore.Get(c)
+        q := datastore.NewQuery("buildbotBuild")
+        q = q.Eq("finished", finished)
+        q = q.Eq("master", masterName)
+        q = q.Eq("builder", builderName)
+        q = q.Limit(25) // TODO(hinoka): This should be adjustable
+        q = q.Order("-number")
+        q.Finalize()
+        buildbots := make([]*buildbotBuild, 0, 25)
+        err := ds.GetAll(q, &buildbots)
+        return buildbots, err
+}
+
+func GetConsoleBuilds(
+        c context.Context, builders []resp.BuilderRef, commits []string) (
+        [][]*resp.ConsoleBuild, error) {
+
+        // First, fetch all the master entries referenced.
+        // TODO(hinoka): Parallel fetch.
+        masters := map[string]*buildbotMaster{}
+        for _, builder := range builders {

[nodir, 2016/08/03 20:26:17]

this loop may take a lot of time because of master unzipping and json parsing.
consider timing it

[hinoka, 2016/08/03 21:55:39]

Actually this entire loop isn't used... I forgot why this was here.  I think it
was to resolve wildcards like chromium.linux/* builders, a feature that I later
stripped out.

+                f := strings.SplitN(builder.Name, "/", 2)

[nodir, 2016/08/03 20:26:17]

validate that len(f) == 2, return error if not so

[hinoka, 2016/08/03 21:55:39]

Deleted

+                masterName := f[0]
+                if builder.Module != "buildbot" {
+                        panic(fmt.Errorf("Module %s is not buildbot", builder.Module))
+                }
+                if _, ok := masters[masterName]; !ok {
+                        master, internal, t, err := getMasterJSON(c, masterName)
+                        if err != nil {
+                                return nil, err
+                        }
+                        if t.Before(clock.Now(c).Add(-time.Minute * 5)) {
+                                return nil, fmt.Errorf("Data for master is outdated, last updated %s", t)
+                        }
+                        if internal {
+                                return nil, fmt.Errorf("Internal masters not supported")

[nodir, 2016/08/03 20:26:17]

if someone defines a config that references an internal master and then milo
starts breaking with "Internal masters not supported", this way an adversary can
sniff internal master names. Just saying, not sure what you can with it with the
current design

[hinoka, 2016/08/03 21:55:39]

Acknowledged. Probably should just treat the builder as "not found".

+                        }
+                        masters[masterName] = master
+                }
+        }
+
+        results := make([][]*resp.ConsoleBuild, len(commits))
+        for i := range results {
+                results[i] = make([]*resp.ConsoleBuild, len(builders))
+        }
+        // Reuse the builder code. This is kind of a hack but it's okay for now.
+        err := parallel.FanOutIn(func(taskC chan<- func() error) {
+                for i, builder := range builders {
+                        i := i // How do scopes werk
+                        f := strings.SplitN(builder.Name, "/", 2)
+                        master := f[0]
+                        builderName := f[1]

[nodir, 2016/08/03 20:26:16]

this will panic if builder.Name does not have a slash

[hinoka, 2016/08/03 21:55:39]

Done.

+                        taskC <- func() error {
+                                t1 := clock.Now(c)
+                                builds, err := getFullBuilds(c, master, builderName, true)

[nodir, 2016/08/03 20:26:16]

it won't show running builds? also does not show a build for a commit if it was
more than 25 builds in the past

[hinoka, 2016/08/03 21:55:39]

Yes that is true, but it hasn't been a problem for the demo.  Added comment to
indicate this is meant to be a hack.

[nodir, 2016/08/04 23:13:12]

ok, makes sense now

+                                if err != nil {
+                                        return err
+                                }
+                                t2 := clock.Now(c)
+                                var currentStatus *resp.Status
+                                for j, commit := range commits {
+                                        for _, build := range builds {
+                                                if build.Sourcestamp.Revision == commit {

[nodir, 2016/08/03 20:26:17]

this is O(n^2)
make a map commit->j before fanOutIn and then use it inside `for _, build :=
range builds` loop

[hinoka, 2016/08/03 21:55:39]

Logic for filling in-between builds on L97 would break then.

I could make a map of commit -> build index to make this O(N), but in terms of
performance speed, L77 takes 95% of the time in this goroutine, so it probably
doesn't matter too much to fix this.

+                                                        results[j][i] = &resp.ConsoleBuild{

[nodir, 2016/08/03 20:26:17]

this code is racy: it mutates results[j] in different goroutines

[hinoka, 2016/08/03 21:55:39]

Each goroutine fully owns a space in [i], so I don't think they would ever
conflict?  Also the entire matrix is pre-allocated in L64-67.

[nodir, 2016/08/04 23:13:12]

Acknowledged.

+                                                                Link: &resp.Link{
+                                                                        Label: strings.Join(build.Text, " "),
+                                                                        URL: fmt.Sprintf(
+                                                                                "/buildbot/%s/%s/%d", master, builderName, build.Number),
+                                                                },
+                                                                Status: build.toStatus(),
+                                                        }
+                                                        currentStatus = &results[j][i].Status
+                                                }
+                                        }
+                                        if currentStatus != nil && results[j][i] == nil {
+                                                results[j][i] = &resp.ConsoleBuild{Status: *currentStatus}

[nodir, 2016/08/03 20:26:17]

I don't understand this logic. It might be simpler and correct to do the
following: after fanOutIn, for each builder, find first non-nil status among all
commits

[hinoka, 2016/08/03 21:55:39]

Each goroutine processes a single builder.  This just remembers the result of
the commit prior, and fills in the status if there isn't a matching build, so
that the entire matrix is filled out.  This isn't used for the "overall" status
of the builder.

[nodir, 2016/08/04 23:13:12]

this does not sound correct. if there was no build for a commit, we cannot tell
if it was greed or red, so we should not lie about it to the user by assuming
that the build result would be the same as build result for the next commit. If
we properly handle merged builds, however, then we won't have to do this filling

+                                        }
+                                }
+                                log.Debugf(c,
+                                        "Builder %s took %s to query, %s to compute.", builderName,
+                                        t2.Sub(t1), clock.Since(c, t2))
+                                return nil
+                        }
+                }
+        })
+
+        return results, err
+

[nodir, 2016/08/03 20:26:17]

remove blank line

[hinoka, 2016/08/03 21:55:39]

Done.

+}