Merge pull request #5420 from travisn/osd-node-name

OSD on PVC node assignment should be the node selector rather than nodeName

pkg/operator/ceph/cluster/osd/osd.go

@@ -636,7 +636,8 @@ func (c *Cluster) getOSDPropsForPVC(pvcName string) (osdProperties, error) {
 				portable:            volumeSource.Portable,
 				tuneSlowDeviceClass: volumeSource.TuneSlowDeviceClass,
 			}
-			// If OSD isn't portable, we're getting the host name of the pod where the osd prepare job pod prepared the OSD.
+			// If OSD isn't portable, we're getting the host name either from the osd deployment that was already initialized
+			// or from the osd prepare job from initial creation.
 			if !volumeSource.Portable {
 				var err error
 				osdProps.crushHostname, err = c.getPVCHostName(pvcName)
@@ -650,21 +651,46 @@ func (c *Cluster) getOSDPropsForPVC(pvcName string) (osdProperties, error) {
 	return osdProperties{}, errors.Errorf("no valid VolumeSource found for pvc %s", pvcName)
 }
 
+// getPVCHostName finds the node where an OSD pod should be assigned with a node selector.
+// First look for the node selector that was previously used for the OSD, or if a new OSD
+// check for the assignment of the OSD prepare job.
 func (c *Cluster) getPVCHostName(pvcName string) (string, error) {
 	listOpts := metav1.ListOptions{LabelSelector: fmt.Sprintf("%s=%s", OSDOverPVCLabelKey, pvcName)}
-	podList, err := c.context.Clientset.CoreV1().Pods(c.Namespace).List(listOpts)
+
+	// Check for the existence of the OSD deployment where the node selector was applied
+	// in a previous reconcile.
+	deployments, err := c.context.Clientset.AppsV1().Deployments(c.Namespace).List(listOpts)
 	if err != nil {
-		return "", err
+		return "", errors.Wrapf(err, "failed to get deployment for osd with pvc %q", pvcName)
 	}
-	for _, pod := range podList.Items {
+	for _, d := range deployments.Items {
+		selectors := d.Spec.Template.Spec.NodeSelector
+		for label, value := range selectors {
+			if label == v1.LabelHostname {
+				return value, nil
+			}
+		}
+	}
+
+	// Since the deployment wasn't found it must be a new deployment so look at the node
+	// assignment of the OSD prepare pod
+	pods, err := c.context.Clientset.CoreV1().Pods(c.Namespace).List(listOpts)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to get pod for osd with pvc %q", pvcName)
+	}
+	for _, pod := range pods.Items {
 		name, err := k8sutil.GetNodeHostName(c.context.Clientset, pod.Spec.NodeName)
 		if err != nil {
 			logger.Warningf("falling back to node name %s since hostname not found for node", pod.Spec.NodeName)
 			name = pod.Spec.NodeName
 		}
+		if name == "" {
+			return "", errors.Errorf("node name not found on the osd pod %q", pod.Name)
+		}
 		return name, nil
 	}
-	return "", err
+
+	return "", errors.Errorf("node selector not found on deployment for osd with pvc %q", pvcName)
 }
 
 func (c *Cluster) getOSDInfo(d *apps.Deployment) ([]OSDInfo, error) {

pkg/operator/ceph/cluster/osd/osd_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/rook/rook/pkg/operator/k8sutil"
 	exectest "github.com/rook/rook/pkg/util/exec/test"
 	"github.com/stretchr/testify/assert"
+	apps "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -301,6 +302,55 @@ func TestAddNodeFailure(t *testing.T) {
 	assert.NotNil(t, startErr)
 }
 
+func TestGetPVCHostName(t *testing.T) {
+	clientset := fake.NewSimpleClientset()
+	c := &Cluster{context: &clusterd.Context{Clientset: clientset}, Namespace: "ns"}
+	pvcName := "test-pvc"
+
+	// fail to get the host name when there is no pod or deployment
+	name, err := c.getPVCHostName(pvcName)
+	assert.Error(t, err)
+	assert.Equal(t, "", name)
+
+	// Create a sample osd deployment
+	osdDeployment := &apps.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "osd-23",
+			Namespace: c.Namespace,
+			Labels:    c.getOSDLabels(23, "", true),
+		},
+	}
+	k8sutil.AddLabelToDeployment(OSDOverPVCLabelKey, pvcName, osdDeployment)
+	osdDeployment.Spec.Template.Spec.NodeSelector = map[string]string{v1.LabelHostname: "testnode"}
+
+	_, err = clientset.AppsV1().Deployments(c.Namespace).Create(osdDeployment)
+	assert.NoError(t, err)
+
+	// get the host name based on the deployment
+	name, err = c.getPVCHostName(pvcName)
+	assert.NoError(t, err)
+	assert.Equal(t, "testnode", name)
+
+	// delete the deployment and get the host name based on the pod
+	err = clientset.AppsV1().Deployments(c.Namespace).Delete(osdDeployment.Name, &metav1.DeleteOptions{})
+	assert.NoError(t, err)
+	osdPod := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "osd-23",
+			Namespace: c.Namespace,
+			Labels:    c.getOSDLabels(23, "", true),
+		},
+	}
+	osdPod.Labels = map[string]string{OSDOverPVCLabelKey: pvcName}
+	osdPod.Spec.NodeName = "testnode"
+	_, err = clientset.CoreV1().Pods(c.Namespace).Create(osdPod)
+	assert.NoError(t, err)
+
+	name, err = c.getPVCHostName(pvcName)
+	assert.NoError(t, err)
+	assert.Equal(t, "testnode", name)
+}
+
 func TestGetOSDInfo(t *testing.T) {
 	c := New(&cephconfig.ClusterInfo{}, &clusterd.Context{}, "ns", "myversion", cephv1.CephVersionSpec{},
 		rookv1.StorageScopeSpec{}, "", rookv1.Placement{}, rookv1.Annotations{}, cephv1.NetworkSpec{},

pkg/operator/ceph/cluster/osd/spec.go

@@ -471,7 +471,7 @@ func (c *Cluster) makeDeployment(osdProps osdProperties, osd OSDInfo, provisionC
 		},
 	}
 	if osdProps.onPVC() {
-		k8sutil.AddLabelToDeployement(OSDOverPVCLabelKey, osdProps.pvc.ClaimName, deployment)
+		k8sutil.AddLabelToDeployment(OSDOverPVCLabelKey, osdProps.pvc.ClaimName, deployment)
 		k8sutil.AddLabelToPod(OSDOverPVCLabelKey, osdProps.pvc.ClaimName, &deployment.Spec.Template)
 	}
 	if !osdProps.portable {

pkg/operator/k8sutil/deployment.go

@@ -220,7 +220,7 @@ func AddRookVersionLabelToObjectMeta(meta *metav1.ObjectMeta) {
 	addRookVersionLabel(meta.Labels)
 }
 
-func AddLabelToDeployement(key, value string, d *v1.Deployment) {
+func AddLabelToDeployment(key, value string, d *v1.Deployment) {
 	if d == nil {
 		return
 	}