state: add safe refcounts for charm storage

This branch adds refcounts for charm storage: when a storage instance is added, a refcount is incremented for (owner, storage-name); and decremented when the storage instance is removed. We also check that no units are added to or removed from an application while its charm is being upgraded. This enables us to safely drop charm storage if it is unreferenced. These changes pave the way for upgrading charms with storage, and make validation of charm-storage min/max count constraints simpler. Previously we were relying on there being a one-to-one mapping between storage instances and attachments, which leaves shared storage out in the cold; this new approach caters for both.
hatched · Sep 21, 2016 · 1a498cf · 1a498cf
1 parent 7332454
commit 1a498cf
Show file tree

Hide file tree

Showing 6 changed files with 276 additions and 194 deletions.
diff --git a/featuretests/storage_test.go b/featuretests/storage_test.go
@@ -521,7 +521,9 @@ func (s *cmdStorageSuite) TestStorageAddToUnitStorageDoesntExist(c *gc.C) {
 	context, err := runAddToUnit(c, u, "nonstorage=1")
 	c.Assert(errors.Cause(err), gc.ErrorMatches, "cmd: error out silently")
 	c.Assert(testing.Stdout(context), gc.Equals, "")
-	c.Assert(testing.Stderr(context), gc.Equals, "failed to add \"nonstorage\": charm storage \"nonstorage\" not found\n")
+	c.Assert(testing.Stderr(context), gc.Equals,
+		`failed to add "nonstorage": adding storage to unit storage-block/0: charm storage "nonstorage" not found`+"\n",
+	)
 
 	instancesAfter, err := s.State.AllStorageInstances()
 	c.Assert(err, jc.ErrorIsNil)

diff --git a/state/application.go b/state/application.go
@@ -452,15 +452,11 @@ func (s *Application) checkRelationsOps(ch *Charm, relations []*Relation) ([]txn
 	return asserts, nil
 }
 
-func (s *Application) checkStorageUpgrade(newMeta *charm.Meta) (_ []txn.Op, err error) {
+func (s *Application) checkStorageUpgrade(newMeta, oldMeta *charm.Meta, units []*Unit) (_ []txn.Op, err error) {
 	defer errors.DeferredAnnotatef(&err, "cannot upgrade application %q to charm %q", s, newMeta.Name)
-	ch, _, err := s.Charm()
-	if err != nil {
-		return nil, errors.Trace(err)
-	}
-	oldMeta := ch.Meta()
+
+	// Make sure no storage instances are added or removed.
 	var ops []txn.Op
-	var units []*Unit
 	for name, oldStorageMeta := range oldMeta.Storage {
 		if _, ok := newMeta.Storage[name]; ok {
 			continue
@@ -472,70 +468,24 @@ func (s *Application) checkStorageUpgrade(newMeta *charm.Meta) (_ []txn.Op, err
 		// are no instances of the store, it can safely be
 		// removed.
 		if oldStorageMeta.Shared {
-			n, err := s.st.countEntityStorageInstancesForName(
-				s.Tag(), name,
-			)
+			op, n, err := s.st.countEntityStorageInstances(s.Tag(), name)
 			if err != nil {
 				return nil, errors.Trace(err)
 			}
 			if n > 0 {
 				return nil, errors.Errorf("in-use storage %q removed", name)
 			}
-			// TODO(axw) if/when it is possible to
-			// add shared storage instance to an
-			// application post-deployment, we must
-			// include a txn.Op here that asserts
-			// that the number of instances is zero.
+			ops = append(ops, op)
 		} else {
-			if units == nil {
-				var err error
-				units, err = s.AllUnits()
+			for _, u := range units {
+				op, n, err := s.st.countEntityStorageInstances(u.Tag(), name)
 				if err != nil {
 					return nil, errors.Trace(err)
 				}
-				ops = append(ops, txn.Op{
-					C:      applicationsC,
-					Id:     s.doc.DocID,
-					Assert: bson.D{{"unitcount", len(units)}},
-				})
-				for _, unit := range units {
-					// Here we check that the storage
-					// attachment count remains the same.
-					// To get around the ABA problem, we
-					// also add ops for the individual
-					// attachments below.
-					ops = append(ops, txn.Op{
-						C:  unitsC,
-						Id: unit.doc.DocID,
-						Assert: bson.D{{
-							"storageattachmentcount",
-							unit.doc.StorageAttachmentCount,
-						}},
-					})
-				}
-			}
-			for _, unit := range units {
-				attachments, err := s.st.UnitStorageAttachments(unit.UnitTag())
-				if err != nil {
-					return nil, errors.Trace(err)
-				}
-				for _, attachment := range attachments {
-					storageTag := attachment.StorageInstance()
-					storageName, err := names.StorageName(storageTag.Id())
-					if err != nil {
-						return nil, errors.Trace(err)
-					}
-					if storageName == name {
-						return nil, errors.Errorf("in-use storage %q removed", name)
-					}
-					// We assert that other storage attachments still exist to
-					// avoid the ABA problem.
-					ops = append(ops, txn.Op{
-						C:      storageAttachmentsC,
-						Id:     storageAttachmentId(unit.Name(), storageTag.Id()),
-						Assert: txn.DocExists,
-					})
+				if n > 0 {
+					return nil, errors.Errorf("in-use storage %q removed", name)
 				}
+				ops = append(ops, op)
 			}
 		}
 	}
@@ -682,12 +632,36 @@ func (s *Application) changeCharmOps(
 		}
 	}
 
+	// Make sure no units are added or removed while the upgrade
+	// transaction is being executed. This allows us to make
+	// changes to units during the upgrade, e.g. add storage
+	// to existing units, or remove optional storage so long as
+	// it is unreferenced.
+	units, err := s.AllUnits()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	unitOps := make([]txn.Op, len(units))
+	for i, u := range units {
+		unitOps[i] = txn.Op{
+			C:      unitsC,
+			Id:     u.doc.DocID,
+			Assert: txn.DocExists,
+		}
+	}
+	unitOps = append(unitOps, txn.Op{
+		C:      applicationsC,
+		Id:     s.doc.DocID,
+		Assert: bson.D{{"unitcount", len(units)}},
+	})
+
 	// Build the transaction.
 	var ops []txn.Op
 	if oldSettings != nil {
 		// Old settings shouldn't change (when they exist).
 		ops = append(ops, oldSettings.assertUnchangedOp())
 	}
+	ops = append(ops, unitOps...)
 	ops = append(ops, incOps...)
 	ops = append(ops, []txn.Op{
 		// Create or replace new settings.
@@ -761,9 +735,8 @@ func (s *Application) changeCharmOps(
 		return nil, errors.Trace(err)
 	}
 
-	// Check storage to ensure no referenced storage is removed, or changed
-	// in an incompatible way.
-	storageOps, err := s.checkStorageUpgrade(ch.Meta())
+	// Upgrade charm storage.
+	storageOps, err := s.upgradeStorageOps(ch.Meta(), units, newStorageConstraints)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -773,6 +746,25 @@ func (s *Application) changeCharmOps(
 	return append(ops, decOps...), nil
 }
 
+func (a *Application) upgradeStorageOps(
+	meta *charm.Meta,
+	units []*Unit,
+	allStorageCons map[string]StorageConstraints,
+) ([]txn.Op, error) {
+	// Check storage to ensure no referenced storage is removed, or changed
+	// in an incompatible way.
+	oldCharm, _, err := a.Charm()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	oldMeta := oldCharm.Meta()
+	ops, err := a.checkStorageUpgrade(meta, oldMeta, units)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return ops, nil
+}
+
 // incCharmModifiedVersionOps returns the operations necessary to increment
 // the CharmModifiedVersion field for the given service.
 func incCharmModifiedVersionOps(serviceID string) []txn.Op {

diff --git a/state/charmref.go b/state/charmref.go
@@ -41,17 +41,17 @@ func appCharmIncRefOps(st modelBackend, appName string, curl *charm.URL, canCrea
 		getIncRefOp = nsRefcounts.StrictIncRefOp
 	}
 	settingsKey := applicationSettingsKey(appName, curl)
-	settingsOp, err := getIncRefOp(refcounts, settingsKey)
+	settingsOp, err := getIncRefOp(refcounts, settingsKey, 1)
 	if err != nil {
 		return nil, errors.Annotate(err, "settings reference")
 	}
 	storageConstraintsKey := applicationStorageConstraintsKey(appName, curl)
-	storageConstraintsOp, err := getIncRefOp(refcounts, storageConstraintsKey)
+	storageConstraintsOp, err := getIncRefOp(refcounts, storageConstraintsKey, 1)
 	if err != nil {
 		return nil, errors.Annotate(err, "storage constraints reference")
 	}
 	charmKey := charmGlobalKey(curl)
-	charmOp, err := getIncRefOp(refcounts, charmKey)
+	charmOp, err := getIncRefOp(refcounts, charmKey, 1)
 	if err != nil {
 		return nil, errors.Annotate(err, "charm reference")
 	}

diff --git a/state/migration_import.go b/state/migration_import.go
@@ -1340,6 +1340,16 @@ func (i *importer) addStorageInstance(storage description.Storage) error {
 		Assert: txn.DocMissing,
 		Insert: doc,
 	})
+
+	refcounts, closer := i.st.getCollection(refcountsC)
+	defer closer()
+	storageRefcountKey := entityStorageRefcountKey(owner, storage.Name())
+	incRefOp, err := nsRefcounts.CreateOrIncRefOp(refcounts, storageRefcountKey, 1)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	ops = append(ops, incRefOp)
+
 	if err := i.st.runTransaction(ops); err != nil {
 		return errors.Trace(err)
 	}

diff --git a/state/refcounts_ns.go b/state/refcounts_ns.go
@@ -68,26 +68,26 @@ func (ns nsRefcounts_) StrictCreateOp(coll mongo.Collection, key string, value i
 }
 
 // CreateOrIncrefOp returns a txn.Op that creates a refcount document as
-// configured with a value of 1; or increments any such refcount doc
+// configured with a specified value; or increments any such refcount doc
 // that already exists.
-func (ns nsRefcounts_) CreateOrIncRefOp(coll mongo.Collection, key string) (txn.Op, error) {
+func (ns nsRefcounts_) CreateOrIncRefOp(coll mongo.Collection, key string, n int) (txn.Op, error) {
 	if exists, err := ns.exists(coll, key); err != nil {
 		return txn.Op{}, errors.Trace(err)
 	} else if !exists {
-		return ns.JustCreateOp(coll.Name(), key, 1), nil
+		return ns.JustCreateOp(coll.Name(), key, n), nil
 	}
-	return ns.JustIncRefOp(coll.Name(), key), nil
+	return ns.JustIncRefOp(coll.Name(), key, n), nil
 }
 
 // StrictIncRefOp returns a txn.Op that increments the value of a
 // refcount doc, or returns an error if it does not exist.
-func (ns nsRefcounts_) StrictIncRefOp(coll mongo.Collection, key string) (txn.Op, error) {
+func (ns nsRefcounts_) StrictIncRefOp(coll mongo.Collection, key string, n int) (txn.Op, error) {
 	if exists, err := ns.exists(coll, key); err != nil {
 		return txn.Op{}, errors.Trace(err)
 	} else if !exists {
 		return txn.Op{}, errors.New("does not exist")
 	}
-	return ns.JustIncRefOp(coll.Name(), key), nil
+	return ns.JustIncRefOp(coll.Name(), key, n), nil
 }
 
 // AliveDecRefOp returns a txn.Op that decrements the value of a
@@ -132,6 +132,29 @@ func (ns nsRefcounts_) RemoveOp(coll mongo.Collection, key string, value int) (t
 	return ns.JustRemoveOp(coll.Name(), key, value), nil
 }
 
+// CurrentOp returns the current reference count value, and a txn.Op that
+// asserts that the refcount has that value, or an error. If the refcount
+// doc does not exist, then the op will assert that the document does not
+// exist instead, and no error is returned.
+func (ns nsRefcounts_) CurrentOp(coll mongo.Collection, key string) (txn.Op, int, error) {
+	refcount, err := ns.read(coll, key)
+	if errors.IsNotFound(err) {
+		return txn.Op{
+			C:      coll.Name(),
+			Id:     key,
+			Assert: txn.DocMissing,
+		}, 0, nil
+	}
+	if err != nil {
+		return txn.Op{}, -1, errors.Trace(err)
+	}
+	return txn.Op{
+		C:      coll.Name(),
+		Id:     key,
+		Assert: bson.D{{"refcount", refcount}},
+	}, refcount, nil
+}
+
 // JustCreateOp returns a txn.Op that creates a refcount document as
 // configured, *without* checking database state for sanity first.
 // You should avoid using this method in most cases.
@@ -145,14 +168,14 @@ func (nsRefcounts_) JustCreateOp(collName, key string, value int) txn.Op {
 }
 
 // JustIncRefOp returns a txn.Op that increments a refcount document by
-// 1, as configured, *without* checking database state for sanity first.
-// You should avoid using this method in most cases.
-func (nsRefcounts_) JustIncRefOp(collName, key string) txn.Op {
+// the specified amount, as configured, *without* checking database state
+// for sanity first. You should avoid using this method in most cases.
+func (nsRefcounts_) JustIncRefOp(collName, key string, n int) txn.Op {
 	return txn.Op{
 		C:      collName,
 		Id:     key,
 		Assert: txn.DocExists,
-		Update: bson.D{{"$inc", bson.D{{"refcount", 1}}}},
+		Update: bson.D{{"$inc", bson.D{{"refcount", n}}}},
 	}
 }
 
@@ -198,7 +221,7 @@ func (nsRefcounts_) exists(coll mongo.Collection, key string) (bool, error) {
 func (nsRefcounts_) read(coll mongo.Collection, key string) (int, error) {
 	var doc refcountDoc
 	if err := coll.FindId(key).One(&doc); err == mgo.ErrNotFound {
-		return 0, errors.NotFoundf("refcount")
+		return 0, errors.NotFoundf("refcount %q", key)
 	} else if err != nil {
 		return 0, errors.Trace(err)
 	}