issue-8878: relief node os deduction error checks (#8891)

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>

changelogs/unreleased/8891-Lyndon-Li

@@ -0,0 +1 @@
+Fix issue 8878, relief node os deduction error checks

pkg/controller/data_upload_controller.go

@@ -810,10 +810,7 @@ func (r *DataUploadReconciler) setupExposeParam(du *velerov2alpha1api.DataUpload
 			return nil, errors.Wrapf(err, "failed to get PVC %s/%s", du.Spec.SourceNamespace, du.Spec.SourcePVC)
 		}
 
-		nodeOS, err := kube.GetPVCAttachingNodeOS(pvc, r.kubeClient.CoreV1(), r.kubeClient.StorageV1(), log)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to get attaching node OS for PVC %s/%s", du.Spec.SourceNamespace, du.Spec.SourcePVC)
-		}
+		nodeOS := kube.GetPVCAttachingNodeOS(pvc, r.kubeClient.CoreV1(), r.kubeClient.StorageV1(), log)
 
 		if err := kube.HasNodeWithOS(context.Background(), nodeOS, r.kubeClient.CoreV1()); err != nil {
 			return nil, errors.Wrapf(err, "no appropriate node to run data upload for PVC %s/%s", du.Spec.SourceNamespace, du.Spec.SourcePVC)

pkg/controller/data_upload_controller_test.go

@@ -408,16 +408,6 @@ func TestReconcile(t *testing.T) {
 			expectedRequeue:   ctrl.Result{},
 			expectedErrMsg:    "failed to get PVC",
 		},
-		{
-			name:              "Dataupload should fail to get PVC attaching node",
-			du:                dataUploadBuilder().Result(),
-			pod:               builder.ForPod("fake-ns", dataUploadName).Volumes(&corev1api.Volume{Name: "test-pvc"}).Result(),
-			pvc:               builder.ForPersistentVolumeClaim("fake-ns", "test-pvc").StorageClass("fake-sc").Result(),
-			expectedProcessed: true,
-			expected:          dataUploadBuilder().Phase(velerov2alpha1api.DataUploadPhaseFailed).Result(),
-			expectedRequeue:   ctrl.Result{},
-			expectedErrMsg:    "error to get storage class",
-		},
 		{
 			name:              "Dataupload should fail because expected node doesn't exist",
 			du:                dataUploadBuilder().Result(),

pkg/util/kube/pvc_pv.go

@@ -467,13 +467,13 @@ func DiagnosePV(pv *corev1api.PersistentVolume) string {
 }
 
 func GetPVCAttachingNodeOS(pvc *corev1api.PersistentVolumeClaim, nodeClient corev1client.CoreV1Interface,
-	storageClient storagev1.StorageV1Interface, log logrus.FieldLogger) (string, error) {
+	storageClient storagev1.StorageV1Interface, log logrus.FieldLogger) string {
 	var nodeOS string
 	var scFsType string
 
 	if pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == corev1api.PersistentVolumeBlock {
 		log.Infof("Use linux node for block mode PVC %s/%s", pvc.Namespace, pvc.Name)
-		return NodeOSLinux, nil
+		return NodeOSLinux
 	}
 
 	if pvc.Spec.VolumeName == "" {
@@ -485,53 +485,53 @@ func GetPVCAttachingNodeOS(pvc *corev1api.PersistentVolumeClaim, nodeClient core
 	}
 
 	nodeName := ""
-	if value := pvc.Annotations[KubeAnnSelectedNode]; value != "" {
-		nodeName = value
-	}
-
-	if nodeName == "" {
-		if pvc.Spec.VolumeName != "" {
-			n, err := GetPVAttachedNode(context.Background(), pvc.Spec.VolumeName, storageClient)
-			if err != nil {
-				return "", errors.Wrapf(err, "error to get attached node for PVC %s/%s", pvc.Namespace, pvc.Name)
-			}
-
+	if pvc.Spec.VolumeName != "" {
+		if n, err := GetPVAttachedNode(context.Background(), pvc.Spec.VolumeName, storageClient); err != nil {
+			log.WithError(err).Warnf("Failed to get attached node for PVC %s/%s", pvc.Namespace, pvc.Name)
+		} else {
 			nodeName = n
 		}
 	}
 
-	if nodeName != "" {
-		os, err := GetNodeOS(context.Background(), nodeName, nodeClient)
-		if err != nil {
-			return "", errors.Wrapf(err, "error to get os from node %s for PVC %s/%s", nodeName, pvc.Namespace, pvc.Name)
+	if nodeName == "" {
+		if value := pvc.Annotations[KubeAnnSelectedNode]; value != "" {
+			nodeName = value
 		}
+	}
 
-		nodeOS = os
+	if nodeName != "" {
+		if os, err := GetNodeOS(context.Background(), nodeName, nodeClient); err != nil {
+			log.WithError(err).Warnf("Failed to get os from node %s for PVC %s/%s", nodeName, pvc.Namespace, pvc.Name)
+		} else {
+			nodeOS = os
+		}
 	}
 
 	if pvc.Spec.StorageClassName != nil {
-		sc, err := storageClient.StorageClasses().Get(context.Background(), *pvc.Spec.StorageClassName, metav1.GetOptions{})
-		if err != nil {
-			return "", errors.Wrapf(err, "error to get storage class %s", *pvc.Spec.StorageClassName)
-		}
-
-		if sc.Parameters != nil {
+		if sc, err := storageClient.StorageClasses().Get(context.Background(), *pvc.Spec.StorageClassName, metav1.GetOptions{}); err != nil {
+			log.WithError(err).Warnf("Failed to get storage class %s for PVC %s/%s", *pvc.Spec.StorageClassName, pvc.Namespace, pvc.Name)
+		} else if sc.Parameters != nil {
 			scFsType = strings.ToLower(sc.Parameters["csi.storage.k8s.io/fstype"])
 		}
 	}
 
 	if nodeOS != "" {
-		log.Infof("Deduced node os %s from selected node for PVC %s/%s (fsType %s)", nodeOS, pvc.Namespace, pvc.Name, scFsType)
-		return nodeOS, nil
+		log.Infof("Deduced node os %s from selected/attached node for PVC %s/%s (fsType %s)", nodeOS, pvc.Namespace, pvc.Name, scFsType)
+		return nodeOS
 	}
 
 	if scFsType == "ntfs" {
-		log.Infof("Deduced Windows node os from fsType for PVC %s/%s", pvc.Namespace, pvc.Name)
-		return NodeOSWindows, nil
+		log.Infof("Deduced Windows node os from fsType %s for PVC %s/%s", scFsType, pvc.Namespace, pvc.Name)
+		return NodeOSWindows
+	}
+
+	if scFsType != "" {
+		log.Infof("Deduced linux node os from fsType %s for PVC %s/%s", scFsType, pvc.Namespace, pvc.Name)
+		return NodeOSLinux
 	}
 
 	log.Warnf("Cannot deduce node os for PVC %s/%s, default to linux", pvc.Namespace, pvc.Name)
-	return NodeOSLinux, nil
+	return NodeOSLinux
 }
 
 func GetPVAttachedNode(ctx context.Context, pv string, storageClient storagev1.StorageV1Interface) (string, error) {

pkg/util/kube/pvc_pv_test.go

@@ -1674,34 +1674,6 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 		},
 	}
 
-	pvcObjWithNode := &corev1api.PersistentVolumeClaim{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace:   "fake-namespace",
-			Name:        "fake-pvc",
-			Annotations: map[string]string{KubeAnnSelectedNode: "fake-node"},
-		},
-	}
-
-	pvcObjWithVolume := &corev1api.PersistentVolumeClaim{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "fake-namespace",
-			Name:      "fake-pvc",
-		},
-		Spec: corev1api.PersistentVolumeClaimSpec{
-			VolumeName: "fake-volume-name",
-		},
-	}
-
-	pvcObjWithStorageClass := &corev1api.PersistentVolumeClaim{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "fake-namespace",
-			Name:      "fake-pvc",
-		},
-		Spec: corev1api.PersistentVolumeClaimSpec{
-			StorageClassName: &storageClass,
-		},
-	}
-
 	pvName := "fake-volume-name"
 	pvcObjWithAll := &corev1api.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1715,17 +1687,6 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 		},
 	}
 
-	pvcObjWithVolumeSC := &corev1api.PersistentVolumeClaim{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "fake-namespace",
-			Name:      "fake-pvc",
-		},
-		Spec: corev1api.PersistentVolumeClaimSpec{
-			VolumeName:       pvName,
-			StorageClassName: &storageClass,
-		},
-	}
-
 	scObjWithoutFSType := &storagev1api.StorageClass{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "fake-storage-class",
@@ -1739,6 +1700,13 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 		Parameters: map[string]string{"csi.storage.k8s.io/fstype": "ntfs"},
 	}
 
+	scObjWithFSTypeExt := &storagev1api.StorageClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "fake-storage-class",
+		},
+		Parameters: map[string]string{"csi.storage.k8s.io/fstype": "ext4"},
+	}
+
 	volAttachEmpty := &storagev1api.VolumeAttachment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "fake-volume-attach-1",
@@ -1753,6 +1721,7 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 			Source: storagev1api.VolumeAttachmentSource{
 				PersistentVolumeName: &pvName,
 			},
+			NodeName: "fake-node",
 		},
 	}
 
@@ -1773,7 +1742,6 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 		pvc            *corev1api.PersistentVolumeClaim
 		kubeClientObj  []runtime.Object
 		expectedNodeOS string
-		err            string
 	}{
 		{
 			name:           "no selected node, volume name and storage class",
@@ -1781,53 +1749,51 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 			expectedNodeOS: NodeOSLinux,
 		},
 		{
-			name: "node doesn't exist",
-			pvc:  pvcObjWithNode,
-			err:  "error to get os from node fake-node for PVC fake-namespace/fake-pvc: error getting node fake-node: nodes \"fake-node\" not found",
+			name:           "fallback",
+			pvc:            pvcObjWithAll,
+			expectedNodeOS: NodeOSLinux,
 		},
 		{
-			name: "node without os label",
-			pvc:  pvcObjWithNode,
+			name: "with selected node, but node without label",
+			pvc:  pvcObjWithAll,
 			kubeClientObj: []runtime.Object{
 				nodeNoOSLabel,
 			},
 			expectedNodeOS: NodeOSLinux,
 		},
 		{
-			name:           "no attach volume",
-			pvc:            pvcObjWithVolume,
-			expectedNodeOS: NodeOSLinux,
-		},
-		{
-			name: "sc doesn't exist",
-			pvc:  pvcObjWithStorageClass,
-			err:  "error to get storage class fake-storage-class: storageclasses.storage.k8s.io \"fake-storage-class\" not found",
-		},
-		{
-			name: "volume attachment not exist",
-			pvc:  pvcObjWithVolume,
+			name: "volume attachment exist, but get node os fails",
+			pvc:  pvcObjWithAll,
 			kubeClientObj: []runtime.Object{
-				nodeWindows,
 				scObjWithFSType,
-				volAttachEmpty,
-				volAttachWithOtherVolume,
+				volAttachWithVolume,
 			},
-			expectedNodeOS: NodeOSLinux,
+			expectedNodeOS: NodeOSWindows,
+		},
+		{
+			name: "volume attachment exist, node without label",
+			pvc:  pvcObjWithAll,
+			kubeClientObj: []runtime.Object{
+				nodeNoOSLabel,
+				scObjWithFSType,
+				volAttachWithVolume,
+			},
+			expectedNodeOS: NodeOSWindows,
 		},
 		{
 			name: "sc without fsType",
-			pvc:  pvcObjWithStorageClass,
+			pvc:  pvcObjWithAll,
 			kubeClientObj: []runtime.Object{
 				scObjWithoutFSType,
 			},
 			expectedNodeOS: NodeOSLinux,
 		},
 		{
-			name: "deduce from node os",
+			name: "deduce from node os by selected node",
 			pvc:  pvcObjWithAll,
 			kubeClientObj: []runtime.Object{
 				nodeWindows,
-				scObjWithFSType,
+				scObjWithFSTypeExt,
 			},
 			expectedNodeOS: NodeOSWindows,
 		},
@@ -1842,13 +1808,13 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 		},
 		{
 			name: "deduce from attached node os",
-			pvc:  pvcObjWithVolumeSC,
+			pvc:  pvcObjWithAll,
 			kubeClientObj: []runtime.Object{
 				nodeWindows,
-				scObjWithFSType,
+				scObjWithFSTypeExt,
 				volAttachEmpty,
-				volAttachWithOtherVolume,
 				volAttachWithVolume,
+				volAttachWithOtherVolume,
 			},
 			expectedNodeOS: NodeOSWindows,
 		},
@@ -1864,13 +1830,7 @@ func TestGetPVCAttachingNodeOS(t *testing.T) {
 
 			var kubeClient kubernetes.Interface = fakeKubeClient
 
-			nodeOS, err := GetPVCAttachingNodeOS(test.pvc, kubeClient.CoreV1(), kubeClient.StorageV1(), velerotest.NewLogger())
-
-			if err != nil {
-				assert.EqualError(t, err, test.err)
-			} else {
-				assert.NoError(t, err)
-			}
+			nodeOS := GetPVCAttachingNodeOS(test.pvc, kubeClient.CoreV1(), kubeClient.StorageV1(), velerotest.NewLogger())
 
 			assert.Equal(t, test.expectedNodeOS, nodeOS)
 		})