Added fix to check length of consistent device paths available before attempting read

Author: dhananjay-ng

pkg/oci/client/client_test.go

@@ -173,6 +173,22 @@ func (c *mockComputeClient) DetachVolume(ctx context.Context, request core.Detac
 }
 
 func (c *mockComputeClient) ListInstanceDevices(ctx context.Context, request core.ListInstanceDevicesRequest) (response core.ListInstanceDevicesResponse, err error) {
+	devicePath := "/dev/oracleoci/oraclevdac"
+
+	if *request.InstanceId == "ocid1.device-path-returns-error" {
+		return core.ListInstanceDevicesResponse{}, errNotFound
+	} else if  *request.InstanceId == "ocid1.device-path-not-available" {
+		return core.ListInstanceDevicesResponse{
+				Items: []core.Device{},
+			}, nil
+	} else if  *request.InstanceId == "ocid1.one-device-path-available" {
+		return core.ListInstanceDevicesResponse{
+			Items: []core.Device{{
+					Name: &devicePath,
+				},
+			},
+		}, nil
+	}
 	return core.ListInstanceDevicesResponse{}, nil
 }
 

pkg/oci/client/volume_attachment.go

@@ -16,6 +16,8 @@ package client
 
 import (
 	"context"
+	"fmt"
+	"math/rand"
 	"time"
 
 	"github.com/oracle/oci-cloud-controller-manager/pkg/util"
@@ -181,7 +183,14 @@ func (c *client) getDevicePath(ctx context.Context, instanceID string) (*string,
 		return nil, errors.WithStack(err)
 	}
 
-	device := listInstanceDevicesResp.Items[0].Name
+	//When more than 32 pods get scheduled at same time on same node, some attach operations will reach this condition where no device is left
+	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)
+	}
+	//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
 
 	return device, nil
 }

pkg/oci/client/volume_attachment_test.go

@@ -0,0 +1,49 @@
+package client
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	"go.uber.org/zap"
+)
+
+func Test_getDevicePath(t *testing.T) {
+	var tests = map[string]struct {
+		instanceID   string
+		want         string
+		wantErr      error
+	}{
+		"getDevicePathNoDeviceAvailable": {
+			instanceID: "ocid1.device-path-not-available",
+			wantErr: fmt.Errorf("Max number of volumes are already attached to instance %s. Please schedule workload on different node.", "ocid1.device-path-not-available"),
+
+		},
+		"getDevicePathOneDeviceAvailable": {
+			instanceID: "ocid1.one-device-path-available",
+			want: "/dev/oracleoci/oraclevdac",
+		},
+		"getDevicePathReturnsError": {
+			instanceID: "ocid1.device-path-returns-error",
+			wantErr: errNotFound,
+		},
+	}
+	
+	vaClient := &client{
+		compute: &mockComputeClient{},
+		logger: zap.S(),
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			result, err := vaClient.getDevicePath(context.Background(), tc.instanceID)
+			if tc.wantErr != nil && !strings.EqualFold(tc.wantErr.Error(), err.Error()) {
+				t.Errorf("getDevicePath() = %v, want %v", err.Error(), tc.wantErr.Error())
+			}
+			if tc.want != "" && !strings.EqualFold(tc.want, *result) {
+				t.Errorf("getDevicePath() = %v, want %v", *result, tc.want)
+			}
+		})
+	}
+}