Merge pull request juju#18749 from jack-w-shaw/JUJU-7406_remove_RunAt…

…omic_CreateApplication juju#18749 The application domain was constructed making liberal use of RunAtomic. But we have since decided that this was an anti-pattern, so it should be removed. Re-write a few methods to not be atomic. Starting with CreateApplication, but spilling over into a few others. This was not a trivial lift-and-switch for in a few ways. Particularly of note: 1. CreateApplication now has a multi-variadic parameter of AddUnitParams. This allows us to create an application and add units within the same transaction. A large number of instances of RunAtomic were doing exactly that. 2. InsertCAASUnit was created. The service method RegisterCAASUnit used to call InsertUnit in it's RunAtomic body. However, this body was very complex. So I have added a single method, InsertCAASUnit, on the state layer which now contains all this complexity. 3. Some params/args structs were moved from /domain/application/service up a level to `/domain/application`. This is because without RunAtomic these structs needed to simply be carried down through service to state 4. Some methods like `SetUnitAgentStatus` have been given the suffix `Atomic`. This is to distinguish them from non-atomic copies. The domain is quite tangled and I had to cut changes off somewhere. As a flyby, I have also converted over a large number of errors.Annotate to errors.Errorf, and renamed an `internalerrors` package import to `errors` ## QA steps Juju 4 seems to currently be broken on main (i.e. independently of this), so we can only test bootstrap for the CAAS changes. ``` $ make microk8s-operator-update $ juju bootstrap microk8s mk8s ``` ``` $ juju bootstrap lxd lxd $ juju add-model m $ juju deploy ubuntu $ juju add-unit ubuntu $ juju status ``` ## Links [JUJU-7406](https://warthogs.atlassian.net/browse/JUJU-7406) [JUJU-7406]: https://warthogs.atlassian.net/browse/JUJU-7406?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
manadart · Jan 31, 2025 · cb5dac9 · cb5dac9
2 parents 97c7e29 + 95fe03c
commit cb5dac9
Show file tree

Hide file tree

Showing 18 changed files with 1,403 additions and 1,113 deletions.
diff --git a/apiserver/facades/agent/caasapplication/application.go b/apiserver/facades/agent/caasapplication/application.go
@@ -25,6 +25,7 @@ import (
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/paths"
 	"github.com/juju/juju/core/unit"
+	"github.com/juju/juju/domain/application"
 	applicationerrors "github.com/juju/juju/domain/application/errors"
 	applicationservice "github.com/juju/juju/domain/application/service"
 	"github.com/juju/juju/internal/password"
@@ -39,7 +40,7 @@ type ControllerConfigService interface {
 
 // ApplicationService instances implement an application service.
 type ApplicationService interface {
-	RegisterCAASUnit(ctx context.Context, appName string, unit applicationservice.RegisterCAASUnitParams) error
+	RegisterCAASUnit(ctx context.Context, appName string, unit application.RegisterCAASUnitArg) error
 	CAASUnitTerminating(ctx context.Context, appName string, unitNum int, broker applicationservice.Broker) (bool, error)
 	GetApplicationLife(ctx context.Context, appName string) (life.Value, error)
 	GetUnitLife(ctx context.Context, unitName unit.Name) (life.Value, error)
@@ -205,7 +206,7 @@ func (f *Facade) UnitIntroduction(ctx context.Context, args params.CAASUnitIntro
 	passwordHash := password.AgentPasswordHash(pass)
 	upsert.PasswordHash = &passwordHash
 
-	if err := f.applicationService.RegisterCAASUnit(ctx, appName, applicationservice.RegisterCAASUnitParams{
+	if err := f.applicationService.RegisterCAASUnit(ctx, appName, application.RegisterCAASUnitArg{
 		UnitName:     unitName,
 		ProviderId:   containerID,
 		PasswordHash: passwordHash,

diff --git a/domain/application/modelmigration/import.go b/domain/application/modelmigration/import.go
@@ -127,7 +127,7 @@ func (i *importOperation) Execute(ctx context.Context, model description.Model)
 				arg.PasswordHash = ptr(unit.PasswordHash())
 			}
 			if cc := unit.CloudContainer(); cc != nil {
-				cldContainer := &service.CloudContainerParams{}
+				cldContainer := &application.CloudContainerParams{}
 				cldContainer.Address, cldContainer.AddressOrigin = i.makeAddress(cc.Address())
 				if cc.ProviderId() != "" {
 					cldContainer.ProviderId = cc.ProviderId()

diff --git a/domain/application/modelmigration/import_test.go b/domain/application/modelmigration/import_test.go
@@ -171,7 +171,7 @@ func (s *importSuite) TestApplicationImportWithMinimalCharm(c *gc.C) {
 		c.Assert(args.Units, gc.DeepEquals, []service.ImportUnitArg{{
 			UnitName:     "prometheus/0",
 			PasswordHash: ptr("passwordhash"),
-			CloudContainer: ptr(service.CloudContainerParams{
+			CloudContainer: ptr(application.CloudContainerParams{
 				ProviderId: "provider-id",
 				Address: ptr(network.SpaceAddress{
 					MachineAddress: network.MachineAddress{

diff --git a/domain/application/service/application.go b/domain/application/service/application.go
diff --git a/domain/application/service/application_test.go b/domain/application/service/application_test.go
@@ -60,7 +60,7 @@ func (s *applicationServiceSuite) TestCreateApplication(c *gc.C) {
 	id := applicationtesting.GenApplicationUUID(c)
 	objectStoreUUID := objectstoretesting.GenObjectStoreUUID(c)
 
-	u := application.AddUnitArg{
+	us := []application.AddUnitArg{{
 		UnitName: "ubuntu/666",
 		UnitStatusArg: application.UnitStatusArg{
 			AgentStatus: application.UnitAgentStatusInfo{
@@ -77,7 +77,7 @@ func (s *applicationServiceSuite) TestCreateApplication(c *gc.C) {
 				},
 			},
 		},
-	}
+	}}
 	ch := applicationcharm.Charm{
 		Metadata: applicationcharm.Metadata{
 			Name:  "ubuntu",
@@ -132,8 +132,7 @@ func (s *applicationServiceSuite) TestCreateApplication(c *gc.C) {
 	}
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("caas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{}, nil)
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "ubuntu", app).Return(id, nil)
-	s.state.EXPECT().AddUnits(domaintesting.IsAtomicContextChecker, id, u)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "ubuntu", app, us).Return(id, nil)
 
 	s.charm.EXPECT().Actions().Return(&charm.Actions{})
 	s.charm.EXPECT().Config().Return(&charm.Config{})
@@ -406,7 +405,7 @@ func (s *applicationServiceSuite) TestCreateApplicationError(c *gc.C) {
 	rErr := errors.New("boom")
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("caas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{}, nil)
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "foo", gomock.Any()).Return(id, rErr)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "foo", gomock.Any(), []application.AddUnitArg{}).Return(id, rErr)
 
 	s.charm.EXPECT().Meta().Return(&charm.Meta{
 		Name: "foo",
@@ -440,7 +439,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlock(c *gc.C) {
 
 	id := applicationtesting.GenApplicationUUID(c)
 
-	u := application.AddUnitArg{
+	us := []application.AddUnitArg{{
 		UnitName: "ubuntu/666",
 		UnitStatusArg: application.UnitStatusArg{
 			AgentStatus: application.UnitAgentStatusInfo{
@@ -457,7 +456,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlock(c *gc.C) {
 				},
 			},
 		},
-	}
+	}}
 	ch := applicationcharm.Charm{
 		Metadata: applicationcharm.Metadata{
 			Name:  "foo",
@@ -496,8 +495,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlock(c *gc.C) {
 	}
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("iaas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{}, nil)
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "foo", app).Return(id, nil)
-	s.state.EXPECT().AddUnits(domaintesting.IsAtomicContextChecker, id, u)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "foo", app, us).Return(id, nil)
 
 	s.charm.EXPECT().Actions().Return(&charm.Actions{})
 	s.charm.EXPECT().Config().Return(&charm.Config{})
@@ -546,7 +544,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlockDefaultSource(c *gc.
 
 	id := applicationtesting.GenApplicationUUID(c)
 
-	u := application.AddUnitArg{
+	us := []application.AddUnitArg{{
 		UnitName: "ubuntu/666",
 		UnitStatusArg: application.UnitStatusArg{
 			AgentStatus: application.UnitAgentStatusInfo{
@@ -563,7 +561,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlockDefaultSource(c *gc.
 				},
 			},
 		},
-	}
+	}}
 	ch := applicationcharm.Charm{
 		Metadata: applicationcharm.Metadata{
 			Name:  "foo",
@@ -603,8 +601,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlockDefaultSource(c *gc.
 	}
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("iaas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{DefaultBlockSource: ptr("fast")}, nil)
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "foo", app).Return(id, nil)
-	s.state.EXPECT().AddUnits(domaintesting.IsAtomicContextChecker, id, u)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "foo", app, us).Return(id, nil)
 
 	s.charm.EXPECT().Actions().Return(&charm.Actions{})
 	s.charm.EXPECT().Config().Return(&charm.Config{})
@@ -657,7 +654,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystem(c *gc.C) {
 
 	id := applicationtesting.GenApplicationUUID(c)
 
-	u := application.AddUnitArg{
+	us := []application.AddUnitArg{{
 		UnitName: "ubuntu/666",
 		UnitStatusArg: application.UnitStatusArg{
 			AgentStatus: application.UnitAgentStatusInfo{
@@ -674,7 +671,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystem(c *gc.C) {
 				},
 			},
 		},
-	}
+	}}
 	ch := applicationcharm.Charm{
 		Metadata: applicationcharm.Metadata{
 			Name:  "foo",
@@ -714,8 +711,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystem(c *gc.C) {
 	}
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("iaas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{}, nil)
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "foo", app).Return(id, nil)
-	s.state.EXPECT().AddUnits(domaintesting.IsAtomicContextChecker, id, u)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "foo", app, us).Return(id, nil)
 
 	s.charm.EXPECT().Actions().Return(&charm.Actions{})
 	s.charm.EXPECT().Config().Return(&charm.Config{})
@@ -765,7 +761,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystemDefaultSource(c
 
 	id := applicationtesting.GenApplicationUUID(c)
 
-	u := application.AddUnitArg{
+	us := []application.AddUnitArg{{
 		UnitName: "ubuntu/666",
 		UnitStatusArg: application.UnitStatusArg{
 			AgentStatus: application.UnitAgentStatusInfo{
@@ -782,7 +778,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystemDefaultSource(c
 				},
 			},
 		},
-	}
+	}}
 	ch := applicationcharm.Charm{
 		Metadata: applicationcharm.Metadata{
 			Name:  "foo",
@@ -822,8 +818,7 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystemDefaultSource(c
 	}
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("iaas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{DefaultFilesystemSource: ptr("fast")}, nil)
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "foo", app).Return(id, nil)
-	s.state.EXPECT().AddUnits(domaintesting.IsAtomicContextChecker, id, u)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "foo", app, us).Return(id, nil)
 
 	s.charm.EXPECT().Actions().Return(&charm.Actions{})
 	s.charm.EXPECT().Config().Return(&charm.Config{})
@@ -957,6 +952,8 @@ func (s *applicationServiceSuite) TestCreateWithStorageValidates(c *gc.C) {
 func (s *applicationServiceSuite) TestAddUnits(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
+	appUUID := applicationtesting.GenApplicationUUID(c)
+
 	u := application.AddUnitArg{
 		UnitName: "ubuntu/666",
 		UnitStatusArg: application.UnitStatusArg{
@@ -975,15 +972,14 @@ func (s *applicationServiceSuite) TestAddUnits(c *gc.C) {
 			},
 		},
 	}
-	appID := applicationtesting.GenApplicationUUID(c)
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("caas", nil)
-	s.state.EXPECT().GetApplicationID(domaintesting.IsAtomicContextChecker, "666").Return(appID, nil)
-	s.state.EXPECT().AddUnits(domaintesting.IsAtomicContextChecker, appID, u).Return(nil)
+	s.state.EXPECT().GetApplicationIDByName(gomock.Any(), "ubuntu").Return(appUUID, nil)
+	s.state.EXPECT().AddUnits(gomock.Any(), appUUID, u).Return(nil)
 
 	a := AddUnitArg{
 		UnitName: "ubuntu/666",
 	}
-	err := s.service.AddUnits(context.Background(), "666", a)
+	err := s.service.AddUnits(context.Background(), "ubuntu", a)
 	c.Assert(err, jc.ErrorIsNil)
 }
 
@@ -1056,47 +1052,26 @@ func (s *applicationServiceSuite) TestGetUnitNamesErrors(c *gc.C) {
 func (s *applicationServiceSuite) TestRegisterCAASUnit(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	unitName := coreunit.Name("foo/666")
-
-	s.state.EXPECT().GetApplicationID(domaintesting.IsAtomicContextChecker, "foo").Return("app-id", nil)
-	s.state.EXPECT().GetUnitLife(domaintesting.IsAtomicContextChecker, unitName).Return(life.Alive, nil)
-	s.state.EXPECT().UpdateUnitContainer(domaintesting.IsAtomicContextChecker, unitName, &application.CloudContainer{
-		ProviderId: "provider-id",
-		Address: &application.ContainerAddress{
-			Device: application.ContainerDevice{
-				Name:              `placeholder for "foo/666" cloud container`,
-				DeviceTypeID:      0,
-				VirtualPortTypeID: 0,
-			},
-			Value:       "10.6.6.6",
-			AddressType: 0,
-			Scope:       3,
-			Origin:      1,
-			ConfigType:  1,
-		},
-		Ports: ptr([]string{"666"}),
-	})
-	unitUUID := unittesting.GenUnitUUID(c)
-	s.state.EXPECT().GetUnitUUID(domaintesting.IsAtomicContextChecker, unitName).Return(unitUUID, nil)
-	s.state.EXPECT().SetUnitPassword(domaintesting.IsAtomicContextChecker, unitUUID, application.PasswordInfo{
-		PasswordHash:  "passwordhash",
-		HashAlgorithm: 0,
-	})
+	appUUID := applicationtesting.GenApplicationUUID(c)
 
-	p := RegisterCAASUnitParams{
-		UnitName:     unitName,
+	p := application.RegisterCAASUnitArg{
+		UnitName:     coreunit.Name("foo/666"),
 		PasswordHash: "passwordhash",
 		ProviderId:   "provider-id",
 		Address:      ptr("10.6.6.6"),
 		Ports:        ptr([]string{"666"}),
 		OrderedScale: true,
 		OrderedId:    1,
 	}
+
+	s.state.EXPECT().GetApplicationIDByName(gomock.Any(), "foo").Return(appUUID, nil)
+	s.state.EXPECT().InsertCAASUnit(gomock.Any(), appUUID, p)
+
 	err := s.service.RegisterCAASUnit(context.Background(), "foo", p)
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-var unitParams = RegisterCAASUnitParams{
+var unitParams = application.RegisterCAASUnitArg{
 	UnitName:     coreunit.Name("foo/666"),
 	PasswordHash: "passwordhash",
 	ProviderId:   "provider-id",
@@ -1166,15 +1141,15 @@ func (s *applicationServiceSuite) TestUpdateCAASUnit(c *gc.C) {
 	s.state.EXPECT().GetUnitUUID(domaintesting.IsAtomicContextChecker, unitName).Return(unitUUID, nil)
 
 	now := time.Now()
-	s.state.EXPECT().SetUnitAgentStatus(domaintesting.IsAtomicContextChecker, unitUUID, application.UnitAgentStatusInfo{
+	s.state.EXPECT().SetUnitAgentStatusAtomic(domaintesting.IsAtomicContextChecker, unitUUID, application.UnitAgentStatusInfo{
 		StatusID: application.UnitAgentStatusIdle,
 		StatusInfo: application.StatusInfo{
 			Message: "agent status",
 			Data:    map[string]string{"foo": "bar"},
 			Since:   now,
 		},
 	})
-	s.state.EXPECT().SetUnitWorkloadStatus(domaintesting.IsAtomicContextChecker, unitUUID, application.UnitWorkloadStatusInfo{
+	s.state.EXPECT().SetUnitWorkloadStatusAtomic(domaintesting.IsAtomicContextChecker, unitUUID, application.UnitWorkloadStatusInfo{
 		StatusID: application.UnitWorkloadStatusWaiting,
 		StatusInfo: application.StatusInfo{
 			Message: "workload status",

diff --git a/domain/application/service/migration.go b/domain/application/service/migration.go
@@ -14,7 +14,6 @@ import (
 	"github.com/juju/juju/core/config"
 	"github.com/juju/juju/core/logger"
 	corestorage "github.com/juju/juju/core/storage"
-	"github.com/juju/juju/domain"
 	"github.com/juju/juju/domain/application"
 	"github.com/juju/juju/domain/application/charm"
 	applicationerrors "github.com/juju/juju/domain/application/errors"
@@ -211,19 +210,16 @@ func (s *MigrationService) ImportApplication(ctx context.Context, name string, a
 		unitArgs[i] = arg
 	}
 
-	err = s.st.RunAtomic(ctx, func(ctx domain.AtomicContext) error {
-		appID, err := s.st.CreateApplication(ctx, name, appArg)
-		if err != nil {
-			return errors.Annotatef(err, "creating application %q", name)
-		}
-		for _, arg := range unitArgs {
-			if err := s.st.InsertUnit(ctx, appID, arg); err != nil {
-				return errors.Annotatef(err, "inserting unit %q", arg.UnitName)
-			}
+	appID, err := s.st.CreateApplication(ctx, name, appArg, nil)
+	if err != nil {
+		return errors.Annotatef(err, "creating application %q", name)
+	}
+	for _, arg := range unitArgs {
+		if err := s.st.InsertUnit(ctx, appID, arg); err != nil {
+			return errors.Annotatef(err, "inserting unit %q", arg.UnitName)
 		}
-		return nil
-	})
-	return err
+	}
+	return nil
 }
 
 // RemoveImportedApplication removes an application that was imported. The

diff --git a/domain/application/service/migration_test.go b/domain/application/service/migration_test.go
@@ -20,7 +20,6 @@ import (
 	domaincharm "github.com/juju/juju/domain/application/charm"
 	applicationerrors "github.com/juju/juju/domain/application/errors"
 	domainstorage "github.com/juju/juju/domain/storage"
-	domaintesting "github.com/juju/juju/domain/testing"
 	"github.com/juju/juju/internal/charm"
 	internalcharm "github.com/juju/juju/internal/charm"
 	loggertesting "github.com/juju/juju/internal/logger/testing"
@@ -416,7 +415,7 @@ func (s *migrationServiceSuite) TestImportApplication(c *gc.C) {
 
 	s.state.EXPECT().GetModelType(gomock.Any()).Return("iaas", nil)
 	s.state.EXPECT().StorageDefaults(gomock.Any()).Return(domainstorage.StorageDefaults{}, nil)
-	s.state.EXPECT().InsertUnit(domaintesting.IsAtomicContextChecker, id, u)
+	s.state.EXPECT().InsertUnit(gomock.Any(), id, u)
 	s.charm.EXPECT().Actions().Return(&charm.Actions{})
 	s.charm.EXPECT().Config().Return(&charm.Config{
 		Options: map[string]charm.Option{
@@ -456,7 +455,7 @@ func (s *migrationServiceSuite) TestImportApplication(c *gc.C) {
 			Trust: true,
 		},
 	}
-	s.state.EXPECT().CreateApplication(domaintesting.IsAtomicContextChecker, "ubuntu", args).Return(id, nil)
+	s.state.EXPECT().CreateApplication(gomock.Any(), "ubuntu", args, nil).Return(id, nil)
 
 	unitArg := ImportUnitArg{
 		UnitName:     "ubuntu/666",