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

Removes utilization check from rebalancer #887

Merged
merged 3 commits into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@dposada
Copy link
Member

dposada commented Jun 14, 2018

Changes proposed in this PR

  • removing the min-utilization-threshold config option
  • removing the mesos-master-hosts config option
  • removing cook.mesos/get-mesos-utilization
  • in the rebalancer, removing the utilization check (i.e. run the rebalancer regardless of cluster utilization)

Why are we making these changes?

The utilization check was useful before the rebalancer took "spare" resources into account when determining if tasks need to be preempted. Now that these "spare" resources are accounted for (via incubating offers), it doesn't make much sense to only rebalance when cluster utilization is below a certain threshold.

@dposada dposada added the wip label Jun 14, 2018

@dposada dposada self-assigned this Jun 14, 2018

@dposada dposada requested a review from wyegelwel Jun 14, 2018

@dposada dposada removed the wip label Jun 15, 2018

_wait_for_cook(cook_url)
mesos_master_hosts = settings(cook_url).get('mesos-master-hosts', ['localhost'])
resp = session.get('http://%s:%s/redirect' % (mesos_master_hosts[0], mesos_port), allow_redirects=False)
resp = session.get(f'http://localhost:{mesos_port}/redirect', allow_redirects=False)

This comment has been minimized.

@wyegelwel

wyegelwel Jun 15, 2018

Contributor

Why is it ok to assume localhost here? I'm imagining a case where you run the integration tests on a production cluster and then the tests that check the number of hosts. What am I missing?

This comment has been minimized.

@dposada

dposada Jun 15, 2018

Member

You are correct, this is only a decent assumption when running locally. For other environments, we will have to set the COOK_MESOS_LEADER_URL environment variable (see 3 lines above).

This comment has been minimized.

@wyegelwel

wyegelwel Jun 15, 2018

Contributor

ah ha! That is what I was missing. Thanks, I knew it was something stupid on my part.

@@ -111,10 +111,6 @@ We'll look at the configurable options in turn:
This option sets the Mesos master connection string.
For example, if you are running Mesos with a Zookeeper node on the local machine (a common development setup), you'd use the connection string `zk://localhost:2181/mesos`.

`:master-hosts`::

This comment has been minimized.

@wyegelwel

wyegelwel Jun 15, 2018

Contributor

Do you think it is worthwhile to have a deprecated or removed section in our config doc so users who upgrade aren't confused why the setting they used to use is no longer documented. Perhaps as an alternative, having the init in the scheduler print that the config is no longer used?

I don't think it is a big deal so I'm kind of on the fence about it.

This comment has been minimized.

@dposada

dposada Jun 15, 2018

Member

I like the idea of printing that it's no longer used. I will add that.

This comment has been minimized.

@dposada

@dposada dposada requested a review from shamsimam Jun 18, 2018

@@ -267,17 +267,10 @@
(when scheduler
(or (:good-enough-fitness scheduler) 0.8)))
:mesos-master (fnk [[:config {mesos nil}]]
(when (:master-hosts mesos)
(log/warn "The :master-hosts configuration field is no longer used"))

This comment has been minimized.

@shamsimam

shamsimam Jun 18, 2018

Contributor

👍

@shamsimam shamsimam merged commit 3278981 into twosigma:master Jun 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dposada dposada deleted the dposada:kill-utilization-check branch Jun 18, 2018

dposada added a commit to dposada/Cook that referenced this pull request Nov 1, 2018

Removes utilization check from rebalancer (twosigma#887)
* Removes utilization check from rebalancer

* Removes more occurrences of the config fields

* Logs warnings when the removed config fields are used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment