Merge lp:~fwereade/juju-core/fix-1233457-for-1.16 into lp:juju-core/1.16

Proposed by William Reade on 2013-11-13
Status: Merged
Approved by: William Reade on 2013-11-13
Approved revision: 1983
Merged at revision: 1983
Proposed branch: lp:~fwereade/juju-core/fix-1233457-for-1.16
Merge into: lp:juju-core/1.16
Diff against target: 120 lines (+80/-3)
3 files modified
state/service.go (+7/-3)
state/unit.go (+22/-0)
state/unit_test.go (+51/-0)
To merge this branch: bzr merge lp:~fwereade/juju-core/fix-1233457-for-1.16
Reviewer Review Type Date Requested Status
Juju Engineering 2013-11-13 Pending
Review via email: mp+195003@code.launchpad.net

Commit message

cherrypick r2042 from trunk

fixes lp:1233457

https://codereview.appspot.com/25920043/

Description of the change

cherrypick r2042 from trunk

fixes lp:1233457

https://codereview.appspot.com/25920043/

To post a comment you must log in.
William Reade (fwereade) wrote :
Download full text (5.0 KiB)

Reviewers: mp+195003_code.launchpad.net,

Message:
Please take a look.

Description:
cherrypick r2042 from trunk

fixes lp:1233457

https://code.launchpad.net/~fwereade/juju-core/fix-1233457-for-1.16/+merge/195003

(do not edit description out of merge proposal)

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

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-20131104221242-cvjcmqbdcnln1c50
+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-13 08:47:41 +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-10-02 13:14:49 +0000
+++ state/unit.go 2013-11-13 08:47:41 +0000
@@ -387,6 +387,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-09-27 15:33:10 +0000
+++ state/unit_test.go 2013-11-13 08:47:41 +0000
@@ -1121,6 +1121,57 @@
   c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
  }

+func (s *UnitSuite) TestRemovePathologicalWithBuggyUniter(c *gc.C) {
+ // Add a relation ...

Read more...

John A Meinel (jameinel) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/service.go'
2--- state/service.go 2013-07-30 14:00:53 +0000
3+++ state/service.go 2013-11-13 08:51:00 +0000
4@@ -718,14 +718,18 @@
5
6 // Relations returns a Relation for every relation the service is in.
7 func (s *Service) Relations() (relations []*Relation, err error) {
8- defer utils.ErrorContextf(&err, "can't get relations for service %q", s)
9+ return serviceRelations(s.st, s.doc.Name)
10+}
11+
12+func serviceRelations(st *State, name string) (relations []*Relation, err error) {
13+ defer utils.ErrorContextf(&err, "can't get relations for service %q", name)
14 docs := []relationDoc{}
15- err = s.st.relations.Find(D{{"endpoints.servicename", s.doc.Name}}).All(&docs)
16+ err = st.relations.Find(D{{"endpoints.servicename", name}}).All(&docs)
17 if err != nil {
18 return nil, err
19 }
20 for _, v := range docs {
21- relations = append(relations, newRelation(s.st, &v))
22+ relations = append(relations, newRelation(st, &v))
23 }
24 return relations, nil
25 }
26
27=== modified file 'state/unit.go'
28--- state/unit.go 2013-10-02 13:14:49 +0000
29+++ state/unit.go 2013-11-13 08:51:00 +0000
30@@ -387,6 +387,28 @@
31 if u.doc.Life != Dead {
32 return stderrors.New("unit is not dead")
33 }
34+
35+ // Now the unit is Dead, we can be sure that it's impossible for it to
36+ // enter relation scopes (once it's Dying, we can be sure of this; but
37+ // EnsureDead does not require that it already be Dying, so this is the
38+ // only point at which we can safely backstop lp:1233457 and mitigate
39+ // the impact of unit agent bugs that leave relation scopes occupied).
40+ relations, err := serviceRelations(u.st, u.doc.Service)
41+ if err != nil {
42+ return err
43+ }
44+ for _, rel := range relations {
45+ ru, err := rel.Unit(u)
46+ if err != nil {
47+ return err
48+ }
49+ if err := ru.LeaveScope(); err != nil {
50+ return err
51+ }
52+ }
53+
54+ // Now we're sure we haven't left any scopes occupied by this unit, we
55+ // can safely remove the document.
56 unit := &Unit{st: u.st, doc: u.doc}
57 for i := 0; i < 5; i++ {
58 switch ops, err := unit.removeOps(isDeadDoc); err {
59
60=== modified file 'state/unit_test.go'
61--- state/unit_test.go 2013-09-27 15:33:10 +0000
62+++ state/unit_test.go 2013-11-13 08:51:00 +0000
63@@ -1121,6 +1121,57 @@
64 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
65 }
66
67+func (s *UnitSuite) TestRemovePathologicalWithBuggyUniter(c *gc.C) {
68+ // Add a relation between wordpress and mysql...
69+ wordpress := s.service
70+ wordpress0 := s.unit
71+ mysql, err := s.State.AddService("mysql", s.AddTestingCharm(c, "mysql"))
72+ c.Assert(err, gc.IsNil)
73+ eps, err := s.State.InferEndpoints([]string{"wordpress", "mysql"})
74+ c.Assert(err, gc.IsNil)
75+ rel, err := s.State.AddRelation(eps...)
76+ c.Assert(err, gc.IsNil)
77+
78+ // The relation holds a reference to wordpress, but that can't keep
79+ // wordpress from being removed -- because the relation will be removed
80+ // if we destroy wordpress.
81+ // However, if a unit of the *other* service joins the relation, that
82+ // will add an additional reference and prevent the relation -- and
83+ // thus wordpress itself -- from being removed when its last unit is.
84+ mysql0, err := mysql.AddUnit()
85+ c.Assert(err, gc.IsNil)
86+ mysql0ru, err := rel.Unit(mysql0)
87+ c.Assert(err, gc.IsNil)
88+ err = mysql0ru.EnterScope(nil)
89+ c.Assert(err, gc.IsNil)
90+
91+ // Destroy wordpress, and remove its last unit.
92+ err = wordpress.Destroy()
93+ c.Assert(err, gc.IsNil)
94+ err = wordpress0.EnsureDead()
95+ c.Assert(err, gc.IsNil)
96+ err = wordpress0.Remove()
97+ c.Assert(err, gc.IsNil)
98+
99+ // Check this didn't kill the service or relation yet...
100+ err = wordpress.Refresh()
101+ c.Assert(err, gc.IsNil)
102+ err = rel.Refresh()
103+ c.Assert(err, gc.IsNil)
104+
105+ // ...and that when the malfunctioning unit agent on the other side
106+ // sets itself to dead *without* departing the relation, the unit's
107+ // removal causes the relation and the other service to be cleaned up.
108+ err = mysql0.EnsureDead()
109+ c.Assert(err, gc.IsNil)
110+ err = mysql0.Remove()
111+ c.Assert(err, gc.IsNil)
112+ err = wordpress.Refresh()
113+ c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
114+ err = rel.Refresh()
115+ c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
116+}
117+
118 func (s *UnitSuite) TestWatchSubordinates(c *gc.C) {
119 w := s.unit.WatchSubordinateUnits()
120 defer testing.AssertStop(c, w)

Subscribers

People subscribed via source and target branches

to all changes: