Return existing access point if one already exists during create workflow#1620
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @jrakas-dev. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
jrakas-dev
force-pushed
the
use-existing-ap
branch
4 times, most recently
from
4b5703f to
b5d049c
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
jrakas-dev
force-pushed
the
use-existing-ap
branch
from
b5d049c to
9bf1574
Compare
|
/retest |
| if isAccessDenied(err) { | ||
| if isAccessPointAlreadyExists(err) { | ||
| klog.V(4).Infof("Access point already exists for client token %s. Retrieving existing access point details.", clientToken) | ||
| existingAccessPoint, err := c.FindAccessPointByClientToken(ctx, clientToken, *createAPInput.FileSystemId) |
There was a problem hiding this comment.
Should we validate that the access point returned matches exactly the the one we are trying to create? Just as an additional safeguard against FindAccessPointByClientToken returning an access point that doesnt exactly match the one we are trying to create. We could have a wrapper around FindAccessPointByClientToken?
| if existingAccessPoint == nil { | ||
| return nil, fmt.Errorf("No access point for client token %s was returned", clientToken) | ||
| } | ||
| return existingAccessPoint, nil |
There was a problem hiding this comment.
can we also verify the Uid, Gid and permission before re-using the existing access point ? I feel there could be a case that we return an access point with wrong permission.
jrakas-dev
force-pushed
the
use-existing-ap
branch
5 times, most recently
from
1d7ca1b to
08d6b56
Compare
jrakas-dev
force-pushed
the
use-existing-ap
branch
from
08d6b56 to
c3d4981
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dankova22, jrakas-dev The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
Is this a bug fix or adding new feature?
Bug fix. This addresses an issue that can arise if access points aren't cleanly deleted when the CSI controller crashes, resulting in
AccessPointAlreadyExistserrors being returned when the controller attempts to recreate the volume.What is this PR about? / Why do we need it?
This PR modifies the controller to adopt the existing access point in the event that one already happens to exist for the same client token, allowing the PVC creation to succeed.
What testing is done?
Unit tests were added, verified fix against a cluster setup with a reproduction of the issue