Merge lp:~dimitern/juju-core/300-lp-1174610-unit-ids-unique into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

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

[Dimiter Naydenov (dimitern)]

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

[Roger Peppe (rogpeppe)]

On 2014/02/17 16:44:18, dimitern wrote:
> Please take a look.

Looks reasonable, but I'm concerned that it can't be applied
until we have mongo-schema-upgrade capability, which might be
a while off yet.

How about making the schema change backwardly compatible?

For example, when a service is created, we mark it with a "uses global
sequence numbering" flag
(this will be false in already existing services).
When a service is destroyed that doesn't have that flag set, we could
update the sequences collection
with the most recent unit number for that service.
When a unit is created, if allocates it from the service sequence number
unless the "uses global sequence" flag is set
in which case it allocates it using the sequences collection.

In that way, we can remain compatible with the old schema but update to
the new schema over time.

Eventually we can delete the flag and its associated logic when we don't
need to support 1.16
environments (assuming this fix goes into 1.18)

https://codereview.appspot.com/64890044/

[William Reade (fwereade)]

On 2014/02/17 17:04:34, rog wrote:
> On 2014/02/17 16:44:18, dimitern wrote:
> > Please take a look.

> Looks reasonable, but I'm concerned that it can't be applied
> until we have mongo-schema-upgrade capability, which might be
> a while off yet.

> How about making the schema change backwardly compatible?

> For example, when a service is created, we mark it with a "uses global
sequence
> numbering" flag
> (this will be false in already existing services).
> When a service is destroyed that doesn't have that flag set, we could
update the
> sequences collection
> with the most recent unit number for that service.
> When a unit is created, if allocates it from the service sequence
number unless
> the "uses global sequence" flag is set
> in which case it allocates it using the sequences collection.

> In that way, we can remain compatible with the old schema but update
to the new
> schema over time.

> Eventually we can delete the flag and its associated logic when we
don't need to
> support 1.16
> environments (assuming this fix goes into 1.18)

This isn't quite enough. While clients still have direct DB access (ie
until past 1.18) we need to *always* read from, and write to, the old
location, as well as the new one -- and this actually renders any change
pointless, because we could *always* have an old client removing then
creating a new service. We actually *can't* do this yet. Sorry Dimiter,
my enthusiasm for fixing something that annoyed hazmat got me
over-excited and I failed to think it through.

FWIW this *is* exactly what we *should* do once we have mongo fully
locked down...

https://codereview.appspot.com/64890044/

[William Reade (fwereade)]

On 2014/02/18 10:25:23, fwereade wrote:
> On 2014/02/17 17:04:34, rog wrote:
> > On 2014/02/17 16:44:18, dimitern wrote:
> > > Please take a look.
> >
> > Looks reasonable, but I'm concerned that it can't be applied
> > until we have mongo-schema-upgrade capability, which might be
> > a while off yet.
> >
> > How about making the schema change backwardly compatible?
> >
> > For example, when a service is created, we mark it with a "uses
global
> sequence
> > numbering" flag
> > (this will be false in already existing services).
> > When a service is destroyed that doesn't have that flag set, we
could update
> the
> > sequences collection
> > with the most recent unit number for that service.
> > When a unit is created, if allocates it from the service sequence
number
> unless
> > the "uses global sequence" flag is set
> > in which case it allocates it using the sequences collection.
> >
> > In that way, we can remain compatible with the old schema but update
to the
> new
> > schema over time.
> >
> > Eventually we can delete the flag and its associated logic when we
don't need
> to
> > support 1.16
> > environments (assuming this fix goes into 1.18)

> This isn't quite enough. While clients still have direct DB access (ie
until
> past 1.18) we need to *always* read from, and write to, the old
location, as
> well as the new one -- and this actually renders any change pointless,
because
> we could *always* have an old client removing then creating a new
service. We
> actually *can't* do this yet. Sorry Dimiter, my enthusiasm for fixing
something
> that annoyed hazmat got me over-excited and I failed to think it
through.

> FWIW this *is* exactly what we *should* do once we have mongo fully
locked
> down...

so, to be explicit, this sadly does not lgtm

https://codereview.appspot.com/64890044/

[William Reade (fwereade)]

On 2014/02/18 10:25:48, fwereade wrote:
> On 2014/02/18 10:25:23, fwereade wrote:
> > On 2014/02/17 17:04:34, rog wrote:
> > > On 2014/02/17 16:44:18, dimitern wrote:
> > > > Please take a look.
> > >
> > > Looks reasonable, but I'm concerned that it can't be applied
> > > until we have mongo-schema-upgrade capability, which might be
> > > a while off yet.
> > >
> > > How about making the schema change backwardly compatible?
> > >
> > > For example, when a service is created, we mark it with a "uses
global
> > sequence
> > > numbering" flag
> > > (this will be false in already existing services).
> > > When a service is destroyed that doesn't have that flag set, we
could update
> > the
> > > sequences collection
> > > with the most recent unit number for that service.
> > > When a unit is created, if allocates it from the service sequence
number
> > unless
> > > the "uses global sequence" flag is set
> > > in which case it allocates it using the sequences collection.
> > >
> > > In that way, we can remain compatible with the old schema but
update to the
> > new
> > > schema over time.
> > >
> > > Eventually we can delete the flag and its associated logic when we
don't
> need
> > to
> > > support 1.16
> > > environments (assuming this fix goes into 1.18)
> >
> > This isn't quite enough. While clients still have direct DB access
(ie until
> > past 1.18) we need to *always* read from, and write to, the old
location, as
> > well as the new one -- and this actually renders any change
pointless, because
> > we could *always* have an old client removing then creating a new
service. We
> > actually *can't* do this yet. Sorry Dimiter, my enthusiasm for
fixing
> something
> > that annoyed hazmat got me over-excited and I failed to think it
through.
> >
> > FWIW this *is* exactly what we *should* do once we have mongo fully
locked
> > down...

> so, to be explicit, this sadly does not lgtm

(it's a shame to lose the incidental fixes though -- can they be easily
extracted?)

https://codereview.appspot.com/64890044/

=== modified file 'state/open.go'
--- state/open.go	2014-02-07 18:38:02 +0000
+++ state/open.go	2014-02-17 16:43:18 +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}

=== modified file 'state/sequence.go'
--- state/sequence.go	2013-07-09 10:32:23 +0000
+++ state/sequence.go	2014-02-17 16:43:18 +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,

=== modified file 'state/service.go'
--- state/service.go	2014-01-15 06:52:56 +0000
+++ state/service.go	2014-02-17 16:43:18 +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 {

=== 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:43:18 +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)
+	}
+}

=== modified file 'state/state.go'
--- state/state.go	2014-02-05 17:46:02 +0000
+++ state/state.go	2014-02-17 16:43:18 +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