Do not call ModifyVolume if the volume is already in the desired state #1741
Conversation
k8s-ci-robot
added
cncf-cla: yes
| } | ||
|
|
||
| // Perform one final check on the volume | ||
| return c.checkDesiredState(ctx, volumeID, newSizeGiB, options) |
There was a problem hiding this comment.
Dumb Q: Is the extra trip to EC2 API worth it? What is being checked here that wasn't checked earlier?
There was a problem hiding this comment.
If you look at the checkDesiredState function, there is a comment above it explaining the potential eventual consistency concerns the previous maintainers implemented it for.
Even if it really isn't necessary, I don't think this PR is the appropriate place to be tearing out such a check.
ConnorJC3
force-pushed
the
modifyvolume-fix
branch
2 times, most recently
from
2ad0f0c to
515b4e6
Compare
| return oldSizeGiB, nil | ||
| realSizeGiB := aws.Int64Value(volume.Size) | ||
|
|
||
| if realSizeGiB < desiredSizeGiB { |
There was a problem hiding this comment.
Nit: This chunk was a bit hard to read on first glance, could you add a comment or refactor this into a helper function?
Something Like "modifyVolumeIdempotencyCheck"
There was a problem hiding this comment.
Isn't that already what this function is? I don't think making a function that will never be reused is a good idea, I'll go with your second suggestion and add a comment unless there are other objections.
|
Great extra unit tests cases, thank you @ConnorJC3! |
ConnorJC3
force-pushed
the
modifyvolume-fix
branch
2 times, most recently
from
95b4241 to
b2f8d53
Compare
Signed-off-by: Connor Catlett <[email protected]>
ConnorJC3
force-pushed
the
modifyvolume-fix
branch
from
b2f8d53 to
0a1b70c
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/approve |
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@ConnorJC3: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest |
Is this a bug fix or adding new feature?
Bug fix
What is this PR about? / Why do we need it?
Today, the EBS CSI Driver will call
ModifyVolumein cases when the volume is already in the desired state (which will trigger a modification in EBS). This PR changes the behavior so that the driver instantly reports a success in the case that the volume already matches the desired state.What testing is done?
Added unit tests, Manual