Merge pull request juju#18755 from jack-w-shaw/JUJU-7406_de-atomise_S…

…etApplicationLife

juju#18755

Starting off with a simple example for reviewers.

Refactor SetApplicationLife to not be a RunAtomic method.

## QA steps

```
$ juju bootstrap lxd lxd
$ juju add-model m
$ juju deploy ubuntu
(wait)
$ juju remove-application ubuntu
```

## Links

[JUJU-7406](https://warthogs.atlassian.net/browse/JUJU-7406)

[JUJU-7406]: https://warthogs.atlassian.net/browse/JUJU-7406?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

domain/application/service/application.go

@@ -102,9 +102,6 @@ type AtomicApplicationState interface {
 	// found.
 	GetApplicationLife(ctx domain.AtomicContext, appName string) (coreapplication.ID, life.Life, error)
 
-	// SetApplicationLife sets the life of the specified application.
-	SetApplicationLife(domain.AtomicContext, coreapplication.ID, life.Life) error
-
 	// GetApplicationScaleState looks up the scale state of the specified
 	// application, returning an error satisfying
 	// [applicationerrors.ApplicationNotFound] if the application is not found.
@@ -202,6 +199,9 @@ type ApplicationState interface {
 	// application; the application name is used to filter.
 	GetApplicationUnitLife(ctx context.Context, appName string, unitUUIDs ...coreunit.UUID) (map[coreunit.UUID]life.Life, error)
 
+	// SetApplicationLife sets the life of the specified application.
+	SetApplicationLife(context.Context, coreapplication.ID, life.Life) error
+
 	// GetCharmByApplicationID returns the charm, charm origin and charm
 	// platform for the specified application ID.
 	//
@@ -1027,35 +1027,33 @@ func (s *Service) deleteApplication(ctx domain.AtomicContext, name string) ([]fu
 // returning an error  satisfying [applicationerrors.ApplicationNotFoundError]
 // if the application doesn't exist.
 func (s *Service) DestroyApplication(ctx context.Context, appName string) error {
+	appID, err := s.st.GetApplicationIDByName(ctx, appName)
+	if errors.Is(err, applicationerrors.ApplicationNotFound) {
+		return nil
+	} else if err != nil {
+		return internalerrors.Errorf("getting application ID: %w", err)
+	}
 	// For now, all we do is advance the application's life to Dying.
-	err := s.st.RunAtomic(ctx, func(ctx domain.AtomicContext) error {
-		appID, err := s.st.GetApplicationID(ctx, appName)
-		if errors.Is(err, applicationerrors.ApplicationNotFound) {
-			return nil
-		}
-		if err != nil {
-			return errors.Trace(err)
-		}
-		return s.st.SetApplicationLife(ctx, appID, life.Dying)
-	})
-	return errors.Annotatef(err, "destroying application %q", appName)
+	err = s.st.SetApplicationLife(ctx, appID, life.Dying)
+	if err != nil {
+		return internalerrors.Errorf("destroying application %q: %w", appName, err)
+	}
+	return nil
 }
 
-// EnsureApplicationDead is called by the cleanup worker if a mongo
+// MarkApplicationDead is called by the cleanup worker if a mongo
 // destroy operation sets the application to dead.
 // TODO(units): remove when everything is in dqlite.
-func (s *Service) EnsureApplicationDead(ctx context.Context, appName string) error {
-	err := s.st.RunAtomic(ctx, func(ctx domain.AtomicContext) error {
-		appID, err := s.st.GetApplicationID(ctx, appName)
-		if errors.Is(err, applicationerrors.ApplicationNotFound) {
-			return nil
-		}
-		if err != nil {
-			return errors.Trace(err)
-		}
-		return s.st.SetApplicationLife(ctx, appID, life.Dead)
-	})
-	return errors.Annotatef(err, "setting application %q life to Dead", appName)
+func (s *Service) MarkApplicationDead(ctx context.Context, appName string) error {
+	appID, err := s.st.GetApplicationIDByName(ctx, appName)
+	if err != nil {
+		return internalerrors.Errorf("getting application ID: %w", err)
+	}
+	err = s.st.SetApplicationLife(ctx, appID, life.Dead)
+	if err != nil {
+		return internalerrors.Errorf("setting application %q life to Dead: %w", appName, err)
+	}
+	return nil
 }
 
 // UpdateApplicationCharm sets a new charm for the application, validating that aspects such

domain/application/service_test.go

@@ -28,6 +28,7 @@ import (
 	"github.com/juju/juju/domain/application/service"
 	"github.com/juju/juju/domain/application/state"
 	"github.com/juju/juju/domain/ipaddress"
+	domainlife "github.com/juju/juju/domain/life"
 	"github.com/juju/juju/domain/schema/testing"
 	domainsecret "github.com/juju/juju/domain/secret"
 	secretstate "github.com/juju/juju/domain/secret/state"
@@ -181,16 +182,16 @@ func (s *serviceSuite) TestDeleteApplicationNotFound(c *gc.C) {
 	c.Assert(err, jc.ErrorIs, applicationerrors.ApplicationNotFound)
 }
 
-func (s *serviceSuite) TestEnsureApplicationDead(c *gc.C) {
+func (s *serviceSuite) TestMarkApplicationDead(c *gc.C) {
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
 
 	s.createApplication(c, "foo")
 
-	err := s.svc.EnsureApplicationDead(context.Background(), "foo")
+	err := s.svc.MarkApplicationDead(context.Background(), "foo")
 	c.Assert(err, jc.ErrorIsNil)
 
-	var gotLife int
+	var gotLife domainlife.Life
 	err = s.TxnRunner().StdTxn(context.Background(), func(ctx context.Context, tx *sql.Tx) error {
 		err := tx.QueryRowContext(ctx, "SELECT life_id FROM application WHERE name = ?", "foo").
 			Scan(&gotLife)
@@ -200,15 +201,15 @@ func (s *serviceSuite) TestEnsureApplicationDead(c *gc.C) {
 		return nil
 	})
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(gotLife, gc.Equals, 2)
+	c.Assert(gotLife, gc.Equals, domainlife.Dead)
 }
 
-func (s *serviceSuite) TestEnsureApplicationDeadNotFound(c *gc.C) {
+func (s *serviceSuite) TestMarkApplicationDeadNotFound(c *gc.C) {
 	ctrl := gomock.NewController(c)
 	defer ctrl.Finish()
 
-	err := s.svc.EnsureApplicationDead(context.Background(), "foo")
-	c.Assert(err, jc.ErrorIsNil)
+	err := s.svc.MarkApplicationDead(context.Background(), "foo")
+	c.Assert(err, jc.ErrorIs, applicationerrors.ApplicationNotFound)
 }
 
 func (s *serviceSuite) TestGetUnitLife(c *gc.C) {

domain/application/state/application.go

@@ -1259,7 +1259,12 @@ WHERE name = $applicationDetails.name
 }
 
 // SetApplicationLife sets the life of the specified application.
-func (st *State) SetApplicationLife(ctx domain.AtomicContext, appUUID coreapplication.ID, l life.Life) error {
+func (st *State) SetApplicationLife(ctx context.Context, appUUID coreapplication.ID, l life.Life) error {
+	db, err := st.DB()
+	if err != nil {
+		return errors.Trace(err)
+	}
+
 	lifeQuery := `
 UPDATE application
 SET life_id = $applicationIDAndLife.life_id
@@ -1273,11 +1278,14 @@ AND life_id <= $applicationIDAndLife.life_id
 		return errors.Trace(err)
 	}
 
-	err = domain.Run(ctx, func(ctx context.Context, tx *sqlair.TX) error {
-		err := tx.Query(ctx, lifeStmt, app).Run()
-		return errors.Trace(err)
+	err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
+		err = tx.Query(ctx, lifeStmt, app).Run()
+		if err != nil {
+			return internalerrors.Errorf("updating application life for %q: %w", appUUID, err)
+		}
+		return nil
 	})
-	return errors.Annotatef(err, "updating application life for %q", appUUID)
+	return errors.Trace(err)
 }
 
 // SetDesiredApplicationScale updates the desired scale of the specified

domain/application/state/application_test.go

@@ -1546,6 +1546,7 @@ func (s *applicationStateSuite) TestSetApplicationScalingState(c *gc.C) {
 
 func (s *applicationStateSuite) TestSetApplicationLife(c *gc.C) {
 	appID := s.createApplication(c, "foo", life.Alive)
+	ctx := context.Background()
 
 	checkResult := func(want life.Life) {
 		var gotLife life.Life
@@ -1558,22 +1559,16 @@ func (s *applicationStateSuite) TestSetApplicationLife(c *gc.C) {
 		c.Assert(gotLife, jc.DeepEquals, want)
 	}
 
-	err := s.state.RunAtomic(context.Background(), func(ctx domain.AtomicContext) error {
-		return s.state.SetApplicationLife(ctx, appID, life.Dying)
-	})
+	err := s.state.SetApplicationLife(ctx, appID, life.Dying)
 	c.Assert(err, jc.ErrorIsNil)
 	checkResult(life.Dying)
 
-	err = s.state.RunAtomic(context.Background(), func(ctx domain.AtomicContext) error {
-		return s.state.SetApplicationLife(ctx, appID, life.Dead)
-	})
+	err = s.state.SetApplicationLife(ctx, appID, life.Dead)
 	c.Assert(err, jc.ErrorIsNil)
 	checkResult(life.Dead)
 
 	// Can't go backwards.
-	err = s.state.RunAtomic(context.Background(), func(ctx domain.AtomicContext) error {
-		return s.state.SetApplicationLife(ctx, appID, life.Dying)
-	})
+	err = s.state.SetApplicationLife(ctx, appID, life.Dying)
 	c.Assert(err, jc.ErrorIsNil)
 	checkResult(life.Dead)
 }

domain/application/state/state.go

@@ -1306,33 +1306,29 @@ WHERE name = $applicationDetails.name
 // returned.
 func (st *State) checkApplicationExists(ctx context.Context, tx *sqlair.TX, ident applicationID) error {
 	query := `
-SELECT &applicationIDAndLife.*
+SELECT &applicationLife.*
 FROM application
 WHERE uuid = $applicationID.uuid;
 `
-	stmt, err := st.Prepare(query, ident, applicationIDAndLife{})
+	stmt, err := st.Prepare(query, ident, applicationLife{})
 	if err != nil {
 		return internalerrors.Errorf("preparing query for application %q: %w", ident.ID, err)
 	}
 
-	var result applicationIDAndLife
+	var result applicationLife
 	err = tx.Query(ctx, stmt, ident).Get(&result)
 	if errors.Is(err, sql.ErrNoRows) {
 		return applicationerrors.ApplicationNotFound
 	} else if err != nil {
 		return internalerrors.Errorf("checking application %q exists: %w", ident.ID, err)
 	}
 
-	// If we're not alive or dying, then the application is dead.
-	if result.LifeID > domainlife.Dying {
+	switch result.LifeID {
+	case domainlife.Dead:
 		return applicationerrors.ApplicationIsDead
+	default:
+		return nil
 	}
-
-	if err := result.ID.Validate(); err != nil {
-		return internalerrors.Errorf("invalid application ID %q: %w", result.ID, err)
-	}
-
-	return nil
 }
 
 func decodeCharmState(state charmState) (charm.Charm, error) {

domain/application/state/types.go

@@ -26,17 +26,22 @@ type KeyValue struct {
 	Value string `db:"value"`
 }
 
-// applicationID is used to get the ID (and life) of an application.
+// applicationID is used to get the ID of an application.
 type applicationID struct {
 	ID coreapplication.ID `db:"uuid"`
 }
 
-// applicationIDAndLife is used to get the ID (and life) of an application.
+// applicationIDAndLife is used to get the ID and life of an application.
 type applicationIDAndLife struct {
 	ID     coreapplication.ID `db:"uuid"`
 	LifeID life.Life          `db:"life_id"`
 }
 
+// applicationLife is used to get the life of an application.
+type applicationLife struct {
+	LifeID life.Life `db:"life_id"`
+}
+
 type applicationChannel struct {
 	ApplicationID coreapplication.ID `db:"application_uuid"`
 	Track         string             `db:"track"`

state/cleanup.go

@@ -176,7 +176,7 @@ type MachineRemover interface {
 type ApplicationAndUnitRemover interface {
 	DestroyApplication(context.Context, string) error
 	DeleteApplication(context.Context, string) error
-	EnsureApplicationDead(ctx context.Context, appName string) error
+	MarkApplicationDead(ctx context.Context, appName string) error
 	EnsureUnitDead(context.Context, coreunit.Name, leadership.Revoker) error
 	DestroyUnit(context.Context, coreunit.Name) error
 	DeleteUnit(context.Context, coreunit.Name) error
@@ -629,7 +629,7 @@ func (st *State) cleanupApplication(ctx context.Context, store objectstore.Objec
 			err = appService.DeleteApplication(ctx, appName)
 		}
 		if op.PostDestroyAppLife == Dead {
-			err = appService.EnsureApplicationDead(ctx, appName)
+			err = appService.MarkApplicationDead(ctx, appName)
 		}
 	}
 	return err