Skip to content

Add force attach to storage class #1254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions pkg/common/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const (
ParameterKeyLabels = "labels"
ParameterKeyProvisionedIOPSOnCreate = "provisioned-iops-on-create"
ParameterKeyProvisionedThroughputOnCreate = "provisioned-throughput-on-create"
ParameterAvailabilityClass = "availability-class"

// Parameters for VolumeSnapshotClass
ParameterKeyStorageLocations = "storage-locations"
Expand All @@ -38,6 +39,10 @@ const (
DiskImageType = "images"
replicationTypeNone = "none"

// Parameters for AvailabilityClass
ParameterNoAvailabilityClass = "none"
ParameterRegionalHardFailoverClass = "regional-hard-failover"

// Keys for PV and PVC parameters as reported by external-provisioner
ParameterKeyPVCName = "csi.storage.k8s.io/pvc/name"
ParameterKeyPVCNamespace = "csi.storage.k8s.io/pvc/namespace"
Expand Down Expand Up @@ -83,6 +88,8 @@ type DiskParameters struct {
// Values: {int64}
// Default: none
ProvisionedThroughputOnCreate int64
// Default: false
ForceAttach bool
}

// SnapshotParameters contains normalized and defaulted parameters for snapshots
Expand Down Expand Up @@ -155,6 +162,14 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
return p, fmt.Errorf("parameters contain invalid provisionedThroughputOnCreate parameter: %w", err)
}
p.ProvisionedThroughputOnCreate = paramProvisionedThroughputOnCreate
case ParameterAvailabilityClass:
paramAvailabilityClass, err := ConvertStringToAvailabilityClass(v)
if err != nil {
return p, fmt.Errorf("parameters contain invalid availability class parameter: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Hyphenate "availability-class" to make it more clear what parameter is invalid.

}
if paramAvailabilityClass == ParameterRegionalHardFailoverClass {
p.ForceAttach = true
}
default:
return p, fmt.Errorf("parameters contains invalid option %q", k)
}
Expand Down
21 changes: 21 additions & 0 deletions pkg/common/parameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,27 @@ func TestExtractAndDefaultParameters(t *testing.T) {
Labels: map[string]string{"key1": "value1", "label-1": "value-a", "label-2": "label-value-2"},
},
},
{
name: "availability class parameters",
parameters: map[string]string{ParameterAvailabilityClass: ParameterRegionalHardFailoverClass},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
ForceAttach: true,
Tags: map[string]string{},
Labels: map[string]string{},
},
},
{
name: "no force attach parameters",
parameters: map[string]string{ParameterAvailabilityClass: ParameterNoAvailabilityClass},
expectParams: DiskParameters{
DiskType: "pd-standard",
ReplicationType: "none",
Tags: map[string]string{},
Labels: map[string]string{},
},
},
}

for _, tc := range tests {
Expand Down
22 changes: 22 additions & 0 deletions pkg/common/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,28 @@ func ConvertMiStringToInt64(str string) (int64, error) {
return volumehelpers.RoundUpToMiB(quantity)
}

// ConvertStringToBool converts a string to a boolean.
func ConvertStringToBool(str string) (bool, error) {
switch strings.ToLower(str) {
case "true":
return true, nil
case "false":
return false, nil
}
return false, fmt.Errorf("Unexpected boolean string %s", str)
}

// ConvertStringToAvailabilityClass converts a string to an availability class string.
func ConvertStringToAvailabilityClass(str string) (string, error) {
switch strings.ToLower(str) {
case ParameterNoAvailabilityClass:
return ParameterNoAvailabilityClass, nil
case ParameterRegionalHardFailoverClass:
return ParameterRegionalHardFailoverClass, nil
}
return "", fmt.Errorf("Unexpected boolean string %s", str)
}

// ParseMachineType returns an extracted machineType from a URL, or empty if not found.
// machineTypeUrl: Full or partial URL of the machine type resource, in the format:
//
Expand Down
108 changes: 108 additions & 0 deletions pkg/common/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,114 @@ func TestConvertMiStringToInt64(t *testing.T) {
}
}

func TestConvertStringToBool(t *testing.T) {
tests := []struct {
desc string
inputStr string
expected bool
expectError bool
}{
{
desc: "valid true",
inputStr: "true",
expected: true,
expectError: false,
},
{
desc: "valid mixed case true",
inputStr: "True",
expected: true,
expectError: false,
},
{
desc: "valid false",
inputStr: "false",
expected: false,
expectError: false,
},
{
desc: "valid mixed case false",
inputStr: "False",
expected: false,
expectError: false,
},
{
desc: "invalid",
inputStr: "yes",
expected: false,
expectError: true,
},
}
for _, tc := range tests {
t.Run(tc.desc, func(t *testing.T) {
got, err := ConvertStringToBool(tc.inputStr)
if err != nil && !tc.expectError {
t.Errorf("Got error %v converting string to bool %s; expect no error", err, tc.inputStr)
}
if err == nil && tc.expectError {
t.Errorf("Got no error converting string to bool %s; expect an error", tc.inputStr)
}
if err == nil && got != tc.expected {
t.Errorf("Got %v for converting string to bool; expect %v", got, tc.expected)
}
})
}
}

func TestConvertStringToAvailabilityClass(t *testing.T) {
tests := []struct {
desc string
inputStr string
expected string
expectError bool
}{
{
desc: "valid none",
inputStr: "none",
expected: ParameterNoAvailabilityClass,
expectError: false,
},
{
desc: "valid mixed case none",
inputStr: "None",
expected: ParameterNoAvailabilityClass,
expectError: false,
},
{
desc: "valid failover",
inputStr: "regional-hard-failover",
expected: ParameterRegionalHardFailoverClass,
expectError: false,
},
{
desc: "valid mixed case failover",
inputStr: "Regional-Hard-Failover",
expected: ParameterRegionalHardFailoverClass,
expectError: false,
},
{
desc: "invalid",
inputStr: "yes",
expected: "",
expectError: true,
},
}
for _, tc := range tests {
t.Run(tc.desc, func(t *testing.T) {
got, err := ConvertStringToAvailabilityClass(tc.inputStr)
if err != nil && !tc.expectError {
t.Errorf("Got error %v converting string to availablity class %s; expect no error", err, tc.inputStr)
}
if err == nil && tc.expectError {
t.Errorf("Got no error converting string to availablity class %s; expect an error", tc.inputStr)
}
if err == nil && got != tc.expected {
t.Errorf("Got %v for converting string to availablity class; expect %v", got, tc.expected)
}
})
}
}

func TestParseMachineType(t *testing.T) {
tests := []struct {
desc string
Expand Down
17 changes: 9 additions & 8 deletions pkg/gce-cloud-provider/compute/fake-gce.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,15 +246,16 @@ func (cloud *FakeCloudProvider) DeleteDisk(ctx context.Context, project string,
return nil
}

func (cloud *FakeCloudProvider) AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string) error {
func (cloud *FakeCloudProvider) AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string, forceAttach bool) error {
source := cloud.GetDiskSourceURI(project, volKey)

attachedDiskV1 := &computev1.AttachedDisk{
DeviceName: volKey.Name,
Kind: diskKind,
Mode: readWrite,
Source: source,
Type: diskType,
DeviceName: volKey.Name,
Kind: diskKind,
Mode: readWrite,
Source: source,
Type: diskType,
ForceAttach: forceAttach,
}
instance, ok := cloud.instances[instanceName]
if !ok {
Expand Down Expand Up @@ -538,14 +539,14 @@ func (cloud *FakeBlockingCloudProvider) DetachDisk(ctx context.Context, project,
return cloud.FakeCloudProvider.DetachDisk(ctx, project, deviceName, instanceZone, instanceName)
}

func (cloud *FakeBlockingCloudProvider) AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string) error {
func (cloud *FakeBlockingCloudProvider) AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string, forceAttach bool) error {
execute := make(chan Signal)
cloud.ReadyToExecute <- execute
val := <-execute
if val.ReportError {
return fmt.Errorf("force mock error for AttachDisk: volkey %s", volKey)
}
return cloud.FakeCloudProvider.AttachDisk(ctx, project, volKey, readWrite, diskType, instanceZone, instanceName)
return cloud.FakeCloudProvider.AttachDisk(ctx, project, volKey, readWrite, diskType, instanceZone, instanceName, forceAttach)
}

func notFoundError() *googleapi.Error {
Expand Down
10 changes: 7 additions & 3 deletions pkg/gce-cloud-provider/compute/gce-compute.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ type GCECompute interface {
ValidateExistingDisk(ctx context.Context, disk *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool) error
InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool) error
DeleteDisk(ctx context.Context, project string, volumeKey *meta.Key) error
AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string) error
AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string, forceAttach bool) error
DetachDisk(ctx context.Context, project, deviceName, instanceZone, instanceName string) error
GetDiskSourceURI(project string, volKey *meta.Key) string
GetDiskTypeURI(project string, volKey *meta.Key, diskType string) string
Expand Down Expand Up @@ -700,7 +700,7 @@ func (cloud *CloudProvider) deleteRegionalDisk(ctx context.Context, project, reg
return nil
}

func (cloud *CloudProvider) AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string) error {
func (cloud *CloudProvider) AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string, forceAttach bool) error {
klog.V(5).Infof("Attaching disk %v to %s", volKey, instanceName)
source := cloud.GetDiskSourceURI(project, volKey)

Expand All @@ -714,9 +714,13 @@ func (cloud *CloudProvider) AttachDisk(ctx context.Context, project string, volK
Mode: readWrite,
Source: source,
Type: diskType,
// This parameter is ignored in the call, the ForceAttach decorator
// (query parameter) is the important one. We'll set it in both places
// in case that behavior changes.
ForceAttach: forceAttach,
}

op, err := cloud.service.Instances.AttachDisk(project, instanceZone, instanceName, attachedDiskV1).Context(ctx).Do()
op, err := cloud.service.Instances.AttachDisk(project, instanceZone, instanceName, attachedDiskV1).Context(ctx).ForceAttach(forceAttach).Do()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually if this goes to production we may want a metric + log

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The api call is in the audit logs, I'm not sure we need much more?

if err != nil {
return fmt.Errorf("failed cloud service attach disk call: %w", err)
}
Expand Down
Loading