Merge pull request juju#17043 from SimonRichardson/model-table-schema

juju#17043

This creates a model table schema for model databases. The model table will be a read-only table. You can't update it or create a new entry in the table, only one row can exist at the same time. The table is a denormalized view of the model_metadata table in the controller database. It is not expected to use this table for foreign keys as the data is not linked via UUIDs, this is cached data from the controller database.

Triggers and constraints are used to ensure that we only allow one row at a time and no modifications can be met. Deletion is currently ignored, but that can also be fixed with a deletion trigger, making it truly read-only (that's a step too far IMO).

I've wired up bootstrap and model migration, with model creation via the model manager left. That will require some thought on how that's possible.

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing

## QA steps

```sh
$ juju bootstrap lxd test
$ juju add-model default
```

## Links

**Jira card:** JUJU-5643

agent/agentbootstrap/bootstrap.go

@@ -263,6 +263,7 @@ func (b *AgentBootstrap) Initialize(ctx stdcontext.Context) (_ *state.Controller
 			controllerModelCreateFunc,
 		),
 		database.BootstrapModelConcern(controllerModelUUID,
+			modelbootstrap.CreateReadOnlyModel(controllerModelArgs.AsReadOnly()),
 			modelconfigbootstrap.SetModelConfig(stateParams.ControllerModelConfig, controllerModelDefaults),
 		),
 	}

core/modelmigration/coordinator.go

@@ -29,7 +29,7 @@ func (b *BaseOperation) Execute(context.Context, description.Model) error {
 }
 
 // Rollback is a no-op by default.
-func (b *BaseOperation) Rollback(context.Context) error {
+func (b *BaseOperation) Rollback(context.Context, description.Model) error {
 	return nil
 }
 
@@ -54,7 +54,7 @@ type Operation interface {
 	// Rollback should only be called on controller DB operations. The
 	// model DB operations are not rolled back, but instead we remove the
 	// db, clearing the model.
-	Rollback(context.Context) error
+	Rollback(context.Context, description.Model) error
 }
 
 // Scope is a collection of database txn runners that can be used by the
@@ -121,7 +121,7 @@ func (m *Coordinator) Perform(ctx context.Context, scope Scope, model descriptio
 	defer func() {
 		if err != nil {
 			for ; current >= 0; current-- {
-				if rollbackErr := m.operations[current].Rollback(ctx); rollbackErr != nil {
+				if rollbackErr := m.operations[current].Rollback(ctx, model); rollbackErr != nil {
 					err = errors.Annotatef(err, "rollback operation at %d with %v", current, rollbackErr)
 				}
 			}

core/modelmigration/coordinator_test.go

@@ -63,7 +63,7 @@ func (s *migrationSuite) TestPerformWithRollbackAtSetup(c *gc.C) {
 	// We do care about the order of the calls.
 	gomock.InOrder(
 		s.op.EXPECT().Setup(s.scope).Return(errors.New("boom")),
-		s.op.EXPECT().Rollback(gomock.Any()).Return(nil),
+		s.op.EXPECT().Rollback(gomock.Any(), s.model).Return(nil),
 	)
 
 	err := m.Perform(context.Background(), s.scope, s.model)
@@ -82,7 +82,7 @@ func (s *migrationSuite) TestPerformWithRollbackAtExecution(c *gc.C) {
 	gomock.InOrder(
 		s.op.EXPECT().Setup(s.scope).Return(nil),
 		s.op.EXPECT().Execute(gomock.Any(), s.model).Return(errors.New("boom")),
-		s.op.EXPECT().Rollback(gomock.Any()).Return(nil),
+		s.op.EXPECT().Rollback(gomock.Any(), s.model).Return(nil),
 	)
 
 	err := m.Perform(context.Background(), s.scope, s.model)
@@ -101,7 +101,7 @@ func (s *migrationSuite) TestPerformWithRollbackError(c *gc.C) {
 	gomock.InOrder(
 		s.op.EXPECT().Setup(s.scope).Return(nil),
 		s.op.EXPECT().Execute(gomock.Any(), s.model).Return(errors.New("boom")),
-		s.op.EXPECT().Rollback(gomock.Any()).Return(errors.New("sad")),
+		s.op.EXPECT().Rollback(gomock.Any(), s.model).Return(errors.New("sad")),
 	)
 
 	err := m.Perform(context.Background(), s.scope, s.model)

core/modelmigration/testing/operation.go

@@ -0,0 +1,22 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import "github.com/juju/juju/core/modelmigration"
+
+// IgnoredSetupOperation is a helper function to test the operation within a
+// coordinator.
+// This just ignores the setup call of the coordinator. It is expected that
+// the operation will have all the information up front.
+func IgnoredSetupOperation(op modelmigration.Operation) modelmigration.Operation {
+	return &ignoredSetupOperation{Operation: op}
+}
+
+type ignoredSetupOperation struct {
+	modelmigration.Operation
+}
+
+func (i *ignoredSetupOperation) Setup(modelmigration.Scope) error {
+	return nil
+}

domain/model/bootstrap/bootstrap.go

@@ -71,3 +71,20 @@ func CreateModel(
 		})
 	}
 }
+
+// CreateReadOnlyModel creates a new model within the model database with all of
+// its associated metadata. The data will be read-only and cannot be modified
+// once created.
+func CreateReadOnlyModel(
+	args model.ReadOnlyModelCreationArgs,
+) func(context.Context, database.TxnRunner) error {
+	return func(ctx context.Context, db database.TxnRunner) error {
+		if err := args.Validate(); err != nil {
+			return fmt.Errorf("model creation args: %w", err)
+		}
+
+		return db.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error {
+			return state.CreateReadOnlyModel(ctx, args, tx)
+		})
+	}
+}

domain/model/bootstrap/bootstrap_test.go

@@ -105,3 +105,22 @@ func (s *bootstrapSuite) TestUUIDIsRespected(c *gc.C) {
 
 	c.Check(uuid, gc.Equals, modelUUID)
 }
+
+type modelBootstrapSuite struct {
+	schematesting.ModelSuite
+}
+
+var _ = gc.Suite(&modelBootstrapSuite{})
+
+func (s *modelBootstrapSuite) TestCreateReadOnlyModel(c *gc.C) {
+	fn := CreateReadOnlyModel(model.ReadOnlyModelCreationArgs{
+		UUID:        modeltesting.GenModelUUID(c),
+		Name:        "test",
+		Type:        coremodel.IAAS,
+		Cloud:       "aws",
+		CloudRegion: "myregion",
+	})
+
+	err := fn(context.Background(), s.ModelTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+}

domain/model/modelmigration/import.go

@@ -14,10 +14,9 @@ import (
 	coremodel "github.com/juju/juju/core/model"
 	"github.com/juju/juju/core/modelmigration"
 	coreuser "github.com/juju/juju/core/user"
-	"github.com/juju/juju/domain/model"
 	domainmodel "github.com/juju/juju/domain/model"
-	"github.com/juju/juju/domain/model/service"
-	"github.com/juju/juju/domain/model/state"
+	modelservice "github.com/juju/juju/domain/model/service"
+	modelstate "github.com/juju/juju/domain/model/state"
 	usererrors "github.com/juju/juju/domain/user/errors"
 	userservice "github.com/juju/juju/domain/user/service"
 	userstate "github.com/juju/juju/domain/user/state"
@@ -36,11 +35,19 @@ func RegisterImport(coordinator Coordinator) {
 	coordinator.Add(&importOperation{})
 }
 
-// ImportService defines the model service used to import models from another
+// ModelService defines the model service used to import models from another
 // controller to this one.
-type ImportService interface {
+type ModelService interface {
 	// CreateModel is responsible for creating a new model that is being imported.
-	CreateModel(context.Context, model.ModelCreationArgs) (coremodel.UUID, error)
+	CreateModel(context.Context, domainmodel.ModelCreationArgs) (coremodel.UUID, error)
+	// DeleteModel is responsible for removing a model from the system.
+	DeleteModel(context.Context, coremodel.UUID) error
+}
+
+type ReadOnlyModelService interface {
+	// CreateModel is responsible for creating a new read only model
+	// that is being imported.
+	CreateModel(context.Context, domainmodel.ReadOnlyModelCreationArgs) error
 }
 
 // UserService defines the user service used for model migration.
@@ -57,20 +64,22 @@ type UserService interface {
 type importOperation struct {
 	modelmigration.BaseOperation
 
-	service     ImportService
-	userService UserService
+	modelService         ModelService
+	readOnlyModelService ReadOnlyModelService
+	userService          UserService
 }
 
 // Setup is responsible for taking the model migration scope and creating the
 // needed services used during import.
 func (i *importOperation) Setup(scope modelmigration.Scope) error {
-	i.service = service.NewService(
-		state.NewState(scope.ControllerDB()),
-		service.DefaultAgentBinaryFinder(),
+	i.modelService = modelservice.NewService(
+		modelstate.NewState(scope.ControllerDB()),
+		modelservice.DefaultAgentBinaryFinder(),
+	)
+	i.readOnlyModelService = modelservice.NewModelService(
+		modelstate.NewModelState(scope.ModelDB()),
 	)
-
 	i.userService = userservice.NewService(userstate.NewState(scope.ControllerDB()))
-
 	return nil
 }
 
@@ -82,52 +91,20 @@ func (i *importOperation) Setup(scope modelmigration.Scope) error {
 // If the user specified for the model cannot be found an error satisfying
 // [usererrors.NotFound] will be returned.
 func (i importOperation) Execute(ctx context.Context, model description.Model) error {
-	modelConfig := model.Config()
-	if modelConfig == nil {
-		return errors.New("model config is empty")
-	}
-
-	modelNameI, exists := modelConfig[config.NameKey]
-	if !exists {
-		return fmt.Errorf(
-			"importing model during migration %w, no model name found in model config",
-			errors.NotValid,
-		)
-	}
-
-	modelNameS, ok := modelNameI.(string)
-	if !ok {
-		return fmt.Errorf(
-			"importing model during migration %w, establishing model name type as string. Got unknown type",
-			errors.NotValid,
-		)
-	}
-
-	uuidI, exists := modelConfig[config.UUIDKey]
-	if !exists {
-		return fmt.Errorf(
-			"importing model during migration %w, no model uuid found in model config",
-			errors.NotValid,
-		)
-	}
-
-	uuidS, ok := uuidI.(string)
-	if !ok {
-		return fmt.Errorf(
-			"importing model during migration %w, establishing model uuid type as string. Got unknown type",
-			errors.NotValid,
-		)
+	modelName, uuid, err := i.getModelNameAndUUID(model)
+	if err != nil {
+		return fmt.Errorf("importing model during migration %w", errors.NotValid)
 	}
 
 	user, err := i.userService.GetUserByName(ctx, model.Owner().Name())
 	if errors.Is(err, usererrors.NotFound) {
 		return fmt.Errorf("cannot import model %q with uuid %q, %w for name %q",
-			modelNameS, uuidS, usererrors.NotFound, model.Owner().Name(),
+			modelName, uuid, usererrors.NotFound, model.Owner().Name(),
 		)
 	} else if err != nil {
 		return fmt.Errorf(
 			"importing model %q with uuid %q during migration, finding user %q: %w",
-			modelNameS, uuidS, model.Owner().Name(), err,
+			modelName, uuid, model.Owner().Name(), err,
 		)
 	}
 
@@ -144,19 +121,88 @@ func (i importOperation) Execute(ctx context.Context, model description.Model) e
 		Cloud:        model.Cloud(),
 		CloudRegion:  model.CloudRegion(),
 		Credential:   credential,
-		Name:         modelNameS,
+		Name:         modelName,
 		Owner:        user.UUID,
 		Type:         coremodel.ModelType(model.Type()),
-		UUID:         coremodel.UUID(uuidS),
+		UUID:         coremodel.UUID(uuid),
 	}
 
-	_, err = i.service.CreateModel(ctx, args)
+	createdModelUUID, err := i.modelService.CreateModel(ctx, args)
 	if err != nil {
 		return fmt.Errorf(
 			"importing model %q with uuid %q during migration: %w",
-			modelNameS, uuidS, err,
+			modelName, uuid, err,
+		)
+	}
+	if createdModelUUID != args.UUID {
+		return fmt.Errorf(
+			"importing model %q with uuid %q during migration, created model uuid %q does not match expected uuid %q",
+			modelName, uuid, createdModelUUID, args.UUID,
+		)
+	}
+
+	// If the model is read only, we need to create a read only model.
+	err = i.readOnlyModelService.CreateModel(ctx, args.AsReadOnly())
+	if err != nil {
+		return fmt.Errorf(
+			"importing read only model %q with uuid %q during migration: %w",
+			modelName, uuid, err,
 		)
 	}
 
+	// NOTE: If we add any more steps to the import operation, we should
+	// consider adding a rollback operation to undo the changes made by the
+	// import operation.
+
 	return nil
 }
+
+// Rollback will attempt to rollback the import operation if it was
+// unsuccessful.
+func (i importOperation) Rollback(ctx context.Context, model description.Model) error {
+	// Attempt to rollback the model database if it was created.
+	modelName, uuid, err := i.getModelNameAndUUID(model)
+	if err != nil {
+		return fmt.Errorf("rollback of model during migration %w", errors.NotValid)
+	}
+
+	modelUUID := coremodel.UUID(uuid)
+
+	if err := i.modelService.DeleteModel(ctx, modelUUID); err != nil {
+		return fmt.Errorf(
+			"rollback of model %q with uuid %q during migration: %w",
+			modelName, uuid, err,
+		)
+	}
+
+	return nil
+}
+
+func (i importOperation) getModelNameAndUUID(model description.Model) (string, string, error) {
+	modelConfig := model.Config()
+	if modelConfig == nil {
+		return "", "", errors.New("model config is empty")
+	}
+
+	modelNameI, exists := modelConfig[config.NameKey]
+	if !exists {
+		return "", "", fmt.Errorf("no model name found in model config")
+	}
+
+	modelNameS, ok := modelNameI.(string)
+	if !ok {
+		return "", "", fmt.Errorf("establishing model name type as string. Got unknown type")
+	}
+
+	uuidI, exists := modelConfig[config.UUIDKey]
+	if !exists {
+		return "", "", fmt.Errorf("no model uuid found in model config")
+	}
+
+	uuidS, ok := uuidI.(string)
+	if !ok {
+		return "", "", fmt.Errorf("establishing model uuid type as string. Got unknown type")
+	}
+
+	return modelNameS, uuidS, nil
+}