Updated UHP post detach logout wait logic

Author: YashasG98

pkg/cloudprovider/providers/oci/instances_test.go

@@ -900,6 +900,10 @@ func (c *MockComputeClient) FindActiveVolumeAttachment(ctx context.Context, comp
 	return nil, nil
 }
 
+func (c *MockComputeClient) WaitForUHPVolumeLoggedOut(ctx context.Context, attachmentID string) error {
+	return nil
+}
+
 func (c *MockBlockStorageClient) AwaitVolumeBackupAvailableOrTimeout(ctx context.Context, id string) (*core.VolumeBackup, error) {
 	return &core.VolumeBackup{}, nil
 }

pkg/csi/driver/bv_controller.go

@@ -855,13 +855,19 @@ func (d *BlockVolumeControllerDriver) ControllerUnpublishVolume(ctx context.Cont
 	multipath := false
 
 	if attachedVolume.GetIsMultipath() != nil {
+		var cancel context.CancelFunc
+		// this will not override parent context, it will evaluate to min(parent context timeout, current time + 90 seconds)
+		ctx, cancel = context.WithTimeout(ctx, 90*time.Second)
+		defer cancel()
 		multipath = *attachedVolume.GetIsMultipath()
-	}
-
-	// sleeping to ensure block volume plugin logs out of iscsi connections on nodes before delete
-	if multipath {
-		log.Info("Waiting for 90 seconds to ensure block volume plugin logs out of iscsi connections on nodes")
-		time.Sleep(90 * time.Second)
+		if multipath {
+			log.With("instanceID", *attachedVolume.GetInstanceId()).Info("Waiting for UHP Volume to logout")
+			err = d.client.Compute().WaitForUHPVolumeLoggedOut(ctx, *attachedVolume.GetId())
+			if err != nil {
+				log.With("service", "compute", "verb", "get", "resource", "volumeAttachment", "statusCode", util.GetHttpStatusCode(err)).
+					With("instanceID", *attachedVolume.GetInstanceId()).Warnf("timed out waiting for UHP volume logout, will not wait further or retry, will mark detach success: %s", err.Error())
+			}
+		}
 	}
 
 	log.Info("Un-publishing Volume Completed")

pkg/csi/driver/bv_controller_test.go

@@ -155,6 +155,15 @@ var (
 			Id:                 common.String("volume-attachment-stuck-in-attaching-state"),
 			InstanceId:         common.String("sample-provider-id"),
 		},
+		"uhp-volume-attachment-stuck-in-logged-in-state": {
+			DisplayName:        common.String("uhp-volume-attachment-stuck-in-logged-in-state"),
+			LifecycleState:     core.VolumeAttachmentLifecycleStateDetached,
+			AvailabilityDomain: common.String("NWuj:PHX-AD-2"),
+			Id:                 common.String("uhp-volume-attachment-stuck-in-logged-in-state"),
+			InstanceId:         common.String("sample-provider-id"),
+			IscsiLoginState:    core.VolumeAttachmentIscsiLoginStateLoginSucceeded,
+			IsMultipath:        common.Bool(true),
+		},
 	}
 
 	subnets = map[string]*core.Subnet{
@@ -747,6 +756,21 @@ func (c *MockComputeClient) WaitForVolumeDetached(ctx context.Context, attachmen
 	return nil
 }
 
+func (c *MockComputeClient) WaitForUHPVolumeLoggedOut(ctx context.Context, attachmentID string) error {
+	if err := wait.PollImmediateUntil(testPollInterval, func() (done bool, err error) {
+		va := volume_attachments[attachmentID]
+
+		if va.GetIscsiLoginState() == core.VolumeAttachmentIscsiLoginStateLogoutSucceeded {
+			return true, nil
+		}
+		return false, nil
+	}, ctx.Done()); err != nil {
+		return errors.WithStack(err)
+	}
+
+	return nil
+}
+
 func (p *MockProvisionerClient) Compute() client.ComputeInterface {
 	return &MockComputeClient{}
 }
@@ -1182,6 +1206,17 @@ func TestControllerDriver_ControllerUnpublishVolume(t *testing.T) {
 			want:    nil,
 			wantErr: errors.New("context deadline exceeded"),
 		},
+		{
+			name: "Unpublish should not fail on UHP Volume logout timeout",
+			args: args{
+				req: &csi.ControllerUnpublishVolumeRequest{
+					VolumeId: "uhp-volume-attachment-stuck-in-logged-in-state",
+					NodeId:   "sample-node-id",
+				},
+			},
+			want:    &csi.ControllerUnpublishVolumeResponse{},
+			wantErr: nil,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/oci/client/volume_attachment.go

@@ -54,6 +54,10 @@ type VolumeAttachmentInterface interface {
 	WaitForVolumeDetached(ctx context.Context, attachmentID string) error
 
 	FindActiveVolumeAttachment(ctx context.Context, compartmentID, volumeID string) (core.VolumeAttachment, error)
+
+	// WaitForUHPVolumeLoggedOut WaitForUHPVolumeLogout polls waiting for a OCI UHP block volume attachment to be in the
+	// LOGGED_OUT state.
+	WaitForUHPVolumeLoggedOut(ctx context.Context, attachmentID string) error
 }
 
 var _ VolumeAttachmentInterface = &client{}
@@ -187,7 +191,7 @@ func (c *client) getDevicePath(ctx context.Context, instanceID string) (*string,
 	if len(listInstanceDevicesResp.Items) == 0 {
 		c.logger.With("service", "compute", "verb", listVerb, "resource", instanceResource).
 			With("instanceID", instanceID).Warn("No consistent device paths available for worker node.")
-		return nil, fmt.Errorf("Max number of volumes are already attached to instance %s. Please schedule workload on different node.",  instanceID)
+		return nil, fmt.Errorf("Max number of volumes are already attached to instance %s. Please schedule workload on different node.", instanceID)
 	}
 	//Picks device path from available path randomly so that 2 volume attachments don't get same path when operations happen concurrently resulting in failure of one of them.
 	device := listInstanceDevicesResp.Items[rand.Intn(len(listInstanceDevicesResp.Items))].Name
@@ -339,3 +343,24 @@ func (c *client) FindActiveVolumeAttachment(ctx context.Context, compartmentID,
 
 	return nil, errors.WithStack(errNotFound)
 }
+
+func (c *client) WaitForUHPVolumeLoggedOut(ctx context.Context, attachmentID string) error {
+	if err := wait.PollImmediateUntil(attachmentPollInterval, func() (done bool, err error) {
+		va, err := c.GetVolumeAttachment(ctx, attachmentID)
+
+		if err != nil {
+			if IsRetryable(err) {
+				return false, nil
+			}
+			return true, errors.WithStack(err)
+		}
+		if va.GetIscsiLoginState() == core.VolumeAttachmentIscsiLoginStateLogoutSucceeded {
+			return true, nil
+		}
+		return false, nil
+	}, ctx.Done()); err != nil {
+		return errors.WithStack(err)
+	}
+
+	return nil
+}

pkg/volume/provisioner/block/block_test.go

@@ -285,6 +285,10 @@ func (c *MockComputeClient) FindActiveVolumeAttachment(ctx context.Context, comp
 	return nil, nil
 }
 
+func (c *MockComputeClient) WaitForUHPVolumeLoggedOut(ctx context.Context, attachmentID string) error {
+	return nil
+}
+
 // MockVirtualNetworkClient mocks VirtualNetwork client implementation
 type MockVirtualNetworkClient struct {
 }

pkg/volume/provisioner/fss/fss_test.go

@@ -270,6 +270,10 @@ func (c *MockComputeClient) FindActiveVolumeAttachment(ctx context.Context, comp
 	return nil, nil
 }
 
+func (c *MockComputeClient) WaitForUHPVolumeLoggedOut(ctx context.Context, attachmentID string) error {
+	return nil
+}
+
 // MockVirtualNetworkClient mocks VirtualNetwork client implementation
 type MockVirtualNetworkClient struct {
 }