[lxc-devel] [lxd/master] Device: Improves readability of disk device validation

tomponline on Github lxc-bot at linuxcontainers.org
Thu Dec 17 17:21:36 UTC 2020


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 386 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20201217/67a31dcc/attachment-0001.bin>
-------------- next part --------------
From 354df8872444809406ccfd14bff49401b07e52de Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 17 Dec 2020 15:08:34 +0000
Subject: [PATCH 1/6] lxd/db/cluster/update: Modifies updateFromV43 and
 updateFromV42 to use IFNULL(node_id, -1) to avoid nodes with 0 ID

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/db/cluster/update.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lxd/db/cluster/update.go b/lxd/db/cluster/update.go
index 614f4845a9..9dbf256818 100644
--- a/lxd/db/cluster/update.go
+++ b/lxd/db/cluster/update.go
@@ -88,7 +88,7 @@ var updates = map[int]schema.Update{
 // This can occur when multiple create requests have been issued when setting up a clustered storage pool.
 func updateFromV42(tx *sql.Tx) error {
 	// Find all duplicated config rows and return comma delimited list of affected row IDs for each dupe set.
-	stmt, err := tx.Prepare(`SELECT storage_pool_id, COALESCE(node_id,0), key, value, COUNT(*) AS rowCount, GROUP_CONCAT(id, ",") AS dupeRowIDs
+	stmt, err := tx.Prepare(`SELECT storage_pool_id, IFNULL(node_id, -1), key, value, COUNT(*) AS rowCount, GROUP_CONCAT(id, ",") AS dupeRowIDs
 			FROM storage_pools_config
 			GROUP BY storage_pool_id, node_id, key, value
 			HAVING rowCount > 1
@@ -157,7 +157,7 @@ func updateFromV42(tx *sql.Tx) error {
 // This can occur when multiple create requests have been issued when setting up a clustered network.
 func updateFromV41(tx *sql.Tx) error {
 	// Find all duplicated config rows and return comma delimited list of affected row IDs for each dupe set.
-	stmt, err := tx.Prepare(`SELECT network_id, COALESCE(node_id,0), key, value, COUNT(*) AS rowCount, GROUP_CONCAT(id, ",") AS dupeRowIDs
+	stmt, err := tx.Prepare(`SELECT network_id, IFNULL(node_id, -1), key, value, COUNT(*) AS rowCount, GROUP_CONCAT(id, ",") AS dupeRowIDs
 			FROM networks_config
 			GROUP BY network_id, node_id, key, value
 			HAVING rowCount > 1

From 245c92e80fa301ee429ec700aa800d97e357ff30 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 17 Dec 2020 14:55:57 +0000
Subject: [PATCH 2/6] lxd/db/cluster: Adds updateFromV43 patch that adds unique
 index to storage_pools_config and networks_config table

Prevents duplicate config rows for the same node and key being inserted.

Fixes #8260

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/db/cluster/schema.go |  4 +++-
 lxd/db/cluster/update.go | 13 +++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/lxd/db/cluster/schema.go b/lxd/db/cluster/schema.go
index 897ea06d22..c0165dd17c 100644
--- a/lxd/db/cluster/schema.go
+++ b/lxd/db/cluster/schema.go
@@ -304,6 +304,7 @@ CREATE TABLE "networks_nodes" (
     FOREIGN KEY (network_id) REFERENCES "networks" (id) ON DELETE CASCADE,
     FOREIGN KEY (node_id) REFERENCES nodes (id) ON DELETE CASCADE
 );
+CREATE UNIQUE INDEX networks_unique_network_id_node_id_key ON networks_config (network_id, IFNULL(node_id, -1), key);
 CREATE TABLE nodes (
     id INTEGER PRIMARY KEY,
     name TEXT NOT NULL,
@@ -495,6 +496,7 @@ CREATE TABLE storage_pools_nodes (
     FOREIGN KEY (storage_pool_id) REFERENCES storage_pools (id) ON DELETE CASCADE,
     FOREIGN KEY (node_id) REFERENCES nodes (id) ON DELETE CASCADE
 );
+CREATE UNIQUE INDEX storage_pools_unique_storage_pool_id_node_id_key ON storage_pools_config (storage_pool_id, IFNULL(node_id, -1), key);
 CREATE TABLE "storage_volumes" (
     id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
     name TEXT NOT NULL,
@@ -591,5 +593,5 @@ CREATE TABLE storage_volumes_snapshots_config (
     UNIQUE (storage_volume_snapshot_id, key)
 );
 
-INSERT INTO schema (version, updated_at) VALUES (43, strftime("%s"))
+INSERT INTO schema (version, updated_at) VALUES (44, strftime("%s"))
 `
diff --git a/lxd/db/cluster/update.go b/lxd/db/cluster/update.go
index 9dbf256818..a64fefc1a7 100644
--- a/lxd/db/cluster/update.go
+++ b/lxd/db/cluster/update.go
@@ -82,6 +82,19 @@ var updates = map[int]schema.Update{
 	41: updateFromV40,
 	42: updateFromV41,
 	43: updateFromV42,
+	44: updateFromV43,
+}
+
+// updateFromV43 adds a unique index to the storage_pools_config and networks_config tables.
+func updateFromV43(tx *sql.Tx) error {
+	_, err := tx.Exec(`CREATE UNIQUE INDEX storage_pools_unique_storage_pool_id_node_id_key ON storage_pools_config (storage_pool_id, IFNULL(node_id, -1), key);
+		CREATE UNIQUE INDEX networks_unique_network_id_node_id_key ON networks_config (network_id, IFNULL(node_id, -1), key);
+	`)
+	if err != nil {
+		return errors.Wrapf(err, "Failed adding unique index to storage_pools_config and networks_config tables")
+	}
+
+	return nil
 }
 
 // updateFromV42 removes any duplicated storage pool config rows that have the same value.

From 528c3c6cd9383e4f74e28cbae08ad766890b23cf Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 17 Dec 2020 17:19:30 +0000
Subject: [PATCH 3/6] shared/util: Adds StringHasPrefix function

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 shared/util.go | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/shared/util.go b/shared/util.go
index 66fba3f287..b7561ceb7c 100644
--- a/shared/util.go
+++ b/shared/util.go
@@ -621,6 +621,16 @@ func StringInSlice(key string, list []string) bool {
 	return false
 }
 
+// StringHasPrefix returns true if value has one of the supplied prefixes.
+func StringHasPrefix(value string, prefixes ...string) bool {
+	for _, prefix := range prefixes {
+		if strings.HasPrefix(value, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 func IntInSlice(key int, list []int) bool {
 	for _, entry := range list {
 		if entry == key {

From ba2d8f96a95f3c708d1d7034d6648afe75437927 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 17 Dec 2020 17:19:46 +0000
Subject: [PATCH 4/6] lxd/device/disk: Adds sourceIsLocalPath function

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/device/disk.go | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lxd/device/disk.go b/lxd/device/disk.go
index dc37ddad17..aace0c7a10 100644
--- a/lxd/device/disk.go
+++ b/lxd/device/disk.go
@@ -58,6 +58,24 @@ func (d *disk) isRequired(devConfig deviceConfig.Device) bool {
 	return false
 }
 
+// sourceIsLocalPath returns true if the source supplied should be considered a local path on the host.
+// It returns false if the disk source is empty, a VM cloud-init config drive, or a remote ceph/cephfs path.
+func (d *disk) sourceIsLocalPath(source string) bool {
+	if source == "" {
+		return false
+	}
+
+	if source == diskSourceCloudInit {
+		return false
+	}
+
+	if shared.StringHasPrefix(d.config["source"], "ceph:", "cephfs:") {
+		return false
+	}
+
+	return true
+}
+
 // validateConfig checks the supplied config for correctness.
 func (d *disk) validateConfig(instConf instance.ConfigReader) error {
 	if !instanceSupported(instConf.Type(), instancetype.Container, instancetype.VM) {

From 96737423117df32dbc4f8d3ca48d6cf78d0dc6d9 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 17 Dec 2020 17:20:00 +0000
Subject: [PATCH 5/6] lxd/device/disk: Use shared.StringHasPrefix when
 validating ceph/cephfs prefixes

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/device/disk.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lxd/device/disk.go b/lxd/device/disk.go
index aace0c7a10..dcfe7c96fb 100644
--- a/lxd/device/disk.go
+++ b/lxd/device/disk.go
@@ -148,7 +148,7 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
 	}
 
 	// Check ceph options are only used when ceph or cephfs type source is specified.
-	if !(strings.HasPrefix(d.config["source"], "ceph:") || strings.HasPrefix(d.config["source"], "cephfs:")) && (d.config["ceph.cluster_name"] != "" || d.config["ceph.user_name"] != "") {
+	if !shared.StringHasPrefix(d.config["source"], "ceph:", "cephfs:") && (d.config["ceph.cluster_name"] != "" || d.config["ceph.user_name"] != "") {
 		return fmt.Errorf("Invalid options ceph.cluster_name/ceph.user_name for source %q", d.config["source"])
 	}
 

From 36533867c873ceca1a2227a511455b7f7cbde7ff Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 17 Dec 2020 17:20:16 +0000
Subject: [PATCH 6/6] lxd/device/disk: Use d.sourceIsLocalPath when validating
 source host path exists

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/device/disk.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lxd/device/disk.go b/lxd/device/disk.go
index dcfe7c96fb..440b21f103 100644
--- a/lxd/device/disk.go
+++ b/lxd/device/disk.go
@@ -173,8 +173,8 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
 	// source path exists when the disk device is required, is not an external ceph/cephfs source and is not a
 	// VM cloud-init drive. We only check this when an instance is loaded to avoid validating snapshot configs
 	// that may contain older config that no longer exists which can prevent migrations.
-	if d.inst != nil && d.config["pool"] == "" && d.config["source"] != "" && d.config["source"] != diskSourceCloudInit && d.isRequired(d.config) && !shared.PathExists(shared.HostPath(d.config["source"])) && !strings.HasPrefix(d.config["source"], "ceph:") && !strings.HasPrefix(d.config["source"], "cephfs:") {
-		return fmt.Errorf("Missing source %q for disk %q", d.config["source"], d.name)
+	if d.inst != nil && d.config["pool"] == "" && d.isRequired(d.config) && d.sourceIsLocalPath(d.config["source"]) && !shared.PathExists(shared.HostPath(d.config["source"])) {
+		return fmt.Errorf("Missing source path %q for disk %q", d.config["source"], d.name)
 	}
 
 	if d.config["pool"] != "" {


More information about the lxc-devel mailing list