Skip to content
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

Support multiple kubernetes compute clusters #1290

Merged
merged 12 commits into from Nov 8, 2019

Conversation

@scrosby
Copy link
Member

scrosby commented Nov 1, 2019

Changes proposed in this PR

  • Iterate over all of the compute clusters in the config.edn and initialize them.

Why are we making these changes?

  • Initial support and testing for multiple kubernetes compute clusters.
Scott Crosby added 2 commits Oct 22, 2019
They are generated with the new cluster generation script.
Scott Crosby
@scrosby scrosby requested a review from dposada Nov 1, 2019
@@ -236,6 +236,7 @@ def test_mea_culpa_retries(self):
finally:
util.kill_jobs(self.cook_url, [uuid])

@unittest.skipUnless(len(util.get_compute_clusters()) == 1, 'Test is only well-founed if there is only a single compute cluster')

This comment has been minimized.

Copy link
@dposada

dposada Nov 1, 2019

Member

Rather than skip the test, let's fix the test so that it works when there are multiple compute clusters.

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 4, 2019

Author Member

Talked in person. @dposada suggets launching the job, looking up the compute cluster it launched as, and validating it in the map and that it has the right type in the map and write framework id (if mesos)

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 4, 2019

Author Member

Changed.

return kubernetes_compute_clusters

@functools.lru_cache()
def get_kubernetes_compute_cluster():

This comment has been minimized.

Copy link
@dposada

dposada Nov 1, 2019

Member

Seems like this function should no longer exist -- there is no longer the concept of getting "the kubernetes compute cluster", since we now support multiple.

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 4, 2019

Author Member

Gone now.

:compute-clusters [{:factory-fn cook.kubernetes.compute-cluster/factory-fn
:config {:compute-cluster-name "minikube"
;; Location of the kubernetes config file, e.g. $HOME/.kube/config
:config-file #config/env "COOK_K8S_CONFIG_FILE"}}]
;; Location of the kubernetes config file. Hardcoded to the location specified by bin/make-gke-test-cluster
:config-file "../scheduler/.cook_kubeconfig_1"}}
{:factory-fn cook.kubernetes.compute-cluster/factory-fn
:config {:compute-cluster-name "minikube-2"
;; Location of the kubernetes config file. Hardcoded to the location specified by bin/make-gke-test-cluster
:config-file "../scheduler/.cook_kubeconfig_2"}}]
Comment on lines 22 to 29

This comment has been minimized.

Copy link
@dposada

dposada Nov 1, 2019

Member

Why are we calling these "minikube"?

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 4, 2019

Author Member

We can call them anything. :) But it helps to keep the same name across local testing runs, so I've not been motivated to change them.

@@ -142,8 +142,10 @@
rebalancer-trigger-chan straggler-trigger-chan]} trigger-chans
{:keys [hostname server-port server-https-port]} server-config
datomic-report-chan (async/chan (async/sliding-buffer 4096))

; TODO: Tech-debt. Should remove compute-cluster when we rethink initialization and leadership.
; TODO: See also cluster-leadership-chan below.
compute-cluster (cc/get-default-cluster-for-legacy)

This comment has been minimized.

Copy link
@dposada

dposada Nov 1, 2019

Member

Main Point

I think we should resolve the initialization and leadership (and remove the compute-cluster variable) in this PR.

Observations

  • The only place I see compute-cluster being used now is here: https://github.com/twosigma/Cook/pull/1290/files#diff-e6dcc5bcf7c428033ee102636b9582e3R249
  • It doesn't make sense to for there to be a current-leader? function on the ComputeCluster protocol.
    • The KubernetesComputeCluster implementation is returning true always, which is obviously useless/not needed.
    • The MesosComputeCluster implementation returns (not (nil? @driver-atom)), and driver-atom only gets reset! to nil when we lose the connection to Mesos.
    • So maybe a better name for this function (if it were still needed) would be connected?.

Suggestions

  • Rename mesos-leadership-atom to just leadership-atom, since its meaning is related to Cook leadership, not Mesos leadership
  • Delete the current-leader? function on ComputeCluster and its implementations (see observations above)
  • In the curator stateChanged handler, where current-leader? is being referenced to see if we should exit, instead reference the (to be renamed) leadership-atom (before the line where it's reset! to false), which will tell us if we're the leader and should exit
  • Rename cluster-leadership-chan to cluster-connected-chan, since it has nothing to do with leadership
  • Instead of <!!-ing on the chan from a single initialize-cluster call, merge the chans from all the initialize-cluster calls and then <!! on the merged chan

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

(Documenting a discussion) I proposed holding off on this refactor/cleanup; we need to rethink leader election: in the new multiple compute cluster world, we might not want cook to go down if any single compute cluster goes bad. We need to decide on what the new policy should be, then implement it. Thus, we would be visiting this code in about a week or so.

The resolution of the discussion was to do this cleanup/refactor as part of this change, but implement it so that we have no functionality changes. Then implement the new policy in a followup change.

integration/tests/cook/test_basic.py Outdated Show resolved Hide resolved
instance_compute_cluster_type = instance['compute-cluster']['type']
self.assert_(instance_compute_cluster_type in ['mesos','kubernetes'],message)
filtered_compute_clusters = [compute_cluster for compute_cluster in settings_dict['compute-clusters']
if compute_cluster['config']['compute-cluster-name'] == instance_compute_cluster_name]

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

Indentation seems off on this line

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

Good catch.

integration/tests/cook/test_basic.py Outdated Show resolved Hide resolved
found_compute_cluster = filtered_compute_clusters[0]

self.assertIsNotNone(found_compute_cluster, message + str(settings_dict['compute-clusters']))
expected_mesos_framework = found_compute_cluster['config'].get('framework-id', None)

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

Seems like we should we move this line to inside this check:

if found_compute_cluster['factory-fn'] == 'cook.mesos.mesos-compute-cluster/factory-fn'

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

Ok

expected_mesos_framework = found_compute_cluster['config'].get('framework-id', None)

self.assertEqual(util.get_compute_cluster_type(found_compute_cluster), instance_compute_cluster_type, message)
self.assertEqual(found_compute_cluster['config']['compute-cluster-name'], instance_compute_cluster_name, message)

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

This check is superfluous because we got the found_compute_cluster specifically by checking that compute_cluster['config']['compute-cluster-name'] == instance_compute_cluster_name

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

Nuked

cluster-connected-chans (pc/map-vals
#(cc/initialize-cluster %
pool-name->fenzo
running-tasks-ents) compute-clusters)
; Note: This doall has a critical side effect of actually initializing all of the clusters.
_ (doall cluster-connected-chans)]
Comment on lines 180 to 185

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

Seems cleaner to just have the doall inline, e.g.

cluster-connected-chans (doall (map ...))

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

Ok.

scheduler/src/cook/mesos.clj Outdated Show resolved Hide resolved
;
; WARNING: This code is very misleading. It looks like we'll suicide if ANY of the cluster's lose leadership.
; However, kubernetes currently can't lose leadership, so this is the equivalent of only looking at mesos.
; During code review, we didn't want to implement the special case for mesos.

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member
Suggested change
; During code review, we didn't want to implement the special case for mesos.
; We didn't want to implement the special case for mesos.

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

ok

scheduler/src/cook/mesos.clj Outdated Show resolved Hide resolved
@@ -234,8 +246,7 @@
;; We will give up our leadership whenever it seems that we lost
;; ZK connection
(when (#{ConnectionState/LOST ConnectionState/SUSPENDED} newState)
(reset! mesos-leadership-atom false)

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

I think we still need this reset!

This comment has been minimized.

Copy link
@scrosby

scrosby Nov 7, 2019

Author Member

I don't think we need it; We suicide a few lines later unless we're running in simulation.

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

Ah, OK

scrosby and others added 2 commits Nov 7, 2019
Co-Authored-By: Daniel Posada <daniel.posada@gmail.com>
Scott Crosby
Copy link
Member

dposada left a comment

One renaming suggestion, otherwise LGTM

@@ -142,8 +143,7 @@
rebalancer-trigger-chan straggler-trigger-chan]} trigger-chans
{:keys [hostname server-port server-https-port]} server-config
datomic-report-chan (async/chan (async/sliding-buffer 4096))

compute-cluster (cc/get-default-cluster-for-legacy)
compute-clusters @cc/cluster-name->compute-cluster-atom

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

If we're going to leave it as a map, seems like it should be named cluster-name->compute-cluster

@@ -234,8 +246,7 @@
;; We will give up our leadership whenever it seems that we lost
;; ZK connection
(when (#{ConnectionState/LOST ConnectionState/SUSPENDED} newState)
(reset! mesos-leadership-atom false)

This comment has been minimized.

Copy link
@dposada

dposada Nov 7, 2019

Member

Ah, OK

Scott Crosby
@scrosby

This comment has been minimized.

Copy link
Member Author

scrosby commented Nov 7, 2019

Variable renamed as requested.

@dposada
dposada approved these changes Nov 7, 2019
@dposada

This comment has been minimized.

Copy link
Member

dposada commented Nov 7, 2019

Will merge when green

@scrosby

This comment has been minimized.

Copy link
Member Author

scrosby commented Nov 7, 2019

Internalgreen on mesos. GKE still running.

@dposada

This comment has been minimized.

Copy link
Member

dposada commented Nov 7, 2019

@scrosby Is this good to merge?

@dposada

This comment has been minimized.

Copy link
Member

dposada commented Nov 8, 2019

Per @scrosby, this can now be merged

@dposada dposada merged commit ca405b8 into twosigma:master Nov 8, 2019
2 checks passed
2 checks passed
Mergeable Mergeable Run has been Completed!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@scrosby scrosby deleted the scrosby:outgoing/multi-compute-cluster branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.