Supply Portworx StorageClass paramters in volume spec labels for server-side processing #49526
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @harsh-px. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
k8s-github-robot
added
size/S
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
| return "", 0, nil, err | ||
| spec, _ := specHandler.SpecFromOpts(p.options.Parameters) | ||
| if spec == nil { | ||
| spec = specHandler.DefaultSpec() |
There was a problem hiding this comment.
I am puzzled by this PR. StorageClass A has these parameters:
repl: "3"
SpecFromOpts returns a spec with HaLevel = 3 and you pass it to driver.Create. Cool.
StorageClass B has these parameters:
repl: "3"
io_priority: "high"
In #49526 you say that SpecFromOpts fails. Now you pass the default spec with HaLevel = 1, Cos = 0 and with repl and io_priority labels to driver.Create. Is this correct? Will driver.Create parse the labels again and override spec.HaLevel and spec.Cos? So what's the point of calling SpecFromOpts if everything can be passed as labels?
There was a problem hiding this comment.
Now you pass the default spec with HaLevel = 1, Cos = 0 and with repl and io_priority labels to driver.Create. Is this correct?
Yes.
Will driver.Create parse the labels again and override spec.HaLevel and spec.Cos?
Yes, our latest server side implementation of driver.Create will overide the spec with entries given in the labels.
So what's the point of calling SpecFromOpts if everything can be passed as labels?
If everyone is running the latest Portworx version, we don't need to call SpecFromOpts
This is done to handle the case, someone updates to the latest k8s but doesn't update Portworx. Since previous versions of Portworx do not overide spec from labels, I am still calling SpecFromOpts for backward compatibility.
The err is ignored because going forward, we don't want any StorageClass parsing errors in the k8s driver to block volume creation.
There was a problem hiding this comment.
A comment about this would be nice, the code is really confusing
There was a problem hiding this comment.
Done in latest incremental.
|
/test pull-kubernetes-e2e-gce-etcd3 |
harsh-px
force-pushed
the
fix-vol-param-parsing
branch
from
f2599c1 to
c692710
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harsh-px, jsafrane Associated issue: 49525 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest Review the full test history for this PR. |
5 similar comments
|
/retest Review the full test history for this PR. |
|
/retest Review the full test history for this PR. |
|
/retest Review the full test history for this PR. |
|
/retest Review the full test history for this PR. |
|
/retest Review the full test history for this PR. |
|
/retest |
|
/retest Review the full test history for this PR. |
|
Automatic merge from submit-queue (batch tested with PRs 49284, 49555, 47639, 49526, 49724) |
What this PR does / why we need it:
This change offloads the requirement of successfully parsing all existing and new portworx volume parameters to it's server-side components. As a result, for fixing bugs in existing volume parameters parsing and adding new support, we will not need to submit a k8s PR.
Which issue this PR fixes: fixes #49525
Release note: