[lxc-devel] [lxd/master] Drop the storage_pool_id filter from the WHERE clause of StoragePoolsConfig

[freeekanayaka on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 828 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20180614/c75df2af/attachment.bin>
-------------- next part --------------
From 009493ce0d4027b1c2673426629317a4e2e53f91 Mon Sep 17 00:00:00 2001
From: Free Ekanayaka <free.ekanayaka at canonical.com>
Date: Thu, 14 Jun 2018 12:28:18 +0000
Subject: [PATCH] Drop the storage_pool_id filter from the WHERE clause of
 StoragePoolsConfig

The filter was broken since it should have been "storage_pools_config.node_id=?"
instead.

However, the filter is not necessary since the only call site that this method
has (in cluster/membership.go) already filters out any config key which is not
node-specific.

Since this method is going to have a new call site for the new join API, that
requires all config keys (not only the node-specific ones), I dropped the filter
altogher.

Signed-off-by: Free Ekanayaka <free.ekanayaka at canonical.com>
---
 lxd/db/storage_pools.go      |  8 +-------
 lxd/db/storage_pools_test.go | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/lxd/db/storage_pools.go b/lxd/db/storage_pools.go
index 00f5160e8..c4795e2fb 100644
--- a/lxd/db/storage_pools.go
+++ b/lxd/db/storage_pools.go
@@ -14,10 +14,6 @@ import (
 
 // StoragePoolConfigs returns a map associating each storage pool name to its
 // config values.
-//
-// The config values are the ones defined for the node this function is run
-// on. They are used by cluster.Join when a new node joins the cluster and its
-// configuration needs to be migrated to the cluster database.
 func (c *ClusterTx) StoragePoolConfigs() (map[string]map[string]string, error) {
 	names, err := query.SelectStrings(c.tx, "SELECT name FROM storage_pools")
 	if err != nil {
@@ -28,9 +24,7 @@ func (c *ClusterTx) StoragePoolConfigs() (map[string]map[string]string, error) {
 		table := `
 storage_pools_config JOIN storage_pools ON storage_pools.id=storage_pools_config.storage_pool_id
 `
-		config, err := query.SelectConfig(
-			c.tx, table, "storage_pools.name=? AND storage_pools_config.storage_pool_id=?",
-			name, c.nodeID)
+		config, err := query.SelectConfig(c.tx, table, "storage_pools.name=?", name)
 		if err != nil {
 			return nil, err
 		}
diff --git a/lxd/db/storage_pools_test.go b/lxd/db/storage_pools_test.go
index 11e503f08..d455a64b3 100644
--- a/lxd/db/storage_pools_test.go
+++ b/lxd/db/storage_pools_test.go
@@ -8,6 +8,42 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// The StoragePoolsConfigs method returns only node-specific config values.
+func TestStoragePoolConfigs(t *testing.T) {
+	cluster, cleanup := db.NewTestCluster(t)
+	defer cleanup()
+
+	// Create a storage pool named "local" (like the default LXD clustering
+	// one), then delete it and create another one.
+	_, err := cluster.StoragePoolCreate("local", "", "dir", map[string]string{
+		"source": "/foo/bar",
+	})
+	require.NoError(t, err)
+
+	_, err = cluster.StoragePoolDelete("local")
+	require.NoError(t, err)
+
+	_, err = cluster.StoragePoolCreate("BTRFS", "", "dir", map[string]string{
+		"source": "/egg/baz",
+	})
+	require.NoError(t, err)
+
+	// Check that the config map returned by StoragePoolsConfigs actually
+	// contains the value of the "BTRFS" storage pool.
+	var config map[string]map[string]string
+
+	err = cluster.Transaction(func(tx *db.ClusterTx) error {
+		var err error
+		config, err = tx.StoragePoolConfigs()
+		return err
+	})
+	require.NoError(t, err)
+
+	assert.Equal(t, config, map[string]map[string]string{
+		"BTRFS": map[string]string{"source": "/egg/baz"},
+	})
+}
+
 func TestStoragePoolsCreatePending(t *testing.T) {
 	tx, cleanup := db.NewTestClusterTx(t)
 	defer cleanup()
@@ -116,7 +152,7 @@ func TestStoragePoolVolume_Ceph(t *testing.T) {
 	cluster, cleanup := db.NewTestCluster(t)
 	defer cleanup()
 
-	// Create a second no (beyond the default one).
+	// Create a second node (beyond the default one).
 	err := cluster.Transaction(func(tx *db.ClusterTx) error {
 		_, err := tx.NodeAdd("n1", "1.2.3.4:666")
 		return err