Merge pull request juju#17026 from wallyworld/storage-constraints-fac…

…ades

juju#17026

This PR copies logic used to validate storage and configure default storage when adding an application from the state package to the new storage domain package. The logic is kept the same but is decoupled from state. So the `domain/storage/validation.*` and `domain/storage/defaults.*` are essentially the transplanted code from state.

As well as when adding a new application, the logic is also used when setting a new charm on an application. The validation ensures the storage requested matches what the charm needs. The defaults handling ensures model config for block and filesystem sources is used if nothing is specified.

With this change, the application service needs a storage provider registry to create apps or update the charm, so that is now provided to the service. A NotImplementedStorageRegistry is used for when the app service is used and no registry is needed, eg just removing apps in the cleanup facade.

As well as putting the new defaults/validation logic in place, this PR wires up the application facade to call the domain service methods - aside from using the new validation and defaults logic, nothing is implemented yet to do the persistence, but a follow up will complete the end to end logic so that storage constraints are fully written to dqlite. 

## QA steps

For now, just bootstrap and deploy a charm with storage as a regression check.

## Links

**Jira card:** JUJU-5634

apiserver/facades/agent/caasapplication/application_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/juju/juju/caas"
 	controllerconfigbootstrap "github.com/juju/juju/domain/controllerconfig/bootstrap"
 	"github.com/juju/juju/domain/servicefactory/testing"
+	"github.com/juju/juju/internal/storage/provider"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
 	coretesting "github.com/juju/juju/testing"
@@ -63,7 +64,7 @@ func (s *CAASApplicationSuite) SetUpTest(c *gc.C) {
 		s.authorizer,
 		s.st, s.st,
 		s.ControllerServiceFactory(c).ControllerConfig(),
-		s.DefaultModelServiceFactory(c).Application(),
+		s.DefaultModelServiceFactory(c).Application(provider.CommonStorageProviders()),
 		s.broker,
 		s.clock,
 		loggo.GetLogger("juju.apiserver.caasaplication"),

apiserver/facades/agent/caasapplication/mock_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/juju/juju/controller"
 	jujucontroller "github.com/juju/juju/controller"
 	"github.com/juju/juju/core/network"
+	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/state"
 	jtesting "github.com/juju/juju/testing"
 )
@@ -98,6 +99,11 @@ type mockModel struct {
 	tag           names.Tag
 }
 
+func (st *mockModel) Config() (*config.Config, error) {
+	attr := jtesting.FakeConfig()
+	return config.New(config.UseDefaults, attr)
+}
+
 func (st *mockModel) Containers(providerIds ...string) ([]state.CloudContainer, error) {
 	st.MethodCall(st, "Containers", providerIds)
 	return st.containers, st.NextErr()

apiserver/facades/agent/caasapplication/register.go

@@ -41,13 +41,14 @@ func newStateFacade(ctx facade.ModelContext) (*Facade, error) {
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
+	registry := stateenvirons.NewStorageProviderRegistry(broker)
 	return NewFacade(
 		resources,
 		authorizer,
 		systemState,
 		&stateShim{State: st},
 		serviceFactory.ControllerConfig(),
-		serviceFactory.Application(),
+		serviceFactory.Application(registry),
 		broker,
 		ctx.StatePool().Clock(),
 		ctx.Logger().Child("caasapplication"),

apiserver/facades/client/application/application.go

@@ -44,6 +44,7 @@ import (
 	"github.com/juju/juju/core/permission"
 	"github.com/juju/juju/core/secrets"
 	"github.com/juju/juju/core/status"
+	applicationservice "github.com/juju/juju/domain/application/service"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/bootstrap"
 	environsconfig "github.com/juju/juju/environs/config"
@@ -125,21 +126,23 @@ func newFacadeBase(stdCtx context.Context, ctx facade.ModelContext) (*APIBase, e
 	stateCharm := CharmToStateCharm
 	serviceFactory := ctx.ServiceFactory()
 
-	var (
-		storagePoolGetter StoragePoolGetter
-		registry          storage.ProviderRegistry
-		caasBroker        caas.Broker
+	registry, err := stateenvirons.NewStorageProviderRegistryForModel(
+		m, serviceFactory.Cloud(), serviceFactory.Credential(),
+		stateenvirons.GetNewEnvironFunc(environs.New),
+		stateenvirons.GetNewCAASBrokerFunc(caas.New),
 	)
-	modelType := model.Type()
-	if modelType == state.ModelTypeCAAS {
+	if err != nil {
+		return nil, errors.Annotate(err, "getting storage provider registry")
+	}
+	storagePoolGetter := serviceFactory.Storage(registry)
+
+	var caasBroker caas.Broker
+	if model.Type() == state.ModelTypeCAAS {
 		caasBroker, err = stateenvirons.GetNewCAASBrokerFunc(caas.New)(model, serviceFactory.Cloud(), serviceFactory.Credential())
 		if err != nil {
 			return nil, errors.Annotate(err, "getting caas client")
 		}
-		registry = stateenvirons.NewStorageProviderRegistry(caasBroker)
-		storagePoolGetter = serviceFactory.Storage(registry)
 	}
-
 	resources := ctx.Resources()
 
 	leadershipReader, err := ctx.LeadershipReader()
@@ -181,7 +184,8 @@ func newFacadeBase(stdCtx context.Context, ctx facade.ModelContext) (*APIBase, e
 		state:              state,
 		storagePoolGetter:  storagePoolGetter,
 	}
-	repoDeploy := NewDeployFromRepositoryAPI(state, serviceFactory.Application(), ctx.ObjectStore(), makeDeployFromRepositoryValidator(stdCtx, validatorCfg))
+	applicationService := serviceFactory.Application(registry)
+	repoDeploy := NewDeployFromRepositoryAPI(state, applicationService, ctx.ObjectStore(), makeDeployFromRepositoryValidator(stdCtx, validatorCfg))
 
 	return NewAPIBase(
 		state,
@@ -195,7 +199,7 @@ func newFacadeBase(stdCtx context.Context, ctx facade.ModelContext) (*APIBase, e
 		serviceFactory.Cloud(),
 		serviceFactory.Credential(),
 		serviceFactory.Machine(),
-		serviceFactory.Application(),
+		applicationService,
 		leadershipReader,
 		stateCharm,
 		DeployApplication,
@@ -1103,6 +1107,7 @@ func (api *APIBase) applicationSetCharm(
 		logger.Warningf("proceeding with upgrade of application %q even though the charm feature requirements could not be met as --force was specified", params.AppName)
 	}
 
+	//
 	force := params.Force
 	cfg := state.SetCharmConfig{
 		Charm:              api.stateCharm(newCharm),
@@ -1127,10 +1132,31 @@ func (api *APIBase) applicationSetCharm(
 		return errors.New("cannot downgrade from v2 charm format to v1")
 	}
 
-	// TODO(wallyworld) - do in a single transaction
+	// TODO(dqlite) - remove SetCharm (replaced below with UpdateApplicationCharm).
 	if err := params.Application.SetCharm(cfg, api.store); err != nil {
 		return errors.Annotate(err, "updating charm config")
 	}
+
+	var storageConstraints map[string]storage.Constraints
+	if len(params.StorageConstraints) > 0 {
+		storageConstraints = make(map[string]storage.Constraints)
+		for name, cons := range params.StorageConstraints {
+			sc := storage.Constraints{Pool: cons.Pool}
+			if cons.Size != nil {
+				sc.Size = *cons.Size
+			}
+			if cons.Count != nil {
+				sc.Count = *cons.Count
+			}
+			storageConstraints[name] = sc
+		}
+	}
+	if err := api.applicationService.UpdateApplicationCharm(ctx, params.AppName, applicationservice.UpdateCharmParams{
+		Charm:   newCharm,
+		Storage: storageConstraints,
+	}); err != nil {
+		return errors.Annotatef(err, "updating charm for application %q", params.AppName)
+	}
 	if attr := appConfig.Attributes(); len(attr) > 0 {
 		return params.Application.UpdateApplicationConfig(attr, nil, appSchema, appDefaults)
 	}

apiserver/facades/client/application/application_test.go

@@ -103,7 +103,7 @@ func (s *applicationSuite) makeAPI(c *gc.C) *application.APIBase {
 		serviceFactory.Cloud(),
 		serviceFactory.Credential(),
 		serviceFactory.Machine(),
-		serviceFactory.Application(),
+		serviceFactory.Application(registry),
 		nil, // leadership not used in these tests.
 		application.CharmToStateCharm,
 		application.DeployApplication,

apiserver/facades/client/application/application_unit_test.go

@@ -323,6 +323,11 @@ func (s *ApplicationSuite) TestSetCharm(c *gc.C) {
 	app.EXPECT().SetCharm(setCharmConfigMatcher{c: c, expected: cfg}, s.store)
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",
 		CharmURL:        curl,
@@ -351,6 +356,11 @@ func (s *ApplicationSuite) TestSetCharmEverything(c *gc.C) {
 	app.EXPECT().UpdateApplicationConfig(coreconfig.ConfigAttributes{"trust": true}, nil, schemaFields, defaults)
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err = s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName:    "postgresql",
 		CharmURL:           curl,
@@ -407,6 +417,12 @@ func (s *ApplicationSuite) TestSetCharmForceUnits(c *gc.C) {
 	app.EXPECT().SetCharm(setCharmConfigMatcher{c: c, expected: cfg}, s.store)
 
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
+
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",
 		CharmURL:        curl,
@@ -459,6 +475,16 @@ func (s *ApplicationSuite) TestSetCharmStorageConstraints(c *gc.C) {
 
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm: ch,
+		Storage: map[string]storage.Constraints{
+			"a": {},
+			"b": {Pool: "radiant"},
+			"c": {Size: 123},
+			"d": {Count: 456},
+		},
+	}).Return(nil)
+
 	toUint64Ptr := func(v uint64) *uint64 {
 		return &v
 	}
@@ -533,6 +559,11 @@ func (s *ApplicationSuite) TestSetCharmConfigSettings(c *gc.C) {
 	}, gomock.Any()).Return(nil)
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",
 		CharmURL:        "ch:postgresql",
@@ -577,6 +608,11 @@ func (s *ApplicationSuite) TestSetCharmConfigSettingsYAML(c *gc.C) {
 	}, gomock.Any()).Return(nil)
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",
 		CharmURL:        curl,
@@ -616,6 +652,11 @@ func (s *ApplicationSuite) TestLXDProfileSetCharmWithNewerAgentVersion(c *gc.C)
 
 	s.model.EXPECT().AgentVersion().Return(version.Number{Major: 2, Minor: 6, Patch: 0}, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",
 		CharmURL:        curl,
@@ -669,6 +710,11 @@ func (s *ApplicationSuite) TestLXDProfileSetCharmWithEmptyProfile(c *gc.C) {
 
 	s.model.EXPECT().AgentVersion().Return(version.Number{Major: 2, Minor: 6, Patch: 0}, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",
 		CharmURL:        curl,
@@ -751,6 +797,11 @@ func (s *ApplicationSuite) TestSetCharmAssumesNotSatisfiedWithForce(c *gc.C) {
 	app.EXPECT().SetCharm(gomock.Any(), gomock.Any()).Return(nil)
 	s.backend.EXPECT().Application("postgresql").Return(app, nil)
 
+	s.applicationService.EXPECT().UpdateApplicationCharm(gomock.Any(), "postgresql", applicationservice.UpdateCharmParams{
+		Charm:   ch,
+		Storage: nil,
+	}).Return(nil)
+
 	// Try to upgrade the charm
 	err := s.api.SetCharm(context.Background(), params.ApplicationSetCharm{
 		ApplicationName: "postgresql",

apiserver/facades/client/application/deploy.go

@@ -79,6 +79,7 @@ type MachineService interface {
 type ApplicationService interface {
 	CreateApplication(ctx context.Context, name string, params applicationservice.AddApplicationParams, units ...applicationservice.AddUnitParams) error
 	AddUnits(ctx context.Context, name string, units ...applicationservice.AddUnitParams) error
+	UpdateApplicationCharm(ctx context.Context, name string, params applicationservice.UpdateCharmParams) error
 }
 
 // DeployApplication takes a charm and various parameters and deploys it.
@@ -146,6 +147,7 @@ func DeployApplication(
 		asa.Constraints = args.Constraints
 	}
 
+	// TODO(dqlite) - remove mongo AddApplication call.
 	// To ensure dqlite unit names match those created in mongo, grab the next unit
 	// sequence number before writing the mongo units.
 	nextUnitNum, err := st.ReadSequence(args.ApplicationName)
@@ -158,8 +160,13 @@ func DeployApplication(
 		unitArgs[i].UnitName = &n
 	}
 	app, err := st.AddApplication(asa, store)
+
+	// Dual write storage constraints to dqlite.
 	if err == nil {
-		err = applicationService.CreateApplication(ctx, args.ApplicationName, applicationservice.AddApplicationParams{}, unitArgs...)
+		err = applicationService.CreateApplication(ctx, args.ApplicationName, applicationservice.AddApplicationParams{
+			Charm:   args.Charm,
+			Storage: args.Storage,
+		}, unitArgs...)
 	}
 	return app, errors.Trace(err)
 }