Justin Wood (:Callek) Assignee
	Description • 9 years ago

+++ This bug was initially created as a clone of Bug #1122385 +++

We should expose which jacuzzi's and their state, alongside non-jacuzzi info.

This patch is messy at a glance (cleanup suggestions accepted)

Anyway, for testing I cloned jacuzzi-allocator to json/test_json directly `git clone https://github.com/mozilla/releng-jacuzzis`

This patch also updates all the json files produced by slave_health.py due a new file and new keys.

There are certainly room for improvements here, but I think this view is much better than the nothing we have now.

	Chris Cooper [:coop] (he/him)
	Comment 1 • 9 years ago

Comment on attachment 8555074 [details] [diff] [review]
[slave_health] add jacuzzi page

Review of attachment 8555074 [details] [diff] [review]:
-----------------------------------------------------------------

The json changes look fine. Let's go ahead and land those, or at least split them into a separate patch next time. 

r- for the design of the jacuzzi page and the info duplicated from index.html.

::: jacuzzis.html
@@ +60,5 @@
> +      </div>
> +    <div class="jacuzzi-dry">
> +      <table id="header">
> +        <tr>
> +        <th><h5>Machines Not In a Jacuzzi</h5></th>

For consistency, we should have a similar heading on the jacuzzi side of the page. However, the larger question is whether we need to replicate the list of slavetypes. Without an automatic way to generate the list of types, we'll need to keep both in sync by hand as things change.

I would prefer to *not* have the slavetypes listed on the right at all. When I select a jacuzzi from the list on the left, I would rather that it pull up a list of slaves in that jacuzzi on the right. I could then click on the slave name to go to the corresponding slave.html page.

::: js/slave_health.js
@@ +928,3 @@
>          var $page = $(data);
>          $page.find('td a').each(function(i, link) {
>              var href = link.getAttribute('href');

https://hg.mozilla.org/build/slave_health/file/31275ba950b1/js/slave_health.js#l920 - we should give this <td class="problem"> for consistency with other tables, e.g. reboots.

@@ +961,5 @@
> +            }
> +            jacuzzi_builders.push(link.textContent);
> +        });
> +        jacuzzi_builders.sort();
> +        $("#jacuzzi-builder-list").append("<tr><th>Builder Names</td></tr>");

For consistency with the other tables on the page, the builder names header should always appear, i.e. bake it into the html.

	Chris Cooper [:coop] (he/him)
	Comment 2 • 9 years ago

Running the patch locally, I also see a "Windows 7 32-bit try opt test mochitest-other" test row at the top of the jacuzzi list. Test jobs aren't in jacuzzis so this baffles me. Something is ending up in the wrong category somehow.

http://i.imgur.com/VsKrsu1.png

	Justin Wood (:Callek) Assignee
	Comment 3 • 9 years ago

(In reply to Chris Cooper [:coop] from comment #2)
> Running the patch locally, I also see a "Windows 7 32-bit try opt test
> mochitest-other" test row at the top of the jacuzzi list. Test jobs aren't
> in jacuzzis so this baffles me. Something is ending up in the wrong category
> somehow.
> 
> http://i.imgur.com/VsKrsu1.png

This was testing html accidentally left in. Not actually part of a jacuzzi.

(In reply to Chris Cooper [:coop] from comment #1)
> Comment on attachment 8555074 [details] [diff] [review]
> [slave_health] add jacuzzi page
> 
> Review of attachment 8555074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The json changes look fine. Let's go ahead and land those, or at least split
> them into a separate patch next time. 

I'll split to a new patch with an explicit seperate review.

> r- for the design of the jacuzzi page and the info duplicated from
> index.html.

Discussed overall on IRC, but i'll post a summary in following comment

> ::: js/slave_health.js
> @@ +928,3 @@
> >          var $page = $(data);
> >          $page.find('td a').each(function(i, link) {
> >              var href = link.getAttribute('href');
> 
> https://hg.mozilla.org/build/slave_health/file/31275ba950b1/js/slave_health.
> js#l920 - we should give this <td class="problem"> for consistency with
> other tables, e.g. reboots.

sure

> @@ +961,5 @@
> > +            }
> > +            jacuzzi_builders.push(link.textContent);
> > +        });
> > +        jacuzzi_builders.sort();
> > +        $("#jacuzzi-builder-list").append("<tr><th>Builder Names</td></tr>");
> 
> For consistency with the other tables on the page, the builder names header
> should always appear, i.e. bake it into the html.

Done.

	Justin Wood (:Callek) Assignee
	Comment 4 • 9 years ago

So from IRC coops ideal is to have:

An basic list, as in the main index, of the build types/classes/etc that shows the jacuzzi's associated with it, rather than a bland list of all jacuzzi's.

This would be basically the index page with "not in a jacuzzi" and "<list of jacuzzi builders>" both present underneath the basic slaveclass status.

I like this idea but it is a significant rework of the current effort here.

Alternatively coop suggested he would be ok with the current page, but with links that worked to display the entirety of the slaves in that jacuzzi.

I'll be working on that latter part now.

In the end, this jacuzzi data is least valuable to releng as it stands, and most valuable to sheriffs, so if they prefer a different solution than all discussed, we'll likely go with what is useful to them as a whole.

:coop, feel free to correct me on any of the above points

	Justin Wood (:Callek) Assignee
	Comment 5 • 9 years ago

This is the same .py from previous patch.

	Justin Wood (:Callek) Assignee
	Comment 6 • 9 years ago

This updates the static test json, based on the .py from previous patch and is matching the update from initial patch.

	Justin Wood (:Callek) Assignee
	Comment 7 • 9 years ago

This is the r-'ed parts from the initial patch here, just split out for ease of bug skimming.

	Justin Wood (:Callek) Assignee
	Comment 8 • 9 years ago

Very little different from v0, mostly just your review nits. (which were for the slave.html view)

	Justin Wood (:Callek) Assignee
	Comment 9 • 9 years ago

This adds an include/exclude command for slavetype.html to list the jacuzzi'd slaves and ones not in a jacuzzi.

The way this code is structured, all the batch actions will automatically operate on only the shown rows.

You can invert via s/exclude/include/ + s/include/exclude/ manually at present. Anything listed excluded takes precedence over anything listed in included.

There are no assertions on valid slave names in exclude/include lists.

	Chris Cooper [:coop] (he/him)
	Comment 10 • 9 years ago

Comment on attachment 8555688 [details] [diff] [review]
[slave_health] add jacuzzi page - v1

Review of attachment 8555688 [details] [diff] [review]:
-----------------------------------------------------------------

::: slave.html
@@ +99,4 @@
>            </div>
>  	  <div id="jacuzzi-container"><h2>Jacuzzis</h2>
>            <table id="jacuzzi-builder-list"></table>
> +            <tr><th>Builder Names</td></tr>

(In reply to Justin Wood (:Callek) from comment #8)
> Very little different from v0, mostly just your review nits. (which were for
> the slave.html view)

Some of which I already fixed because I realized they were my fault, so you may have some conflicts. (Sorry)

	Chris Cooper [:coop] (he/him)
	Comment 11 • 9 years ago

Comment on attachment 8555691 [details] [diff] [review]
[slave_health] add slave list support for jacuzzis

Review of attachment 8555691 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Justin Wood (:Callek) from comment #9)
> You can invert via s/exclude/include/ + s/include/exclude/ manually at
> present. Anything listed excluded takes precedence over anything listed in
> included.

What's the eventual plan for this? Three-state toggle: in, out, and both?

	Justin Wood (:Callek) Assignee
	Comment 12 • 9 years ago

Comment on attachment 8555405 [details] [diff] [review]
[slave_health] update json generation

http://hg.mozilla.org/build/slave_health/rev/b4ad94663084

	Justin Wood (:Callek) Assignee
	Comment 13 • 9 years ago

Comment on attachment 8555407 [details] [diff] [review]
[slave_health] update static test json

http://hg.mozilla.org/build/slave_health/rev/3a3db5641808

	Justin Wood (:Callek) Assignee
	Comment 14 • 9 years ago

Comment on attachment 8555688 [details] [diff] [review]
[slave_health] add jacuzzi page - v1

http://hg.mozilla.org/build/slave_health/rev/8b57fb7c8e7f

	Justin Wood (:Callek) Assignee
	Comment 15 • 9 years ago

Comment on attachment 8555691 [details] [diff] [review]
[slave_health] add slave list support for jacuzzis

http://hg.mozilla.org/build/slave_health/rev/6a159cefbb35

	Justin Wood (:Callek) Assignee
	Comment 16 • 9 years ago

Besides this patch I pushed a followup for earlier ones: http://hg.mozilla.org/build/slave_health/rev/81419ef34c71 which removed a debugging alert I had.

This patch consolidates the buildername-to-ID logic I had spread out, and strips the '+' character from the ID (and CSSID as a consequence). Allowing buildernames like:

Android armv7 API 11+ b2g-inbound build

To have status and links.