Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Updated to use default Kubernetes client discovery logic #3432

Merged
merged 5 commits into from
Jan 17, 2020

Conversation

nicknezis
Copy link
Contributor

The defaultClient() call incorporates the following logic:
If $KUBECONFIG is defined, use that config file.
If $HOME/.kube/config can be found, use that.
If the in-cluster service account can be found, assume in cluster config.
Default to localhost:8080 as a last resort.

With this code change, we'd no longer be using the scheduler config item for K8s scheduler URI. But the config item is used to create Job Links. So I couldn't get rid of that config item.

Any thoughts on how I could fully remove the scheduler URI config item?
Do you think the defaultClient() code is good enough? It seemed to work in my Minikube testing and I figured worst case that last option would point to kubectl-proxy if we update it to use port 8080. I assume if someone needs to run the Heron API Server outside of the k8s cluster, then can configure with the $KUBECONFIG environment variable.

@nicknezis
Copy link
Contributor Author

Hopefully this addresses part of what was asked for in #3259

@joshfischer1108
Copy link
Member

joshfischer1108 commented Jan 11, 2020

I haven't looked at the code yet, but does this PR relate to the issue listed here? #3259

@nicknezis
Copy link
Contributor Author

Yes, I tested it in Minikube and was able to spin up a StatefulSet without the kubectl-proxy. I'll update this PR to remove that container from the Kubernetes deploy yamls and Helm chart. I wanted to also remove the Scheduler URI as was suggested in #3259, but the config item was used in the Job link. But the current implementation does not generate job specific job links.

@joshfischer1108
Copy link
Member

Ahh yeah I see you mentioned the issue right above where I mentioned the issue. Consider that a palm to forehead moment 😄 . I haven't tested the code, but It sounds like it addresses the issue after the proxy container is taken out.

@joshfischer1108
Copy link
Member

@cckellogg If you have some time could you review this?

@joshfischer1108
Copy link
Member

@nicknezis It looks like the build is failing because of checkstyle errors.

@nicknezis
Copy link
Contributor Author

I think the test failure might not be related to my changes. I am able to run the tests locally with success.

@nwangtw
Copy link
Contributor

nwangtw commented Jan 12, 2020

I think the test failure might not be related to my changes. I am able to run the tests locally with success.

//heron/schedulers/tests/java:KubernetesSchedulerTest FAILED in 1.2s
FAILED org.apache.heron.scheduler.kubernetes.KubernetesSchedulerTest.testGetJobLinks (0.0s)

Unit tests should be normally stable. Let me trigger another build and see.

@nicknezis
Copy link
Contributor Author

Looks like it failed again. I'll try to figure out why. Thanks!

@nicknezis
Copy link
Contributor Author

Is this PR ok to be accepted?

@joshfischer1108
Copy link
Member

@nicknezis LGTM. I'll wait a bit to see if anyone else has feedback.

@joshfischer1108 joshfischer1108 merged commit 91dbabf into apache:master Jan 17, 2020
@nicknezis nicknezis deleted the nicknezis/k8s-client branch January 17, 2020 05:48
sreev pushed a commit to sreev/incubator-heron that referenced this pull request Apr 9, 2020
* Updated to use default Kubernetes client discovery logic

* Removed kubectl-proxy container

* Remove kubectl-proxy image property

* Syntax cleanup

* Added missing kubernetes dependency
nicknezis added a commit that referenced this pull request Sep 14, 2020
* Updated to use default Kubernetes client discovery logic

* Removed kubectl-proxy container

* Remove kubectl-proxy image property

* Syntax cleanup

* Added missing kubernetes dependency
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants