Merge pull request juju#17156 from SimonRichardson/generate-triggers

juju#17156

Triggers were written whereby any update to the database would cause a new insert into the change log, even if the value was identical. This causes a lot of churn in the change log. We can generate the triggers using the columns from the actual database schema. The triggers will always be correct and always up to date.

Part of the change set includes fixing watchers and watcher tests:

### Cloud watcher

The watch cloud test wasn't working how it was expected. The upsert was just triggering a new change in the old code, prior to this patch. The cloud-config is currently immutable, I'll open a bug to fix this. As the config is not updated and the trigger prevents a noop change then the test fails. The solution is to then update a field on the cloud that will cause a change. In this case, the endpoint was updated.

Bug about cloud-config: https://bugs.launchpad.net/juju/+bug/2066410

### Model config watcher

The model config watch wasn't correct once the generated triggers were added. Firstly, we deleted all of the keys and then re-added them again even if they were the same. This caused a lot of churn in the changestream. The fix was just to ensure that we inserted, updated, and deleted rows correctly.

The second problem was that we would see the initial changes from bootstrap and then see them again when the change stream was processing requests. This might be ok for some watchers (I highly doubt that), but the concept of watchers are that, watch changes and then react. For most things that take information from the API server, the initial
event is thrown away, so it's pointless trying to get that. It's better to just read the state of the world and then respond to changes.

The model_config watcher will now emit an empty initial event and then subsequent events will tell you what has changed.


<!-- Why this change is needed and what it does. -->

## 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
$ make rebuild-triggers
```

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

## Links

**Jira card:** [JUJU-6055](https://warthogs.atlassian.net/browse/JUJU-6055)



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

Makefile

@@ -551,6 +551,12 @@ else
 		./apiserver/facades/schema.json
 endif
 
+.PHONY: rebuild-triggers
+rebuild-triggers:
+## rebuild-triggers: Rebuild the SQL trigger schema
+	@echo "Generating trigger schema..."
+	@env GOOS= GOARCH= CGO_ENABLED=1 go generate -tags="libsqlite3" $(COMPILE_FLAGS) -x ./domain/schema
+
 .PHONY: install-snap-dependencies
 # Install packages required to develop Juju and run tests. The stable
 # PPA includes the required mongodb-server binaries.

core/watcher/eventsource/namespace.go

@@ -185,3 +185,10 @@ func InitialNamespaceChanges(selectAll string) NamespaceQuery {
 		return keys, errors.Trace(err)
 	}
 }
+
+// EmptyInitialNamespaceChanges returns a query that returns no initial changes.
+func EmptyInitialNamespaceChanges() NamespaceQuery {
+	return func(ctx context.Context, runner database.TxnRunner) ([]string, error) {
+		return nil, nil
+	}
+}

domain/blockdevice/state/state.go

@@ -177,24 +177,7 @@ func (st *State) updateBlockDevices(ctx context.Context, tx *sqlair.TX, machineU
 		fsTypeByName[fsType.Name] = fsType.ID
 	}
 
-	insertQuery := `
-INSERT INTO block_device (uuid, machine_uuid, name, label, device_uuid, hardware_id, wwn, bus_address, serial_id, mount_point, size_mib, filesystem_type_id, in_use)
-VALUES (
-    $BlockDevice.uuid,
-    $BlockDevice.machine_uuid,
-    $BlockDevice.name,
-    $BlockDevice.label,
-    $BlockDevice.device_uuid,
-    $BlockDevice.hardware_id,
-    $BlockDevice.wwn,
-    $BlockDevice.bus_address,
-    $BlockDevice.serial_id,
-    $BlockDevice.mount_point,
-    $BlockDevice.size_mib,
-    $BlockDevice.filesystem_type_id,
-    $BlockDevice.in_use
-)
-`
+	insertQuery := `INSERT INTO block_device (*) VALUES ($BlockDevice.*)`
 	insertStmt, err := st.Prepare(insertQuery, BlockDevice{})
 	if err != nil {
 		return errors.Trace(err)
@@ -212,7 +195,6 @@ VALUES (
 		return errors.Trace(err)
 	}
 
-	blockDevicesByUUID := make(map[uuid.UUID]blockdevice.BlockDevice, len(devices))
 	for _, bd := range devices {
 		fsTypeID, ok := fsTypeByName[bd.FilesystemType]
 		if !ok {
@@ -222,7 +204,6 @@ VALUES (
 		if err != nil {
 			return errors.Trace(err)
 		}
-		blockDevicesByUUID[id] = bd
 		dbBlockDevice := BlockDevice{
 			ID:             id.String(),
 			MachineUUID:    machineUUID,

domain/blockdevice/state/types.go

@@ -16,7 +16,7 @@ type BlockDevice struct {
 	MachineUUID string `db:"machine_uuid"`
 
 	DeviceName     string `db:"name"`
-	Label          string `db:"label"`
+	Label          string `db:"label,omitempty"`
 	DeviceUUID     string `db:"device_uuid"`
 	HardwareId     string `db:"hardware_id"`
 	WWN            string `db:"wwn"`

domain/cloud/state/state_test.go

@@ -729,28 +729,31 @@ func (s *stateSuite) TestWatchCloudNotFound(c *gc.C) {
 }
 
 func (s *stateSuite) TestWatchCloud(c *gc.C) {
-	cld := testCloud
 	st := NewState(s.TxnRunnerFactory())
+
 	cloudUUID := uuid.MustNewUUID().String()
+
+	cld := testCloud
 	err := st.CreateCloud(context.Background(), "admin", cloudUUID, cld)
 	c.Assert(err, jc.ErrorIsNil)
 
 	w, err := st.WatchCloud(context.Background(), s.watcherFunc(c, cloudUUID), "fluffy")
 	c.Assert(err, jc.ErrorIsNil)
-	s.AddCleanup(func(c *gc.C) { workertest.CleanKill(c, w) })
+	defer workertest.CleanKill(c, w)
 
 	wc := watchertest.NewNotifyWatcherC(c, w)
 	wc.AssertChanges(time.Second) // Initial event.
 
-	cld.Config = map[string]any{"foo": "bar"}
+	cld.Endpoint = "https://endpoint2"
 	err = s.TxnRunner().StdTxn(context.Background(), func(ctx context.Context, tx *sql.Tx) error {
 		err := st.UpdateCloud(ctx, cld)
 		return err
 	})
 	c.Assert(err, jc.ErrorIsNil)
-	wc.AssertOneChange()
 
-	workertest.CleanKill(c, w)
+	s.AssertChangeStreamIdle(c)
+
+	wc.AssertOneChange()
 }
 
 // TestNullCloudType is a regression test to make sure that we don't allow null

domain/modelconfig/bootstrap/bootstrap.go

@@ -8,11 +8,12 @@ import (
 	"database/sql"
 	"fmt"
 
+	"github.com/canonical/sqlair"
+
 	"github.com/juju/juju/core/database"
 	coremodel "github.com/juju/juju/core/model"
 	modelstate "github.com/juju/juju/domain/model/state"
 	"github.com/juju/juju/domain/modelconfig/service"
-	"github.com/juju/juju/domain/modelconfig/state"
 	"github.com/juju/juju/environs/config"
 	internaldatabase "github.com/juju/juju/internal/database"
 )
@@ -78,13 +79,31 @@ func SetModelConfig(
 			return fmt.Errorf("validating model config to set for model: %w", err)
 		}
 
-		rawCfg, err := service.CoerceConfigForStorage(cfg.AllAttrs())
+		insert, err := service.CoerceConfigForStorage(cfg.AllAttrs())
 		if err != nil {
 			return fmt.Errorf("coercing model config for storage: %w", err)
 		}
 
-		return model.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error {
-			return state.SetModelConfig(ctx, rawCfg, tx)
+		insertQuery := `INSERT INTO model_config (*) VALUES ($dbKeyValue.*)`
+		insertStmt, err := sqlair.Prepare(insertQuery, dbKeyValue{})
+		if err != nil {
+			return fmt.Errorf("preparing insert query: %w", err)
+		}
+
+		return model.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
+			insertKV := make([]dbKeyValue, 0, len(insert))
+			for k, v := range insert {
+				insertKV = append(insertKV, dbKeyValue{Key: k, Value: v})
+			}
+			if err := tx.Query(ctx, insertStmt, insertKV).Run(); err != nil {
+				return fmt.Errorf("inserting model config values: %w", err)
+			}
+			return nil
 		})
 	}
 }
+
+type dbKeyValue struct {
+	Key   string `db:"key"`
+	Value string `db:"value"`
+}

domain/modelconfig/modelconfig_test.go

@@ -0,0 +1,160 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package modelconfig
+
+import (
+	"context"
+
+	jc "github.com/juju/testing/checkers"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/juju/cloud"
+	"github.com/juju/juju/core/changestream"
+	"github.com/juju/juju/core/credential"
+	"github.com/juju/juju/core/model"
+	modeltesting "github.com/juju/juju/core/model/testing"
+	"github.com/juju/juju/core/permission"
+	coreuser "github.com/juju/juju/core/user"
+	"github.com/juju/juju/core/watcher/watchertest"
+	"github.com/juju/juju/domain"
+	userbootstrap "github.com/juju/juju/domain/access/bootstrap"
+	cloudbootstrap "github.com/juju/juju/domain/cloud/bootstrap"
+	credentialbootstrap "github.com/juju/juju/domain/credential/bootstrap"
+	domainmodel "github.com/juju/juju/domain/model"
+	modelbootstrap "github.com/juju/juju/domain/model/bootstrap"
+	"github.com/juju/juju/domain/model/state/testing"
+	"github.com/juju/juju/domain/modelconfig/bootstrap"
+	"github.com/juju/juju/domain/modelconfig/service"
+	"github.com/juju/juju/domain/modelconfig/state"
+	"github.com/juju/juju/domain/modeldefaults"
+	"github.com/juju/juju/environs/config"
+	changestreamtesting "github.com/juju/juju/internal/changestream/testing"
+	loggertesting "github.com/juju/juju/internal/logger/testing"
+	"github.com/juju/juju/internal/uuid"
+	jujutesting "github.com/juju/juju/testing"
+	jujuversion "github.com/juju/juju/version"
+)
+
+type modelConfigSuite struct {
+	changestreamtesting.ControllerSuite
+	changestreamtesting.ModelSuite
+
+	modelID model.UUID
+}
+
+var _ = gc.Suite(&modelConfigSuite{})
+
+func (s *modelConfigSuite) SetUpTest(c *gc.C) {
+	s.ControllerSuite.SetUpTest(c)
+	s.ModelSuite.SetUpTest(c)
+
+	userID, fn := userbootstrap.AddUser(coreuser.AdminUserName, permission.ControllerForAccess(permission.SuperuserAccess))
+	err := fn(context.Background(), s.ControllerTxnRunner(), s.ControllerSuite.NoopTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+
+	cloudName := "test"
+	fn = cloudbootstrap.InsertCloud(coreuser.AdminUserName, cloud.Cloud{
+		Name:      cloudName,
+		Type:      "ec2",
+		AuthTypes: cloud.AuthTypes{cloud.EmptyAuthType},
+	})
+
+	err = fn(context.Background(), s.ControllerTxnRunner(), s.ControllerSuite.NoopTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+
+	credentialName := "test"
+	fn = credentialbootstrap.InsertCredential(credential.Key{
+		Cloud: cloudName,
+		Name:  credentialName,
+		Owner: coreuser.AdminUserName,
+	},
+		cloud.NewCredential(cloud.EmptyAuthType, nil),
+	)
+
+	err = fn(context.Background(), s.ControllerTxnRunner(), s.ControllerSuite.NoopTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+
+	testing.CreateInternalSecretBackend(c, s.ControllerTxnRunner())
+
+	modelUUID := modeltesting.GenModelUUID(c)
+	modelFn := modelbootstrap.CreateModel(domainmodel.ModelCreationArgs{
+		AgentVersion: jujuversion.Current,
+		Cloud:        cloudName,
+		Credential: credential.Key{
+			Cloud: cloudName,
+			Name:  credentialName,
+			Owner: coreuser.AdminUserName,
+		},
+		Name:  "test",
+		Owner: userID,
+		UUID:  modelUUID,
+	})
+	s.modelID = modelUUID
+
+	err = modelFn(context.Background(), s.ControllerTxnRunner(), s.ControllerSuite.NoopTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+
+	err = modelbootstrap.CreateReadOnlyModel(modelUUID, uuid.MustNewUUID())(context.Background(), s.ControllerTxnRunner(), s.ModelTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+}
+
+func (s *modelConfigSuite) TestWatchModelConfig(c *gc.C) {
+	ctx, cancel := jujutesting.LongWaitContext()
+	defer cancel()
+
+	var defaults modelDefaultsProviderFunc = func(_ context.Context) (modeldefaults.Defaults, error) {
+		return modeldefaults.Defaults{
+			"foo": modeldefaults.DefaultAttributeValue{
+				Source: config.JujuControllerSource,
+				Value:  "bar",
+			},
+		}, nil
+	}
+
+	attrs := map[string]any{
+		"agent-version":  jujuversion.Current.String(),
+		"uuid":           s.modelID.String(),
+		"type":           "iaas",
+		"logging-config": "<root>=ERROR",
+	}
+
+	st := state.NewState(s.ModelSuite.TxnRunnerFactory())
+	factory := domain.NewWatcherFactory(
+		changestream.NewWatchableDBFactoryForNamespace(s.ModelSuite.GetWatchableDB, s.modelID.String()),
+		loggertesting.WrapCheckLog(c))
+	svc := service.NewWatchableService(defaults, config.ModelValidator(), st, factory)
+
+	watcher, err := svc.Watch()
+	c.Assert(err, jc.ErrorIsNil)
+
+	err = bootstrap.SetModelConfig(s.modelID, attrs, defaults)(ctx, s.ControllerTxnRunner(), s.ModelTxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+
+	w := watchertest.NewStringsWatcherC(c, watcher)
+
+	// Changestream becomes idle and then we receive the bootstrap changes
+	// from the model config.
+	w.AssertChange("name", "uuid", "type", "foo", "secret-backend", "logging-config")
+
+	// Ensure that the changestream is idle.
+	s.ModelSuite.AssertChangeStreamIdle(c)
+
+	// Now insert the change and watch it come through.
+	attrs["logging-config"] = "<root>=WARNING"
+
+	err = svc.SetModelConfig(ctx, attrs)
+	c.Assert(err, jc.ErrorIsNil)
+
+	s.ModelSuite.AssertChangeStreamIdle(c)
+
+	w.AssertChange("logging-config")
+}
+
+type modelDefaultsProviderFunc func(context.Context) (modeldefaults.Defaults, error)
+
+func (f modelDefaultsProviderFunc) ModelDefaults(
+	c context.Context,
+) (modeldefaults.Defaults, error) {
+	return f(c)
+}

domain/modelconfig/package_test.go

@@ -0,0 +1,14 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package modelconfig
+
+import (
+	"testing"
+
+	gc "gopkg.in/check.v1"
+)
+
+func TestPackage(t *testing.T) {
+	gc.TestingT(t)
+}

domain/modelconfig/service/providerservice.go

@@ -74,13 +74,11 @@ func NewWatchableProviderService(
 	}
 }
 
-// It's for testing.
-var InitialNamespaceChanges = eventsource.InitialNamespaceChanges
-
 // Watch returns a watcher that returns keys for any changes to model
 // config.
 func (s *WatchableProviderService) Watch() (watcher.StringsWatcher, error) {
 	return s.watcherFactory.NewNamespaceWatcher(
-		"model_config", changestream.All, InitialNamespaceChanges(s.st.AllKeysQuery()),
+		"model_config", changestream.All,
+		eventsource.InitialNamespaceChanges(s.st.AllKeysQuery()),
 	)
 }

domain/modelconfig/service/service.go

@@ -375,6 +375,6 @@ func NewWatchableService(
 func (s *WatchableService) Watch() (watcher.StringsWatcher, error) {
 	return s.watcherFactory.NewNamespaceWatcher(
 		"model_config", changestream.All,
-		InitialNamespaceChanges(s.st.AllKeysQuery()),
+		eventsource.InitialNamespaceChanges(s.st.AllKeysQuery()),
 	)
 }

domain/modelconfig/service/service_test.go

@@ -10,7 +10,6 @@ import (
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/core/watcher/eventsource"
 	"github.com/juju/juju/domain/modelconfig/service/testing"
 	"github.com/juju/juju/domain/modeldefaults"
 	"github.com/juju/juju/environs/config"
@@ -56,11 +55,6 @@ func (s *serviceSuite) TestSetModelConfig(c *gc.C) {
 
 	svc := NewWatchableService(defaults, config.ModelValidator(), st, st)
 
-	s.PatchValue(&InitialNamespaceChanges, func(selectAll string) eventsource.NamespaceQuery {
-		c.Assert(selectAll, gc.Equals, st.AllKeysQuery())
-		return nil
-	})
-
 	watcher, err := svc.Watch()
 	c.Assert(err, jc.ErrorIsNil)
 	var changes []string