Code review comment for lp:~dimitern/juju-core/300-lp-1174610-unit-ids-unique

Reviewers: mp+206765_code.launchpad.net,

Message:
Please take a look.

Description:
state: Make unit ids unique. Fix #1174610

This changes the way unit names are generated
when adding a new unit to a service. Before,
we used serviceDoc.UnitSeq to generate the
last part of a unit name. Now, we use the
sequence collection with a key "s#<svcname>#unit"
as the unit ids sequence (as we do for machine,
containers, etc.).

This is a schema change (removing of UnitSeq from
the service document). So it should be handled the
same way as any schema change during an upgrade,
but this CL does not do that.

I realized the sequence collection is not created
as all the other collections in state.Initialize,
so I added it there.

Also added a test to guarantee unit ids increase
each time for the same service name (like the bug
suggests and how it was before).

Per fwereade's suggestion, I changed the error
message AddUnit() returns when a service is not
found: "service is no longer alive" rather than
"service "blah" not found", and also fixed a
test that relied on that.

https://code.launchpad.net/~dimitern/juju-core/300-lp-1174610-unit-ids-unique/+merge/206765

(do not edit description out of merge proposal)

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

Affected files (+35, -14 lines):
   A [revision details]
   M state/open.go
   M state/sequence.go
   M state/service.go
   M state/service_test.go
   M state/state.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-20140217133244-ukuzvegy1wqly6nm
+New revision:
<email address hidden>

Index: state/open.go
=== modified file 'state/open.go'
--- state/open.go 2014-02-07 18:38:02 +0000
+++ state/open.go 2014-02-17 16:32:56 +0000
@@ -255,6 +255,7 @@
    annotations: db.C("annotations"),
    statuses: db.C("statuses"),
    stateServers: db.C("stateServers"),
+ sequences: db.C("sequence"),
   }
   log := db.C("txns.log")
   logInfo := mgo.CollectionInfo{Capped: true, MaxBytes: logSize}

Index: state/sequence.go
=== modified file 'state/sequence.go'
--- state/sequence.go 2013-07-09 10:32:23 +0000
+++ state/sequence.go 2014-02-17 16:32:56 +0000
@@ -16,7 +16,7 @@
  }

  func (s *State) sequence(name string) (int, error) {
- query := s.db.C("sequence").Find(D{{"_id", name}})
+ query := s.sequences.FindId(name)
   inc := mgo.Change{
    Update: bson.M{"$inc": bson.M{"counter": 1}},
    Upsert: true,

Index: state/service.go
=== modified file 'state/service.go'
--- state/service.go 2014-01-15 06:52:56 +0000
+++ state/service.go 2014-02-17 16:32:56 +0000
@@ -37,7 +37,6 @@
   CharmURL *charm.URL
   ForceCharm bool
   Life Life
- UnitSeq int
   UnitCount int
   RelationCount int
   Exposed bool
@@ -529,15 +528,11 @@

  // newUnitName returns the next unit name.
  func (s *Service) newUnitName() (string, error) {
- change := mgo.Change{Update: D{{"$inc", D{{"unitseq", 1}}}}}
- result := serviceDoc{}
- if _, err := s.st.services.Find(D{{"_id", s.doc.Name}}).Apply(change,
&result); err == mgo.ErrNotFound {
- return "", errors.NotFoundf("service %q", s)
- } else if err != nil {
- return "", fmt.Errorf("cannot increment unit sequence: %v", err)
+ seq, err := s.st.sequence(s.globalKey() + "#unit")
+ if err != nil {
+ return "", err
   }
- name := s.doc.Name + "/" + strconv.Itoa(result.UnitSeq)
- return name, nil
+ return s.doc.Name + "/" + strconv.Itoa(seq), nil
  }

  // addUnitOps returns a unique name for a new unit, and a list of txn
operations
@@ -591,7 +586,7 @@
    })
   } else {
    scons, err := s.Constraints()
- if err != nil {
+ if err != nil && !errors.IsNotFoundError(err) {
     return "", nil, err
    }
    econs, err := s.st.EnvironConstraints()
@@ -631,7 +626,7 @@
    if alive, err := isAlive(s.st.services, s.doc.Name); err != nil {
     return nil, err
    } else if !alive {
- return nil, fmt.Errorf("service is not alive")
+ return nil, fmt.Errorf("service is no longer alive")
    }
    return nil, fmt.Errorf("inconsistent state")
   } else if err != nil {

Index: state/service_test.go
=== modified file 'state/service_test.go'
--- state/service_test.go 2014-01-15 06:52:56 +0000
+++ state/service_test.go 2014-02-17 16:32:56 +0000
@@ -6,6 +6,7 @@
  import (
   "fmt"
   "sort"
+ "strconv"

   "labix.org/v2/mgo"
   gc "launchpad.net/gocheck"
@@ -815,13 +816,13 @@
   err = s.mysql.Destroy()
   c.Assert(err, gc.IsNil)
   _, err = s.mysql.AddUnit()
- c.Assert(err, gc.ErrorMatches, `cannot add unit to service "mysql":
service is not alive`)
+ c.Assert(err, gc.ErrorMatches, `cannot add unit to service "mysql":
service is no longer alive`)
   err = u.EnsureDead()
   c.Assert(err, gc.IsNil)
   err = u.Remove()
   c.Assert(err, gc.IsNil)
   _, err = s.mysql.AddUnit()
- c.Assert(err, gc.ErrorMatches, `cannot add unit to service "mysql":
service "mysql" not found`)
+ c.Assert(err, gc.ErrorMatches, `cannot add unit to service "mysql":
service is no longer alive`)
  }

  func (s *ServiceSuite) TestReadUnit(c *gc.C) {
@@ -1429,3 +1430,24 @@
   c.Assert(state.GetServiceOwnerTag(service), gc.Equals, "")
   c.Assert(service.GetOwnerTag(), gc.Equals, "user-admin")
  }
+
+func (s *ServiceSuite) TestUnitIdsAreUnique(c *gc.C) {
+ for i, charmName := range []string{"wordpress", "riak", "dummy"} {
+ // Add a new charm and service with the same name.
+ charm := s.AddTestingCharm(c, charmName)
+ service := s.AddTestingService(c, "myservice", charm)
+
+ // Add a unit and check the id increases each time.
+ unit, err := service.AddUnit()
+ c.Check(err, gc.IsNil)
+ c.Check(unit.Name(), gc.Equals, "myservice/"+strconv.Itoa(i))
+
+ // Destroy the unit and service and retry.
+ err = unit.Destroy()
+ c.Check(err, gc.IsNil)
+ c.Check(unit.Refresh(), jc.Satisfies, errors.IsNotFoundError)
+ err = service.Destroy()
+ c.Check(err, gc.IsNil)
+ c.Check(service.Refresh(), jc.Satisfies, errors.IsNotFoundError)
+ }
+}

Index: state/state.go
=== modified file 'state/state.go'
--- state/state.go 2014-02-05 17:46:02 +0000
+++ state/state.go 2014-02-17 16:32:56 +0000
@@ -64,6 +64,7 @@
   annotations *mgo.Collection
   statuses *mgo.Collection
   stateServers *mgo.Collection
+ sequences *mgo.Collection
   runner *txn.Runner
   transactionHooks chan ([]transactionHook)
   watcher *watcher.Watcher