817035 - add comments for devices, and a locked-out state

Status: RESOLVED FIXED

(Testing Graveyard :: Mozpool, defect)

Description

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

For the short term, we'll have a bunch of pandas that shouldn't be managed by mozpool - they'll be running Fennec tests and not using the mozpool API for power control.

We've also got a few teams using a lot of pandas for all manner of different and overlapping purposes.

I'd like to add a state, "locked-out", in which a panda will not be touched by mozpool at all.  I'd also like to add a "comments" field where we can indicate who's using the panda.

This is a short-term solution.  Eventually, all pandas will be managed by mozpool.

Comment 1

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

I'm adding a bunch of additional mozpool-related tasks here:
 * show the last PXE config used to boot the device (eventually to be replaced by an image name)
 * UI rework to support the large and growing set of operations we want to support
 * Add a 'pool' to each device, and allow requests to specify a pool (defaulting to 'production')

Comment 2

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

Attachment: comments.patch

You can see the whole patch series here:
  https://github.com/djmitche/mozpool/compare/ui-updates
I'm going to try to spread the r? love around a little bit.  Feel free to look at the patches on github instead.

Comment 3

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

Oh, header for attachment 688386 [details] [diff] [review] is

Subject: [PATCH] Bug 817035: Add comments for devices and a
 /device/{id}/set-comments/ API call to set them

This also adds a 'detail' option to the list_devices function, and adds
comments and last_pxe_config to its return value.  This allows
dump_devices to be specifically intended for inventory sync, which is
sensitive to the full set of keys for each device.

Comment 4

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

Attachment: lockedout.patch

(this also removes an unused failed_* state)

Comment 5

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

Attachment: ui-refactor.patch

Comment 6

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

Attachment: logging.patch

Comment 7

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

Attachment: environments.patch

Comment 8

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

With all of the backward-compatible API changes and schema changes, this will be 1.1.0.

Comment 9

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

Comment on attachment 688387 [details] [diff] [review]
lockedout.patch

Armen punts to jhopkins

Comment 10

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

Comment on attachment 688391 [details] [diff] [review]
environments.patch

Armen punts to jhopkins

Comment 11

[John Hopkins (:jhopkins)]

Comment on attachment 688391 [details] [diff] [review]
environments.patch

- 'whatevs' debug string must be replaced
- "print body" debug should be removed (2 instances)

Can you ask someone more familiar with JavaScript to review the .js chunks?

Comment 12

[John Hopkins (:jhopkins)]

dustin: could you also update testdata.py to include some environment information that I can test against?

Comment 13

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

Attachment: environments-r2.patch

This has the fixes jhopkins requested; over to mcote for JS review.

Comment 14

[Mark Côté [:mcote]]

Comment on attachment 688390 [details] [diff] [review]
logging.patch

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

Looks really good!

::: API.txt
@@ +167,3 @@
>    a 'log' key containing a list of objects representing log lines.
> +  If the query parameter '?timeperiod=hour' is added, only log entries from
> +  the last hour will be included

The format of this parameter strikes me as a bit odd, but I guess we can leave it to future expansion.

::: mozpool/html/ui/js/models.js
@@ +174,2 @@
>      model: LogLine,
> +    refreshInterval: 5000, // 5s, so we don't crush the DB

I'm wondering if even 5 s is a bit aggressive as a default value.

::: mozpool/html/ui/js/views.js
@@ +450,5 @@
> +
> +        span = $('<span>')
> +        span.attr('class', 'message');
> +        span.text(l.get('message'));
> +        line.append(span);

I personally prefer templating like ICanHaz.js for this, but fine for now.

Comment 15

[Mark Côté [:mcote]]

Comment on attachment 688488 [details] [diff] [review]
environments-r2.patch

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

This is pretty nice, but I think some refactoring should occur in a couple places where the code is virtually identical.  r-ing just on that.

::: API.txt
@@ +39,5 @@
>    the only supported value is "b2g", for which a "b2gbase" key must also be
>    present.  The value of "b2gbase" must be a URL to a b2g build directory
> +  containing boot, system, and userdata tarballs.  If supplied, the
> +  "environment" key limits the available devices to those in the given
> +  environment; the default is 'any', which can also be supplied explicitly.

Any reason you didn't go with "pool" instead of "environment"?

::: mozpool/html/ui/js/controllers.js
@@ +223,5 @@
> +        var job_args = this.running.get('job_args');
> +        var url = '//' + this.running.get('device').get('imaging_server') + '/api/device/'
> +            + this.running.get('device_name') + '/set-environment/';
> +        var post_params = { environment: job_args.environment };
> +        

Trailing whitespace.

::: mozpool/html/ui/js/models.js
@@ +174,5 @@
> +        });
> +
> +        // this just instructs the fetch/add to not anything else:
> +        return [];
> +    },

This is a cut & paste of the DeviceNames collection.  Please refactor so that we have generic updateable collections for updateable and nonupdateable models.  Could be just a boolean in UpdateableCollection that determines whether it examines the intersection or not.

::: mozpool/html/ui/js/views.js
@@ +613,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +1198,5 @@
> +    render: function() {
> +        $(this.el).attr('value', this.model.get('id')).html(this.model.get('id'));
> +        return this;
> +    }
> +});

Very close copies of MozpoolRequestedDeviceSelectView and MozpoolRequestedDeviceOptionView.  The only difference, the option defaulting to "any", should apply to devices as well.  Please refactor.

Comment 16

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

(In reply to Mark Côté ( :mcote ) from comment #14)
> ::: API.txt
> @@ +167,3 @@
> >    a 'log' key containing a list of objects representing log lines.
> > +  If the query parameter '?timeperiod=hour' is added, only log entries from
> > +  the last hour will be included
> 
> The format of this parameter strikes me as a bit odd, but I guess we can
> leave it to future expansion.

I can't really explain that one!  I guess I could have allowed it to specify a number of seconds, too.  And that might make more sense - the Model could look at the number of seconds since its last poll, round up a bit, and request that time period.  Then if a laptop sleeps or for some reason the poll doesn't fire on time, messages won't be missed.

I'll re-draft with that fixed.

> ::: mozpool/html/ui/js/models.js
> @@ +174,2 @@
> >      model: LogLine,
> > +    refreshInterval: 5000, // 5s, so we don't crush the DB
> 
> I'm wondering if even 5 s is a bit aggressive as a default value.

Because we're using partitioning and selecting only recent events, this is actually a straight-up index select on a fairly small table, so it should be OK.  And 5s gets users the "tailing" behavior they want in order to monitor progress.

> ::: mozpool/html/ui/js/views.js
> @@ +450,5 @@
> > +
> > +        span = $('<span>')
> > +        span.attr('class', 'message');
> > +        span.text(l.get('message'));
> > +        line.append(span);
> 
> I personally prefer templating like ICanHaz.js for this, but fine for now.

Next time :)

(In reply to Mark Côté ( :mcote ) from comment #15)
> Review of attachment 688488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is pretty nice, but I think some refactoring should occur in a couple
> places where the code is virtually identical.  r-ing just on that.

Sounds good.

> Any reason you didn't go with "pool" instead of "environment"?

Historically, using the same name for multiple things has caused un-ending confusion.  It's bad enough "mozpool" is the tool and its top layer.  We need some term to refer to the whole set of devices managed by a mozpool cluster, and that seems the most natural definition of "pool".  We also have a *different* definition of "pool" used in Slavealloc.  The intended use by releng of the subdivision I've added in this patch is to separate production devices from staging devices, so "environment" made sense.  It also matches the use of "environment" in Slavealloc.

Thanks!

Comment 17

[Mark Côté [:mcote]]

Comment on attachment 688389 [details] [diff] [review]
ui-refactor.patch

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

This looks pretty great.  Sorry for taking so long; wanted to be thorough. :)

A few questions and comments below.  No show stoppers, so r+.

::: mozpool/html/ui/css/ui.css
@@ +135,4 @@
>  }
>  
> +div#toolbar ul li:before {
> +    content: "«";

An...interesting choice. :) Would square brackets not be better? As someone who occasionally reads French, the choice of French quotation marks is confusing. :)

::: mozpool/html/ui/js/bmm.js
@@ +23,5 @@
> +                BmmPowerControlView,
> +                BmmPxeBootView,
> +                UpdateCommentsView,
> +            ],
> +        }).render();

Not really related to this patch, but I find this lack of view ownership to be confusing.  How do the View objects persist, since they are created but not assigned to anything?

::: mozpool/html/ui/js/controllers.js
@@ +202,5 @@
> +        var job_args = this.running.get('job_args');
> +        var url = '//' + this.running.get('device').get('imaging_server') + '/api/device/'
> +            + this.running.get('device_name') + '/set-comments/';
> +        var post_params = { comments: job_args.comments };
> +        

Trailing whitespace.

@@ +212,5 @@
> +            },
> +            complete: this.jobFinished
> +        });
> +    },
> +

Clear that the commands need some refactoring to share code, but I believe you are aware of this. :)

::: mozpool/html/ui/js/lifeguard.js
@@ +23,5 @@
> +                LifeguardPleaseView,
> +                LifeguardForceStateView,
> +                UpdateCommentsView,
> +            ],
> +        }).render();

I like this idea of having a hierarchy of views.  Definitely feels more organized.

::: mozpool/html/ui/js/models.js
@@ +206,1 @@
>      }

Ah I like this a lot.  I was concerned that the number of models was exploding.

::: mozpool/html/ui/js/ui.js
@@ +27,4 @@
>      .thenLoad(
>          '//cdnjs.cloudflare.com/ajax/libs/datatables/1.9.4/jquery.dataTables.min.js',
>          '//cdnjs.cloudflare.com/ajax/libs/jqueryui/1.9.0/jquery-ui.min.js',
> +        //'http://backbonejs.org/backbone.js')

Remove commented-out code.

::: mozpool/html/ui/js/views.js
@@ +279,5 @@
>          args.checkboxRE = /request-(\d+)/;
>          TableView.prototype.initialize.call(this, args);
> +
> +        // get our "include closed" checkbox hooked up
> +        new IncludeClosedCheckboxView({ el: $('#include-closed-checkbox') }).render();

I am thinking that this should this be a subview like the footer/toolbar, just for consistency.

@@ +445,5 @@
> +
> +var ToolbarView = Backbone.View.extend({
> +    tagName: 'div',
> +    initialize: function(args) {
> +        this.subview_classes = args['subview_classes'];

Just a random comment--should we not be using mixedCaps for variable names in general instead of underscores?  There's quite a mix here, but afaik the standard is usually the former.

@@ +509,4 @@
>      }
>  });
>  
> +/* 

Trailing whitespace.

@@ +519,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +557,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +592,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +594,5 @@
> +        var view;
> +
> +        view = new HelpView({help_text: 
> +            'This view allows you to comment on one or more devices.  ' +
> +            'To clear out old comments, tick the checkbox but leave the text field blank.'});

This is a bit odd, having a checkbox beside the comment box.  It's not immediately clear what it's for.  Later down in the diff the box is unchecked once the job is run so it doesn't reapply, but wouldn't you have to click update again to apply it anyway?  Can we not just clear the comment if one or more devices are selected, the text field left blank, and the 'comment' button clicked?  Also changing the button to 'update' might make it clearer that comments are always replaced, not appended to existing ones.

Anyway feels like I'm missing something here.

@@ +618,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +660,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +691,5 @@
> +
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +712,5 @@
>  
> +    render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 

Trailing whitespace.

@@ +741,5 @@
>      render: function() {
> +        var view;
> +
> +        view = new HelpView({help_text: 
> +            'This tool allows you to request a new device.  ' + 

Trailing whitespace on two lines.

@@ +819,5 @@
>  
> +    refreshButtonStatus: function() {
> +        var self = this;
> +
> +        // check through the args for disabled_unless

Very nice, I like the generic approach to disabling the buttons.

@@ +840,5 @@
> +            job_queue.push(job);
> +        } else {
> +            // new job for each selected element
> +            var set_comments = window.control_state.get('set_comments')
> +                && (-1 != $.inArray('comments', self.job_args));

This comment-handling code seems very specific considering the rest of the NewJobButtonView is quite generic.  And it doesn't apply to a lot of views, unless I'm misunderstanding something. Maybe this can be subclassed?

Comment 18

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

Attachment: logging-r2.patch

I had to make some changes to the schema, here, too.

Comment 19

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

Attachment: environments-r3.patch

You want a refactor?  I'll show you a refactor!

I got pretty into it!  I restrained myself from refactoring controller.js, which honestly really needs it.

Comment 20

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

(In reply to Mark Côté ( :mcote ) from comment #17)
> > +div#toolbar ul li:before {
> > +    content: "«";
> 
> An...interesting choice. :) Would square brackets not be better? As someone
> who occasionally reads French, the choice of French quotation marks is
> confusing. :)

Fair enough.. really, I should find a way to make some pretty tabs, but I'm not that skilled..

> Not really related to this patch, but I find this lack of view ownership to
> be confusing.  How do the View objects persist, since they are created but
> not assigned to anything?

The magic of events!  I could stick them in a variable, but then I'd never refer to it..

> ::: mozpool/html/ui/js/ui.js
> @@ +27,4 @@
> >      .thenLoad(
> >          '//cdnjs.cloudflare.com/ajax/libs/datatables/1.9.4/jquery.dataTables.min.js',
> >          '//cdnjs.cloudflare.com/ajax/libs/jqueryui/1.9.0/jquery-ui.min.js',
> > +        //'http://backbonejs.org/backbone.js')
> 
> Remove commented-out code.

Done.

> > +        new IncludeClosedCheckboxView({ el: $('#include-closed-checkbox') }).render();
> 
> I am thinking that this should this be a subview like the footer/toolbar,
> just for consistency.

Meh, let's save that for a rainy-day project :)

> @@ +445,5 @@
> > +
> > +var ToolbarView = Backbone.View.extend({
> > +    tagName: 'div',
> > +    initialize: function(args) {
> > +        this.subview_classes = args['subview_classes'];
> 
> Just a random comment--should we not be using mixedCaps for variable names
> in general instead of underscores?  There's quite a mix here, but afaik the
> standard is usually the former.

I suck at this -- Python pulls me back and forth between pep8 and Twisted (interCaps), and apparently the result is that I decide randomly.  Perhaps I could be wired into a cryptographic RNG, but aside from that it's just annoying.  I cleaned this up a bit in attachment 688908 [details] [diff] [review], but not completely.

> > +        view = new HelpView({help_text: 
> > +            'This view allows you to comment on one or more devices.  ' +
> > +            'To clear out old comments, tick the checkbox but leave the text field blank.'});
> 
> This is a bit odd, having a checkbox beside the comment box.  It's not
> immediately clear what it's for.  Later down in the diff the box is
> unchecked once the job is run so it doesn't reapply, but wouldn't you have
> to click update again to apply it anyway?  Can we not just clear the comment
> if one or more devices are selected, the text field left blank, and the
> 'comment' button clicked?  Also changing the button to 'update' might make
> it clearer that comments are always replaced, not appended to existing ones.
> 
> Anyway feels like I'm missing something here.

Well, I'm missing something in terms of UI design.  This is the one field where you want to be able to distinguish "set an empty value" from "don't change".  I also want to avoid the error of leaving the checkbox checked and accidentally applying comments during the next user operation -- which is why I uncheck the box afterward.

I'm open to better UI ideas (if they come with an implementation), but let's do them on another bug..

> @@ +840,5 @@
> > +            job_queue.push(job);
> > +        } else {
> > +            // new job for each selected element
> > +            var set_comments = window.control_state.get('set_comments')
> > +                && (-1 != $.inArray('comments', self.job_args));
> 
> This comment-handling code seems very specific considering the rest of the
> NewJobButtonView is quite generic.  And it doesn't apply to a lot of views,
> unless I'm misunderstanding something. Maybe this can be subclassed?

This got refactored in attachment 688908 [details] [diff] [review] to be a bit more agnostic.  The complexity is there because comments are set in a separate job from other actions.

So I'll fix up the whitespace and whatnot on this, but no new review.

Comment 21

[Mark Côté [:mcote]]

Comment on attachment 688907 [details] [diff] [review]
logging-r2.patch

Cool.

Comment 22

[Mark Côté [:mcote]]

Comment on attachment 688908 [details] [diff] [review]
environments-r3.patch

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

Great!  I mostly looked at the the newly refactored stuff (UpdateableCollection and SelectView and derivations thereof); I assume the rest of the stuff is unmodified (hard to tell with everything moving between files).  Anyway the refactoring looks great!  Glad you were able to do this now; should make maintenance a lot easier.  A hearty r+.

::: mozpool/html/ui/js/models.js
@@ +140,2 @@
>      responseAttr: 'devices',
> +    namesOnly: true

Super nit-picky nit: the order of variables here is different from UpdateableCollection and Environments.

Comment 23

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

The schema diff is:

diff --git a/sql/schema.sql b/sql/schema.sql
index 9e26b05..a1fcd03 100644
--- a/sql/schema.sql
+++ b/sql/schema.sql
@@ -141,6 +141,10 @@ CREATE TABLE devices (
   foreign key (last_pxe_config_id) references pxe_configs(id) on delete restrict,
   -- config the device will use on its next boot (JSON blob)
   boot_config text,
+  -- free-form comments about the device (for BMM + Lifeguard)
+  comments text,
+  -- fields for filtering devices when requesting
+  environment varchar(32) not null default 'none',
 
   unique index name_idx (name),
   index state_timeout_idx (state_timeout)
@@ -163,7 +167,9 @@ CREATE TABLE requests (
   -- state machine variables
   state varchar(32) not null,
   state_counters text not null,
-  state_timeout datetime
+  state_timeout datetime,
+  -- constraining fields for the request
+  environment varchar(32) not null default 'any',
 );
 
 DROP TABLE IF EXISTS device_requests;
@@ -176,6 +182,7 @@ CREATE TABLE device_requests (
 
 DROP TABLE IF EXISTS device_logs;
 CREATE TABLE device_logs (
+    id bigint not null auto_increment,
     -- foreign key for the device
     device_id integer not null,
     ts timestamp not null,
@@ -185,12 +192,14 @@ CREATE TABLE device_logs (
     message text not null,
     -- indices
     index device_id_idx (device_id),
-    index ts_idx (ts)
+    index ts_idx (ts),
+    primary key pk (id, ts)
 );
 CALL init_log_partitions('device_logs', 14, 1);
 
 DROP TABLE IF EXISTS request_logs;
 CREATE TABLE request_logs (
+    id bigint not null auto_increment,
     -- request this log is for
     request_id bigint not null,
     ts timestamp not null,
@@ -200,7 +209,8 @@ CREATE TABLE request_logs (
     message text not null,
     -- indices
     index request_id_idx (request_id),
-    index ts_idx (ts)
+    index ts_idx (ts),
+    primary key pk (id, ts)
 );
 CALL init_log_partitions('request_logs', 14, 1);
 


and the sql to achieve the same thing (noting the result is compatible with 1.0.0 before upgrade to 1.1.0) is:

alter table devices add column comments text;
alter table devices add column environment varchar(32) not null default 'none';
alter table device_logs add column id bigint not null auto_increment, add primary key pk (id, ts);
alter table request_logs add column id bigint not null auto_increment, add primary key pk (id, ts);

So, I'm going to land this now, SQL changes first.

Comment 24

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

This eagle has landed.