Merge lp:~fwereade/juju-core/unit-remove-depart-scopes into lp:~go-bot/juju-core/trunk

[William Reade]

state: unit removal departs relation scopes

Fixes lp:1233457

https://codereview.appspot.com/23080045/

state: unit removal departs relation scopes

Fixes lp:1233457

https://codereview.appspot.com/23080045/

[Ian Booth (wallyworld)]

Very nice. I love the well commented test.

review: Approve

[William Reade (fwereade)]

Reviewers: mp+194389_code.launchpad.net,

Message:
Please take a look.

Description:
state: unit removal departs relation scopes

Fixes lp:1233457

https://code.launchpad.net/~fwereade/juju-core/unit-remove-depart-scopes/+merge/194389

(do not edit description out of merge proposal)

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

Affected files (+82, -3 lines):
   A [revision details]
   M state/service.go
   M state/unit.go
   M state/unit_test.go

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: tarmac-20131107094222-rb2jpeg465spb0qa
+New revision: <email address hidden>

Index: state/service.go
=== modified file 'state/service.go'
--- state/service.go 2013-07-30 14:00:53 +0000
+++ state/service.go 2013-11-07 17:30:14 +0000
@@ -718,14 +718,18 @@

  // Relations returns a Relation for every relation the service is in.
  func (s *Service) Relations() (relations []*Relation, err error) {
- defer utils.ErrorContextf(&err, "can't get relations for service %q", s)
+ return serviceRelations(s.st, s.doc.Name)
+}
+
+func serviceRelations(st *State, name string) (relations []*Relation, err
error) {
+ defer utils.ErrorContextf(&err, "can't get relations for service %q",
name)
   docs := []relationDoc{}
- err = s.st.relations.Find(D{{"endpoints.servicename",
s.doc.Name}}).All(&docs)
+ err = st.relations.Find(D{{"endpoints.servicename", name}}).All(&docs)
   if err != nil {
    return nil, err
   }
   for _, v := range docs {
- relations = append(relations, newRelation(s.st, &v))
+ relations = append(relations, newRelation(st, &v))
   }
   return relations, nil
  }

Index: state/unit.go
=== modified file 'state/unit.go'
--- state/unit.go 2013-11-07 06:44:41 +0000
+++ state/unit.go 2013-11-07 17:30:38 +0000
@@ -418,6 +418,28 @@
   if u.doc.Life != Dead {
    return stderrors.New("unit is not dead")
   }
+
+ // Now the unit is Dead, we can be sure that it's impossible for it to
+ // enter relation scopes (once it's Dying, we can be sure of this; but
+ // EnsureDead does not require that it already be Dying, so this is the
+ // only point at which we can safely backstop lp:1233457 and mitigate
+ // the impact of unit agent bugs that leave relation scopes occupied).
+ relations, err := serviceRelations(u.st, u.doc.Service)
+ if err != nil {
+ return err
+ }
+ for _, rel := range relations {
+ ru, err := rel.Unit(u)
+ if err != nil {
+ return err
+ }
+ if err := ru.LeaveScope(); err != nil {
+ return err
+ }
+ }
+
+ // Now we're sure we haven't left any scopes occupied by this unit, we
+ // can safely remove the document.
   unit := &Unit{st: u.st, doc: u.doc}
   for i := 0; i < 5; i++ {
    switch ops, err := unit.removeOps(isDeadDoc); err {

Index: state/unit_test.go
=== modified file 'state/unit_test.go'
--- state/unit_test.go 2013-11-05 07:19:16 +0000
+++ state/unit_test.go 2013-11-07 17:30:38 +0000
@@ -1127,6 +1127,57 @@
   c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
  }

+func (s *UnitSuite) TestRemovePathologicalWithBuggyUniter(c *gc.C) {...

[John A Meinel (jameinel)]

Approving this for you so that it will hopefully make the follow up patch easier to review once this has landed.

review: Approve

[John A Meinel (jameinel)]

FWIW, this patch is small enough that it seems like it might be something that could be backported to 1.16. I don't know if it is worthwhile right now, but maybe something to consider.

=== modified file 'state/service.go'
--- state/service.go	2013-07-30 14:00:53 +0000
+++ state/service.go	2013-11-07 17:37:25 +0000
@@ -718,14 +718,18 @@
 
 // Relations returns a Relation for every relation the service is in.
 func (s *Service) Relations() (relations []*Relation, err error) {
-	defer utils.ErrorContextf(&err, "can't get relations for service %q", s)
+	return serviceRelations(s.st, s.doc.Name)
+}
+
+func serviceRelations(st *State, name string) (relations []*Relation, err error) {
+	defer utils.ErrorContextf(&err, "can't get relations for service %q", name)
 	docs := []relationDoc{}
-	err = s.st.relations.Find(D{{"endpoints.servicename", s.doc.Name}}).All(&docs)
+	err = st.relations.Find(D{{"endpoints.servicename", name}}).All(&docs)
 	if err != nil {
 		return nil, err
 	}
 	for _, v := range docs {
-		relations = append(relations, newRelation(s.st, &v))
+		relations = append(relations, newRelation(st, &v))
 	}
 	return relations, nil
 }
 
=== modified file 'state/unit.go'
--- state/unit.go	2013-11-07 06:44:41 +0000
+++ state/unit.go	2013-11-07 17:37:25 +0000
@@ -418,6 +418,28 @@
 	if u.doc.Life != Dead {
 		return stderrors.New("unit is not dead")
 	}
+
+	// Now the unit is Dead, we can be sure that it's impossible for it to
+	// enter relation scopes (once it's Dying, we can be sure of this; but
+	// EnsureDead does not require that it already be Dying, so this is the
+	// only point at which we can safely backstop lp:1233457 and mitigate
+	// the impact of unit agent bugs that leave relation scopes occupied).
+	relations, err := serviceRelations(u.st, u.doc.Service)
+	if err != nil {
+		return err
+	}
+	for _, rel := range relations {
+		ru, err := rel.Unit(u)
+		if err != nil {
+			return err
+		}
+		if err := ru.LeaveScope(); err != nil {
+			return err
+		}
+	}
+
+	// Now we're sure we haven't left any scopes occupied by this unit, we
+	// can safely remove the document.
 	unit := &Unit{st: u.st, doc: u.doc}
 	for i := 0; i < 5; i++ {
 		switch ops, err := unit.removeOps(isDeadDoc); err {
 
=== modified file 'state/unit_test.go'
--- state/unit_test.go	2013-11-05 07:19:16 +0000
+++ state/unit_test.go	2013-11-07 17:37:25 +0000
@@ -1127,6 +1127,57 @@
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 }
 
+func (s *UnitSuite) TestRemovePathologicalWithBuggyUniter(c *gc.C) {
+	// Add a relation between wordpress and mysql...
+	wordpress := s.service
+	wordpress0 := s.unit
+	mysql, err := s.State.AddService("mysql", s.AddTestingCharm(c, "mysql"))
+	c.Assert(err, gc.IsNil)
+	eps, err := s.State.InferEndpoints([]string{"wordpress", "mysql"})
+	c.Assert(err, gc.IsNil)
+	rel, err := s.State.AddRelation(eps...)
+	c.Assert(err, gc.IsNil)
+
+	// The relation holds a reference to wordpress, but that can't keep
+	// wordpress from being removed -- because the relation will be removed
+	// if we destroy wordpress.
+	// However, if a unit of the *other* service joins the relation, that
+	// will add an additional reference and prevent the relation -- and
+	// thus wordpress itself -- from being removed when its last unit is.
+	mysql0, err := mysql.AddUnit()
+	c.Assert(err, gc.IsNil)
+	mysql0ru, err := rel.Unit(mysql0)
+	c.Assert(err, gc.IsNil)
+	err = mysql0ru.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+
+	// Destroy wordpress, and remove its last unit.
+	err = wordpress.Destroy()
+	c.Assert(err, gc.IsNil)
+	err = wordpress0.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	err = wordpress0.Remove()
+	c.Assert(err, gc.IsNil)
+
+	// Check this didn't kill the service or relation yet...
+	err = wordpress.Refresh()
+	c.Assert(err, gc.IsNil)
+	err = rel.Refresh()
+	c.Assert(err, gc.IsNil)
+
+	// ...and that when the malfunctioning unit agent on the other side
+	// sets itself to dead *without* departing the relation, the unit's
+	// removal causes the relation and the other service to be cleaned up.
+	err = mysql0.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	err = mysql0.Remove()
+	c.Assert(err, gc.IsNil)
+	err = wordpress.Refresh()
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+	err = rel.Refresh()
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+}
+
 func (s *UnitSuite) TestWatchSubordinates(c *gc.C) {
 	w := s.unit.WatchSubordinateUnits()
 	defer testing.AssertStop(c, w)