Fix Incorrect Error Handling for Volumes in Optimizing State

[torredil]

Is this a bug fix or adding new feature?

Bug

What is this PR about? / Why do we need it?

This PR fixes a bug where an error is incorrectly returned if a volume is in an optimizing state.

According to cloud, a modification is considered done if the volume is an optimizing state:

aws-ebs-csi-driver/pkg/cloud/cloud.go

Lines 1320 to 1323 in d071db8

	func volumeModificationDone(state string) bool {
	if state == ec2.VolumeModificationStateCompleted || state == ec2.VolumeModificationStateOptimizing {
	return true
	}

In the case where ResizeOrModifyDisk hasn't been previously called for a volume, we simply move on to check if the volume is in the desired state. If it is, ResizeOrModifyDisk succeeds even if the volume is still optimizing. However, if checkDesiredState returns an error or the sidecar times out, latestMod won't be nil the second time around and we error out if the volume is optimizing:

aws-ebs-csi-driver/pkg/cloud/cloud.go

Lines 1298 to 1300 in d071db8

	} else if state == ec2.VolumeModificationStateOptimizing {
	return true, 0, fmt.Errorf("volume %q in OPTIMIZING state, cannot currently modify", volumeID)
	}

What testing is done?

• make test
• CI

[ConnorJC3]

Not attempting to start a modification when a volume is in the OPTIMIZING state is intentional

[AndrewSirenko]

Not attempting to start a modification when a volume is in the OPTIMIZING state is intentional

Why are we validating this in our driver instead of relying on the EC2 API? Won't EC2 ModifyVolume error out on this case?

[torredil]

Not attempting to start a modification when a volume is in the OPTIMIZING state is intentional

By my understanding not starting a modification if the volume is already in the desired state would be a more appropriate check, could you elaborate?

[ConnorJC3]

Why are we validating this in our driver instead of relying on the EC2 API? Won't EC2 ModifyVolume error out on this case?

Because it produces spurious failed API calls. A volume in the OPTIMIZING state cannot be modified, attempting to make an API call is 100% guaranteed to fail. It produces additional unnecessary load on the AWS API in a case that we can 100% prevent.

By my understanding not starting a modification if the volume is already in the desired state would be a more appropriate check, could you elaborate?

The appropriate fix here is that this check should not be running until we are sure we need to make a modification. Removing an entire block of checks is not the appropriate fix for this bug.

[AndrewSirenko]

The appropriate fix here is that this check should not be running until we are sure we need to make a modification. Removing an entire block of checks is not the appropriate fix for this bug.

If we really want to prevent an extra ModifyVolume API call, I think the most appropriate check would be something along the lines of:

if lastModification.State == OPTIMIZING
    if lastModification.NewValues == ModifyVolumeRequest.DesiredValues
           return modification success
    else
        return error("Cannot modify volume in optimizing state")

[ConnorJC3]

If we really want to prevent an extra ModifyVolume API call, I think the most appropriate check would be something along the lines of:

Agree that would work, but I think the simplest fix given the existing code is just to move this if block to AFTER the needsVolumeModification check and leave everything else the same - that would prevent it from firing in the case where the sidecar times out, and only fire in the case where we are attempting to start a new modification that is needed.

else if state == ec2.VolumeModificationStateOptimizing {
  return true, 0, fmt.Errorf("volume %q in OPTIMIZING state, cannot currently modify", volumeID)
}

[ConnorJC3]

/lgtm

[AndrewSirenko]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndrewSirenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [AndrewSirenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment