Add tests for backend refCount fixes;

state/migration_import_tasks.go

@@ -617,7 +617,6 @@ func (ImportSecrets) Execute(src SecretsInput, runner TransactionRunner, knownSe
 				}
 				if !utils.IsValidUUIDString(valueRef.BackendID) && !seenBackendIds.Contains(valueRef.BackendID) {
 					if !knownSecretBackends.Contains(valueRef.BackendID) {
-						logger.Criticalf("target controller does not have secret backend %q, seenBackendIds %v", valueRef.BackendID, seenBackendIds.Values())
 						return errors.New("target controller does not have all required secret backends set up")
 					}
 					ops = append(ops, txn.Op{

state/migration_import_test.go

@@ -3111,6 +3111,7 @@ func (s *MigrationImportSuite) TestSecrets(c *gc.C) {
 		},
 	})
 	c.Assert(err, jc.ErrorIsNil)
+
 	err = s.State.GrantSecretAccess(uri, state.SecretAccessParams{
 		LeaderToken: &fakeToken{},
 		Scope:       owner.Tag(),
@@ -3139,8 +3140,16 @@ func (s *MigrationImportSuite) TestSecrets(c *gc.C) {
 	})
 	c.Assert(err, jc.ErrorIsNil)
 
+	backendRefCount, err := s.State.ReadBackendRevisionCount(backendID)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 1)
+
 	_, newSt := s.importModel(c, s.State)
 
+	backendRefCount, err = s.State.ReadBackendRevisionCount(backendID)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 2)
+
 	store = state.NewSecrets(newSt)
 	all, err := store.ListSecrets(state.SecretsFilter{})
 	c.Assert(err, jc.ErrorIsNil)
@@ -3187,6 +3196,10 @@ func (s *MigrationImportSuite) TestSecrets(c *gc.C) {
 		CurrentRevision: 667,
 		LatestRevision:  2,
 	})
+
+	backendRefCount, err = newSt.ReadBackendRevisionCount(backendID)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 2)
 }
 
 func (s *MigrationImportSuite) TestSecretsMissingBackend(c *gc.C) {

state/package_test.go

@@ -6,6 +6,7 @@ package state
 import (
 	"testing"
 
+	"github.com/juju/errors"
 	"github.com/juju/mgo/v3/bson"
 	"github.com/juju/mgo/v3/txn"
 	jc "github.com/juju/testing/checkers"
@@ -105,3 +106,12 @@ func MustCloseUnitPortRanges(c *gc.C, st *State, machine *Machine, unitName, end
 	}
 	c.Assert(st.ApplyOperation(machPortRanges.Changes()), jc.ErrorIsNil)
 }
+
+func (st *State) ReadBackendRevisionCount(backendID string) (int, error) {
+	refCountCollection, ccloser := st.db().GetCollection(globalRefcountsC)
+	defer ccloser()
+
+	key := secretBackendRefCountKey(backendID)
+	_, count, err := nsRefcounts.CurrentOp(refCountCollection, key)
+	return count, errors.Trace(err)
+}

state/secrets.go

@@ -807,8 +807,6 @@ func (s *secretsStore) ChangeSecretBackend(arg ChangeSecretBackendParams) error
 	}
 
 	buildTxn := func(attempt int) ([]txn.Op, error) {
-		// !!!!!!
-
 		var ops []txn.Op
 		if doc.ValueRef != nil {
 			op, err := s.st.decSecretBackendRefCountOp(doc.ValueRef.BackendID)

state/secrets_test.go

@@ -934,32 +934,54 @@ func (s *SecretsSuite) TestChangeSecretBackend(c *gc.C) {
 		UpdateSecretParams: state.UpdateSecretParams{
 			LeaderToken: &fakeToken{},
 			Data:        map[string]string{"foo": "bar"},
+			ValueRef: &secrets.ValueRef{
+				BackendID:  "old-backend-id",
+				RevisionID: "rev-id",
+			},
 		},
 	}
 	_, err := s.store.CreateSecret(uri, p)
 	c.Assert(err, jc.ErrorIsNil)
 
+	backendRefCount, err := s.State.ReadBackendRevisionCount("old-backend-id")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 1)
+	backendRefCount, err = s.State.ReadBackendRevisionCount("new-backend-id")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 0)
+
 	val, valRef, err := s.store.GetSecretValue(uri, 1)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(val, jc.DeepEquals, secrets.NewSecretValue(map[string]string{"foo": "bar"}))
-	c.Assert(valRef, gc.IsNil)
+	c.Assert(valRef, gc.DeepEquals, &secrets.ValueRef{
+		BackendID:  "old-backend-id",
+		RevisionID: "rev-id",
+	})
 
 	err = s.store.ChangeSecretBackend(state.ChangeSecretBackendParams{
 		URI:      uri,
 		Token:    &fakeToken{},
 		Revision: 1,
 		ValueRef: &secrets.ValueRef{
-			BackendID:  "backend-id",
+			BackendID:  "new-backend-id",
 			RevisionID: "rev-id",
 		},
 	})
 	c.Assert(err, jc.ErrorIsNil)
 
+	backendRefCount, err = s.State.ReadBackendRevisionCount("old-backend-id")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 0)
+
+	backendRefCount, err = s.State.ReadBackendRevisionCount("new-backend-id")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(backendRefCount, gc.Equals, 1)
+
 	val, valRef, err = s.store.GetSecretValue(uri, 1)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(val.IsEmpty(), jc.IsTrue)
 	c.Assert(valRef, jc.DeepEquals, &secrets.ValueRef{
-		BackendID:  "backend-id",
+		BackendID:  "new-backend-id",
 		RevisionID: "rev-id",
 	})
 }

state/state.go

@@ -24,7 +24,6 @@ import (
 	jujutxn "github.com/juju/txn/v3"
 	"github.com/juju/utils/v3"
 	"github.com/juju/version/v2"
-	"github.com/kr/pretty"
 
 	"github.com/juju/juju/core/arch"
 	corecharm "github.com/juju/juju/core/charm"
@@ -268,7 +267,6 @@ func cleanupSecretBackendRefCountAfterModelMigrationDone(st *State) error {
 	}
 
 	var ops []txn.Op
-	logger.Criticalf("cleanupSecretBackendRefCountAfterModelMigrationDone: %s", pretty.Sprint(result))
 	for _, r := range result {
 		for i := r.Count; i > 0; i-- {
 			refCountOps, err := st.decSecretBackendRefCountOp(r.ID)
@@ -278,7 +276,6 @@ func cleanupSecretBackendRefCountAfterModelMigrationDone(st *State) error {
 			ops = append(ops, refCountOps)
 		}
 	}
-	logger.Criticalf("cleanupSecretBackendRefCountAfterModelMigrationDone: ops %s", pretty.Sprint(ops))
 	return st.db().RunTransaction(ops)
 }
 

worker/secretdrainworker/worker.go

@@ -177,11 +177,10 @@ func (w *Worker) drainSecret(md coresecrets.SecretMetadataForDrain, client jujus
 			if err != nil {
 				return errors.Trace(err)
 			}
-			// revID := rev.ValueRef.RevisionID
 			cleanUpInExternalBackend = func() error {
 				w.config.Logger.Debugf("cleanup secret %s/%d from old backend %q", md.Metadata.URI.ID, rev.Revision, rev.ValueRef.BackendID)
 				if valueRef.BackendID == rev.ValueRef.BackendID {
-					// Ideally, We should have done all these drain steps in the controller via transaction but we decided to only allow
+					// Ideally, We should have done all these drain steps in the controller via transaction, but by design, we only allow
 					// uniters to be able to access secret content. So we have to do these extra checks to avoid
 					// secret gets deleted wrongly when the model's secret backend is changed back to
 					// the old backend while the secret is being drained.

worker/secretdrainworker/worker_test.go

@@ -277,16 +277,19 @@ func (s *workerSuite) TestDrainPartiallyFailed(c *gc.C) {
 				},
 			},
 		}, nil),
+
+		// revision 1
 		s.backendClient.EXPECT().GetBackend(nil).DoAndReturn(func(_ *string) (*provider.SecretsBackend, string, error) {
 			return nil, "backend-3", nil
 		}),
-
-		// revision 1
 		s.backendClient.EXPECT().GetRevisionContent(uri, 1).Return(secretValue, nil),
 		s.backendClient.EXPECT().SaveContent(uri, 1, secretValue).Return(coresecrets.ValueRef{}, errors.NotSupportedf("")),
 		s.backendClient.EXPECT().GetBackend(ptr("backend-1")).Return(oldBackend, "", nil),
 
 		// revision 2
+		s.backendClient.EXPECT().GetBackend(nil).DoAndReturn(func(_ *string) (*provider.SecretsBackend, string, error) {
+			return nil, "backend-3", nil
+		}),
 		s.backendClient.EXPECT().GetRevisionContent(uri, 2).Return(secretValue, nil),
 		s.backendClient.EXPECT().SaveContent(uri, 2, secretValue).Return(coresecrets.ValueRef{}, errors.NotSupportedf("")),
 		s.backendClient.EXPECT().GetBackend(ptr("backend-2")).Return(oldBackend, "", nil),
@@ -314,7 +317,7 @@ func (s *workerSuite) TestDrainPartiallyFailed(c *gc.C) {
 			return nil
 		}),
 	)
-	start(`failed to drain secret revisions for "secret:.*" to the active backend "backend-3"`)
+	start(`failed to drain secret revisions for "secret:.*" to the active backend`)
 }
 
 func ptr[T any](v T) *T {