Add force attach to storage class #1254
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/L
|
/test all |
|
/test all |
1 similar comment
|
/test all |
mattcary
force-pushed
the
forceattach
branch
2 times, most recently
from
789c0d7 to
d6917fe
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
What's the plan for detach? Should repd always use force attach regardless? |
We don't know. For the PoC of an availability controller we're building we will be doing force attach unconditionally, because as far as we can tell it takes too long to figure out if it's necessary or not (this is what cloud sql does for instance). But we have to evaluate. This is why I have the scary alpha/unsafe name, I think there's a good chance well figure out a better solution after we work through real-world options. I expect that this parameter will be removed & replaced with a better one. |
|
I'm wondering for the purpose of POC you can just have a feature gate, instead of an opt-in storageclass parameter that ends up as an immutable field on the PV object |
Do you mean a command-line flag that would enable this for the driver per-cluster? We need to enable this only on certain volumes, we'll be testing it in clusters where we only want some workloads to use this as we don't want to risk data loss by force-attaching cavalierly. |
| } | ||
|
|
||
| op, err := cloud.service.Instances.AttachDisk(project, instanceZone, instanceName, attachedDiskV1).Context(ctx).Do() | ||
| op, err := cloud.service.Instances.AttachDisk(project, instanceZone, instanceName, attachedDiskV1).Context(ctx).ForceAttach(forceAttach).Do() |
There was a problem hiding this comment.
Eventually if this goes to production we may want a metric + log
There was a problem hiding this comment.
The api call is in the audit logs, I'm not sure we need much more?
mattcary
force-pushed
the
forceattach
branch
2 times, most recently
from
f6494ff to
6e7feb7
Compare
k8s-ci-robot
added
size/XL
|
Hmm, the e2e test succeeded locally after fixing the request-vs-query param problem. I'll have to dig. |
Change-Id: Id07dbe608613d905dffa96aa119d7deadb1dca5d
| case ParameterAvailabilityClass: | ||
| paramAvailabilityClass, err := ConvertStringToAvailabilityClass(v) | ||
| if err != nil { | ||
| return p, fmt.Errorf("parameters contain invalid availability class parameter: %w", err) |
There was a problem hiding this comment.
nit: Hyphenate "availability-class" to make it more clear what parameter is invalid.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattcary, pwschuurman 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 |
|
/cherry-pick release-1.10 |
|
@mattcary: new pull request created: #1257 In response to this:
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. |
/kind feature
What this PR does / why we need it:
This adds an alpha-level storage class parameter to always perform a force attach. This is only valid for RePD disks (although this is not validated --- using this parameter with zonal disks will cause attaches to fail).
This parameter is not guaranteed to have continued support or to be stable in any way. In addition, using this can cause data loss.