Ensure infeasible PVC modifications are retried at slower pace #453
Conversation
k8s-ci-robot
added
release-note
AndrewSirenko
changed the title
k8s-ci-robot
added
the
size/L
| if *curVacName == targetVacName { | ||
| // if somehow both curVacName and targetVacName is same, what does this mean?? | ||
| // I am not sure about this. |
There was a problem hiding this comment.
Why was this branch initially included? Will bring up in next SIG Implementation meeting, because this week's iteration was skipped.
There was a problem hiding this comment.
If curVacName == targetVacName is true, then does the volume needs modification at all? cc @sunnylovestiramisu
k8s-ci-robot
added
the
needs-rebase
|
@sunnylovestiramisu can you also review this PR? I was hoping we can merge this PR before we release the next minor version of external-resizer. |
AndrewSirenko
force-pushed
the
modifyInfeasibleSlowSet
branch
from
da332d6 to
e3e2148
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AndrewSirenko The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/XL
| func (ctrl *modifyController) delayModificationIfRecentlyInfeasible(pvc *v1.PersistentVolumeClaim, pvcKey string) error { | ||
| // Do not delay modification if PVC updated with new VAC | ||
| s := pvc.Status.ModifyVolumeStatus | ||
| if s == nil || pvc.Spec.VolumeAttributesClassName == nil || s.TargetVolumeAttributesClassName != *pvc.Spec.VolumeAttributesClassName { |
There was a problem hiding this comment.
@gnufied @sunnylovestiramisu I should also ensure slowset key is deleted if targetVac != pvc.spec.vac right?
There was a problem hiding this comment.
Correct, we try at slow interval for Infeasible after the ControllerModifyVolume call failed. On the other hand, is there any possibility to end up in the state of targetVac != pvc.spec.vac and Infeasible?
There was a problem hiding this comment.
is there any possibility to end up in the state of targetVac != pvc.spec.vac and Infeasible?
I see a world where the following could happen:
- Patch PVC with invalid VAC
- pvc.Status.TargetVacName set and ModifyVolumeStatus marked InProgress
- ModifyVolume RPC called, Infeasible error code
- ModifyController marks PVC ModifyVolumeStatus infeasible
- Future ModifyVolume RPCs called at slow interval
- Patch PVC with valid valid VAC
- VAC ModifyVolumeStatus infeasible has not been cleared yet (
markControllerModifyVolumeStatusnot yet called) - pvc.Status.TargetVacName still refers to invalidVac
- PVCKey in slowset
Therefore targetVac != pvc.spec.vac and Infeasible.
ModifyVolume RPC would not be triggered until slowset entry expires. But this if-statement here would prevent that.
AndrewSirenko
force-pushed
the
modifyInfeasibleSlowSet
branch
from
e3e2148 to
810bc51
Compare
|
In addition to unit tests, tested the following with EBS CSI Driver:
These tests should probably be added to k/k via csimock driver, once we have mock csi driver VAC tests. |
AndrewSirenko
force-pushed
the
modifyInfeasibleSlowSet
branch
from
810bc51 to
be155d3
Compare
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR ensures infeasible PVC modifications (E.g. InvalidArgument) are retried at slower pace than normal failures (they are retried at the sidecar's
max-interval-retry).This prevents spamming relevant PVC with events when future ModifyVolume RPC Triggers will likely fail, like when a user's VolumeAttributeClass has incorrect parameters.
See related resizer controller PR #418.
Which issue(s) this PR fixes:
Fixes #407
Special notes for your reviewer:
This PR is stacked on top of the 2 commits of my unit test PR, #447. That should probably be reviewed first.
This PR was tested alongside AWS EBS CSI Driver (ensuring that only 1 ModifyVolume RPC was triggered every
max-interval-retry, if RPC failed with an infeasible error code). Ideally we would add csi mock e2e tests in k/k, but those do not exist yet for ModifyVolume.Does this PR introduce a user-facing change?: