Validate resource constraint (RAM and CPU) in RoundRobinPacking #3142

xiaoyao1991 · 2019-01-02T23:46:40Z

This PR might break some mis-configured topologies in production

This PR addresses some issues raised previously in the slack chat that the packing algorithm did not honor container-level and instance-level constraints on CPU resource. Additionally, we added resource constraint validation in RoundRobinPacking. Specifically:

topology.container.cpu and topology.container.ram are honored and considered as the hard cap constraint for the corresponding resource in one container.
topology.component.cpumap and topology.component.rammap are honored and considered as the instance resource mapping in one container.
The sum of instance resources in one container, plus padding, should not exceed the container-level constraint, otherwise the scheduling would fail.

heron/common/src/java/org/apache/heron/common/basics/CPUShare.java

heron/packing/src/java/org/apache/heron/packing/roundrobin/RoundRobinPacking.java

nlu90 · 2019-01-03T18:33:02Z

heron/common/src/java/org/apache/heron/common/basics/ResourceMeasure.java

+
+  T increaseBy(int percentage);
+
+  boolean greaterThan(T other);


The Comparable interface already provides the compareTo method. Do we really need these 4 comparison methods?

Good question. The ByteAmount had these methods and I preserved these to guarantee minimum changes. I guess they were there just for readability?

nlu90 · 2019-01-03T18:33:27Z

heron/common/src/java/org/apache/heron/common/basics/ResourceMeasure.java

+
+  T plus(T other);
+
+  T multiply(int factor);


why the factor and percentage are int?

The semantics for factor is percentage in ByteAmount and don't allow decimal spaces. The original method signatures in ByteAmount use int too.

heron/packing/src/java/org/apache/heron/packing/roundrobin/RoundRobinPacking.java

nlu90 · 2019-01-03T18:47:23Z

heron/common/src/java/org/apache/heron/common/basics/CPUShare.java

+import java.util.HashMap;
+import java.util.Map;
+
+public final class CPUShare implements ResourceMeasure<CPUShare> {


Do we really need to create a separate CPUShare class to handle the CPU resource calculation? Is that possible to reuse what we already have in the ram resource calculation? Or will there be RamShare and DiskShare classes in following patches?

It doesn't seem necessary to separate RamShare and DiskShare as RAM and disk are both measured in ByteAmount. CPU is measured in share of time used.

Previously, we didn't take CPU resource constraints into consideration when composing packing plan. Now that we do -- namely we need to collect instance cpu resource mapping, and the logic of that is very similar to that of instance RAM resource mapping (previously as getInstancesRAMMapInContainer()). But due to the difference in the type of measurement(ByteAmount vs double), we might end up with a lot of code duplicates. Hence, I abstract out CPUShare so that it looks very similar to what we have in ByteAmount, and thus we can have one calculateInstancesResourceMapInContainer() that works for both CPU and RAM (potentially disk if needed) instance resource mapping collection.

heron/common/src/java/org/apache/heron/common/basics/ByteAmount.java

heron/common/src/java/org/apache/heron/common/basics/CPUShare.java

heron/packing/src/java/org/apache/heron/packing/roundrobin/RoundRobinPacking.java

heron/packing/tests/java/org/apache/heron/packing/roundrobin/RoundRobinPackingTest.java

nwangtw · 2019-01-04T18:46:38Z

heron/packing/tests/java/org/apache/heron/packing/roundrobin/RoundRobinPackingTest.java

+   * Test the scenario CPU map config is completely set
+   */
+  @Test
+  public void testCompleteCpuMapRequested() throws Exception {


Can we add these cases:

container cpu is more than needed by instances.

container cpu is less than needed by instances.

also, other processes in container could need a fixed amount of CPU as well. So if container cpu == needed by instances, it might be better to return false.

👍 Added more tests to cover the mentioned scenarios

nwangtw · 2019-01-04T22:43:09Z

heron/packing/src/java/org/apache/heron/packing/roundrobin/RoundRobinPacking.java


    ByteAmount containerDiskInBytes = getContainerDiskHint(roundRobinAllocation);
-    double containerCpu = getContainerCpuHint(roundRobinAllocation);
+    double containerCpuHint = getContainerCpuHint(roundRobinAllocation);


containerCpuHint (configured container cpu, or max (padding + 1 per instance)) is not used. As the result, I think user specify container cpu is ignored. Maybe need a max(containerCpuHint, containerCpu) I feel?

It is actually used down in calculateInstancesResourceMapInContainer as the containerResHint. It is used to validate the all instances + padding in roundRobinAllocation does not exceed the containerResHint. It's just not used anymore in packingInternal, similar to how we treated containerRamHint before the change

That is true. Assuming user configures:

container cpu to be 5

1 cpu per instance

3 instances in container

Then:
cpu hint is 5,
instance cpu is valid (enough for padding and instances)

In the old calculation, container cpu is 5 because user configured it.
In the new calculation, container cpu is 4 (1 * 3 + 1 padding).

With "max(containerCpuHint, containerCpu)", container cpu is the same as before.

I think it is important to make the result consistent. Some users might rely on setting container cpu to allocate more processing power to their instances instead of configuring per instance cpu (although it is technically correct).

hmm. I see. Then I think we don't even need to take the max out of 2. ContainerCpuHint is always gonna be greater no matter what.

But if container cpu is not configured, the getContainerCpuHint() would return padding + 1 * instance number, which is smaller than your new value, unless you update the getContainerCpuHint() function.

Yeh you are right, I just figured that situation :) Updated

The original logic is messy and makes it hard to update. :(

nlu90

LGTM

examples/src/java/org/apache/heron/examples/api/ExampleResources.java

…he#3142) * init * general resource constraint validation * pass existing unit tests * add more tests * rename * rename * generic ResourceMeasure * fixed wc example * even more general generics * address comments * address comments by putting more tests * set safe amount of cpu * meaningful constants in ExampleResource

* init * general resource constraint validation * pass existing unit tests * add more tests * rename * rename * generic ResourceMeasure * fixed wc example * even more general generics * address comments * address comments by putting more tests * set safe amount of cpu * meaningful constants in ExampleResource

nwangtw reviewed Jan 3, 2019

View reviewed changes

nlu90 reviewed Jan 3, 2019

View reviewed changes

xiaoyao1991 added 7 commits January 3, 2019 14:13

init

6a4e0e6

general resource constraint validation

025194a

pass existing unit tests

fb34cba

add more tests

2ff1d1b

rename

af71317

rename

d1edc3b

generic ResourceMeasure

5bd71c0

xiaoyao1991 force-pushed the validate_resource_constraint_in_RRPacking branch from dc78dbb to 5bd71c0 Compare January 3, 2019 22:13

xiaoyao1991 added 2 commits January 3, 2019 14:44

fixed wc example

0f6e7dd

even more general generics

b5e23fb

nwangtw reviewed Jan 4, 2019

View reviewed changes

xiaoyao1991 added 2 commits January 4, 2019 11:16

address comments

3c9fe27

address comments by putting more tests

15feedf

nwangtw reviewed Jan 4, 2019

View reviewed changes

set safe amount of cpu

3fa2693

nwangtw approved these changes Jan 5, 2019

View reviewed changes

nlu90 approved these changes Jan 7, 2019

View reviewed changes

examples/src/java/org/apache/heron/examples/api/ExampleResources.java Outdated Show resolved Hide resolved

meaningful constants in ExampleResource

4b20552

huijunwu merged commit a1b761f into apache:master Jan 10, 2019

xiaoyao1991 deleted the validate_resource_constraint_in_RRPacking branch February 15, 2019 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate resource constraint (RAM and CPU) in RoundRobinPacking #3142

Validate resource constraint (RAM and CPU) in RoundRobinPacking #3142

xiaoyao1991 commented Jan 2, 2019 •

edited

nlu90 Jan 3, 2019

xiaoyao1991 Jan 3, 2019

nlu90 Jan 3, 2019

xiaoyao1991 Jan 3, 2019

nlu90 Jan 3, 2019

xiaoyao1991 Jan 3, 2019

nwangtw Jan 4, 2019

xiaoyao1991 Jan 4, 2019

nwangtw Jan 4, 2019

xiaoyao1991 Jan 4, 2019

nwangtw Jan 4, 2019

xiaoyao1991 Jan 4, 2019

nwangtw Jan 5, 2019

xiaoyao1991 Jan 5, 2019

nwangtw Jan 5, 2019

nlu90 left a comment

Validate resource constraint (RAM and CPU) in RoundRobinPacking #3142

Validate resource constraint (RAM and CPU) in RoundRobinPacking #3142

Conversation

xiaoyao1991 commented Jan 2, 2019 • 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

nlu90 left a comment

Choose a reason for hiding this comment

xiaoyao1991 commented Jan 2, 2019 •

edited