Implemented host-placement group constraints #284

DiegoAlbertoTorres · 2017-04-04T16:01:09Z

Multiple code review changes, including better docstrings, many variable name changes, and filtering host->scored-tasks.

Created a new namespace for constraints

Generalized the balanced constraint to any attribute, exposed expected count parameter

Multiple code review changes: made unconstrained filtering in rebalancer simpler and more efficient, improved naming of cotask functions, added host placement explanation to groups.md and other small changes

Made offer cache maximum size and ttl configurable.

Added rebalancer tests for balanced constraint

CR changes: clarifications in group.md, extracted common code from fenzo constraints into shared functions, cotasks are now both unknown and running instances, host attributes now extracted from offers, renamed cotask-getter functions, added test for make-guuid->juuids

Added many docstrings. Made sure that order of considerable jobs is preserved before handing to scheduleOnce. Wrote a test to check that considerable jobs are passed in right order.

compute-preemption-decision now considers preemptions done in the same cycle to validate constraints.

Refactored constraints.clj. Now all logic for constraints is in the evaluation function, which is actually a multi-method. This multi-method can then be wrapped to make it fenzo-compatible (in make-fenzo-hp-constraint) or rebalancer-compatible (in make-rebalacer-hp-constaint).

Added unique-host constraint to the rebalancer. Refactored constraints.clj to define JobConstraint and GroupConstraint protocols, which make the code more modular, easier to modify and harder to unintentionally break.

Now synthetic instances in state have slave-id, which is used to find attributes of constraints. Fixed lack of nil check when placement constraint is 'all'.

Multiple minor improvements to address CR by shams

Small improvements as suggested in CR by Wil

Removed gpu-host-constraint-helper

Multiple small CR improvements as suggested by Wil.

More CR fixes as suggested by Wil, mostly on constraints.clj, msotly small refactoring and renaming

Signed-off-by: Diego Torres Quintanilla diego.torresquintanilla@twosigma.com

dposada

I haven't made it all the way through this yet, but here are some initial comments

dposada · 2017-04-06T14:44:11Z

scheduler/docs/groups.md

+Example:
+```json
+{"groups": [{"uuid": "410198dd-c171-470e-aeb8-6ffa4e6a2ada",
+            "host-placement": {"type": "unique"}],


Missing one more } before the ]

dposada · 2017-04-06T14:47:11Z

scheduler/docs/groups.md

+            "host-placement": {"type": "unique"}],
+ "jobs": [{"uuid": "831836ee-d5c3-47f7-a052-f4c3ce502495",
+           "group": "410198dd-c171-470e-aeb8-6ffa4e6a2ada". ...},
+           ...]}}


In the 2 lines above, why not have only one set of ellipses? Also, there's an extra trailing } it seems

I guess the first one indicates there are more fields inside each job, and the second shows there can be more jobs?

dposada · 2017-04-06T14:51:02Z

scheduler/src/cook/mesos/util.clj

            [clojure.walk :as walk]
            [datomic.api :as d :refer (q)]
+            [plumbing.core :refer (map-vals map-keys)]


Can we combine this with the existing plumbing.core require below?

dposada · 2017-04-06T14:52:43Z

scheduler/test/cook/test/testutil.clj

+
+(defn init-offer-cache
+  [& init]
+  (-> (if (nil? init) {} init)


Maybe (or init {})?

dposada · 2017-04-06T14:53:32Z

scheduler/project.clj

@@ -67,7 +67,7 @@
                 ;;Networking
                 [clj-http "2.0.0"]
                 [io.netty/netty "3.10.1.Final"]
-                 [cc.qbits/jet "0.5.7" :exclusions [org.eclipse.jetty/jetty-io
+                 [cc.qbits/jet "0.6.4" :exclusions [org.eclipse.jetty/jetty-io


Is the jet upgrade needed for the host placement feature?

It was needed for testing

dposada · 2017-04-06T15:24:17Z

scheduler/docs/groups.md

+```json
+{"groups": [{"uuid": "410198dd-c171-470e-aeb8-6ffa4e6a2ada",
+            "host-placement": {"type": "attribute-equals",
+                               "parameters": {"attribute": "HOSTNAME"}}],


Why not use availability zone since that's the example in the paragraph above? Also missing one more } before the ]

dposada · 2017-04-06T15:24:51Z

scheduler/docs/groups.md

+                               "parameters": {"attribute": "HOSTNAME"}}],
+ "jobs": [{"uuid": "831836ee-d5c3-47f7-a052-f4c3ce502495",
+           "group": "410198dd-c171-470e-aeb8-6ffa4e6a2ada". ...},
+           ...]}}


Same ellipses comment, and extra trailing }

dposada · 2017-04-06T15:25:39Z

scheduler/src/cook/components.clj

@@ -19,6 +19,7 @@
            [clj-time.core :as t]
            [clojure.java.io :as io]
            [clojure.pprint :refer (pprint)]
+            [clojure.core.cache :as cache]


Can we keep these sorted?

dposada · 2017-04-06T15:27:13Z

scheduler/src/cook/components.clj

+                                          (cache/fifo-cache-factory :threshold max-size)
+                                          (cache/ttl-cache-factory :ttl ttl-ms)
+                                          atom)]
+                            cache))


Nitpick: the let is not really needed

I remember this giving me trouble without the let... Let me try again

Got rid of let 👍

dposada · 2017-04-06T15:27:41Z

scheduler/src/cook/components.clj

@@ -346,6 +354,11 @@
                              task-constraints))
     :offer-incubate-time-ms (fnk [[:config [:scheduler {offer-incubate-ms 15000}]]]
                                  offer-incubate-ms)
+     :offer-cache (fnk [[:config [:scheduler {offer-cache nil}]]]
+                            (merge


This seems to be indented too far in

dposada · 2017-04-06T19:57:40Z

scheduler/src/cook/mesos/api.clj

@@ -250,12 +249,20 @@
  "A schema for the parameters of a host placement with type attribute-equals"
  {:attribute s/Str})

+(def Balanced-Parameters
+  "A schema for the parameters of a host placement with type balanced"
+  {:attribute s/Str


Can the attribute really be any string?

Pretty much: any string that is a valid mesos attribute name

dposada · 2017-04-06T20:59:36Z

scheduler/src/cook/mesos/constraints.clj

+(ns cook.mesos.constraints
+  (:require [clojure.tools.logging :as log]
+            [cook.mesos.util :as util]
+            [cook.mesos.group :as group]


Can we sort these?

dposada · 2017-04-06T21:00:49Z

scheduler/src/cook/mesos/constraints.clj

+            [cook.mesos.util :as util]
+            [cook.mesos.group :as group]
+            [swiss.arrows :refer :all])
+  (import com.netflix.fenzo.VirtualMachineLease))


:import instead of import

dposada · 2017-04-06T21:06:02Z

scheduler/src/cook/mesos/constraints.clj

+(defn get-class-name
+  "Returns the name of a class without package information."
+  [object]
+  (last (clojure.string/split (str (type object)) #"\.")))


Could we do (.getSimpleName (type object))?

Ooh, knew there must have been a better way for this. Now using .getSimpleName 👍

dposada · 2017-04-06T21:17:38Z

scheduler/src/cook/mesos/rebalancer.clj

@@ -539,5 +568,5 @@
    @(d/transact conn [{:db/id :rebalancer/config
                        :rebalancer.config/min-utilization-threshold 0.0
                        :rebalancer.config/safe-dru-threshold 0.0
-                        :rebalancer.config/min-dru-diff 0.0000000001
+                        :rebalancer.config/min-dru-diff 0.000000001


Why the change to min-dru-diff?

This is inside a comment block, so it does not matter. But will change it back to avoid confusion.

dposada · 2017-04-06T21:34:47Z

scheduler/src/cook/mesos/scheduler.clj

+                                                        (:value %))))
+                              (into {}))
+        cook-attributes {"HOSTNAME" (:hostname offer)
+                         "COOK_GPU?" (> (or (offer-resource-scalar offer "gpus") 0.0) 0)}]


Maybe:

(-> offer (offer-resource-scalar "gpus") (or 0.0) (> 0))

Yep, this is way better.

dposada

Done with my pass

dposada · 2017-04-06T22:18:19Z

scheduler/test/cook/test/mesos/constraints.clj

+
+(ns cook.test.mesos.constraints
+  (:use [clojure.test])
+  (:require [mesomatic.types :as mtypes]


Can we sort these?

dposada · 2017-04-06T22:18:36Z

scheduler/test/cook/test/mesos/constraints.clj

+            [cook.mesos.constraints :as constraints]
+            [cook.test.testutil :refer (restore-fresh-database! create-dummy-group create-dummy-job create-dummy-instance)]
+            [datomic.api :as d :refer (db)])
+  (import org.mockito.Mockito


:import instead of import

dposada · 2017-04-06T22:20:00Z

scheduler/test/cook/test/mesos/rebalancer.clj

+                scored->expected (zipmap scored expected)]
+            (doall (for [[scored expected] scored->expected]
+              (is (= scored expected)))))
+          )))))


Nitpick: close parens can go on previous line

dposada · 2017-04-06T22:21:33Z

scheduler/test/cook/test/testutil.clj

  (:require [clojure.core.async :as async]
+	    [clojure.core.cache :as cache]


Indenting off by a little

Multiple code review changes, including better docstrings, many variable name changes, and filtering host->scored-tasks. Created a new namespace for constraints Generalized the balanced constraint to any attribute, exposed expected count parameter Multiple code review changes: made unconstrained filtering in rebalancer simpler and more efficient, improved naming of cotask functions, added host placement explanation to groups.md and other small changes Made offer cache maximum size and ttl configurable. Added rebalancer tests for balanced constraint CR changes: clarifications in group.md, extracted common code from fenzo constraints into shared functions, cotasks are now both unknown and running instances, host attributes now extracted from offers, renamed cotask-getter functions, added test for make-guuid->juuids Added many docstrings. Made sure that order of considerable jobs is preserved before handing to scheduleOnce. Wrote a test to check that considerable jobs are passed in right order. compute-preemption-decision now considers preemptions done in the same cycle to validate constraints. Refactored constraints.clj. Now all logic for constraints is in the evaluation function, which is actually a multi-method. This multi-method can then be wrapped to make it fenzo-compatible (in make-fenzo-hp-constraint) or rebalancer-compatible (in make-rebalacer-hp-constaint). Added unique-host constraint to the rebalancer. Refactored constraints.clj to define JobConstraint and GroupConstraint protocols, which make the code more modular, easier to modify and harder to unintentionally break. Now synthetic instances in state have slave-id, which is used to find attributes of constraints. Fixed lack of nil check when placement constraint is 'all'. Multiple minor improvements to address CR by shams Small improvements as suggested in CR by Wil Removed gpu-host-constraint-helper Multiple small CR improvements as suggested by Wil. More CR fixes as suggested by Wil, mostly on constraints.clj, msotly small refactoring and renaming Signed-off-by: Diego Torres Quintanilla <diego.torresquintanilla@twosigma.com>

dposada · 2017-04-11T15:00:44Z

@pschorf Are you planning on taking a look at this?

pschorf · 2017-04-07T14:56:17Z

scheduler/src/cook/components.clj

@@ -19,6 +19,7 @@
            [clj-time.core :as t]
            [clojure.java.io :as io]
            [clojure.pprint :refer (pprint)]
+            [clojure.core.cache :as cache]


pschorf · 2017-04-07T17:31:52Z

scheduler/src/cook/mesos/constraints.clj

+                           [this target-vm-resources target-vm-attrs target-vm-tasks-assigned]
+    "Evaluates whether a vm with resources 'target-vm-resources', attributes 'target-vm-attrs'
+     and TaskRequests (optional) 'target-vm-tasks-assigned' passes the job's constraint. Must return a
+     2-element vector, where the first element is a boolean (whether the constraint was passsed) and


What about just returning the message if it failed, an nil otherwise?

I think returning passes? explicitly is convenient, as it encourages constraint code to name a variable called passes?. Also, this is the format that Fenzo expects, so eventually both reason string and boolean will be in memory.

pschorf · 2017-04-07T17:40:24Z

scheduler/src/cook/mesos/constraints.clj

+    (concat running-cotasks assigned-cotasks)))
+
+(defn get-fenzo-cotasks
+  "Given a group, a map of guuids to task-ids, a task-id and a Fenzo task-tracker-state, returns a


guuids->guids

guuid = Group UUID (I use it in many other places)

pschorf · 2017-04-07T17:54:26Z

scheduler/src/cook/mesos/constraints.clj

+  (group-constraint-name [self] (get-class-name self))
+  (group-constraint-type [self] (get-group-constraint-type (:group self)))
+  (group-constraint-evaluate [this target-attr-map cohost-attr-maps]
+    (let [group (:group this)


is this line necessary?

Well group is reused in the next 2 lines and the last let definition.

DiegoAlbertoTorres force-pushed the host-placement branch 16 times, most recently from 1b06a13 to c1d49bf Compare April 5, 2017 23:33

DiegoAlbertoTorres changed the title ~~WIP: Implemented host-placement group constraints~~ Implemented host-placement group constraints Apr 6, 2017

dposada reviewed Apr 6, 2017

View reviewed changes

dposada requested a review from pschorf April 6, 2017 22:22

DiegoAlbertoTorres force-pushed the host-placement branch 5 times, most recently from 079ce5d to 4b47f93 Compare April 10, 2017 20:40

DiegoAlbertoTorres force-pushed the host-placement branch from 4b47f93 to 4fec554 Compare April 11, 2017 14:03

pschorf reviewed Apr 11, 2017

View reviewed changes

wyegelwel merged commit 083b255 into master Apr 12, 2017

shamsimam mentioned this pull request Apr 2, 2018

Add support for host constraints #42

Closed

scrosby deleted the host-placement branch November 20, 2018 18:16

		(:require [clojure.core.async :as async]
		[clojure.core.cache :as cache]

Implemented host-placement group constraints #284

Implemented host-placement group constraints #284

Conversation

DiegoAlbertoTorres commented Apr 4, 2017

dposada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAlbertoTorres Apr 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dposada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dposada commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAlbertoTorres Apr 10, 2017 •

edited