-
Notifications
You must be signed in to change notification settings - Fork 39.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove controller node plugin driver dependency for non-attachable fl… #47503
Conversation
Hi @chakri-nelluri. 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-bot ok to test |
pkg/volume/flexvolume/probe.go
Outdated
ok, err := attachablePlugin.isAttachable() | ||
if err != nil { | ||
glog.Errorf("Unable to probe plugin: %s", attachablePlugin.driverName) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to require all existing flex drivers to implement the new probe call. Can we have a default instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the capabilities call isn't implemented, isAttachable() will return true, and the old behavior stays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This change doesn't require you to modify the existing plugins.
cc @kubernetes/sig-storage-pr-reviews |
pkg/volume/flexvolume/plugin.go
Outdated
flexVolumePlugin | ||
} | ||
|
||
var _ volume.AttachableVolumePlugin = &flexVolumeAttachablePlugin{} | ||
var _ volume.PersistentVolumePlugin = &flexVolumePlugin{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming here might cause confusions (attachable vs persistent?) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not flex objects. These are infra objects and we are just declaring it.
pkg/volume/flexvolume/plugin.go
Outdated
} | ||
|
||
// By default all plugins are attachable, unless they report otherwise. | ||
cap, ok := res.CapabilityMap[attachCapability] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like cap is a keyword. Should be OK here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to rename it to capabilities, similar to annotations & labels. Will update the PR.
pkg/volume/flexvolume/plugin.go
Outdated
|
||
// By default all plugins are attachable, unless they report otherwise. | ||
cap, ok := res.CapabilityMap[attachCapability] | ||
if ok && !cap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"&& !cap" isn't necessary here. The two return statements can also be collapsed into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep.. remove !cap :) We still need two return statements.
pkg/volume/flexvolume/probe.go
Outdated
} else { | ||
// Plugin does not support attach/detach, so return flexVolumePlugin which supports | ||
// SetupAt & TearDown functionality | ||
plugin := &flexVolumePlugin{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could reuse the same instance in line 45.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
pkg/volume/flexvolume/probe.go
Outdated
// Check whether the plugin is attachable. | ||
ok, err := attachablePlugin.isAttachable() | ||
if err != nil { | ||
glog.Errorf("Unable to probe plugin: %s", attachablePlugin.driverName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend including the isAttachable() error in the message to give a reason why probe is not successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. Copy & paste typo. will fix it.
pkg/volume/flexvolume/driver-call.go
Outdated
@@ -198,6 +200,10 @@ type DriverStatus struct { | |||
VolumeName string `json:"volumeName,omitempty"` | |||
// Represents volume is attached on the node | |||
Attached bool `json:"attached,omitempty"` | |||
// Returns capabilites of the driver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
pkg/volume/flexvolume/probe.go
Outdated
ok, err := attachablePlugin.isAttachable() | ||
if err != nil { | ||
glog.Errorf("Unable to probe plugin: %s", attachablePlugin.driverName) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the capabilities call isn't implemented, isAttachable() will return true, and the old behavior stays.
pkg/volume/flexvolume/attacher.go
Outdated
|
||
return (*attacherDefaults)(a).GetDeviceMountPath(spec, mountsDir) | ||
volumeName, err := a.plugin.GetVolumeName(spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetVolumeName() is called before every makeGlobalMountPath(). Maybe move the call inside makeGlobalMountPath().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
pkg/volume/flexvolume/probe.go
Outdated
unsupportedCommands: []string{}, | ||
}) | ||
|
||
attachablePlugin := &flexVolumeAttachablePlugin{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Factoring is a little strange here. Two suggestions:
- How about introducing a constructor method
NewFlexVolumePlugin(...)
that takes all theflexVolumePlugin
fields as a parameter. It would internally test for attachability and return either aflexVolumeAttachablePlugin
or aflexVolumePlugin
--that'll simplify the code here. - For testing attachability, instead of constructing a
flexVolumePlugin
object and callingisAttachable()
on it, it would be cleaner to haveIsAttachable(...)
be a static method that takes all theflexVolumePlugin
fields as a parameter, make the appropriate calls to the driver to determine attachability, and then return true/false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Refactored it. PTAL.
return false, err | ||
} | ||
|
||
// By default all plugins are attachable, unless they report otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making the default false, aka non-attachable? And if a plugin implements attach, then they must explicitly also set capability to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is going to break existing plugins. That is the only reason. Doesn't benefit too much either-way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify it would break existing post-1.6 drivers right? Pre-1.6 drivers would continue to work as is if the default was false, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For Post 1.6 drivers. Users have to modify their drivers to work for 1.6+ anyways, because of change mount callout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Ok fine to leave as is.
I don't understand how this PR "Removes requirement to install flex volume drivers on master node" - if there is no flex volume driver on the master, then controller-manager does not create a volume plugin for it and the situation is exactly the same as before this PR. Calling |
@jsafrane. This will fix it on Kubelet side. Now Kubelet won't wait for controller-manager to attach the volume for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
runner: exec.New(), | ||
unsupportedCommands: []string{}, | ||
}) | ||
plugin, err := NewFlexVolumePlugin(pluginDir, f.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! Thanks for refactoring, so much cleaner!
/lgtm I added a release note to original comment. |
…ex volume drivers (Ex: NFS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
pkg/volume/flexvolume/plugin.go
Outdated
@@ -71,21 +123,13 @@ func (plugin *flexVolumePlugin) GetVolumeName(spec *volume.Spec) (string, error) | |||
call := plugin.NewDriverCall(getVolumeNameCmd) | |||
call.AppendSpec(spec, plugin.host, nil) | |||
|
|||
_, err := call.Run() | |||
status, err := call.Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Catch. Oops.. Mangled in rebase. Let me fix it.
/lgtm |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
/approve |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chakri-nelluri, saad-ali, verult Assign the PR to them by writing Associated issue: 47109 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 |
/test pull-kubernetes-e2e-gce-etcd3 |
2 similar comments
/test pull-kubernetes-e2e-gce-etcd3 |
/test pull-kubernetes-e2e-gce-etcd3 |
This test suite is super flaky! @marun you may want to consider manual merge. |
/retest |
/retest |
Automatic merge from submit-queue (batch tested with PRs 47878, 47503, 47857) |
Will this fix be cherry-picked to 1.7 branch? The milestone is v1.7 but there is no cherry-pick labels. |
Looks like it's already in the 1.7 branch: |
Automatic merge from submit-queue Updating FlexVolume doc with capabilities addition. The following PR introduces a FlexVolume API change, so the documentation needs to be updated. kubernetes/kubernetes#47503
FYI: you need to use 1.7.4+ to include this fix. |
@kokhang as long as the driver exists on master, everything should work as expected. If the flag is set, other plugins may run into attach race conditions between nodes. This was the main problem AttachDetachController solves. |
…ex volume drivers (Ex: NFS).
What this PR does / why we need it:
Removes requirement to install flex volume drivers on master node for non-attachable drivers likes NFS.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #47109