refactor(flexvolume): simplify capabilities handling #51169
Conversation
mcluseau
commented
Aug 23, 2017
•
mcluseau
commented
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @MikaelCluseau. 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-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/S
mcluseau
force-pushed
the
wip-flexvolume-caps
branch
from
8742b6e to
b937208
Compare
k8s-github-robot
added
size/M
mcluseau
force-pushed
the
wip-flexvolume-caps
branch
from
b937208 to
bd9f099
Compare
mcluseau
force-pushed
the
wip-flexvolume-caps
branch
from
bd9f099 to
1c34107
Compare
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
k8s-github-robot
added
the
needs-rebase
pkg/volume/flexvolume/plugin.go
Outdated
| call := plugin.NewDriverCall(initCmd) | ||
| _, err := call.Run() | ||
| return err | ||
| // no-op, init in the FlexVolume API is not the same thing as VolumePlugin.Init() |
There was a problem hiding this comment.
*Nit - Rephrase and add, plugin already initialized during probe.
pkg/volume/flexvolume/plugin.go
Outdated
| @@ -64,44 +65,25 @@ func NewFlexVolumePlugin(pluginDir, name string) (volume.VolumePlugin, error) { | |||
| unsupportedCommands: []string{}, | |||
| } | |||
|
|
|||
| // Check whether the plugin is attachable. | |||
| ok, err := isAttachable(flexPlugin) | |||
| call := flexPlugin.NewDriverCall(initCmd) | |||
There was a problem hiding this comment.
Add comment. Initialize the plugin and probe the capabilities.
There was a problem hiding this comment.
We should make it very explicit that driver initialization is done when the plugin is instantiated, instead of during the VolumePlugin.Init() call like one might expect.
mcluseau
force-pushed
the
wip-flexvolume-caps
branch
2 times, most recently
from
3da79cb to
29a9a6b
Compare
k8s-github-robot
added
release-note
mcluseau
changed the title
|
@chakri-nelluri @verult addressed the issues, added a test. PTAL :) |
mcluseau
force-pushed
the
wip-flexvolume-caps
branch
from
29a9a6b to
c2a8bd2
Compare
mcluseau
force-pushed
the
wip-flexvolume-caps
branch
from
c2a8bd2 to
22d5e48
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve no-issue |
k8s-github-robot
added
the
approved
|
/lgtm |
|
/cc @TerraTech |
|
@verult: GitHub didn't allow me to request PR reviews from the following users: TerraTech. Note that only kubernetes members can review this PR, and authors cannot review their own PRs. 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. |
|
/lgtm I like that DriverCapabilities was promoted as that simplifies the flow by handling it directly during the unmarshalling. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikaelCluseau, TerraTech, chakri-nelluri, verult Associated issue requirement bypassed by: chakri-nelluri 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 |
|
folks, there are now flexvolume e2e tests so if you make a breaking change please take a second to update the tests: https://github.com/kubernetes/kubernetes/blob/90a45b2df3a68313d7a768c487e20a6d8c5913e6/test/e2e/storage/flexvolume.go and https://github.com/kubernetes/kubernetes/tree/90a45b2df3a68313d7a768c487e20a6d8c5913e6/test/e2e/testing-manifests/flexvolume thanks. |
|
@wongma7 what is failing? Why isn't it reported by the CI? I have "All checks have passed |
|
@MikaelCluseau it’s in the alpha suite so won’t block prs. They’re not meant to slow down alpha development, just there to make sure breaking changes are being made deliberately. the tests are really basic right now, but as we add more functionality to flex (namely dynamic installation), I think the e2es will become essential so i’d appreciate help maintaining them is all |
|
@wongma7 I completely agree, but I need to know where I can see if something is broken. Also, without at least an automated report, it's easy to forget to check. For instance, you had to provide links ;-) And they're not test log so it's hard to know what failed without having a way to rerun those tests myself. @chakri-nelluri from static code analysis, I think this e2e is broken because it uses |
|
@MikaelCluseau yeah, the workflow is not pleasant. Even if the tests did automatically run, the best they can do atm is give us a black/white failure that I will have to investigate: i.e. we have to check logs to see if the plugin init failed? or mount/attach callout? I will try to make them runnable on hack/local-up-cluster.sh so that they're more useful during development. So (for now) nobody should feel any obligation to help maintain, the best I can do is raise awareness and plea for help : p In this particular case I'll be working on it today |
|
Automatic merge from submit-queue (batch tested with PRs 46986, 51214, 51169, 50155, 51261) |
