PWX-32580: Do not create pre-flight storage node objects for nodes wh…

pkg/controller/storagecluster/controller_test.go

@@ -5,7 +5,9 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"sort"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -9132,6 +9134,187 @@ func TestEKSPreflightCheck(t *testing.T) {
 	require.Equal(t, corev1.ClusterConditionStatusInProgress, condition.Status)
 }
 
+func TestPreflightStorageNodeCreation(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	driverName := "mock-driver"
+	cluster := createStorageCluster()
+
+	cluster.Spec.Placement.NodeAffinity = &v1.NodeAffinity{
+		RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{
+			NodeSelectorTerms: []v1.NodeSelectorTerm{
+				{
+					MatchExpressions: []v1.NodeSelectorRequirement{
+						{
+							Key:      "px/enabled",
+							Operator: v1.NodeSelectorOpNotIn,
+							Values:   []string{"false"},
+						},
+						{
+							Key:      k8s.NodeRoleLabelControlPlane,
+							Operator: v1.NodeSelectorOpExists,
+						},
+						{
+							Key:      k8s.NodeRoleLabelWorker,
+							Operator: v1.NodeSelectorOpExists,
+						},
+					},
+				},
+				{
+					MatchExpressions: []v1.NodeSelectorRequirement{
+						{
+							Key:      "px/enabled",
+							Operator: v1.NodeSelectorOpNotIn,
+							Values:   []string{"false"},
+						},
+						{
+							Key:      k8s.NodeRoleLabelMaster,
+							Operator: v1.NodeSelectorOpDoesNotExist,
+						},
+						{
+							Key:      k8s.NodeRoleLabelControlPlane,
+							Operator: v1.NodeSelectorOpDoesNotExist,
+						},
+						{
+							Key:      k8s.NodeRoleLabelInfra,
+							Operator: v1.NodeSelectorOpDoesNotExist,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	k8sNode1 := createK8sNode("k8s-node-1", 1)
+	k8sNode2 := createK8sNode("k8s-node-2", 1)
+	k8sNode3 := createK8sNode("k8s-node-3", 1)
+	k8sNode4 := createK8sNode("k8s-node-4", 1)
+
+	k8sNode1.Labels["node-role.kubernetes.io/worker"] = ""
+	k8sNode2.Labels["node-role.kubernetes.io/worker"] = ""
+	k8sNode3.Labels["node-role.kubernetes.io/control-plane"] = ""
+	k8sNode4.Labels["node-role.kubernetes.io/worker"] = ""
+
+	driver := testutil.MockDriver(mockCtrl)
+	k8sClient := testutil.FakeK8sClient(cluster, k8sNode1, k8sNode2, k8sNode3, k8sNode4)
+
+	driver.EXPECT().GetSelectorLabels().Return(nil).AnyTimes()
+	driver.EXPECT().String().Return(driverName).AnyTimes()
+
+	controller := Controller{
+		client: k8sClient,
+		Driver: driver,
+	}
+
+	storageNodeNames := func(storageNodes *corev1.StorageNodeList) []string {
+		sNames := []string{}
+
+		for _, snode := range storageNodes.Items {
+			sNames = append(sNames, snode.Name)
+		}
+		sort.Strings(sNames)
+		return sNames
+	}
+
+	logrus.Infof("Skipping only 'control-plane' node")
+	expectedStorageNodeNames := []string{"k8s-node-1", "k8s-node-2", "k8s-node-4"}
+	err := controller.createPreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	storageNodes := &corev1.StorageNodeList{}
+	err = testutil.List(k8sClient, storageNodes)
+	require.NoError(t, err)
+	require.Len(t, storageNodes.Items, len(expectedStorageNodeNames))
+	sNames := storageNodeNames(storageNodes)
+	require.Equal(t, strings.Join(expectedStorageNodeNames, " "), strings.Join(sNames, " "))
+
+	err = controller.deletePreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	logrus.Infof("Skipping 'control-plane' node & 'px/enabled=false' node")
+	k8sNode2.Labels["px/enabled"] = "false"
+	err = k8sClient.Update(context.TODO(), k8sNode2)
+	require.NoError(t, err)
+
+	expectedStorageNodeNames = []string{"k8s-node-1", "k8s-node-4"}
+
+	err = controller.createPreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	storageNodes = &corev1.StorageNodeList{}
+	err = testutil.List(k8sClient, storageNodes)
+	require.NoError(t, err)
+	require.Len(t, storageNodes.Items, len(expectedStorageNodeNames))
+	sNames = storageNodeNames(storageNodes)
+	require.Equal(t, strings.Join(expectedStorageNodeNames, " "), strings.Join(sNames, " "))
+
+	err = controller.deletePreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	logrus.Infof("Skipping 'master' node & creating storage node for 'control-plane + worker'")
+	// Remove px/enabled=false
+	k8sNode2.Labels = map[string]string{"node-role.kubernetes.io/worker": ""}
+	err = k8sClient.Update(context.TODO(), k8sNode2)
+	require.NoError(t, err)
+
+	// Add 'worker' to the 'control-plane' node
+	k8sNode3.Labels["node-role.kubernetes.io/worker"] = ""
+	err = k8sClient.Update(context.TODO(), k8sNode3)
+	require.NoError(t, err)
+
+	// Add 'master'
+	k8sNode1.Labels = map[string]string{k8s.NodeRoleLabelMaster: ""}
+	err = k8sClient.Update(context.TODO(), k8sNode1)
+	require.NoError(t, err)
+
+	expectedStorageNodeNames = []string{"k8s-node-2", "k8s-node-3", "k8s-node-4"}
+
+	err = controller.createPreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	storageNodes = &corev1.StorageNodeList{}
+	err = testutil.List(k8sClient, storageNodes)
+	require.NoError(t, err)
+	require.Len(t, storageNodes.Items, len(expectedStorageNodeNames))
+	sNames = storageNodeNames(storageNodes)
+	require.Equal(t, strings.Join(expectedStorageNodeNames, " "), strings.Join(sNames, " "))
+
+	err = controller.deletePreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	logrus.Infof("Skipping 'infra' node only")
+	// Remove 'master'
+	k8sNode1.Labels = map[string]string{"node-role.kubernetes.io/worker": ""}
+	err = k8sClient.Update(context.TODO(), k8sNode1)
+	require.NoError(t, err)
+
+	// Remove control-plane label
+	k8sNode3.Labels = map[string]string{"node-role.kubernetes.io/worker": ""}
+	err = k8sClient.Update(context.TODO(), k8sNode3)
+	require.NoError(t, err)
+
+	// Add 'infra'
+	k8sNode4.Labels = map[string]string{k8s.NodeRoleLabelInfra: ""}
+	err = k8sClient.Update(context.TODO(), k8sNode4)
+	require.NoError(t, err)
+
+	expectedStorageNodeNames = []string{"k8s-node-1", "k8s-node-2", "k8s-node-3"}
+
+	err = controller.createPreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+
+	storageNodes = &corev1.StorageNodeList{}
+	err = testutil.List(k8sClient, storageNodes)
+	require.NoError(t, err)
+	require.Len(t, storageNodes.Items, len(expectedStorageNodeNames))
+	sNames = storageNodeNames(storageNodes)
+	require.Equal(t, strings.Join(expectedStorageNodeNames, " "), strings.Join(sNames, " "))
+
+	err = controller.deletePreFlightStorageNodes(cluster)
+	require.NoError(t, err)
+}
+
 func TestStorageClusterStateDuringValidation(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	cluster := &corev1.StorageCluster{

pkg/controller/storagecluster/storagecluster.go

@@ -63,7 +63,6 @@
 	"github.com/libopenstorage/operator/pkg/preflight"
 	"github.com/libopenstorage/operator/pkg/util"
 	"github.com/libopenstorage/operator/pkg/util/k8s"
-	coreops "github.com/portworx/sched-ops/k8s/core"
 )
 
 const (
@@ -437,26 +436,57 @@
 	return false
 }
 
+func (c *Controller) createPreFlightStorageNodes(toUpdate *corev1.StorageCluster) error {
+	k8sNodeList := &v1.NodeList{}
+	err := c.client.List(context.TODO(), k8sNodeList)
+	if err != nil {
+		return fmt.Errorf("failed to get cluster nodes for preflight storag node creation: %v", err)
+	}
+
+	// Create StorageNodes to return pre-flight checks used by c.Driver.Validate().
+	for _, node := range k8sNodeList.Items {
+		shouldRun, _, err := c.nodeShouldRunStoragePod(&node, toUpdate)
+		if err != nil {
+			logrus.Warnf("Skipping pre-flight storage node entry for %s node. Error: %v", node.Name, err)
+			continue
+		}
+
+		if shouldRun {
+			logrus.Infof("Create pre-flight storage node entry for node: %s", node.Name)
+			c.createStorageNode(toUpdate, node.Name)
+		} else {
+			logrus.Warnf("Skipping pre-flight storage node entry for node: %s", node.Name)
+		}
+	}
+	return nil
+}
+
+func (c *Controller) deletePreFlightStorageNodes(toUpdate *corev1.StorageCluster) error {
+	storageNodes := &corev1.StorageNodeList{}
+	err := c.client.List(context.TODO(), storageNodes,
+		&client.ListOptions{Namespace: toUpdate.Namespace})
+	if err != nil {
+		return fmt.Errorf("failed to get StorageNodes used for preflight validation: %v", err)
+	}
+
+	for _, storageNode := range storageNodes.Items {
+		logrus.Infof("Delete validate() storage node entry for node: %s", storageNode.Name)
+		err = c.client.Delete(context.TODO(), storageNode.DeepCopy())
+		if err != nil {
+			logrus.WithError(err).Errorf("failed to delete storage node entry %s: %v", storageNode.Name, err)
+		}
+	}
+	return nil
+}
+
 func (c *Controller) driverValidate(toUpdate *corev1.StorageCluster) (*corev1.ClusterCondition, error) {
 	storageNodes := &corev1.StorageNodeList{}
 	serr := c.client.List(context.TODO(), storageNodes,
 		&client.ListOptions{Namespace: toUpdate.Namespace})
 	if serr == nil && len(storageNodes.Items) == 0 { // Only do if storageNodes don't exist
-		k8sNodeList := &v1.NodeList{}
-		err := c.client.List(context.TODO(), k8sNodeList)
-		if err == nil {
-			// Create StorageNodes to return pre-flight checks used by c.Driver.Validate().
-			for _, node := range k8sNodeList.Items {
-				if !coreops.Instance().IsNodeMaster(node) {
-					logrus.Infof("Create pre-flight storage node entry for node: %s", node.Name)
-					c.createStorageNode(toUpdate, node.Name)
-				} else {
-					logrus.Infof("Skipping pre-flight storage node entry for master node: %s", node.Name)
-				}
-			}
-		} else {
-			logrus.WithError(err).Errorf("Failed to get cluster nodes")
-			// *** Should we just return an error on pre-flight here? ***
+		serr = c.createPreFlightStorageNodes(toUpdate)
+		if serr != nil {
+			logrus.WithError(serr).Errorf("Failed to create preflight storage nodes")
 		}
 	}
 
@@ -483,18 +513,8 @@
 
 	if condition.Status != corev1.ClusterConditionStatusInProgress { // Status not in progress must be done or an issue occurred
 		// Delete StorageNodes created for c.Driver.Validate() checks.
-		storageNodes = &corev1.StorageNodeList{}
-		serr = c.client.List(context.TODO(), storageNodes,
-			&client.ListOptions{Namespace: toUpdate.Namespace})
-		if serr == nil {
-			for _, storageNode := range storageNodes.Items {
-				logrus.Infof("Delete validate() storage node entry for node: %s", storageNode.Name)
-				serr = c.client.Delete(context.TODO(), storageNode.DeepCopy())
-				if serr != nil {
-					logrus.WithError(serr).Errorf("failed to delete storage node entry %s: %v", storageNode.Name, serr)
-				}
-			}
-		} else {
+		serr = c.deletePreFlightStorageNodes(toUpdate)
+		if serr != nil {
 			logrus.WithError(serr).Errorf("Failed to get StorageNodes used for validate.")
 		}
 	}