Allow Attach volume call to handle undefined errors (#3761)

Signed-off-by: Deepak Kinni <[email protected]>

Author: deepakkinni

pkg/common/cns-lib/volume/manager.go

@@ -1118,47 +1118,50 @@ func (m *defaultManager) AttachVolume(ctx context.Context,
 		}
 
 		volumeOperationRes := taskResult.GetCnsVolumeOperationResult()
-		if volumeOperationRes.Fault != nil && volumeOperationRes.Fault.Fault != nil {
+		if volumeOperationRes.Fault != nil {
 			faultType = ExtractFaultTypeFromVolumeResponseResult(ctx, volumeOperationRes)
-			_, isResourceInUseFault := volumeOperationRes.Fault.Fault.(*vim25types.ResourceInUse)
-			if isResourceInUseFault {
-				log.Infof("observed ResourceInUse fault while attaching volume: %q with vm: %q", volumeID, vm.String())
-				// Check if volume is already attached to the requested node.
-				diskUUID, err := IsDiskAttached(ctx, vm, volumeID, checkNVMeController)
-				if err != nil {
-					return "", faultType, err
-				}
-				if diskUUID != "" {
-					return diskUUID, "", nil
+
+			if volumeOperationRes.Fault.Fault != nil {
+				_, isResourceInUseFault := volumeOperationRes.Fault.Fault.(*vim25types.ResourceInUse)
+				if isResourceInUseFault {
+					log.Infof("observed ResourceInUse fault while attaching volume: %q with vm: %q", volumeID, vm.String())
+					// Check if volume is already attached to the requested node.
+					diskUUID, err := IsDiskAttached(ctx, vm, volumeID, checkNVMeController)
+					if err != nil {
+						return "", faultType, err
+					}
+					if diskUUID != "" {
+						return diskUUID, "", nil
+					}
 				}
-			}
 
-			// Check if this is a CnsFault with NotSupported fault cause
-			if cnsFault, isCnsFault := volumeOperationRes.Fault.Fault.(*cnstypes.CnsFault); isCnsFault {
-				if cnsFault.FaultCause != nil {
-					notSupportedFault, isNotSupportedFault := cnsFault.FaultCause.Fault.(*vim25types.NotSupported)
-					if isNotSupportedFault {
-						log.Infof("observed CnsFault with NotSupported fault cause while attaching volume: %q with vm: %q",
-							volumeID, vm.String())
+				// Check if this is a CnsFault with NotSupported fault cause
+				if cnsFault, isCnsFault := volumeOperationRes.Fault.Fault.(*cnstypes.CnsFault); isCnsFault {
+					if cnsFault.FaultCause != nil {
+						notSupportedFault, isNotSupportedFault := cnsFault.FaultCause.Fault.(*vim25types.NotSupported)
+						if isNotSupportedFault {
+							log.Infof("observed CnsFault with NotSupported fault cause while attaching volume: %q with vm: %q",
+								volumeID, vm.String())
+
+							// Extract the specific error message from NotSupported fault's FaultMessage array
+							var errorMessages []string
+							for _, faultMsg := range notSupportedFault.FaultMessage {
+								if faultMsg.Message != "" {
+									errorMessages = append(errorMessages, faultMsg.Message)
+								}
+							}
 
-						// Extract the specific error message from NotSupported fault's FaultMessage array
-						var errorMessages []string
-						for _, faultMsg := range notSupportedFault.FaultMessage {
-							if faultMsg.Message != "" {
-								errorMessages = append(errorMessages, faultMsg.Message)
+							if len(errorMessages) > 0 {
+								extractedMessage := strings.Join(errorMessages, " - ")
+								log.Infof("NotSupported fault extracted message: %s", extractedMessage)
+								return "", faultType, logger.LogNewErrorf(log,
+									"%q Failed to attach cns volume: %q to node vm: %q. fault: %q. opId: %q",
+									extractedMessage, volumeID, vm.String(), spew.Sdump(volumeOperationRes.Fault), taskInfo.ActivationId)
 							}
-						}
 
-						if len(errorMessages) > 0 {
-							extractedMessage := strings.Join(errorMessages, " - ")
-							log.Infof("NotSupported fault extracted message: %s", extractedMessage)
-							return "", faultType, logger.LogNewErrorf(log,
-								"%q Failed to attach cns volume: %q to node vm: %q. fault: %q. opId: %q",
-								extractedMessage, volumeID, vm.String(), spew.Sdump(volumeOperationRes.Fault), taskInfo.ActivationId)
+							// Fallback to detailed dump for debugging
+							log.Debugf("NotSupported fault details: %+v", spew.Sdump(cnsFault.FaultCause))
 						}
-
-						// Fallback to detailed dump for debugging
-						log.Debugf("NotSupported fault details: %+v", spew.Sdump(cnsFault.FaultCause))
 					}
 				}
 			}

pkg/common/cns-lib/volume/manager_test.go

@@ -168,3 +168,119 @@ func TestCnsFaultNotSupportedDetection(t *testing.T) {
 		})
 	}
 }
+
+// TestAttachVolumeFaultHandling tests that AttachVolume correctly handles faults,
+// including unknown fault types that govmomi cannot deserialize.
+func TestAttachVolumeFaultHandling(t *testing.T) {
+	tests := []struct {
+		name                   string
+		volumeOperationResult  *cnstypes.CnsVolumeOperationResult
+		expectFaultHandled     bool
+		expectSpecificHandling bool
+		faultDescription       string
+	}{
+		{
+			name: "Known fault - ResourceInUse",
+			volumeOperationResult: &cnstypes.CnsVolumeOperationResult{
+				VolumeId: cnstypes.CnsVolumeId{Id: "test-volume-1"},
+				Fault: &vim25types.LocalizedMethodFault{
+					LocalizedMessage: "Resource is in use",
+					Fault: &vim25types.ResourceInUse{
+						Type: "Disk",
+						Name: "test-disk",
+					},
+				},
+			},
+			expectFaultHandled:     true,
+			expectSpecificHandling: true,
+			faultDescription:       "ResourceInUse fault with Fault.Fault populated",
+		},
+		{
+			name: "Known fault - CnsFault with NotSupported cause",
+			volumeOperationResult: &cnstypes.CnsVolumeOperationResult{
+				VolumeId: cnstypes.CnsVolumeId{Id: "test-volume-2"},
+				Fault: &vim25types.LocalizedMethodFault{
+					LocalizedMessage: "Operation not supported",
+					Fault: &cnstypes.CnsFault{
+						MethodFault: vim25types.MethodFault{
+							FaultCause: &vim25types.LocalizedMethodFault{
+								Fault: &vim25types.NotSupported{},
+							},
+						},
+						Reason: "Not supported",
+					},
+				},
+			},
+			expectFaultHandled:     true,
+			expectSpecificHandling: true,
+			faultDescription:       "CnsFault with NotSupported cause",
+		},
+		{
+			name: "Unknown fault - Fault.Fault is nil (e.g., CnsNotRegisteredFault)",
+			volumeOperationResult: &cnstypes.CnsVolumeOperationResult{
+				VolumeId: cnstypes.CnsVolumeId{Id: "test-volume-3"},
+				Fault: &vim25types.LocalizedMethodFault{
+					LocalizedMessage: "The input volume is not registered as a CNS volume",
+					// Fault field is nil because govmomi doesn't recognize CnsNotRegisteredFault
+					Fault: nil,
+				},
+			},
+			expectFaultHandled:     true,
+			expectSpecificHandling: false,
+			faultDescription:       "Unknown fault type with Fault.Fault nil (simulates CnsNotRegisteredFault)",
+		},
+		{
+			name: "Generic CnsFault",
+			volumeOperationResult: &cnstypes.CnsVolumeOperationResult{
+				VolumeId: cnstypes.CnsVolumeId{Id: "test-volume-4"},
+				Fault: &vim25types.LocalizedMethodFault{
+					LocalizedMessage: "Generic CNS fault",
+					Fault: &cnstypes.CnsFault{
+						Reason: "Generic error",
+					},
+				},
+			},
+			expectFaultHandled:     true,
+			expectSpecificHandling: false,
+			faultDescription:       "Generic CnsFault without special handling",
+		},
+		{
+			name: "No fault",
+			volumeOperationResult: &cnstypes.CnsVolumeOperationResult{
+				VolumeId: cnstypes.CnsVolumeId{Id: "test-volume-5"},
+				Fault:    nil,
+			},
+			expectFaultHandled:     false,
+			expectSpecificHandling: false,
+			faultDescription:       "No fault present",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Test the fault handling logic structure that AttachVolume uses
+			if tt.volumeOperationResult.Fault != nil {
+				// This should handle ALL faults, including unknown ones
+				assert.True(t, tt.expectFaultHandled, "Fault should be detected: %s", tt.faultDescription)
+
+				// Test nested handling for known fault types
+				if tt.volumeOperationResult.Fault.Fault != nil {
+					// Can perform specific type assertions here
+					_, isResourceInUse := tt.volumeOperationResult.Fault.Fault.(*vim25types.ResourceInUse)
+					_, isCnsFault := tt.volumeOperationResult.Fault.Fault.(*cnstypes.CnsFault)
+
+					hasSpecificHandling := isResourceInUse || isCnsFault
+					if tt.expectSpecificHandling {
+						assert.True(t, hasSpecificHandling, "Should have specific fault handling")
+					}
+				} else {
+					// Unknown fault types will have Fault.Fault == nil
+					// These should still be caught by the outer check
+					assert.False(t, tt.expectSpecificHandling, "Unknown faults should not have specific handling")
+				}
+			} else {
+				assert.False(t, tt.expectFaultHandled, "No fault should be detected")
+			}
+		})
+	}
+}