[SPARK-13232][YARN] Fix executor node label

[AtkinsChang]

Specify node label for executor will cause Spark not working on Yarn due to Yarn does not support node container request with both label and locality(racks/nodes).

Add test to YarnAllocatorSuite to reproduce this situation and change YarnAllocator#createContainerRequest to apply node label only to locality free request.

[tgravescs]

Jenkins, test this please

[SparkQA]

Test build #51369 has finished for PR 11129 at commit 28f1491.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[jerryshao]

From my understanding, currently in your implementation if nodes or racks is not empty, label expression will not be worked even it is explicitly set through configuration.

IMO I would choose to set nodes and racks to null if label expression is configured, otherwise user will be confused why explicitly setting lab expression is not worked.

[AtkinsChang]

Yeah, I had this idea before, but imo if totally disregard the data locality may cause a lot of network overhead.

[jerryshao]

Normally if user set this label expression configuration, they want the label-based scheduling obviously. But in your implementation you silently disable this, this will make user confuse. Also since label-based scheduling cannot be worked with locality preferences in YARN side (I think it is intentionally), it would be better to ignore the locality information here.

[jerryshao]

Any further updates on it? CC @sryza about this.

[srowen]

CC @sryza or maybe @steveloughran or @vanzin . I don't speak YARN

[steveloughran]

Yarn and labels, joy.

• Currently, a node can have exactly one label. That may change at a time in the future, a time called, provisionally "the patch that broke all the code trying to be clever" :)
• you can ask for work =anywhere, in which case you also need to set relaxLocality=true. Otherwise some validation rejects the request
• you can do explicit nodes/racks requests, with relax=true or relax=false. Relax = true means relax placement after a couple of heartbeats, 10-15s. I've seen bugreps implying the fair scheduler relaxes more aggressively; not looked @ it.
• If you want labels, you don't get to ask for locations
• if you want to mix labels with locality, you need to be clever and grab the nodemap, parse the labels, make your own decisions. That's not easy given there's nothing in the AMRM API to query that nodemap; you need to do it on the client or in the AM by passing in the relevant client/AM delegation token, do it as a one-off then subscribe to events. Then you need to scan the labelled nodes, find those that match and use it as your placement subset.

Yes, I have done this. No, I would not recommend it. I only did it for anti-affinity placement, where we needed a guarantee that there'd be only one instance per node, labelled or not. (i.e we wanted containers to be away from each other, rather than near the data).

The request validation logic. I think; it failed for a while (SLIDER-1051) until I turned off some of the checks.

There's one more corner case: app doesn't ask for labels, but the queue is bonded to a label. I actually don't know what happens to located requests here.

[srowen]

@steveloughran do you think this is a good or not-good change overall?

[steveloughran]

It's actually being fixed right now in Hadoop 2.8, which will take a while to surface.

YARN-4925.

Looking at the patch, all Bibin is doing is cutting that validation check out from the container request submission...it's up to the scheduler how it handles things at that point. Presumably (hopefully) it does the right thing.

If the patch goes in to spark, then eventually, there may be benefits in pulling it. If it goes in now, it stops things breaking today.

This is a tough call...nobody wants to add another switch for this do they?

(now, maximally devious would be to catch the exception and downgrade, but that's both hard to test and inevitably brittle in some way)

[jerryshao]

now, maximally devious would be to catch the exception and downgrade

Maybe we could do this in Spark side, though a little complicated but doable.

Yes it is hard to test label related things in Spark side, at least we could manually verify it locally.

[vanzin]

It's been a while since discussion died down here; I don't like the current version because it will break things with a fixed YARN. So the options are either implement Steve's suggestion, or not do anything and tell users to not use labels in broken version of YARN. I'm kinda leaning towards the latter but don't really feel strongly.

[HyukjinKwon]

Hi @AtkinsChang is it still active?

[movwei]

But In Hadoop 2.8.2 there still have this problem when request a container with both node label and locality(rack/node). ResourceManager will also checked it, and the related code is as follows:

RMAppManager#validateAndCreateResourceRequest -> SchedulerUtils#validateResourceRequest

org.apache.hadoop.yarn.server.resourcemanager.scheduler.SchedulerUtils.javaprivatestaticvoidvalidateResourceRequest(ResourceRequestresReq,
ResourcemaximumResource, QueueInfoqueueInfo, RMContextrmContext)
throwsInvalidResourceRequestException {
.......
StringlabelExp = resReq.getNodeLabelExpression();
// we don't allow specify label expression other than resourceName=ANY nowif (!ResourceRequest.ANY.equals(resReq.getResourceName())
&& labelExp != null && !labelExp.trim().isEmpty()) {
thrownewInvalidLabelResourceRequestException(
"Invalid resource request, queue=" + queueInfo.getQueueName()
+ " specified node label expression in a "
+ "resource request has resource name = "
+ resReq.getResourceName());
}
// we don't allow specify label expression with more than one node labels nowif (labelExp != null && labelExp.contains("&&")) {
thrownewInvalidLabelResourceRequestException(
"Invailid resource request, queue=" + queueInfo.getQueueName()
+ " specified more than one node label "
+ "in a node label expression, node label expression = "
+ labelExp);
}
......
}
}

It will cause spark ApplicationMaster failed with InvalidLabelResourceRequestException, so we still need this patch.