Milo: Buildbucket PubSub ingestion outline

Milo: Buildbucket PubSub ingestion outline

This is a basic buildbucket ingestion outline which:
1. Sets up the buildbucket pubsub subscription, given the correct permissions
2. Receives pubsub messages, acks them, and logs them.

BUG=624960
Review-Url: https://codereview.chromium.org/2955223002
Committed: https://github.com/luci/luci-go/commit/20a1165ab0ef98cce14e4eac859879a4630abc92

[Ryan Tseng, 2017-06-27 22:11:52]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 22:11:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 22:16:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 22:16:28]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37046e2701747910)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37046e2716424d10)

[Ryan Tseng, 2017-06-28 22:00:34]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 22:00:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 22:06:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 22:06:52]

Dry run: Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37098a22a7399410)

[Ryan Tseng, 2017-06-28 22:14:17]

hinoka@google.com changed reviewers:
+ iannucci@chromium.org

[Ryan Tseng, 2017-06-28 22:14:19]

ptal.  tests are red because they depend on
https://codereview.chromium.org/2957413002/

Deployed and working on -dev right now

https://codereview.chromium.org/2955223002/diff/1/milo/build_source/buildbot/...
File milo/build_source/buildbot/pubsub.go (left):

https://codereview.chromium.org/2955223002/diff/1/milo/build_source/buildbot/...
milo/build_source/buildbot/pubsub.go:54: type pubSubMessage struct {
Moved to common/

https://codereview.chromium.org/2955223002/diff/1/milo/common/pubsub.go
File milo/common/pubsub.go (right):

https://codereview.chromium.org/2955223002/diff/1/milo/common/pubsub.go#newco...
milo/common/pubsub.go:48: topicID := "builds"
This is hardcoded in the buildbucket code

[Ryan Tseng, 2017-06-28 22:16:09]

See http://shortn/_U2MehmgA9G for monitoring

[Ryan Tseng, 2017-07-06 00:27:16]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 00:27:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 00:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 00:33:37]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/372e1d0d7a357a10)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/372e1d0d7aa8d010)
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/372e1d0d9713c110)

[Ryan Tseng, 2017-07-06 19:57:37]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 19:57:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 20:14:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 20:14:18]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-07-06 20:51:32]

this green and ready for review now

[iannucci, 2017-07-08 00:05:26]

lgtm

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
File milo/build_source/buildbucket/pubsub.go (right):

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:32: type transientError error
use transient.Tag instead, and ditch these error types

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:37: return
strings.HasPrefix(build.Bucket, "luci.")
this seems hard-codey? what is this for?

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:52: // TODO(hinoka): Save this into
datastore.
:)

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:56: func pubSubHandlerImpl(c
context.Context, r *http.Request) int {
lets have this return an error and sort out the status code in PubSubHandler? Or
have some httpCodeFromError(error) function (which knows how to interpret tags
etc.)

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:68: return http.StatusOK // This is a
hard failure, we don't want PubSub to retry.
i.e. we should return an error tagged as 'badPayload' or something here, have
the outer method log it and turn that into OK.

That said, if we actually fail to decode this message from pubsub, we shouldn't
ACK it; pubsub may have changed its JSON format or something and we wouldn't
want to drop all that data (rather we should change this handler and let it
buffer in pubsub until we can fix milo)

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go
File milo/common/pubsub.go (right):

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:56: // it does not exist, or an error if there was an
error.
move the docstrings to the interface

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:69: // getSubscription returns the pubsub subscriptionif
it exists,
nit: missing space

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:72: *pubsub.Subscription, error) {
no newline... 80 cols is not a thing in go

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:85: id string, cfg pubsub.SubscriptionConfig) (
nix newlines

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:91: // getPubSubClient tries to extract a debug PubSub
client out of the context.
I would consider instead just having GetTopic(context, ...),
GetSubscription(context, ...) and CreateSubscription(context, ...) be top-level
methods on the package so that the usage of the client is totally internal.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:117: return fmt.Errorf("missing buildbucket project")
errors.New

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:127: err = fmt.Errorf("topic %#v does not exist", topic)
why not have getTopic do this?

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:133: if strings.Contains(err.Error(), "PermissionDenied")
{
oof, is this the best way?

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:134: URL :=
"https://console.cloud.google.com/iam-admin/iam/project?project=" + projectID
may be worth figuring out the gcloud sdk command for this if possible

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:180: registerURL :=
"https://pantheon.corp.google.com/apis/credentials/domainverification?project="
+ projectID
this should be a console url

[Ryan Tseng, 2017-07-08 01:42:22]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-08 01:42:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-08 01:47:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-08 01:47:20]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3738ae774910dd10)

[Ryan Tseng, 2017-07-08 01:47:34]

Not sure why it's failing, nothing appears to be failing in the logs o_0

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
File milo/build_source/buildbucket/pubsub.go (right):

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:32: type transientError error
On 2017/07/08 00:05:25, iannucci wrote:
> use transient.Tag instead, and ditch these error types

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:37: return
strings.HasPrefix(build.Bucket, "luci.")
On 2017/07/08 00:05:25, iannucci wrote:
> this seems hard-codey? what is this for?

To filter out buildbot builds.  the luci-migration app does the same thing so i
copied it.

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:52: // TODO(hinoka): Save this into
datastore.
On 2017/07/08 00:05:25, iannucci wrote:
> :)

Sooooon™

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:56: func pubSubHandlerImpl(c
context.Context, r *http.Request) int {
On 2017/07/08 00:05:25, iannucci wrote:
> lets have this return an error and sort out the status code in PubSubHandler?
Or
> have some httpCodeFromError(error) function (which knows how to interpret tags
> etc.)

There's basically just two failure modes that needs to be differentiated -
transient and permanent.  Will default to permanent unless theres an transient
tag.

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:68: return http.StatusOK // This is a
hard failure, we don't want PubSub to retry.
On 2017/07/08 00:05:25, iannucci wrote:
> i.e. we should return an error tagged as 'badPayload' or something here, have
> the outer method log it and turn that into OK.
> 
> That said, if we actually fail to decode this message from pubsub, we
shouldn't
> ACK it; pubsub may have changed its JSON format or something and we wouldn't
> want to drop all that data (rather we should change this handler and let it
> buffer in pubsub until we can fix milo)

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go
File milo/common/pubsub.go (right):

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:56: // it does not exist, or an error if there was an
error.
On 2017/07/08 00:05:25, iannucci wrote:
> move the docstrings to the interface

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:69: // getSubscription returns the pubsub subscriptionif
it exists,
On 2017/07/08 00:05:25, iannucci wrote:
> nit: missing space

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:72: *pubsub.Subscription, error) {
On 2017/07/08 00:05:25, iannucci wrote:
> no newline... 80 cols is not a thing in go

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:85: id string, cfg pubsub.SubscriptionConfig) (
On 2017/07/08 00:05:25, iannucci wrote:
> nix newlines

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:91: // getPubSubClient tries to extract a debug PubSub
client out of the context.
On 2017/07/08 00:05:25, iannucci wrote:
> I would consider instead just having GetTopic(context, ...),
> GetSubscription(context, ...) and CreateSubscription(context, ...) be
top-level
> methods on the package so that the usage of the client is totally internal.

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:117: return fmt.Errorf("missing buildbucket project")
On 2017/07/08 00:05:25, iannucci wrote:
> errors.New

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:127: err = fmt.Errorf("topic %#v does not exist", topic)
On 2017/07/08 00:05:25, iannucci wrote:
> why not have getTopic do this?

Done.

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:133: if strings.Contains(err.Error(), "PermissionDenied")
{
On 2017/07/08 00:05:25, iannucci wrote:
> oof, is this the best way?

As far as I can tell :(

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:134: URL :=
"https://console.cloud.google.com/iam-admin/iam/project?project=" + projectID
On 2017/07/08 00:05:25, iannucci wrote:
> may be worth figuring out the gcloud sdk command for this if possible

gcloud doesn't appear to be able to add/manipulate iam users & roles :(

https://codereview.chromium.org/2955223002/diff/20001/milo/common/pubsub.go#n...
milo/common/pubsub.go:180: registerURL :=
"https://pantheon.corp.google.com/apis/credentials/domainverification?project="
+ projectID
On 2017/07/08 00:05:25, iannucci wrote:
> this should be a console url

derp.  everytime i go to console.cloud.google.com it just redirects me, so
copypasta links are a trap

[iannucci, 2017-07-08 02:08:41]

lgtm

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
File milo/build_source/buildbucket/pubsub.go (right):

https://codereview.chromium.org/2955223002/diff/20001/milo/build_source/build...
milo/build_source/buildbucket/pubsub.go:56: func pubSubHandlerImpl(c
context.Context, r *http.Request) int {
On 2017/07/08 01:47:33, Ryan Tseng wrote:
> On 2017/07/08 00:05:25, iannucci wrote:
> > lets have this return an error and sort out the status code in
PubSubHandler?
> Or
> > have some httpCodeFromError(error) function (which knows how to interpret
tags
> > etc.)
> 
> There's basically just two failure modes that needs to be differentiated -
> transient and permanent.  Will default to permanent unless theres an transient
> tag.

I was meaning that we could just tag the error and return it then do the logging
and status code stuff in PubSubHandler

[Ryan Tseng, 2017-07-10 20:52:31]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 20:52:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 20:54:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 20:54:31]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/374718354ea0ce10)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/374718355e1e8d10)

[Ryan Tseng, 2017-07-10 21:47:36]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 21:47:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 21:54:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 21:54:55]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-07-10 21:55:32]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2017-07-10 21:55:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2955223002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-07-10 21:55:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 22:01:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499723732358070,
"parent_rev": "51e30bd208ab3463dd0690ffd0c40403d93912f1", "commit_rev":
"20a1165ab0ef98cce14e4eac859879a4630abc92"}

[commit-bot: I haz the power, 2017-07-10 22:01:28]

Description was changed from

==========
Milo: Buildbucket PubSub ingestion outline

This is a basic buildbucket ingestion outline which:
1. Sets up the buildbucket pubsub subscription, given the correct permissions
2. Receives pubsub messages, acks them, and logs them.

BUG=624960
==========

to

==========
Milo: Buildbucket PubSub ingestion outline

This is a basic buildbucket ingestion outline which:
1. Sets up the buildbucket pubsub subscription, given the correct permissions
2. Receives pubsub messages, acks them, and logs them.

BUG=624960

Review-Url: https://codereview.chromium.org/2955223002
Committed:
https://github.com/luci/luci-go/commit/20a1165ab0ef98cce14e4eac859879a4630abc92
==========

[commit-bot: I haz the power, 2017-07-10 22:01:30]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/20a1165ab0ef98cce14e4eac859879a4630abc92