-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Lookup PX api port from k8s service #70392
Conversation
/kind bug |
func lookupPXAPIPortFromService(svc *v1.Service) int32 { | ||
for _, p := range svc.Spec.Ports { | ||
if p.Port == osdMgmtDefaultPort { | ||
return p.TargetPort.IntVal |
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.
What if the service uses named port?
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.
Portworx users use a fixed portworx-service spec generated from https://install.portworx.com. Hence above logic that looks for port
and targetPort
works.
The spec is
kind: Service
apiVersion: v1
metadata:
name: portworx-service
namespace: kube-system
labels:
name: portworx
spec:
selector:
name: portworx
ports:
- name: px-api
protocol: TCP
port: 9001
targetPort: 9001
pkg/volume/portworx/portworx_util.go
Outdated
} | ||
|
||
opts := metav1.GetOptions{} | ||
svc, err := kubeClient.CoreV1().Services(api.NamespaceSystem).Get(pxServiceName, opts) |
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 issues API call for each portworx mount / unmount in kubelet. Can the port number change in a running cluster? Can be the port cached in any 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.
@jsafrane I've improved this in the incremental commit.
Now I use an existing saved client as long as it's valid. If port changes, a new client will be created and used subsequently.
I have also refactored the methods and created separate methods for getting a client from local calls (mount, unmount, attach and detach) vs cluster wide calls.
/retest |
/test pull-kubernetes-e2e-kops-aws |
@jsafrane Can you take a second look and see if it addressed your comments. Let me know ! |
Fixes kubernetes#70033 Signed-off-by: Harsh Desai <harsh@portworx.com>
- reused client whenever possible - refactor get client function into explicit cluster-wide and local functions Signed-off-by: Harsh Desai <harsh@portworx.com>
Signed-off-by: Harsh Desai <harsh@portworx.com>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harsh-px, jsafrane 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 |
…92-upstream-release-1.13 Automated cherry pick of #70392: Lookup PX api port from k8s service
…92-upstream-release-1.12 Automated cherry pick of #70392: Lookup PX api port from k8s service
…92-upstream-release-1.11 Automated cherry pick of #70392: Lookup PX api port from k8s service
Fixes #70033
Signed-off-by: Harsh Desai harsh@portworx.com
What type of PR is this?
What this PR does / why we need it: Refer to #70033
Which issue(s) this PR fixes :
Fixes #70033
Special notes for your reviewer: