mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 58967: Set the working directory to parent task's for DEBUG containers.
Date Tue, 09 May 2017 23:17:15 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58967/#review174386
-----------------------------------------------------------




include/mesos/slave/containerizer.proto
Lines 194 (patched)
<https://reviews.apache.org/r/58967/#comment247518>

    s/in the root filesystem/(e.g., when task defines an image)



src/slave/containerizer/mesos/containerizer.cpp
Lines 1501-1505 (original), 1505-1536 (patched)
<https://reviews.apache.org/r/58967/#comment247530>

    Instead of overwriting the working directory in some special cases in a nested way, I
personally find it easier to follow the logic if it is done in a nested "if else loop". Also,
please add comments because this logic is not straightforward.
    
    ```
    if (debug container) {
       if (parent has launch info) {
          if (parent is a command task) {
             if (parent task has a rootfs) {
                // Set working directory to the parent task's work directory.
             } else {
               // Set working directory to the parent task's executor's work directory.
             }
          
          } else { // Parent is a non-command task
             // Set working directory to the parent task's work directory.
          }
       } else { // Paren't work directory is unknown.
          // No working directory is set.
       }
    }
    
    ```



src/slave/containerizer/mesos/containerizer.cpp
Lines 1522 (patched)
<https://reviews.apache.org/r/58967/#comment247523>

    For clarify, I would move this after the foreach loop and do
    
    ```
    // If "--working_directory" argument is not found, default to the sandbox directory.
    if (!launchInfo.has_working_directory()) {
      launchInfo.set_working_directory(flags.sandbox_directory);
    }
    ```


- Vinod Kone


On May 9, 2017, 2:32 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58967/
> -----------------------------------------------------------
> 
> (Updated May 9, 2017, 2:32 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7433
>     https://issues.apache.org/jira/browse/MESOS-7433
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A DEBUG container's working directory should be set to the parent
> task's working directory. For the command executor case, even if
> the task itself has a root filesystem, the executor container still
> uses the host filesystem, hence
> `ContainerLaunchInfo.working_directory`, pointing to the executor's
> working directory in the host filesystem, may be different from the
> task's working directory in the root filesystem.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/containerizer.proto 41f2905df690bfe88ed762f1cd1246689fa4d3ea 
>   src/slave/containerizer/mesos/containerizer.cpp 3d724945812c0359ed175ce232f70886dc4401c8

> 
> 
> Diff: https://reviews.apache.org/r/58967/diff/2/
> 
> 
> Testing
> -------
> 
> make check on Mac OS and Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message