1335953 - Wrap up and deploy tc-pulse, tc-lib-pulse

Status: RESOLVED FIXED

(Taskcluster :: General, defect)

Description

[Brian Stack [:bstack]]

I think it's pretty much ready, but I want to have some quality time with it before we smash the champagne on the bow.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

I'll brew some beer to smash on it while you do that ;)

Seriously, though, let me know how I can help.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

https://docs.taskcluster.net/reference/integrations/taskcluster-pulse/docs/usage
 .. should probably be written :)

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

API cleanup:
A) Remove namespace() and overview(), possibly just rename overview to serverInfo
B) serverInfo should only include rabbitmq version,  as key rabbitMQVersion
C) exchanges() should not return: vhost, type, durable, auto-delete, arguments
   (these are not relevant to pulse users)
D) createNamespace() should be renamed namespace(), and MUST return:
     - expiration time
     - rabbitmq connection string
   (should not return contact information)

----
Further more, I bet that the alerting rules are only slightly smarter than what we have today:
  messagePublishRateTolerance
  messageCountTolerance
We need to find a more robust strategy to be alerting on. That's half the motivation for this project in the first place.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

It's a minor cleanup, but currently all API endpoints that take a namespace verify its validity manually.  TC-lib-api allows the API declaration to specify a regex for parameters, so we should use that instead.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Also, right now the issued users can only read `taskcluster/.*`.  In bug 1345954 I'm looking for what that should be, but perhaps `.*`.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Whoops, this is about tc-pulse, not the pulse libraries.  This doesn't need love right now (it's on hold..)

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Jonas, I think this is on you to do a mix of cleanup, file followups, and accept suboptimal design :)

Comment 8

[Jonas Finnemann Jensen (:jonasfj)]

We just agreed, that this is shippable, and we can improve monitoring later :)

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

It looks like pulse-publisher is OK for now, and so far we're planning to just configure a lot of pulse credentials for the first few drafts of r13y.  This is still on my list of things to do, but lower priority.

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #3)
> API cleanup:
> A) Remove namespace()

I think this was only to free up the method name for D; see below.

> C) exchanges() should not return: vhost, type, durable, auto-delete,
> arguments
>    (these are not relevant to pulse users)

This appears to have been removed in the last year :)51

> D) createNamespace() should be renamed namespace(), and MUST return:
>      - expiration time
>      - rabbitmq connection string
>    (should not return contact information)

This is already renamed `claimNamespace`, parallel to `claimWork` for example.  And thus doesn't overlap with `namespace`.

it already returns the expiration time and connection string.

Probably none of the methods should return contact information, mostly the listNamespaces / namespace methods, for PII protection.

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #4)
> It's a minor cleanup, but currently all API endpoints that take a namespace
> verify its validity manually.  TC-lib-api allows the API declaration to
> specify a regex for parameters, so we should use that instead.

In fact, this requires bug 1344043, but that's done now..

https://github.com/taskcluster/taskcluster-pulse/pull/96

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

Oh, that bug is *not* finished.  Yet the tests pass.  Curiouser and curiouser.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

I just turned this service up in production, including the rotation/expiration but without the monitoring component.  I'd like to stub some of the more destructive bits of that out, set up a papertrail alert for when it would *try* to run those bits, and make sure it's not going to delete the world.

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

https://github.com/taskcluster/taskcluster-lib-pulse/pull/3 -- implements PulseConsumer.  I've filed bugs for other required functionality.