[
https://issues.apache.org/jira/browse/MAPREDUCE-896?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12786575#action_12786575
]
Hemanth Yamijala commented on MAPREDUCE-896:
--------------------------------------------
I looked at the Y! 20 patch. Some comments:
- TaskTracker.buildPathForDeletion need not be public.
- Was there a need to change CleanupQueue.addToQueue to take a FileSystem as
argument instead of Configuration ? It has caused more changes than required by
this patch - like in JobTracker and JobInProgress. Can we retain the original
API and pass in a Configuration as before ?
- When adding a task directory to delete, we are adding paths from all the
local directories instead of just the one where files for the task are actually
created. At a minimum, this is more work being done than necessary. More
importantly, I don't know if there are any side effects this will cause. We can
check which among the local directories the path belongs to (by doing a
startsWith on the path) and all only that I think.
- Shouldn't getLocalDirs take the tasktracker's configuration always ? In which
case, it doesn't need to take the JobConf as a parameter, but can use fConf.
- The log statements in CleanupQueue.PathCleanupThread.run are printing
context.path which will only be the mapred local dir. We actually need the full
path, otherwise the log statements could be confusing. Indeed, I would suggest
a slight refactoring of the PathDeletionContext class, because as it exists
currently we have one mode or the other that works - either a fullPath is
provided or the path is built from other bits of data - like jobId, taskId etc.
So, I would suggest something along the following lines:
{code}
class PathDeletionContext {
String fullPath;
FileSystem fs;
protected String getPathForDeletion() {
return fullPath;
}
protected void enablePathForCleanup() {
// do nothing
}
}
class TaskControllerPathDeletionContext extends PathDeletionContext {
String user;
String jobId;
String taskId;
boolean isCleanupTask;
boolean isWorkDir;
TaskController taskController;
Path p;
@Override
protected String getPathForDeletion() {
TaskTracker.buildPathForDeletion(this);
}
@Override
protected void enablePathForCleanup() {
taskController.enablePathForCleanup(this);
}
}
{code}
Then we can use PathDeletionContext in all cases where we don't need to use the
taskController and the sub-class in other cases. CleanupQueue will naturally
take and store PathDeletionContext objects. getPathForDeletion can be called to
get the final path for deletion. I feel this design is cleaner. Thoughts ?
- DefaultTaskController.enableTaskForCleanup should be package private.
- In other APIs of LinuxTaskController - like buildLaunchTaskArgs, we find out
if the task is a cleanup task and adjust paths directly. I think we can do the
same thing for the new command also. This is not less secure, because we are
still constructing the full path from the command args, but we abstract the
task-controller from details like cleanup task. It is less clear whether the
same thing should be done for workDir (i.e. should we append that to taskid in
LinuxTaskController itself.) For that we may need a flag still, but I am OK if
that is also resolved in LinuxTaskController itself and we completely eliminate
flags to pass to task-controller.
- The List of args in buildChangePathPermissionsArgs should be of the right
size. (It's not 5). Also, I think it is useful to retain the order of commands
the same. i.e. Let the mapred local dir be the first argument, then job-id,
then task-id.
- I think we must allocate the exact amount of memory required in
build_dir_path. This can be done by defining a format string like
TT_LOCAL_TASK_SCRIPT_PATTERN and then summing the lengths of this string, and
the arguments like jobid, taskid, mapred local dir etc. Then we can use
snprintf to build the path instead of multiple (unsafe) strcat and strcpy
calls. Again, please look at get_task_file_path for an example.
- Return values of calls like malloc should all be checked. When this is done,
calls to build_dir_path can fail, which must also be checked.
- In TaskRunner.deleteDirContents, I think if we get an InterruptedException,
we should return immediately. Because otherwise, the operation is not really
interrupted and it can get stuck permanently.
- The intent of the testcase in TestChildTaskDirs is nice. But I am worried
that since directory cleanup happens asynchronously, this might fail due to
timing issues (like the TODO in the comment says). One option could be to use
an inline directory cleaner. Can we try that ?
- Should we also verify that the taskattemptdir is also cleaned up ?
- There are some TODOs in the tests, can you please remove them after
addressing the concerns ?
> Users can set non-writable permissions on temporary files for TT and can
> abuse disk usage.
> ------------------------------------------------------------------------------------------
>
> Key: MAPREDUCE-896
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-896
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Components: tasktracker
> Affects Versions: 0.21.0
> Reporter: Vinod K V
> Assignee: Ravi Gummadi
> Fix For: 0.21.0
>
> Attachments: MR-896.patch, MR-896.v1.patch, y896.v1.patch
>
>
> As of now, irrespective of the TaskController in use, TT itself does a full
> delete on local files created by itself or job tasks. This step, depending
> upon TT's umask and the permissions set by files by the user, for e.g in
> job-work/task-work or child.tmp directories, may or may not go through
> successful completion fully. Thus is left an opportunity for abusing disk
> space usage either accidentally or intentionally by TT/users.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
