Not Returning Infeasible If Tags Were Modified

[mdzraf]

What type of PR is this?

/kind bug

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

This PR makes it so that when there is a modifyVolume request and the request to modify tags which happens before the ResizeOrModifyDisk, succeeds, and then there is a failure when ResizeOrModifyDisk is called due to an InvalidParameter, the driver does not return codes.InvalidArgument which is considered an infeasible error by the resizer. Because there was some modification done to the volume, an infeasible error is not to be returned as this would signal that no changes were done at all.

How was this change tested?

Running make test for unit tests.

Does this PR introduce a user-facing change?

NONE

[github-actions]

Code Coverage Diff

File	Old Coverage	New Coverage	Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go	77.8%	76.1%	-1.7

[AndrewSirenko]

I thought we wanted to undo the tag modification then return InvalidArgument in this case?

By returning internal error here customer will not be able to rollback vacname AND we will spam EC2 because there is no delayed retry interval.

[ConnorJC3]

Is undoing the tags reasonably possible? Don't we get completely wrecked by restarts/crashes? I thought we were going to add a MV dry run, and bail infreasibly on that.

[mdzraf]

Offline Discussion: We will merge this PR and work on designing a long-term fix to ensure that the volume is not changed at all when we return an infeasible error.

[mdzraf]

/retest

[AndrewSirenko]

Going to have one more standup discussion on next steps because:

• Changing invalidArg -> internal leads to spamming EC2 API Calls on wrong parameters
• KEP-3751: add error handling may constrain whether customers can roll forward or roll back internal vs InvalidArg errors Nvm final wording is relaxed about this.

Instead a dry-run modifyVolume before createTags would help customers catch invalid VACs before modifying volume at all.

[AndrewSirenko]

/hold

Until standup discussion

[AndrewSirenko]

/unhold

[ConnorJC3]

/lgtm

[torredil]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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 [torredil]

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