Derive access mode from the CreateVolume request instead of parameters #1945
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
size/XXL
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
/retest |
k8s-ci-robot
added
the
needs-rebase
|
Thanks for sorting this out, @tonyzhc ! one minor comment above |
k8s-ci-robot
removed
the
needs-rebase
|
/retest |
pkg/gce-pd-csi-driver/controller.go
Outdated
| multiWriter := false | ||
| if common.IsHyperdisk(params.DiskType) { | ||
| if !disksWithUnsettableAccessMode[params.DiskType] { | ||
| if am, err := getHyperdiskAccessModeFromCapabilities(req.GetVolumeCapabilities()); err != nil { |
There was a problem hiding this comment.
Just making sure I understand, this check is actually redundant, as we should prevent unsettable access modes from getting set up in createVolumeInternal?
There was a problem hiding this comment.
We need this because we could still derive "READ_WRITE_SINGLE" from the volume capabilities, which is a valid mode for these disks; createVolumeInternal only throws error if the volume capabilities contain multi-node access modes (namely "READ_WRITE_MANY" and "READ_ONLY_MANY").
There was a problem hiding this comment.
So if this is an unsettable type (HdT etc), then we'll silently ignore req.GetVolumeCapabilities (or at least the access mode from them)? I'm not sure that's right, I think we should error if disksWithUnsettableAccessMode && am != RW_SINGLE.
Or am I getting confused about something?
There was a problem hiding this comment.
That check is performed somewhere else (see other comment). Let me know if you think there's improvement I can make here to make the code more understandable.
There was a problem hiding this comment.
I think we should return an error if disksWithUnsettableAccessMode[params.DiskType] == true here, even if it's redundant with the check above. It will save us if there is a future refactor that changes the order of validation.
access mode for these types of disks.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattcary, tonyzhc 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
|
/retest |
|
@tonyzhc: 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-sigs/prow repository. I understand the commands that are listed here. |
|
Looks like the integration tests are failing with the same volume snapshot flakes. |
|
The windows test isn't required, so this was able to merge. The main e2e passed (the snapshot tests are just flakey IIUC) |
What type of PR is this?
What this PR does / why we need it:
Previously #1919 introduces multi-writer mode support for hyperdisks, but the access mode was derived through the parameters list. This is not the standard Kubernetes way of configuring access mode; rather we should derive it from the Volume Capabilities list in the CreateVolume request. This would confuse customers if they provide conflicting information in StorageClass vs PVC.
Which issue(s) this PR fixes:
Fixes b/397726676.
Special notes for your reviewer:
Does this PR introduce a user-facing change?: