From e54b85590caf3742f07e935bfebd7327363efb7c Mon Sep 17 00:00:00 2001 From: wallyworld Date: Wed, 19 Feb 2025 14:06:06 +1000 Subject: [PATCH 1/5] chore: tweak storage ddl Add missing indices. Rename size colume in storage_instance table. Remove unnecessary pool columns from filesystem and volume tables. --- domain/annotation/state/state_test.go | 2 +- domain/schema/model/sql/0011-storage.sql | 23 +++++++++++-------- .../model/triggers/storage-triggers.gen.go | 6 ++--- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/domain/annotation/state/state_test.go b/domain/annotation/state/state_test.go index 14ae45e7746..7f09ec31cc5 100644 --- a/domain/annotation/state/state_test.go +++ b/domain/annotation/state/state_test.go @@ -438,7 +438,7 @@ VALUES (?, ?, ?, ?, ?) func (s *stateSuite) ensureStorage(c *gc.C, name, uuid, charmUUID string) { err := s.TxnRunner().StdTxn(context.Background(), func(ctx context.Context, tx *sql.Tx) error { _, err := tx.ExecContext(ctx, ` -INSERT INTO storage_instance (uuid, storage_id, storage_pool, size_mib, charm_uuid, storage_name, life_id) +INSERT INTO storage_instance (uuid, storage_id, storage_pool, requested_size_mib, charm_uuid, storage_name, life_id) VALUES (?, ?, ?, ?, ?, ?, ?) `, uuid, name+"/0", "loop", 100, charmUUID, name, 0) return err diff --git a/domain/schema/model/sql/0011-storage.sql b/domain/schema/model/sql/0011-storage.sql index 5eb41c8a84d..a4b6a8c26cd 100644 --- a/domain/schema/model/sql/0011-storage.sql +++ b/domain/schema/model/sql/0011-storage.sql @@ -111,7 +111,7 @@ CREATE TABLE storage_instance ( -- with an ID. Storage pools, once created, cannot be renamed so -- this will not be able to become "orphaned". storage_pool TEXT NOT NULL, - size_mib INT NOT NULL, + requested_size_mib INT NOT NULL, CONSTRAINT fk_storage_instance_life FOREIGN KEY (life_id) REFERENCES life (id), @@ -120,6 +120,9 @@ CREATE TABLE storage_instance ( REFERENCES charm_storage (charm_uuid, name) ); +CREATE UNIQUE INDEX idx_storage_instance_id +ON storage_instance (storage_id); + -- storage_unit_owner is used to indicate when -- a unit is the owner of a storage instance. -- This is different to a storage attachment. @@ -169,10 +172,10 @@ INSERT INTO storage_provisioning_status VALUES CREATE TABLE storage_volume ( uuid TEXT NOT NULL PRIMARY KEY, + volume_id TEXT NOT NULL, life_id INT NOT NULL, - name TEXT NOT NULL, + name TEXT, provider_id TEXT, - storage_pool_uuid TEXT, size_mib INT, hardware_id TEXT, wwn TEXT, @@ -181,14 +184,14 @@ CREATE TABLE storage_volume ( CONSTRAINT fk_storage_instance_life FOREIGN KEY (life_id) REFERENCES life (id), - CONSTRAINT fk_storage_volume_pool - FOREIGN KEY (storage_pool_uuid) - REFERENCES storage_pool (uuid), CONSTRAINT fk_storage_vol_provisioning_status FOREIGN KEY (provisioning_status_id) REFERENCES storage_provisioning_status (id) ); +CREATE UNIQUE INDEX idx_storage_volume_id +ON storage_volume (volume_id); + -- An instance can have at most one volume. -- A volume can have at most one instance. CREATE TABLE storage_instance_volume ( @@ -232,22 +235,22 @@ CREATE TABLE storage_volume_attachment ( CREATE TABLE storage_filesystem ( uuid TEXT NOT NULL PRIMARY KEY, + filesystem_id TEXT NOT NULL, life_id INT NOT NULL, provider_id TEXT, - storage_pool_uuid TEXT, size_mib INT, provisioning_status_id INT NOT NULL, CONSTRAINT fk_storage_instance_life FOREIGN KEY (life_id) REFERENCES life (id), - CONSTRAINT fk_storage_filesystem_pool - FOREIGN KEY (storage_pool_uuid) - REFERENCES storage_pool (uuid), CONSTRAINT fk_storage_fs_provisioning_status FOREIGN KEY (provisioning_status_id) REFERENCES storage_provisioning_status (id) ); +CREATE UNIQUE INDEX idx_storage_filesystem_id +ON storage_filesystem (filesystem_id); + -- An instance can have at most one filesystem. -- A filesystem can have at most one instance. CREATE TABLE storage_instance_filesystem ( diff --git a/domain/schema/model/triggers/storage-triggers.gen.go b/domain/schema/model/triggers/storage-triggers.gen.go index 49cccb566dc..b3bbbb7f981 100644 --- a/domain/schema/model/triggers/storage-triggers.gen.go +++ b/domain/schema/model/triggers/storage-triggers.gen.go @@ -114,9 +114,9 @@ CREATE TRIGGER trg_log_storage_filesystem_update AFTER UPDATE ON storage_filesystem FOR EACH ROW WHEN NEW.uuid != OLD.uuid OR + NEW.filesystem_id != OLD.filesystem_id OR NEW.life_id != OLD.life_id OR (NEW.provider_id != OLD.provider_id OR (NEW.provider_id IS NOT NULL AND OLD.provider_id IS NULL) OR (NEW.provider_id IS NULL AND OLD.provider_id IS NOT NULL)) OR - (NEW.storage_pool_uuid != OLD.storage_pool_uuid OR (NEW.storage_pool_uuid IS NOT NULL AND OLD.storage_pool_uuid IS NULL) OR (NEW.storage_pool_uuid IS NULL AND OLD.storage_pool_uuid IS NOT NULL)) OR (NEW.size_mib != OLD.size_mib OR (NEW.size_mib IS NOT NULL AND OLD.size_mib IS NULL) OR (NEW.size_mib IS NULL AND OLD.size_mib IS NOT NULL)) OR NEW.provisioning_status_id != OLD.provisioning_status_id BEGIN @@ -195,10 +195,10 @@ CREATE TRIGGER trg_log_storage_volume_update AFTER UPDATE ON storage_volume FOR EACH ROW WHEN NEW.uuid != OLD.uuid OR + NEW.volume_id != OLD.volume_id OR NEW.life_id != OLD.life_id OR - NEW.name != OLD.name OR + (NEW.name != OLD.name OR (NEW.name IS NOT NULL AND OLD.name IS NULL) OR (NEW.name IS NULL AND OLD.name IS NOT NULL)) OR (NEW.provider_id != OLD.provider_id OR (NEW.provider_id IS NOT NULL AND OLD.provider_id IS NULL) OR (NEW.provider_id IS NULL AND OLD.provider_id IS NOT NULL)) OR - (NEW.storage_pool_uuid != OLD.storage_pool_uuid OR (NEW.storage_pool_uuid IS NOT NULL AND OLD.storage_pool_uuid IS NULL) OR (NEW.storage_pool_uuid IS NULL AND OLD.storage_pool_uuid IS NOT NULL)) OR (NEW.size_mib != OLD.size_mib OR (NEW.size_mib IS NOT NULL AND OLD.size_mib IS NULL) OR (NEW.size_mib IS NULL AND OLD.size_mib IS NOT NULL)) OR (NEW.hardware_id != OLD.hardware_id OR (NEW.hardware_id IS NOT NULL AND OLD.hardware_id IS NULL) OR (NEW.hardware_id IS NULL AND OLD.hardware_id IS NOT NULL)) OR (NEW.wwn != OLD.wwn OR (NEW.wwn IS NOT NULL AND OLD.wwn IS NULL) OR (NEW.wwn IS NULL AND OLD.wwn IS NOT NULL)) OR From 43672303e07aa027e702e43f705fe53ba548ccc4 Mon Sep 17 00:00:00 2001 From: wallyworld Date: Wed, 19 Feb 2025 14:17:27 +1000 Subject: [PATCH 2/5] chore: add enums for storage lookup values defined in the db --- domain/application/state/storage_test.go | 2 +- domain/storage/provisionngstatus.go | 14 ++++++++ domain/storage/provisionngstatus_test.go | 41 ++++++++++++++++++++++++ domain/storage/storagekind.go | 13 ++++++++ domain/storage/storagekind_test.go | 41 ++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 domain/storage/provisionngstatus.go create mode 100644 domain/storage/provisionngstatus_test.go create mode 100644 domain/storage/storagekind.go create mode 100644 domain/storage/storagekind_test.go diff --git a/domain/application/state/storage_test.go b/domain/application/state/storage_test.go index 3f1066d8bcd..cea0494ba63 100644 --- a/domain/application/state/storage_test.go +++ b/domain/application/state/storage_test.go @@ -198,7 +198,7 @@ func (s *applicationStateSuite) TestGetStorageUUIDByID(c *gc.C) { err := s.TxnRunner().StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error { _, err := tx.ExecContext(ctx, ` -INSERT INTO storage_instance(uuid, charm_uuid, storage_name, storage_id, life_id, storage_pool, size_mib) +INSERT INTO storage_instance(uuid, charm_uuid, storage_name, storage_id, life_id, storage_pool, requested_size_mib) VALUES (?, ?, ?, ?, ?, ?, ?)`, uuid, charmUUID, "pgdata", "pgdata/0", 0, "pool", 666) return err }) diff --git a/domain/storage/provisionngstatus.go b/domain/storage/provisionngstatus.go new file mode 100644 index 00000000000..8b4c8c51e22 --- /dev/null +++ b/domain/storage/provisionngstatus.go @@ -0,0 +1,14 @@ +// Copyright 2025 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package storage + +// ProvisioningStatus represents the status of a storage entity +// as recorded in the storage provisioning status lookup table. +type ProvisioningStatus int + +const ( + ProvisioningStatusPending ProvisioningStatus = iota + ProvisioningStatusProvisioned + ProvisioningStatusError +) diff --git a/domain/storage/provisionngstatus_test.go b/domain/storage/provisionngstatus_test.go new file mode 100644 index 00000000000..cc2bdd10b12 --- /dev/null +++ b/domain/storage/provisionngstatus_test.go @@ -0,0 +1,41 @@ +// Copyright 2025 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package storage + +import ( + jc "github.com/juju/testing/checkers" + gc "gopkg.in/check.v1" + + schematesting "github.com/juju/juju/domain/schema/testing" +) + +type provisioningStatusSuite struct { + schematesting.ModelSuite +} + +var _ = gc.Suite(&provisioningStatusSuite{}) + +// TestProvisioningStatusDBValues ensures there's no skew between what's in the +// database table for provisioning_status and the typed consts used in the state packages. +func (s *provisioningStatusSuite) TestProvisioningStatusDBValues(c *gc.C) { + db := s.DB() + rows, err := db.Query("SELECT id, name FROM storage_provisioning_status") + c.Assert(err, jc.ErrorIsNil) + defer func() { _ = rows.Close() }() + + dbValues := make(map[ProvisioningStatus]string) + for rows.Next() { + var ( + id int + value string + ) + c.Assert(rows.Scan(&id, &value), jc.ErrorIsNil) + dbValues[ProvisioningStatus(id)] = value + } + c.Assert(dbValues, jc.DeepEquals, map[ProvisioningStatus]string{ + ProvisioningStatusPending: "pending", + ProvisioningStatusProvisioned: "provisioned", + ProvisioningStatusError: "error", + }) +} diff --git a/domain/storage/storagekind.go b/domain/storage/storagekind.go new file mode 100644 index 00000000000..6ccd0aee542 --- /dev/null +++ b/domain/storage/storagekind.go @@ -0,0 +1,13 @@ +// Copyright 2025 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package storage + +// StorageKind represents the kind of storage +// as recorded in the storage kind lookup table. +type StorageKind int + +const ( + StorageKindBlock StorageKind = iota + StorageKindFilesystem +) diff --git a/domain/storage/storagekind_test.go b/domain/storage/storagekind_test.go new file mode 100644 index 00000000000..858138beca1 --- /dev/null +++ b/domain/storage/storagekind_test.go @@ -0,0 +1,41 @@ +// Copyright 2025 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package storage + +import ( + jc "github.com/juju/testing/checkers" + gc "gopkg.in/check.v1" + + schematesting "github.com/juju/juju/domain/schema/testing" +) + +type storageKindSuite struct { + schematesting.ModelSuite +} + +var _ = gc.Suite(&storageKindSuite{}) + +// TestStorageKindDBValues ensures there's no skew between what's in the +// database table for charm_storage_kind and the typed consts used in the state packages. +func (s *storageKindSuite) TestStorageKindDBValues(c *gc.C) { + db := s.DB() + rows, err := db.Query("SELECT id, kind FROM charm_storage_kind") + c.Assert(err, jc.ErrorIsNil) + defer func() { _ = rows.Close() }() + + dbValues := make(map[StorageKind]string) + for rows.Next() { + var ( + id int + value string + ) + + c.Assert(rows.Scan(&id, &value), jc.ErrorIsNil) + dbValues[StorageKind(id)] = value + } + c.Assert(dbValues, jc.DeepEquals, map[StorageKind]string{ + StorageKindFilesystem: "filesystem", + StorageKindBlock: "block", + }) +} From 051234a184394e1a97e7d2098b8bf25206ee5d64 Mon Sep 17 00:00:00 2001 From: wallyworld Date: Thu, 20 Feb 2025 11:50:33 +1000 Subject: [PATCH 3/5] chore: remove volume name column --- domain/schema/model/sql/0011-storage.sql | 1 - domain/schema/model/triggers/storage-triggers.gen.go | 1 - 2 files changed, 2 deletions(-) diff --git a/domain/schema/model/sql/0011-storage.sql b/domain/schema/model/sql/0011-storage.sql index a4b6a8c26cd..e4fb08899f0 100644 --- a/domain/schema/model/sql/0011-storage.sql +++ b/domain/schema/model/sql/0011-storage.sql @@ -174,7 +174,6 @@ CREATE TABLE storage_volume ( uuid TEXT NOT NULL PRIMARY KEY, volume_id TEXT NOT NULL, life_id INT NOT NULL, - name TEXT, provider_id TEXT, size_mib INT, hardware_id TEXT, diff --git a/domain/schema/model/triggers/storage-triggers.gen.go b/domain/schema/model/triggers/storage-triggers.gen.go index b3bbbb7f981..0b25e7c72d9 100644 --- a/domain/schema/model/triggers/storage-triggers.gen.go +++ b/domain/schema/model/triggers/storage-triggers.gen.go @@ -197,7 +197,6 @@ WHEN NEW.uuid != OLD.uuid OR NEW.volume_id != OLD.volume_id OR NEW.life_id != OLD.life_id OR - (NEW.name != OLD.name OR (NEW.name IS NOT NULL AND OLD.name IS NULL) OR (NEW.name IS NULL AND OLD.name IS NOT NULL)) OR (NEW.provider_id != OLD.provider_id OR (NEW.provider_id IS NOT NULL AND OLD.provider_id IS NULL) OR (NEW.provider_id IS NULL AND OLD.provider_id IS NOT NULL)) OR (NEW.size_mib != OLD.size_mib OR (NEW.size_mib IS NOT NULL AND OLD.size_mib IS NULL) OR (NEW.size_mib IS NULL AND OLD.size_mib IS NOT NULL)) OR (NEW.hardware_id != OLD.hardware_id OR (NEW.hardware_id IS NOT NULL AND OLD.hardware_id IS NULL) OR (NEW.hardware_id IS NULL AND OLD.hardware_id IS NOT NULL)) OR From 671e0b411440d3b5e62483e194e020f1c6be93d9 Mon Sep 17 00:00:00 2001 From: wallyworld Date: Tue, 25 Feb 2025 16:21:32 +1000 Subject: [PATCH 4/5] refactor: use separate columnes for storage pool and storage type --- domain/annotation/state/state_test.go | 2 +- domain/application/service/application.go | 8 +- .../application/service/application_test.go | 32 +++--- domain/application/state/storage.go | 27 ++++- domain/application/state/storage_test.go | 82 ++++++++++----- domain/application/state/types.go | 18 ++-- domain/application/types.go | 8 +- domain/schema/model/sql/0011-storage.sql | 99 +++++++++++++++---- domain/schema/model_schema_test.go | 3 + 9 files changed, 203 insertions(+), 76 deletions(-) diff --git a/domain/annotation/state/state_test.go b/domain/annotation/state/state_test.go index 7f09ec31cc5..aa7dfd48d27 100644 --- a/domain/annotation/state/state_test.go +++ b/domain/annotation/state/state_test.go @@ -438,7 +438,7 @@ VALUES (?, ?, ?, ?, ?) func (s *stateSuite) ensureStorage(c *gc.C, name, uuid, charmUUID string) { err := s.TxnRunner().StdTxn(context.Background(), func(ctx context.Context, tx *sql.Tx) error { _, err := tx.ExecContext(ctx, ` -INSERT INTO storage_instance (uuid, storage_id, storage_pool, requested_size_mib, charm_uuid, storage_name, life_id) +INSERT INTO storage_instance (uuid, storage_id, storage_type, requested_size_mib, charm_uuid, storage_name, life_id) VALUES (?, ?, ?, ?, ?, ?, ?) `, uuid, name+"/0", "loop", 100, charmUUID, name, 0) return err diff --git a/domain/application/service/application.go b/domain/application/service/application.go index a12051ab923..242a9bd5d0a 100644 --- a/domain/application/service/application.go +++ b/domain/application/service/application.go @@ -783,10 +783,10 @@ func makeStorageArgs(storage map[string]storage.Directive) []application.AddAppl var result []application.AddApplicationStorageArg for name, stor := range storage { result = append(result, application.AddApplicationStorageArg{ - Name: name, - Pool: stor.Pool, - Size: stor.Size, - Count: stor.Count, + Name: name, + PoolNameOrType: stor.Pool, + Size: stor.Size, + Count: stor.Count, }) } return result diff --git a/domain/application/service/application_test.go b/domain/application/service/application_test.go index 3ea7915dc5c..e2d14460455 100644 --- a/domain/application/service/application_test.go +++ b/domain/application/service/application_test.go @@ -683,10 +683,10 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlock(c *gc.C) { }, Platform: platform, Storage: []application.AddApplicationStorageArg{{ - Name: "data", - Pool: "loop", - Size: 10, - Count: 1, + Name: "data", + PoolNameOrType: "loop", + Size: 10, + Count: 1, }}, Scale: 1, } @@ -792,10 +792,10 @@ func (s *applicationServiceSuite) TestCreateWithStorageBlockDefaultSource(c *gc. }, Platform: platform, Storage: []application.AddApplicationStorageArg{{ - Name: "data", - Pool: "fast", - Size: 10, - Count: 2, + Name: "data", + PoolNameOrType: "fast", + Size: 10, + Count: 2, }}, Scale: 1, } @@ -905,10 +905,10 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystem(c *gc.C) { }, Platform: platform, Storage: []application.AddApplicationStorageArg{{ - Name: "data", - Pool: "rootfs", - Size: 10, - Count: 1, + Name: "data", + PoolNameOrType: "rootfs", + Size: 10, + Count: 1, }}, Scale: 1, } @@ -1015,10 +1015,10 @@ func (s *applicationServiceSuite) TestCreateWithStorageFilesystemDefaultSource(c }, Platform: platform, Storage: []application.AddApplicationStorageArg{{ - Name: "data", - Pool: "fast", - Size: 10, - Count: 2, + Name: "data", + PoolNameOrType: "fast", + Size: 10, + Count: 2, }}, Scale: 1, } diff --git a/domain/application/state/storage.go b/domain/application/state/storage.go index bd27b585fe4..d4d35f49ebb 100644 --- a/domain/application/state/storage.go +++ b/domain/application/state/storage.go @@ -34,6 +34,25 @@ WHERE charm_uuid = $applicationDetails.charm_uuid return errors.Capture(err) } + // Storage is either a storage type or a pool name. + // Get a mapping of pool name to pool UUID for all pools. + // The number of pools is always small - we'll limit the size + // of the result set just in case. If there's more than 200 there's + // much bigger problems in play. + storageQuery, err := st.Prepare(`SELECT &storagePool.* FROM storage_pool LIMIT 200`, storagePool{}) + if err != nil { + return errors.Capture(err) + } + var allPools []storagePool + err = tx.Query(ctx, storageQuery).GetAll(&allPools) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + return errors.Errorf("querying all storage pools: %w", err) + } + poolsByName := make(map[string]string) + for _, p := range allPools { + poolsByName[p.Name] = p.UUID + } + var storageMetadata []charmStorage err = tx.Query(ctx, queryStmt, appDetails).GetAll(&storageMetadata) if err != nil && !errors.Is(err, sql.ErrNoRows) { @@ -58,10 +77,16 @@ WHERE charm_uuid = $applicationDetails.charm_uuid ApplicationUUID: appDetails.UUID.String(), CharmUUID: appDetails.CharmID, StorageName: stor.Name, - StoragePool: stor.Pool, Size: uint(stor.Size), Count: uint(stor.Count), } + // PoolNameOrType has already been validated to either be + // a pool name or a valid storage type for the relevant cloud. + if uuid, ok := poolsByName[stor.PoolNameOrType]; ok { + storage[i].StoragePool = &uuid + } else { + storage[i].StorageType = &stor.PoolNameOrType + } } insertStmt, err := st.Prepare(` diff --git a/domain/application/state/storage_test.go b/domain/application/state/storage_test.go index cea0494ba63..ff92dada3bc 100644 --- a/domain/application/state/storage_test.go +++ b/domain/application/state/storage_test.go @@ -16,6 +16,7 @@ import ( "github.com/juju/juju/domain/application/charm" storageerrors "github.com/juju/juju/domain/storage/errors" "github.com/juju/juju/internal/errors" + "github.com/juju/juju/internal/uuid" ) // TestCreateApplicationWithResources tests creation of an application with @@ -23,28 +24,48 @@ import ( // It verifies that the charm_resource table is populated, alongside the // resource and application_resource table with datas from charm and arguments. func (s *applicationStateSuite) TestCreateApplicationWithStorage(c *gc.C) { + ctx := context.Background() + uuid := uuid.MustNewUUID().String() + err := s.TxnRunner().StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, ` +INSERT INTO storage_pool (uuid, name, type) VALUES (?, ?, ?)`, + uuid, "fast", "ebs") + if err != nil { + return errors.Capture(err) + } + return nil + }) chStorage := []charm.Storage{{ Name: "database", Type: "block", }, { Name: "logs", Type: "filesystem", + }, { + Name: "cache", + Type: "block", }} addStorageArgs := []application.AddApplicationStorageArg{ { - Name: "database", - Pool: "ebs", - Size: 10, - Count: 2, + Name: "database", + PoolNameOrType: "ebs", + Size: 10, + Count: 2, + }, + { + Name: "logs", + PoolNameOrType: "rootfs", + Size: 20, + Count: 1, }, { - Name: "logs", - Pool: "rootfs", - Size: 20, - Count: 1, + Name: "cache", + PoolNameOrType: "fast", + Size: 30, + Count: 1, }, } - ctx := context.Background() + c.Assert(err, jc.ErrorIsNil) appUUID, err := s.state.CreateApplication(ctx, "666", s.addApplicationArgForStorage(c, "666", chStorage, addStorageArgs), nil) @@ -61,6 +82,7 @@ WHERE name=?`, "666").Scan(&charmUUID) var ( foundCharmStorage []charm.Storage foundAppStorage []application.AddApplicationStorageArg + poolUUID string ) err = s.TxnRunner().StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error { @@ -87,7 +109,7 @@ WHERE charm_uuid=?`, charmUUID) err = s.TxnRunner().StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error { rows, err := tx.QueryContext(ctx, ` SELECT storage_name, storage_pool, size_mib, count -FROM application_storage_directive +FROM v_application_storage_directive WHERE application_uuid = ? AND charm_uuid = ?`, appUUID, charmUUID) if err != nil { return errors.Capture(err) @@ -95,16 +117,30 @@ WHERE application_uuid = ? AND charm_uuid = ?`, appUUID, charmUUID) defer func() { _ = rows.Close() }() for rows.Next() { var stor application.AddApplicationStorageArg - if err := rows.Scan(&stor.Name, &stor.Pool, &stor.Size, &stor.Count); err != nil { + if err := rows.Scan(&stor.Name, &stor.PoolNameOrType, &stor.Size, &stor.Count); err != nil { return errors.Capture(err) } foundAppStorage = append(foundAppStorage, stor) } + rows, err = tx.QueryContext(ctx, ` +SELECT storage_pool_uuid +FROM application_storage_directive +WHERE storage_type IS NULL AND application_uuid = ? AND charm_uuid = ?`, appUUID, charmUUID) + if err != nil { + return errors.Capture(err) + } + defer func() { _ = rows.Close() }() + for rows.Next() { + if err := rows.Scan(&poolUUID); err != nil { + return errors.Capture(err) + } + } return nil }) c.Assert(err, jc.ErrorIsNil) - c.Assert(foundCharmStorage, jc.SameContents, chStorage) - c.Assert(foundAppStorage, jc.SameContents, addStorageArgs) + c.Check(foundCharmStorage, jc.SameContents, chStorage) + c.Check(foundAppStorage, jc.SameContents, addStorageArgs) + c.Assert(poolUUID, gc.Equals, uuid) } func (s *applicationStateSuite) TestCreateApplicationWithUnrecognisedStorage(c *gc.C) { @@ -113,10 +149,10 @@ func (s *applicationStateSuite) TestCreateApplicationWithUnrecognisedStorage(c * Type: "block", }} addStorageArgs := []application.AddApplicationStorageArg{{ - Name: "foo", - Pool: "rootfs", - Size: 20, - Count: 1, + Name: "foo", + PoolNameOrType: "rootfs", + Size: 20, + Count: 1, }} ctx := context.Background() @@ -127,10 +163,10 @@ func (s *applicationStateSuite) TestCreateApplicationWithUnrecognisedStorage(c * func (s *applicationStateSuite) TestCreateApplicationWithStorageButCharmHasNone(c *gc.C) { addStorageArgs := []application.AddApplicationStorageArg{{ - Name: "foo", - Pool: "rootfs", - Size: 20, - Count: 1, + Name: "foo", + PoolNameOrType: "rootfs", + Size: 20, + Count: 1, }} ctx := context.Background() @@ -198,8 +234,8 @@ func (s *applicationStateSuite) TestGetStorageUUIDByID(c *gc.C) { err := s.TxnRunner().StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error { _, err := tx.ExecContext(ctx, ` -INSERT INTO storage_instance(uuid, charm_uuid, storage_name, storage_id, life_id, storage_pool, requested_size_mib) -VALUES (?, ?, ?, ?, ?, ?, ?)`, uuid, charmUUID, "pgdata", "pgdata/0", 0, "pool", 666) +INSERT INTO storage_instance(uuid, charm_uuid, storage_name, storage_id, life_id, storage_type, requested_size_mib) +VALUES (?, ?, ?, ?, ?, ?, ?)`, uuid, charmUUID, "pgdata", "pgdata/0", 0, "rootfs", 666) return err }) c.Assert(err, jc.ErrorIsNil) diff --git a/domain/application/state/types.go b/domain/application/state/types.go index fb8b4c372a1..3ba27953e59 100644 --- a/domain/application/state/types.go +++ b/domain/application/state/types.go @@ -640,13 +640,19 @@ type resourceToAdd struct { CreatedAt time.Time `db:"created_at"` } +type storagePool struct { + UUID string `db:"uuid"` + Name string `db:"name"` +} + type storageToAdd struct { - ApplicationUUID string `db:"application_uuid"` - CharmUUID string `db:"charm_uuid"` - StorageName string `db:"storage_name"` - StoragePool string `db:"storage_pool"` - Size uint `db:"size_mib"` - Count uint `db:"count"` + ApplicationUUID string `db:"application_uuid"` + CharmUUID string `db:"charm_uuid"` + StorageName string `db:"storage_name"` + StoragePool *string `db:"storage_pool_uuid"` + StorageType *string `db:"storage_type"` + Size uint `db:"size_mib"` + Count uint `db:"count"` } type linkResourceApplication struct { diff --git a/domain/application/types.go b/domain/application/types.go index f03facd7e74..d1249b8237a 100644 --- a/domain/application/types.go +++ b/domain/application/types.go @@ -62,10 +62,10 @@ type AddApplicationResourceArg struct { // AddApplicationStorageArg defines the arguments required to add storage to an application. type AddApplicationStorageArg struct { - Name string - Pool string - Size uint64 - Count uint64 + Name string + PoolNameOrType string + Size uint64 + Count uint64 } // Channel represents the channel of a application charm. diff --git a/domain/schema/model/sql/0011-storage.sql b/domain/schema/model/sql/0011-storage.sql index e4fb08899f0..ae1b9aae559 100644 --- a/domain/schema/model/sql/0011-storage.sql +++ b/domain/schema/model/sql/0011-storage.sql @@ -38,18 +38,25 @@ CREATE TABLE application_storage_directive ( -- been fixed (since first implemented). We don't envisage -- any change to how these are modelled. -- - -- Note: one might wonder why storage_pool below is not a - -- FK to a row defined in the storage pool table. This value - -- can also be one of the pool types. As with the comment on the - -- type column in the storage pool table, it's problematic to use a lookup - -- with an ID. Storage pools, once created, cannot be renamed so - -- this will not be able to become "orphaned". - storage_pool TEXT NOT NULL, + -- One of storage_pool_uuid or storage_type must be set. + -- Storage types are provider sourced, so we do not use a lookup with ID. + -- This constitutes "repeating data" and would tend to indicate + -- bad relational design. However we choose that here over the + -- burden of: + -- - Knowing every possible type up front to populate a look-up or; + -- - Sourcing the lookup from the provider and keeping it updated. + storage_pool_uuid TEXT, + storage_type TEXT, size_mib INT NOT NULL, count INT NOT NULL, + CONSTRAINT chk_application_storage_specified + CHECK (storage_pool_uuid IS NOT NULL OR storage_type IS NOT NULL), CONSTRAINT fk_application_storage_directive_application FOREIGN KEY (application_uuid) REFERENCES application (uuid), + CONSTRAINT fk_application_storage_directive_storage_pool + FOREIGN KEY (storage_pool_uuid) + REFERENCES storage_pool (uuid), CONSTRAINT fk_application_storage_directive_charm_storage FOREIGN KEY (charm_uuid, storage_name) REFERENCES charm_storage (charm_uuid, name), @@ -60,6 +67,17 @@ CREATE TABLE application_storage_directive ( CREATE INDEX idx_application_storage_directive ON application_storage_directive (application_uuid); +CREATE VIEW v_application_storage_directive AS +SELECT + asd.application_uuid, + asd.charm_uuid, + asd.storage_name, + asd.size_mib, + asd.count, + COALESCE(sp.name, asd.storage_type) AS storage_pool +FROM application_storage_directive AS asd +LEFT JOIN storage_pool AS sp ON asd.storage_pool_uuid = sp.uuid; + -- This table stores storage directive values for each named storage item -- defined by the unit's current charm. If the charm is updated, then -- so too will be the rows in this table to reflect the current charm's @@ -78,15 +96,25 @@ CREATE TABLE unit_storage_directive ( -- been fixed (since first implemented). We don't envisage -- any change to how these are modelled. -- - -- Note: one might wonder why storage_pool below is not a - -- FK to a row defined in the storage pool table. This value - -- can also be one of the pool types. As with the comment on the - -- type column in the storage pool table, it's problematic to use a lookup - -- with an ID. Storage pools, once created, cannot be renamed so - -- this will not be able to become "orphaned". - storage_pool TEXT NOT NULL, + -- One of storage_pool_uuid or storage_type must be set. + -- Storage types are provider sourced, so we do not use a lookup with ID. + -- This constitutes "repeating data" and would tend to indicate + -- bad relational design. However we choose that here over the + -- burden of: + -- - Knowing every possible type up front to populate a look-up or; + -- - Sourcing the lookup from the provider and keeping it updated. + storage_pool_uuid TEXT, + storage_type TEXT, size_mib INT NOT NULL, count INT NOT NULL, + CONSTRAINT chk_unit_storage_specified + CHECK (storage_pool_uuid IS NOT NULL OR storage_type IS NOT NULL), + CONSTRAINT fk_unit_storage_directive_unit + FOREIGN KEY (unit_uuid) + REFERENCES unit (uuid), + CONSTRAINT fk_unit_storage_directive_storage_pool + FOREIGN KEY (storage_pool_uuid) + REFERENCES storage_pool (uuid), CONSTRAINT fk_unit_storage_directive_charm_storage FOREIGN KEY (charm_uuid, storage_name) REFERENCES charm_storage (charm_uuid, name), @@ -97,6 +125,17 @@ CREATE TABLE unit_storage_directive ( CREATE INDEX idx_unit_storage_directive ON unit_storage_directive (unit_uuid); +CREATE VIEW v_unit_storage_directive AS +SELECT + usd.unit_uuid, + usd.charm_uuid, + usd.storage_name, + usd.size_mib, + usd.count, + COALESCE(sp.name, usd.storage_type) AS storage_pool +FROM unit_storage_directive AS usd +LEFT JOIN storage_pool AS sp ON usd.storage_pool_uuid = sp.uuid; + CREATE TABLE storage_instance ( uuid TEXT NOT NULL PRIMARY KEY, charm_uuid TEXT NOT NULL, @@ -104,17 +143,24 @@ CREATE TABLE storage_instance ( -- storage_id is created from the storage name and a unique id number. storage_id TEXT NOT NULL, life_id INT NOT NULL, - -- Note: one might wonder why storage_pool below is not a - -- FK to a row defined in the storage pool table. This value - -- can also be one of the pool types. As with the comment on the - -- type column in the storage pool table, it's problematic to use a lookup - -- with an ID. Storage pools, once created, cannot be renamed so - -- this will not be able to become "orphaned". - storage_pool TEXT NOT NULL, + -- One of storage_pool_uuid or storage_type must be set. + -- Storage types are provider sourced, so we do not use a lookup with ID. + -- This constitutes "repeating data" and would tend to indicate + -- bad relational design. However we choose that here over the + -- burden of: + -- - Knowing every possible type up front to populate a look-up or; + -- - Sourcing the lookup from the provider and keeping it updated. + storage_pool_uuid TEXT, + storage_type TEXT, requested_size_mib INT NOT NULL, + CONSTRAINT chk_storage_instance_storage_specified + CHECK (storage_pool_uuid IS NOT NULL OR storage_type IS NOT NULL), CONSTRAINT fk_storage_instance_life FOREIGN KEY (life_id) REFERENCES life (id), + CONSTRAINT fk_storage_instance_storage_pool + FOREIGN KEY (storage_pool_uuid) + REFERENCES storage_pool (uuid), CONSTRAINT fk_storage_instance_charm_storage FOREIGN KEY (charm_uuid, storage_name) REFERENCES charm_storage (charm_uuid, name) @@ -123,6 +169,17 @@ CREATE TABLE storage_instance ( CREATE UNIQUE INDEX idx_storage_instance_id ON storage_instance (storage_id); +CREATE VIEW v_storage_instance AS +SELECT + si.unit_uuid, + si.charm_uuid, + si.storage_name, + si.size_mib, + si.count, + COALESCE(sp.name, si.storage_type) AS storage_pool +FROM storage_instance AS si +LEFT JOIN storage_pool AS sp ON si.storage_pool_uuid = sp.uuid; + -- storage_unit_owner is used to indicate when -- a unit is the owner of a storage instance. -- This is different to a storage attachment. diff --git a/domain/schema/model_schema_test.go b/domain/schema/model_schema_test.go index 19f311141f1..8970570a833 100644 --- a/domain/schema/model_schema_test.go +++ b/domain/schema/model_schema_test.go @@ -292,6 +292,7 @@ func (s *modelSchemaSuite) TestModelViews(c *gc.C) { "v_application_constraint", "v_application_exposed_endpoint", "v_application_resource", + "v_application_storage_directive", "v_charm_annotation_index", "v_charm_config", "v_charm_container", @@ -318,7 +319,9 @@ func (s *modelSchemaSuite) TestModelViews(c *gc.C) { "v_revision_updater_application", "v_secret_permission", "v_space_subnet", + "v_storage_instance", "v_unit_resource", + "v_unit_storage_directive", ) got := readEntityNames(c, s.DB(), "view") c.Assert(got, jc.SameContents, expected.SortedValues(), gc.Commentf( From 0b832fe8d3db104a76a5418d549614c9a58ef296 Mon Sep 17 00:00:00 2001 From: wallyworld Date: Wed, 26 Feb 2025 20:04:49 +1000 Subject: [PATCH 5/5] refactor: use IN clause to load storage pools for lookup --- domain/application/state/storage.go | 55 +++++++++++++++--------- domain/application/state/types.go | 2 +- domain/schema/model/sql/0011-storage.sql | 7 +-- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/domain/application/state/storage.go b/domain/application/state/storage.go index d4d35f49ebb..e1732703ee5 100644 --- a/domain/application/state/storage.go +++ b/domain/application/state/storage.go @@ -18,6 +18,28 @@ import ( "github.com/juju/juju/internal/storage" ) +func (st *State) loadStoragePoolUUIDByName(ctx context.Context, tx *sqlair.TX, poolNames []string) (map[string]string, error) { + type poolnames []string + storageQuery, err := st.Prepare(` +SELECT &storagePool.* +FROM storage_pool +WHERE name IN ($poolnames[:]) +`, storagePool{}, poolnames{}) + if err != nil { + return nil, errors.Capture(err) + } + var dbPools []storagePool + err = tx.Query(ctx, storageQuery, poolnames(poolNames)).GetAll(&dbPools) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + return nil, errors.Errorf("querying storage pools: %w", err) + } + poolsByName := make(map[string]string) + for _, p := range dbPools { + poolsByName[p.Name] = p.UUID + } + return poolsByName, nil +} + // insertStorage constructs inserts storage directive records for the application. func (st *State) insertStorage(ctx context.Context, tx *sqlair.TX, appDetails applicationDetails, appStorage []application.AddApplicationStorageArg) error { if len(appStorage) == 0 { @@ -34,25 +56,6 @@ WHERE charm_uuid = $applicationDetails.charm_uuid return errors.Capture(err) } - // Storage is either a storage type or a pool name. - // Get a mapping of pool name to pool UUID for all pools. - // The number of pools is always small - we'll limit the size - // of the result set just in case. If there's more than 200 there's - // much bigger problems in play. - storageQuery, err := st.Prepare(`SELECT &storagePool.* FROM storage_pool LIMIT 200`, storagePool{}) - if err != nil { - return errors.Capture(err) - } - var allPools []storagePool - err = tx.Query(ctx, storageQuery).GetAll(&allPools) - if err != nil && !errors.Is(err, sql.ErrNoRows) { - return errors.Errorf("querying all storage pools: %w", err) - } - poolsByName := make(map[string]string) - for _, p := range allPools { - poolsByName[p.Name] = p.UUID - } - var storageMetadata []charmStorage err = tx.Query(ctx, queryStmt, appDetails).GetAll(&storageMetadata) if err != nil && !errors.Is(err, sql.ErrNoRows) { @@ -71,6 +74,18 @@ WHERE charm_uuid = $applicationDetails.charm_uuid return errors.Errorf("storage %q is not supported", unsupportedStorage.SortedValues()) } + // Storage is either a storage type or a pool name. + // Get a mapping of pool name to pool UUID for any + // pools specified in the app storage directives. + poolNames := make([]string, len(appStorage)) + for i, stor := range appStorage { + poolNames[i] = stor.PoolNameOrType + } + poolsByName, err := st.loadStoragePoolUUIDByName(ctx, tx, poolNames) + if err != nil { + return errors.Errorf("loading storage pool UUIDs: %w", err) + } + storage := make([]storageToAdd, len(appStorage)) for i, stor := range appStorage { storage[i] = storageToAdd{ @@ -83,7 +98,7 @@ WHERE charm_uuid = $applicationDetails.charm_uuid // PoolNameOrType has already been validated to either be // a pool name or a valid storage type for the relevant cloud. if uuid, ok := poolsByName[stor.PoolNameOrType]; ok { - storage[i].StoragePool = &uuid + storage[i].StoragePoolUUID = &uuid } else { storage[i].StorageType = &stor.PoolNameOrType } diff --git a/domain/application/state/types.go b/domain/application/state/types.go index 3ba27953e59..2c00944d275 100644 --- a/domain/application/state/types.go +++ b/domain/application/state/types.go @@ -649,7 +649,7 @@ type storageToAdd struct { ApplicationUUID string `db:"application_uuid"` CharmUUID string `db:"charm_uuid"` StorageName string `db:"storage_name"` - StoragePool *string `db:"storage_pool_uuid"` + StoragePoolUUID *string `db:"storage_pool_uuid"` StorageType *string `db:"storage_type"` Size uint `db:"size_mib"` Count uint `db:"count"` diff --git a/domain/schema/model/sql/0011-storage.sql b/domain/schema/model/sql/0011-storage.sql index ae1b9aae559..56fc19c1a60 100644 --- a/domain/schema/model/sql/0011-storage.sql +++ b/domain/schema/model/sql/0011-storage.sql @@ -171,11 +171,12 @@ ON storage_instance (storage_id); CREATE VIEW v_storage_instance AS SELECT - si.unit_uuid, + si.uuid, si.charm_uuid, si.storage_name, - si.size_mib, - si.count, + si.storage_id, + si.life_id, + si.requested_size_mib, COALESCE(sp.name, si.storage_type) AS storage_pool FROM storage_instance AS si LEFT JOIN storage_pool AS sp ON si.storage_pool_uuid = sp.uuid;