Merge into trunk : bug-1169973-dying-services : Code : juju-gui

Status:

Merged

Merged at revision:

572

Proposed branch:

lp:~frankban/juju-gui/bug-1169973-dying-services

Merge into:

lp:juju-gui/experimental

Diff against target:

0 lines

To merge this branch:

bzr merge lp:~frankban/juju-gui/bug-1169973-dying-services

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-04-17	Pending
Review via email: mp+159443@code.launchpad.net

Description of the change

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://codereview.appspot.com/8835043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2013-04-17:

#

Download full text (7.2 KiB)

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.hasEr...

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: nicola.larosa@canonical.com-20130417152239-u3dd33qfk8dk3b0v
+New revision:  
francesco.banconi@canonical.com-20130417163300-m13r5ffubpbcbwzj

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')

Revision history for this message

Gary Poster (gary) wrote on 2013-04-17:

#

LGTM. Thank you!

Gary

https://codereview.appspot.com/8835043/

Revision history for this message

Nicola Larosa (teknico) wrote on 2013-04-18:

#

LGTM, one trivial.

https://codereview.appspot.com/8835043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8835043/diff/1/test/test_model.js#newcode689
test/test_model.js:689: it('instances identify if they they have
errors', function() {
Double "they".

https://codereview.appspot.com/8835043/diff/1/test/test_model.js#newcode693
test/test_model.js:693: it('instances identify if they they do not have
errors', function() {
Double "they".

https://codereview.appspot.com/8835043/

lp:~frankban/juju-gui/bug-1169973-dying-services updated on 2013-04-18

571. By Francesco Banconi on 2013-04-18: Changes as per review.
572. By Francesco Banconi on 2013-04-18: Merged trunk.

Revision history for this message

Francesco Banconi (frankban) wrote on 2013-04-18:

#

*** Submitted:

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.

R=gary.poster, teknico
CC=
https://codereview.appspot.com/8835043

https://codereview.appspot.com/8835043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8835043/diff/1/test/test_model.js#newcode689
test/test_model.js:689: it('instances identify if they they have
errors', function() {
On 2013/04/18 09:04:02, teknico wrote:
> Double "they".

Done.

https://codereview.appspot.com/8835043/diff/1/test/test_model.js#newcode693
test/test_model.js:693: it('instances identify if they they do not have
errors', function() {
On 2013/04/18 09:04:02, teknico wrote:
> Double "they".

Done.

https://codereview.appspot.com/8835043/

Revision history for this message

Francesco Banconi (frankban) wrote on 2013-04-18:

#

Thanks for the reviews Gary and Nicola!

https://codereview.appspot.com/8835043/

juju-gui

Merge lp:~frankban/juju-gui/bug-1169973-dying-services into lp:juju-gui/experimental

Commit message

Description of the change

Preview Diff

Subscribers