You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by kkhatua <gi...@git.apache.org> on 2018/04/23 23:50:19 UTC
[GitHub] drill pull request #1239: CGroup Support for Drill-on-YARN
GitHub user kkhatua opened a pull request:
https://github.com/apache/drill/pull/1239
CGroup Support for Drill-on-YARN
Original commit works for stand-alone Drill. During testing with Drill-on-YARN, it was discovered that while the environment is sourced, the DrillApplicationManager uses a different resource for spinning up the Drillbits. This commit applies the CGroups application logic for YARN specifically.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kkhatua/drill DRILL-143_yarn
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1239.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1239
----
commit d23b5d0674eb59743e6ff08310834f6baa8f21cc
Author: Kunal Khatua <ku...@...>
Date: 2018-04-23T17:31:21Z
CGroup Support for Drill-on-YARN
Initial patch
----
---
[GitHub] drill pull request #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by Ben-Zvi <gi...@git.apache.org>.
Github user Ben-Zvi commented on a diff in the pull request:
https://github.com/apache/drill/pull/1239#discussion_r183607110
--- Diff: distribution/src/resources/yarn-drillbit.sh ---
@@ -110,6 +114,36 @@
# Enables Java GC logging. Passed from the drill.yarn.drillbit.log-gc
# garbage collection option.
+### Function to enforce CGroup (Refer local drillbit.sh)
+check_and_enforce_cgroup(){
+ dbitPid=$1;
+ kill -0 $dbitPid
+ if [ $? -gt 0 ]; then
+ echo "ERROR: Failed to add Drillbit to CGroup ( $DRILLBIT_CGROUP ) for 'cpu'. Ensure that the Drillbit ( pid=$dbitPid ) started up." >&2
+ exit 1
+ fi
+ SYS_CGROUP_DIR=${SYS_CGROUP_DIR:-"/sys/fs/cgroup"}
+ if [ -f $SYS_CGROUP_DIR/cpu/$DRILLBIT_CGROUP/cgroup.procs ]; then
+ echo $dbitPid > $SYS_CGROUP_DIR/cpu/$DRILLBIT_CGROUP/cgroup.procs
+ # Verify Enforcement
+ cgroupStatus=`grep -w $pid $SYS_CGROUP_DIR/cpu/${DRILLBIT_CGROUP}/cgroup.procs`
+ if [ -z "$cgroupStatus" ]; then
--- End diff --
I'm confused: Is this checking for $dbitPid (in cgroup.procs) or for $pid ?
In case the former, then need to negate the following "-z" condition.
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1239
There may be some misunderstanding of how DoY works. The only info that users can pass to DoY is that which is in the DoY config file. We should add arguments to that file which will be passed through the DoY client to the DoY AM, and from there, as an env var, to the Drillbit containers. We already do this for memory so that both Drill and YARN agree on memory. We should do the same for CPU so that Drill, YARN and cgroups agree on the number of CPUs that Drill can use.
Since this feature is MapR-only, it might be OK to require that users alter their `drill-env.sh` to set an environment variable the forces Drill to police itself under YARN.
---
[GitHub] drill issue #1239: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1239
@Ben-Zvi please review. QA verified that Drill-on-YARN works with this patch.
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1239
@paul-rogers DoY is no more a MapR-only feature, and if it helps to have Drill self-enforce, this works. If YARN is able to enforce for Drill, the user need not specify the settings in their `drill-env.sh`.
I see https://issues.apache.org/jira/browse/YARN-810 as being an open issue. (Source: [Issue List](https://issues.apache.org/jira/issues/?jql=project%20%3D%20YARN%20AND%20status%20in%20(Open%2C%20"Patch%20Available")%20AND%20text%20~%20"cgroup"%20ORDER%20BY%20key%20%2C%20status%20ASC) ). So, I probably don't need to do any explicit checks for hadoop distros and documentation should be sufficient to address this, IMO.
---
[GitHub] drill pull request #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1239#discussion_r183732780
--- Diff: distribution/src/resources/yarn-drillbit.sh ---
@@ -110,6 +114,36 @@
# Enables Java GC logging. Passed from the drill.yarn.drillbit.log-gc
# garbage collection option.
+### Function to enforce CGroup (Refer local drillbit.sh)
+check_and_enforce_cgroup(){
+ dbitPid=$1;
+ kill -0 $dbitPid
+ if [ $? -gt 0 ]; then
+ echo "ERROR: Failed to add Drillbit to CGroup ( $DRILLBIT_CGROUP ) for 'cpu'. Ensure that the Drillbit ( pid=$dbitPid ) started up." >&2
+ exit 1
+ fi
+ SYS_CGROUP_DIR=${SYS_CGROUP_DIR:-"/sys/fs/cgroup"}
+ if [ -f $SYS_CGROUP_DIR/cpu/$DRILLBIT_CGROUP/cgroup.procs ]; then
+ echo $dbitPid > $SYS_CGROUP_DIR/cpu/$DRILLBIT_CGROUP/cgroup.procs
+ # Verify Enforcement
+ cgroupStatus=`grep -w $pid $SYS_CGROUP_DIR/cpu/${DRILLBIT_CGROUP}/cgroup.procs`
+ if [ -z "$cgroupStatus" ]; then
--- End diff --
You're right. I seem to have missed negating the check. Since this check only affects publication of a message and not the actual application of the CGroup, we didn't catch it during testing. I'll fix this port and the original patch as well.
---
[GitHub] drill pull request #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1239#discussion_r183805056
--- Diff: distribution/src/resources/yarn-drillbit.sh ---
@@ -110,6 +114,36 @@
# Enables Java GC logging. Passed from the drill.yarn.drillbit.log-gc
# garbage collection option.
+### Function to enforce CGroup (Refer local drillbit.sh)
+check_and_enforce_cgroup(){
+ dbitPid=$1;
+ kill -0 $dbitPid
+ if [ $? -gt 0 ]; then
+ echo "ERROR: Failed to add Drillbit to CGroup ( $DRILLBIT_CGROUP ) for 'cpu'. Ensure that the Drillbit ( pid=$dbitPid ) started up." >&2
+ exit 1
+ fi
+ SYS_CGROUP_DIR=${SYS_CGROUP_DIR:-"/sys/fs/cgroup"}
+ if [ -f $SYS_CGROUP_DIR/cpu/$DRILLBIT_CGROUP/cgroup.procs ]; then
+ echo $dbitPid > $SYS_CGROUP_DIR/cpu/$DRILLBIT_CGROUP/cgroup.procs
+ # Verify Enforcement
+ cgroupStatus=`grep -w $pid $SYS_CGROUP_DIR/cpu/${DRILLBIT_CGROUP}/cgroup.procs`
+ if [ -z "$cgroupStatus" ]; then
--- End diff --
Fixed the changes.
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1239
Yes, I agree. Support folks gave me similar feedback, so I'll commit this change in the mapr distro _IFF_ there is a request for that. YARN is already a complex beast with numerous settings. Introducing caveats like this will only add to the confusion.
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1239
Just to be clear, I have no objection Drill enforcing its own cgroup limits.
My point is rather that CPU limits must be integrated with YARN, via the DoY config file, so that the user specifies CPU limits in one place and that limit is the same one passed to YARN for container allocation and to whichever code is doing cgroup enforcement. By default, it will be YARN if cgroups are enabled in YARN, as they can be for Apache YARN.
It is then fine to have an additional option to enable self-enforcement in Drill for those odd cases where users cannot or choose not to use versions of YARN that do the work.
The problem would be if the user has to configure CPU in two places: in a shell script to enable self-enforcement, and in the DoY config to tell YARN the container size. This goes against the ease-of-use experience DoY was designed to provide. (That is, getting multiple to work consistently correct is very hard if you're just trying to get Drill to run and are not a Drill internals expert.)
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1239
@kkhatua, it turns out that upstream YARN has long had effective cgroup support per container. ( have the pleasure of sitting near the guy who maintains that work.)There has long been a discussion about whether the MapR version of YARN picked up those changes, we believe that MapR does *not* support this upstream work.
As a result, under Apache YARN, the YARN NM itself will impose cgroup controls and Drill need not do it itself. For MapR YARN (only) Drill (and all other YARN apps) must do their own cgroup control.
Please make sure that this feature is off by default to allow YARN to do the work. Only enable it for versions of YARN (such as MapR) which do not provide cgroup control in YARN itself.
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1239
One other thing to highlight from an earlier comment. CPU is something that the user specifies in the DoY config file. That information is passed to YARN in container requests. This feature asks the user to modify the drill-env.sh file to enable cgroups to limit CPU. As a result, the user must specify the CPU limit in two places.
DoY went to extreme lengths to unify memory configuration so it is set in one place. The assumption was that, since Apache YARN handles cgroups, we've also got unified CPU specification. But, in this "side-car" approach we don't. So, would be good to capture the CPU amount from the YARN config as explained earlier and use that to set the Drill cgroups env vars -- but only if some "self-enforcing cgroup" flag is enabled in the config (which can be done by default for the limited MapR YARN.)
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1239
@kkhatua, putting on my Apache hat... Apache Drill is an Apache project that must work with other Apache projects such as Apache YARN. The Apache Drill DoY support is designed to work well with Apache YARN (and has a few special additions for MapR YARN's unique limitations.) It is important that the Apache DoY work well with the generic YARN. No harm in adding tweaks (such as this one) to work with vendor-specific limitations. But, the overriding concern is that the DoY feature be useful in Apache.
---
[GitHub] drill pull request #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1239#discussion_r184153040
--- Diff: distribution/src/resources/yarn-drillbit.sh ---
@@ -175,4 +209,11 @@ fi
echo "`date` Starting drillbit on `hostname` under YARN, logging to $DRILLBIT_LOG_PATH"
echo "`ulimit -a`" >> "$DRILLBIT_LOG_PATH" 2>&1
-"$DRILL_HOME/bin/runbit" exec
+# Run in background
+"$DRILL_HOME/bin/runbit" exec &
--- End diff --
The process is momentarily in the background to capture the PID. We eventually wait for it. Are you saying that a process will not continue to run because it is in the background??
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1239
@paul-rogers I went through with support on this and found that the issue is not specific to MapR. However, you make a strong argument in favor of letting YARN handle the CGroup management rather than Drill over-reaching.
I've reverted the change to `yarn-drillbit.sh` in the latest commit.
---
[GitHub] drill pull request #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1239#discussion_r184169114
--- Diff: distribution/src/resources/yarn-drillbit.sh ---
@@ -175,4 +209,11 @@ fi
echo "`date` Starting drillbit on `hostname` under YARN, logging to $DRILLBIT_LOG_PATH"
echo "`ulimit -a`" >> "$DRILLBIT_LOG_PATH" 2>&1
-"$DRILL_HOME/bin/runbit" exec
+# Run in background
+"$DRILL_HOME/bin/runbit" exec &
--- End diff --
Under YARN, it is YARN that maintains the pid, not Drill. YARN expects its child processes to run in the foreground and will handle capturing the pid.
This is a case in which "native" Apache YARN works differently than "MapR YARN." Since Apache YARN handles cgroups, it is the one that needs the pid. Under MapR's limited YARN, then Drill is second-guessing YARN and needs the pid. It may be that MapR's YARN can handle a background process; I don't recall the details.
Is there way way to run Drill in the background, get the pid, then return to the foreground so that the script does not exit until Drill itself exits?
In fact, if I remember correctly, the scripts have two layers; in one layer, the script replaces itself with the Drill process. Something to check.
---
[GitHub] drill pull request #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1239#discussion_r184144439
--- Diff: distribution/src/resources/yarn-drillbit.sh ---
@@ -175,4 +209,11 @@ fi
echo "`date` Starting drillbit on `hostname` under YARN, logging to $DRILLBIT_LOG_PATH"
echo "`ulimit -a`" >> "$DRILLBIT_LOG_PATH" 2>&1
-"$DRILL_HOME/bin/runbit" exec
+# Run in background
+"$DRILL_HOME/bin/runbit" exec &
--- End diff --
This can't be for YARN. Under YARN, Drill must be run in the foreground. The original Drill-on-YARN work ensured that al this works.
---
[GitHub] drill issue #1239: DRILL-143: CGroup Support for Drill-on-YARN
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1239
Thanks for that pointer, @paul-rogers ! I'll make the relevant changes and add to this commit.
---