Datacenter balance constraint #2168

Crypto89 · 2017-01-09T14:16:26Z

This patch implements a dynamic constraint to force balancing of a job/task across multiple datacenters.

For some tasks, such as applications requiring quorum, it is desired (required) to spread them equally across all datacenters to prevent a loss of quorum when multiple allocations are running on the same DC.

Implements #517

dadgar

Hey,

Thanks for the great PR. I have left specific feedback in the code but the larger comment is that the way it is implemented right now will not work as expected.

Currently the iterator is acting a bit like roundrobin between the various DCs by restricting the count difference from being more than 1. However that breaks down if the input nodes aren't ordered round robin :)

We need the iterator to return true as long as the count does not exceed the desired balance. As in if we want to spread 9 instances on 3 DCs, it knows each DC should have 3 instances so as long as there aren't more than 3 in the DC it should return true.

dadgar · 2017-01-30T20:52:55Z

nomad/structs/structs.go

@@ -2864,6 +2864,7 @@ func (ta *TaskArtifact) Validate() error {
 }

 const (
+	ConstraintBalance       = "balance_datacenter"


Can we change it to balance_datacenters as it is balancing among multiple DCs

dadgar · 2017-01-30T20:54:53Z

nomad/structs/structs.go

@@ -2864,6 +2864,7 @@ func (ta *TaskArtifact) Validate() error {
 }

 const (
+	ConstraintBalance       = "balance_datacenter"


ConstraintBalance -> ConstraintBalanceDataCenters

dadgar · 2017-01-30T20:57:03Z

website/source/docs/job-specification/constraint.html.md

@@ -114,6 +114,19 @@ constraint {
    }
    ```

+- `"balance_datacenter"` - Instructs the scheduler to force an equal spread across
+  all datacenters specified in the Job. When specified as a job constraint, it


in the job's [datacenter list](https://www.nomadproject.io/docs/job-specification/job.html#datacenters)

dadgar · 2017-01-30T20:58:32Z

scheduler/feasible.go

@@ -157,6 +159,9 @@ type ProposedAllocConstraintIterator struct {
 	// they don't have to be calculated every time Next() is called.
 	tgDistinctHosts  bool
 	jobDistinctHosts bool
+
+	tgBalance  bool


Can you put a comment above these

dadgar · 2017-01-30T20:59:26Z

scheduler/feasible.go

@@ -157,6 +159,9 @@ type ProposedAllocConstraintIterator struct {
 	// they don't have to be calculated every time Next() is called.
 	tgDistinctHosts  bool
 	jobDistinctHosts bool
+
+	tgBalance  bool
+	jobBalance bool


tgBalance -> tgBalanceDCs
jobBalance -> jobBalanceDCs

dadgar · 2017-01-30T22:02:42Z

scheduler/feasible.go

+			jobCollision := alloc.JobID == iter.job.ID
+			taskCollision := alloc.TaskGroup == iter.tg.Name
+
+			// skip jobs not in this job or taskgroup (for jobBalance/tgBalance)


// Skip allocations on the node that are not for this job or for the task group when using the balance constraint on the group

dadgar · 2017-01-30T22:06:06Z

scheduler/feasible.go

+			taskCollision := alloc.TaskGroup == iter.tg.Name
+
+			// skip jobs not in this job or taskgroup (for jobBalance/tgBalance)
+			if !(jobCollision && (iter.jobBalance || taskCollision)) {


// It has to also collide on the job because there is nothing forcing the group name to be unique across different // jobs that may be on the node. groupCollision = alloc.TaskGroup == iter.tg.Name && job.Collision if !jobCollision && !groupCollision || !iter.tgBalance { continue }

dadgar · 2017-01-30T23:48:21Z

scheduler/feasible_test.go

+		Datacenters: []string{"dc1", "dc2"},
+		TaskGroups:  []*structs.TaskGroup{tg1, tg2},
+	}
+


I think what you want to test is adding an allocation to the proposed and seeing that it returns nothing

dadgar · 2017-01-30T23:59:56Z

scheduler/feasible.go

+		}
+	}
+
+	min := math.MaxInt32


I think this logic should be detect the desired count of the task group and then divide it by the number of DCs.
For any DC that has less than that max you would return true and for any over you return false.

There is an edge case to consider that if the count is 10 and len(dcs) == 3, the code would have to detect that only one DC should be allowed to get to count 4.

There is a problem with the current implementation because imagine we have count = 4 and we have 2 nodes in DC1, and one in DC2 and DC3. You would expect this to work but if the input was [1, 1, 2, 3]. It would return false on the second node in DC1.

dadgar · 2017-01-31T00:00:25Z

scheduler/feasible_test.go

+	if len(out) != 2 {
+		t.Fatalf("Bad: %#v", out)
+	}
+}


Can you add a test to stack_test.go.

That is where you can really test the behavior is working correctly.

nanoz · 2017-02-11T12:08:19Z

Interesting feature @Crypto89 !

* Add Balance constraint * Fix constraint checker * fix node iteration * Add proposed allocs in the balancing * Add balancing on tg and job level * Add some more tests

* to rely on currently allocated allocations rather then on the RR nature of the iterator. The allocations per datacenter is now calculated deterministically based on the number of allocations and on the number of datacenters.

Crypto89 · 2017-02-24T10:58:16Z

So I've changed a couple of things here:

The constraint is now called balance_datacenters and I've changed all variables/constants (and docs) to reflex this name so it should match
The allocations are now deterministically determined abased on the number of allocations (tg counts) and the number of datacenters in the jobspec. this removes the need to calculate the entire state over and over again (see Datacenter balance constraint #2168 (comment)). any overflow allocations are assigned in order to the first x datacenters.
Changed the tests to reflect this new behaviour

This last change does mean that is one of the datacenters dies with more allocations then the others, the overflow of allocations will not be rescheduled to the other datacenters. Since I personally think this is an edge case I doubt this will cause any problems in real-world usecases, however it is worth mentioning.

Last but not least, the testcases do test something, but probably not everything. since it requires a little bit more knowledge of the scheduler I would appreciate someone to give me some points/append this PR.

preetapan · 2019-02-23T17:09:53Z

Closing this because the work in 0.9 with the spread stanza achieves this.

github-actions · 2023-02-14T02:18:45Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dadgar requested changes Jan 31, 2017

View reviewed changes

dadgar mentioned this pull request Feb 14, 2017

[feature] Scheduling type: datacenter (& region) #2307

Closed

Jorn Wijnands and others added 4 commits February 23, 2017 14:01

Add balance_datacenter constraint

d2442d4

* Add Balance constraint * Fix constraint checker * fix node iteration * Add proposed allocs in the balancing * Add balancing on tg and job level * Add some more tests

Add documentation

8e50242

Fix duplicate allocations

8f10b43

Change the balance_datacenters constraint

ede929f

* to rely on currently allocated allocations rather then on the RR nature of the iterator. The allocations per datacenter is now calculated deterministically based on the number of allocations and on the number of datacenters.

Crypto89 force-pushed the f-balance-constraint-1 branch from a4151ad to ede929f Compare February 24, 2017 10:38

ygersie mentioned this pull request Aug 26, 2017

Distinct Property supports arbitrary limit #2942

Merged

ygersie mentioned this pull request Nov 19, 2017

Support for uniform allocation placements across DCs #3507

Closed

preetapan closed this Feb 23, 2019

github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datacenter balance constraint #2168

Datacenter balance constraint #2168

Crypto89 commented Jan 9, 2017

dadgar left a comment

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 30, 2017

dadgar Jan 31, 2017

nanoz commented Feb 11, 2017

Crypto89 commented Feb 24, 2017

preetapan commented Feb 23, 2019

github-actions bot commented Feb 14, 2023

Datacenter balance constraint #2168

Datacenter balance constraint #2168

Conversation

Crypto89 commented Jan 9, 2017

dadgar 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

nanoz commented Feb 11, 2017

Crypto89 commented Feb 24, 2017

preetapan commented Feb 23, 2019

github-actions bot commented Feb 14, 2023