packages/system/linstor/images/linstor-csi/patches/001-rwx-validation.diff (2)

158-158: Consider using Debug level for this log statement.

Info level for every validation call will be noisy in production. This diagnostic message is better suited to Debug level, consistent with the similar log at line 303.

Suggested change

-	log.WithField("volumeID", volumeID).Info("validateRWXBlockAttachment called")
+	log.WithField("volumeID", volumeID).Debug("validateRWXBlockAttachment called")

---

236-247: Original error context is lost when GetVMNameFromPod fails.

When GetVMNameFromPod returns an error (e.g., "failed to get owner pod"), the error is logged as a warning but the pod continues with vmName = "". If this later triggers the strict mode check (line 280-285), the user sees a generic "does not have the vm.kubevirt.io/name label" error, not the actual root cause.

Consider propagating the underlying error for better debuggability:

Alternative: Propagate the actual error

 			vmName, err := GetVMNameFromPod(ctx, kubeClient, log, &item)
 			if err != nil {
-				log.WithError(err).WithField("pod", item.GetName()).Warn("failed to get VM name from pod")
-				// Continue with empty vmName - will be caught by strict mode check
-				vmName = ""
+				return "", fmt.Errorf("failed to determine VM for pod %s: %w", item.GetName(), err)
 			}

Alternatively, keep the current soft failure behavior but enhance the strict mode error message to include more context.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

packages/system/linstor/images/linstor-csi/patches/001-rwx-validation.diff (2)

196-209: Consider consistent error handling for claimRef extraction.

Multiple errors from NestedString and NestedMap are silently ignored. While the fail-open design is intentional, suppressing errors without even logging them could make debugging harder.

♻️ Suggested improvement

-	claimRef, found, _ := unstructured.NestedMap(pv.Object, "spec", "claimRef")
+	claimRef, found, err := unstructured.NestedMap(pv.Object, "spec", "claimRef")
+	if err != nil {
+		log.WithError(err).Warn("cannot validate RWX attachment: failed to read claimRef")
+		return "", nil
+	}
 	if !found {
 		log.Warn("cannot validate RWX attachment: PV has no claimRef")
 		return "", nil
 	}

-	pvcName, _, _ := unstructured.NestedString(claimRef, "name")
-	pvcNamespace, _, _ := unstructured.NestedString(claimRef, "namespace")
+	pvcName, _, err := unstructured.NestedString(claimRef, "name")
+	if err != nil {
+		log.WithError(err).Warn("cannot validate RWX attachment: failed to read PVC name")
+		return "", nil
+	}
+	pvcNamespace, _, err := unstructured.NestedString(claimRef, "namespace")
+	if err != nil {
+		log.WithError(err).Warn("cannot validate RWX attachment: failed to read PVC namespace")
+		return "", nil
+	}

---

576-582: Test assertion could be more precise.

The assertion if len(tc.pods) > 0 { assert.NotEmpty(t, vmName) } checks if test pods were defined, not if matching pods were found. This works for current test cases but could give false positives if a test case defines pods that all fail to match (different namespace + different PVC).

Consider adding an expectedVMName field to the test cases for explicit verification.

♻️ Suggested improvement

 	testCases := []struct {
 		name        string
 		pods        []*unstructured.Unstructured
 		pvcName     string
 		namespace   string
 		expectError bool
 		errorMsg    string
+		expectedVM  string // expected VM name, empty if no VM should be found
 	}{
 		{
 			name:        "no pods using PVC",
 			pods:        []*unstructured.Unstructured{},
 			pvcName:     "test-pvc",
 			namespace:   "default",
 			expectError: false,
+			expectedVM:  "",
 		},
 		{
 			name: "single pod using PVC",
 			pods: []*unstructured.Unstructured{
 				createUnstructuredPod("pod1", "default", "test-pvc", map[string]string{KubeVirtVMLabel: "vm1"}, "Running"),
 			},
 			pvcName:     "test-pvc",
 			namespace:   "default",
 			expectError: false,
+			expectedVM:  "vm1",
 		},
 		// ... add expectedVM to other cases

Then in the assertion:

 		} else {
 			assert.NoError(t, err)
-			// VM name is returned when there are pods using the volume
-			if len(tc.pods) > 0 {
-				assert.NotEmpty(t, vmName)
-			}
+			assert.Equal(t, tc.expectedVM, vmName)
 		}

Configuration used: defaults

Review profile: CHILL

Plan: Pro