Code review comment for lp:~frankban/juju-gui/bug-1169973-dying-services

Reviewers: mp+159443_code.launchpad.net,

Message:
Please take a look.

Description:
Do not hide dying services in an error state

Due to bug 1168154, removing a service in an error state
does not really destroy the service, that will be
eventually destroyed once all its units are resolved.
A dying unit could also fail executing the stop hook,
leaving the service undead in the juju database.
For this reason, we don't wont to hide services in an
erro state, even if they are dying.

QA:
- deploy buildbot-master;
- destroy it;
- wait a second, you should see it return like a
   red zombie.

https://code.launchpad.net/~frankban/juju-gui/bug-1169973-dying-services/+merge/159443

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8835043/

Affected files:
   A [revision details]
   M app/models/models.js
   M app/views/topology/service.js
   M test/test_model.js
   M test/test_service_module.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision:
<email address hidden>

Index: test/test_model.js
=== modified file 'test/test_model.js'
--- test/test_model.js 2013-04-17 13:28:40 +0000
+++ test/test_model.js 2013-04-17 15:45:59 +0000
@@ -646,7 +646,7 @@

  describe('service models', function() {
- var models, list, django, rails, mysql;
+ var models, list, django, rails, wordpress, mysql;

    before(function(done) {
      YUI(GlobalConfig).use(['juju-models'], function(Y) {
@@ -657,9 +657,22 @@

    beforeEach(function() {
      django = new models.Service({id: 'django'});
- rails = new models.Service({id: 'rails', life: 'dying'});
- mysql = new models.Service({id: 'mysql', life: 'dead'});
- list = new models.ServiceList({items: [rails, django, mysql]});
+ rails = new models.Service({
+ id: 'rails',
+ life: 'dying',
+ aggregated_status: {}
+ });
+ wordpress = new models.Service({
+ id: 'wordpress',
+ life: 'dying',
+ aggregated_status: {error: 42}
+ });
+ mysql = new models.Service({
+ id: 'mysql',
+ life: 'dead',
+ aggregated_status: {error: 0}
+ });
+ list = new models.ServiceList({items: [rails, django, wordpress,
mysql]});
    });

    it('instances identify if they are alive', function() {
@@ -668,14 +681,25 @@
    });

    it('instances identify if they are not alive (dying or dead)',
function() {
- assert.isFalse(rails.isAlive());
- assert.isFalse(mysql.isAlive());
- });
-
- it('can be filtered so that it returns only alive models', function() {
- var filtered = list.alive();
- assert.strictEqual(1, filtered.size());
- assert.deepEqual([django], filtered.toArray());
+ assert.isFalse(rails.isAlive(), rails.get('id'));
+ assert.isFalse(wordpress.isAlive(), wordpress.get('id'));
+ assert.isFalse(mysql.isAlive(), mysql.get('id'));
+ });
+
+ it('instances identify if they they have errors', function() {
+ assert.isTrue(wordpress.hasErrors());
+ });
+
+ it('instances identify if they they do not have errors', function() {
+ assert.isFalse(django.hasErrors(), django.get('id'));
+ assert.isFalse(rails.hasErrors(), rails.get('id'));
+ assert.isFalse(mysql.hasErrors(), mysql.get('id'));
+ });
+
+ it('can be filtered so that it returns only visible models', function() {
+ var filtered = list.visible();
+ assert.strictEqual(2, filtered.size());
+ assert.deepEqual([django, wordpress], filtered.toArray());
    });

  });

Index: test/test_service_module.js
=== modified file 'test/test_service_module.js'
--- test/test_service_module.js 2013-04-17 10:20:36 +0000
+++ test/test_service_module.js 2013-04-17 15:45:59 +0000
@@ -312,22 +312,29 @@
      assert.isFalse(topo.ignoreServiceClick);
    });

- it('should hide dying or dead services', function() {
+ it('should show only visible services', function() {
      var haproxy = db.services.getById('haproxy'); // Added in beforeEach.
      db.services.add([
        {id: 'rails', life: 'dying'},
        {id: 'mysql', life: 'dead'}
      ]);
      var django = db.services.add({id: 'django'});
+ var wordpress = db.services.add({
+ id: 'wordpress',
+ life: 'dying',
+ aggregated_status: {error: 42}
+ });
      serviceModule.update();
      var boxes = topo.service_boxes;
- // There are four services in total.
- assert.strictEqual(4, db.services.size());
- // But only two of those are actually displayed.
- assert.strictEqual(2, Y.Object.size(boxes));
- // And they are the alive ones.
+ // There are five services in total.
+ assert.strictEqual(5, db.services.size(), 'total');
+ // But only three of those are actually displayed.
+ assert.strictEqual(3, Y.Object.size(boxes), 'displayed');
+ // And they are the visible ones.
      assert.deepPropertyVal(boxes, 'haproxy.model', haproxy);
      assert.deepPropertyVal(boxes, 'django.model', django);
+ // Service wordpress is displayed because it has units in an error
state.
+ assert.deepPropertyVal(boxes, 'wordpress.model', wordpress);
    });

  });

Index: app/models/models.js
=== modified file 'app/models/models.js'
--- app/models/models.js 2013-04-17 13:28:40 +0000
+++ app/models/models.js 2013-04-17 15:45:59 +0000
@@ -108,6 +108,20 @@
       */
      isAlive: function() {
        return this.get('life') === ALIVE;
+ },
+
+ /**
+ Return true if one or more units in this service are in an error
state.
+
+ Return false otherwise.
+
+ @method hasErrors
+ @return {Boolean} Whether one or more unit are in an error state.
+ */
+ hasErrors: function() {
+ var aggregates = this.get('aggregated_status') || {},
+ errors = aggregates.error || false;
+ return errors && errors >= 1;
      }

    }, {
@@ -147,14 +161,19 @@
      model: Service,

      /**
- Return a list of alive model instances.
+ Return a list of visible model instances.
+
+ A model instance is visible when it is alive or when, even if it is
dying
+ or dead, one or more of its units are in an error state.
+ In the latter case, we want to still display the service in order to
+ allow users to retry or resolve its units.

        @method alive
- @return {Y.ModelList} The model instances having life === 'alive'.
+ @return {Y.ModelList} The resulting visible model instances.
      */
- alive: function() {
+ visible: function() {
        return this.filter({asList: true}, function(model) {
- return model.isAlive();
+ return model.isAlive() || model.hasErrors();
        });
      },

Index: app/views/topology/service.js
=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js 2013-04-17 14:22:02 +0000
+++ app/views/topology/service.js 2013-04-17 16:33:00 +0000
@@ -353,7 +353,7 @@
        var vis = topo.vis;
        var db = topo.get('db');

- views.toBoundingBoxes(this, db.services.alive(), topo.service_boxes);
+ views.toBoundingBoxes(this, db.services.visible(),
topo.service_boxes);

        // Nodes are mapped by modelId tuples.
        this.node = vis.selectAll('.service')