state: Track charm URLs using a sequence

Instead of relying on dead charm documents, use a sequence to track used charm URL revisions.
hatched · Jan 29, 2017 · 7c67bcb · 7c67bcb
1 parent ddccea7
commit 7c67bcb
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 53 deletions.
diff --git a/state/charm.go b/state/charm.go
@@ -52,14 +52,7 @@ type charmDoc struct {
 
 	// Life manages charm lifetime in the usual way, but only local
 	// charms can actually be "destroyed"; store charms are
-	// immortal. When a local charm is removed, its document is left
-	// in place, with Life set to Dead, to ensure we don't
-	// accidentally reuse the charm URL, which must be unique within
-	// a model.
-	//
-	// Note that this aligns with the existing contract implied by
-	// Dead: that most clients should see it as not existing at all.
-	// Nothing strictly obliges us to clean up the doc.
+	// immortal.
 	Life Life `bson:"life"`
 
 	// These fields are flags; if any of them is set, the charm
@@ -535,18 +528,25 @@ func (st *State) AddCharm(info CharmInfo) (stch *Charm, err error) {
 		return nil, errors.Trace(err)
 	}
 
-	query := charms.FindId(info.ID.String()).Select(bson.D{{"placeholder", 1}})
+	query := charms.FindId(info.ID.String()).Select(bson.M{"placeholder": 1})
 
 	buildTxn := func(attempt int) ([]txn.Op, error) {
-		var placeholderDoc struct {
+		var doc struct {
 			Placeholder bool `bson:"placeholder"`
 		}
-		if err := query.One(&placeholderDoc); err == mgo.ErrNotFound {
-
+		if err := query.One(&doc); err == mgo.ErrNotFound {
+			if info.ID.Schema == "local" {
+				curl, err := st.PrepareLocalCharmUpload(info.ID)
+				if err != nil {
+					return nil, errors.Trace(err)
+				}
+				info.ID = curl
+				return updateCharmOps(st, info, stillPending)
+			}
 			return insertCharmOps(st, info)
 		} else if err != nil {
 			return nil, errors.Trace(err)
-		} else if placeholderDoc.Placeholder {
+		} else if doc.Placeholder {
 			return updateCharmOps(st, info, stillPlaceholder)
 		}
 		return nil, errors.AlreadyExistsf("charm %q", info.ID)
@@ -651,42 +651,23 @@ func (st *State) PrepareLocalCharmUpload(curl *charm.URL) (chosenURL *charm.URL,
 	if curl.Revision < 0 {
 		return nil, errors.Errorf("expected charm URL with revision, got %q", curl)
 	}
-	// Get a regex with the charm URL and no revision.
-	noRevURL := curl.WithRevision(-1)
-	curlRegex := "^" + regexp.QuoteMeta(st.docID(noRevURL.String()))
-
-	charms, closer := st.getCollection(charmsC)
-	defer closer()
 
-	buildTxn := func(attempt int) ([]txn.Op, error) {
-		// Find the highest revision of that charm in state.
-		var docs []charmDoc
-		query := bson.D{{"_id", bson.D{{"$regex", curlRegex}}}}
-		err = charms.Find(query).Select(bson.D{{"_id", 1}, {"url", 1}}).All(&docs)
-		if err != nil {
-			return nil, errors.Trace(err)
-		}
-		// Find the highest revision.
-		maxRevision := -1
-		for _, doc := range docs {
-			if doc.URL.Revision > maxRevision {
-				maxRevision = doc.URL.Revision
-			}
-		}
+	revisionSeq := "charmrev-" + curl.WithRevision(-1).String()
+	revision, err := st.sequenceWithMin(revisionSeq, curl.Revision)
+	if err != nil {
+		return nil, errors.Annotate(err, "unable to allocate charm revision")
+	}
+	allocatedURL := curl.WithRevision(revision)
 
-		// Respect the local charm's revision first.
-		chosenRevision := curl.Revision
-		if maxRevision >= chosenRevision {
-			// More recent revision exists in state, pick the next.
-			chosenRevision = maxRevision + 1
-		}
-		chosenURL = curl.WithRevision(chosenRevision)
-		return insertPendingCharmOps(st, chosenURL)
+	ops, err := insertPendingCharmOps(st, allocatedURL)
+	if err != nil {
+		return nil, errors.Trace(err)
 	}
-	if err = st.run(buildTxn); err == nil {
-		return chosenURL, nil
+
+	if err := st.runTransaction(ops); err != nil {
+		return nil, errors.Trace(err)
 	}
-	return nil, errors.Trace(err)
+	return allocatedURL, nil
 }
 
 // PrepareStoreCharmUpload must be called before a charm store charm

diff --git a/state/charmref.go b/state/charmref.go
@@ -120,10 +120,6 @@ func finalAppCharmRemoveOps(appName string, curl *charm.URL) []txn.Op {
 func charmDestroyOps(st modelBackend, curl *charm.URL) ([]txn.Op, error) {
 
 	if curl.Schema != "local" {
-		// local charms keep a document around to prevent reuse
-		// of charm URLs, which several components believe to be
-		// unique keys (this is always true within a model).
-		//
 		// it's not so much that it's bad to delete store
 		// charms; but we don't have a way to reinstate them
 		// once purged, so we don't allow removal in the first
@@ -162,10 +158,9 @@ func charmRemoveOps(st modelBackend, curl *charm.URL) ([]txn.Op, error) {
 	charms, closer := st.getCollection(charmsC)
 	defer closer()
 
-	// NOTE: we do *not* actually remove the charm document, to
-	// prevent its URL from being recycled, and breaking caches.
-	// The "remove" terminology refers to the client's view of the
-	// change (after which the charm really will be inaccessible).
+	// NOTE: we don't actually remove the charm document.  The
+	// "remove" terminology refers to the client's view of the change
+	// (after which the charm really will be inaccessible).
 	charmKey := curl.String()
 	charmOp, err := nsLife.dieOp(charms, charmKey, nil)
 	if err != nil {