-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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 requirement to run the Portworx volume driver on master node #45518
Conversation
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 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. |
/assign @jsafrane |
@k8s-bot ok to test |
pkg/volume/portworx/portworx_util.go
Outdated
|
||
if driverClient != nil { | ||
util.portworxClient = driverClient | ||
break OUTER |
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 really don't like this break, can't you just return here?
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 suggestion. Fixed in the latest incremental (4164374)
pkg/volume/portworx/portworx_util.go
Outdated
e = err | ||
kubeClient := volumeHost.GetKubeClient() | ||
if kubeClient != nil { | ||
nodes, err := kubeClient.CoreV1().Nodes().List(metav1.ListOptions{}) |
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.
Please add a label selector to ListOptions to save the label check below and transfer less data.
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.
Removed filtering logic in latest incremental. Please see reasoning in comment below.
pkg/volume/portworx/portworx_util.go
Outdated
pxdDriverName = "pxd" | ||
pwxSockName = "pwx" | ||
pvcClaimLabel = "pvc" | ||
labelNodeRoleMaster = "node-role.kubernetes.io/master" |
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 Portworx specific label and thus should have portworx
prefix. In addition, masters and nodes can be separate machines, /master
is not a good name here... Perhaps portworx.kubernetes.io/driver
? Feel free to suggest a better 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.
In latest incremental (4164374), I decided to remove filtering based on a specific label.
Previously the goal was to filter out master nodes but for that we needed to label master nodes during deployement. We weren't gaining enough for this added step so I decided to remove the filtering altogether.
With the latest commit, you scan all nodes on every mount and unmount, that's IMO very inefficient. There can be thousands of nodes, only few of them can run something on port 9001, yet you try all of them. That's no-go for me. I don't know Portworx internals and I am confused by the terminology here. You are looking for a "master" node. What is this "master"? Is it Kubernetes API server(s)? That's not listed in nodes in most cases. Is it a set of nodes that runs some sort of Portworx server pods? Use a service (with well-known name in a well-known namespace) to get to it. In addition, can I, as malicious user, run a dummy Portworx service on port 9001 on a random node and steal your data or even credentials? I hope you at least validate server ssl certificates... |
@jsafrane There is some confusion here. Let me try to clear it.
Let me know that clears your concerns. |
No. Kubelet creates new mounter for each mount. And mounter gets empty PortworxVolumeUtil. https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/portworx/portworx.go#L92 And if you are going to cache the node, you should recover somehow when the node is deleted and you should create a new client, pointing to a different node.
Why do you need to list all nodes then? If the container is on all nodes, kubelet can easily try localhost:9001 (or hostname:9001) or get its own node addresses, you don't need to list all of them. |
Are you scanning every node in the cluster, looking for something that tastes like your management server? With no credentials? That any pod in the cluster can trivially fake? This is a really bad idea, and really needs to be re-thought. Why not publish a Service? It still has no credentials, but at least you're not just hunting. |
I'm out, assigning to saad |
@thockin and @jsafrane: Good inputs. And yes agreed ! We are working on a service-based design. A quick question: What do you guys think about running a NodePort type service and the Portworx volume plugin always sends the request to localhost:. The NodePort service will take care of routing the request to one of the pods backing the service. Our primary use case here is GKE where our pod won't be running on the master but the volume create request is sent to the master. So if we have a NodePort service running, we can update our plugin driver running on master to send requests to localhost:. Alternately, I can use the default clusterIP service and talk to the service using it's cluster IP. |
Now it looks much better (and shorter). I don'd like only some copying of code, which can be IMO easily refactored into a function, the rest is OK. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harsh-px, jsafrane
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/release-note-none |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
@harsh-px: The following test(s) failed:
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/test-infra repository. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 45518, 46127, 46146, 45932, 45003) |
…18-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #45518 upstream release 1.6 Cherrypick of #45518 **What this PR does / why we need it**: This change removes requirement to run the Portworx volume driver on Kubernetes master node. **Special notes for your reviewer**: Before this pull request, in order to use a Portworx volume, users had to run the Portworx container on the master node. Since it isn't ideal (and impossible on GKE) to schedule any pods on the master node, this PR removes that requirement.
Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
@harsh-px, master is the 1.7 branch, so this PR will be in 1.7 |
What this PR does / why we need it:
This change removes requirement to run the Portworx volume driver on Kubernetes master node.
Special notes for your reviewer:
Before this pull request, in order to use a Portworx volume, users had to run the Portworx container on the master node. Since it isn't ideal (and impossible on GKE) to schedule any pods on the master node, this PR removes that requirement.