diff --git a/changelogs/unreleased/8891-Lyndon-Li b/changelogs/unreleased/8891-Lyndon-Li new file mode 100644 index 000000000..c6ae6a433 --- /dev/null +++ b/changelogs/unreleased/8891-Lyndon-Li @@ -0,0 +1 @@ +Fix issue 8878, relief node os deduction error checks \ No newline at end of file diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index 2936211df..faa40ad9a 100644 --- a/pkg/controller/data_upload_controller.go +++ b/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) diff --git a/pkg/controller/data_upload_controller_test.go b/pkg/controller/data_upload_controller_test.go index f392a89fa..6d186d2d0 100644 --- a/pkg/controller/data_upload_controller_test.go +++ b/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(), diff --git a/pkg/util/kube/pvc_pv.go b/pkg/util/kube/pvc_pv.go index 3b4ddc275..3ccba32ae 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/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) { diff --git a/pkg/util/kube/pvc_pv_test.go b/pkg/util/kube/pvc_pv_test.go index 5f0abcf85..1bed61162 100644 --- a/pkg/util/kube/pvc_pv_test.go +++ b/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) })