-
Notifications
You must be signed in to change notification settings - Fork 90
[WIP] Get endpoints for service instead of pods #11
Conversation
@jimmidyson sorry about the formatting changes and some cosmetic changes but I tried hard to keep up with your conventions. How does this look? I'm going to need you help testing this with perhaps a snapshot release or something? I'm open to ideas. |
// pod IP is a single IP Address. We need to build a TransportAddress from it | ||
final TransportAddress[] addresses; | ||
try { | ||
addresses = transportService.addressesFromString(endpointIp.concat(":9300")); |
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.
Is there no way to get the port from the endpoint list?
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 should be doable. See https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/types.go#L1091 ff for more details. Basically it's a Cartesian product between addresses and ports in each endpoint subset.
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.
While I'm hardcoding port 9300 and thus limiting the user choices here, I'm concerned that a multiport service will make it harder for us to determine which port to use.
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 point. I think using port 9300 for now is fine. Almost all deployments will use that I'm sure.
@pires Thanks for this. Overall looks good. I wonder if we could use fabric8 arquillian to add some integration tests for this? Happy to have that later though. For now you should be able to build locally & use in a local docker image? Don't like publishing snapshots personally & would rather not do that. |
Probably would be a good thing to do. Would we need something like
OK. Will do that tomorrow then. |
No - fabric8 arquillian can spin up pods in a kubernetes cluster for integration testing. See http://fabric8.io/guide/testing.html for more details. It's pretty awesome! We're using it in some CD scenarios & it works great. But like I said, this can come later - we will merge this once you're happy with it. Let me know how your testing goes. |
+1 |
I'm sorry, I have been overwhelmed with unrelated work.
Yes, will do this for now. |
I'll try and look at Arquillian and test this out locally. |
@mulloymorrow that would be A-W-E-S-O-M-E! |
Can we merge this pull request with a new branch other than master? I'd like to include these commits in a fork for testing. |
@mullowmorrow Perhaps @pires could add you as a collaborator on his fork? |
// We can ignore it in the list of DiscoveryNode | ||
logger.trace("current node found. Ignoring {} - {}", pod.getId(), podIp); | ||
// get all endpoints for service | ||
final Endpoints endpoints = getKubernetes().endpointsForService(this.selector, null); |
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.
IAE thrown by kubernetes-api-2.1.0::KubernetesClient.validateNamespace: Template parameter set to null.
2015-05-28T18:17:16.071899300Z [2015-05-28 18:17:16,071][WARN ][io.fabric8.elasticsearch.discovery.k8s.K8sUnicastHostsProvider] [Cayman] Exception caught during discovery java.lang.IllegalArgumentException : Template parameter value is set to null
2015-05-28T18:17:16.071899300Z java.lang.IllegalArgumentException: Template parameter value is set to null
2015-05-28T18:17:16.071899300Z at org.apache.cxf.jaxrs.impl.UriBuilderImpl.doBuild(UriBuilderImpl.java:97)
2015-05-28T18:17:16.071899300Z at org.apache.cxf.jaxrs.impl.UriBuilderImpl.buildFromEncoded(UriBuilderImpl.java:230)
2015-05-28T18:17:16.071899300Z at org.apache.cxf.jaxrs.client.ClientProxyImpl.invoke(ClientProxyImpl.java:180)
2015-05-28T18:17:16.071899300Z at com.sun.proxy.$Proxy29.endpointsForService(Unknown Source)
2015-05-28T18:17:16.071899300Z at io.fabric8.elasticsearch.discovery.k8s.K8sUnicastHostsProvider.getNodesFromKubernetesSelector(K8sUnicastHostsProvider.java:119)
2015-05-28T18:17:16.071899300Z at io.fabric8.elasticsearch.discovery.k8s.K8sUnicastHostsProvider.buildDynamicNodes(K8sUnicastHostsProvider.java:102)
2015-05-28T18:17:16.071899300Z at org.elasticsearch.discovery.zen.ping.unicast.UnicastZenPing.sendPings(UnicastZenPing.java:313)
2015-05-28T18:17:16.071899300Z at org.elasticsearch.discovery.zen.ping.unicast.UnicastZenPing$2.doRun(UnicastZenPing.java:228)
2015-05-28T18:17:16.071899300Z at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:36)
2015-05-28T18:17:16.071899300Z at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
2015-05-28T18:17:16.071899300Z at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
2015-05-28T18:17:16.071899300Z at java.lang.Thread.run(Thread.java:745)
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.
We probably need to set a default namespace, like default
.
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.
Testing default
ok to test |
Can one of the admins verify this patch? |
Refer to this link for build results (access rights to CI server needed): |
retest this please |
Please comment with '[merge]' to automerge this request |
Refer to this link for build results (access rights to CI server needed): |
@pires @mulloymorrow I've merged this locally & made some changes to support your use case, getting endpoints from service name. I've tested & it works fine. Please can you review |
We're running in v1beta2, currently. Services in our environment can be found vi |
As v1beta1 & v1beta2 has been dropped from kubernetes we've removed those api versions from the fabric8 client, only supporting v1beta3 ATM. Soon that will change to only v1. Sorry but you'll need to upgrade your cluster in order to use this plugin going forward. We're targeting the v1 API in line with kubernetes 1.0 release. |
I agree with Jimmi. You really need to upgrade.
|
Switched over to Gcloud! Will be testing there. |
You mean GKE? Also, I'm sorry for not updating this, but right now there's a lot going on regarding reading the Kubernetes API that will have an impact on a lot of the stuff I'm working on and most probably this project as well. @jimmidyson did you check the |
@pires , Yes. The Google Container Engine. |
@pires I'm working on the fabric8 kubernetes API support for service accounts now, but that shouldn't stop this PR going through. Separate issue for that please. |
@jimmidyson point taken |
@pires Cool thanks for the feedback. Btw seen the advice to use SRV records rather than API server calls? |
@jimmidyson yes, but see kubernetes/kubernetes#9107. We should be able to work without DNS, because it's not something that is granted in every Kubernetes cluster. |
@pires Agreed - was just pointing out that I prefer DNS if available. |
Superseded by #12 |
Thanks @pires! |
Thank you @jimmidyson |
Thanks @pires @jimmidyson |
This is work-in-progress to fix #9 and improve node-discovery efficiency.