mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 46228: Create persistent volume with a supplied user.
Date Sun, 20 Nov 2016 16:51:35 GMT


> On Nov. 15, 2016, 8 p.m., Greg Mann wrote:
> > src/slave/slave.cpp, lines 2848-2850
> > <https://reviews.apache.org/r/46228/diff/2/?file=1540382#file1540382line2848>
> >
> >     What's the rationale for including the sticky bit here? I wonder if the gain
in security is worth the restriction?

The sticky bit ensures that the tasks only deletes files owned by it. This is particularly
true for shared volumes where multiple containers have access to the same volume simultaneously
and we would not like a running task deleting content from the shared volume which is actually
owned by another running task at the same point of time.

Even for non-shared volumes, sticky bit can be enforced to disallow a task deleting content
from the volume which is owned by a preceding task. A `DESTROY` of the volume is done using
the user of mesos-slave process (typically root) which should be able to `rmdir()` the directory
to reclaim the complete disk space.


> On Nov. 15, 2016, 8 p.m., Greg Mann wrote:
> > src/slave/slave.cpp, lines 2863-2872
> > <https://reviews.apache.org/r/46228/diff/2/?file=1540382#file1540382line2863>
> >
> >     The `mkdir` is performed recursively; it looks like some parent directories
may be left behind after this `rmdir` call?
> >     
> >     It may be difficult to ensure that all directories are cleaned up without getting
too entangled with the implementation of `getPersistentVolumePath`. Since `mkdir` can also
leave directories behind if it fails in the middle of a recursive call, it's probably not
a big deal if we leave something behind here as well, but perhaps you should include a comment
indicating that this may happen?

Added a comment that we deem this transaction to succeed if we are able to successfully do
a `mkdir()` followed by an optional `chown()` [if user is specified in the `CREATE` operation].
If any of these transactions fail, we do not allow the `CREATE` to succeed to ensure we do
not leak persistent volumes.


- Anindya


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


On Nov. 20, 2016, 4:50 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46228/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2016, 4:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
>     https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If user is specified in Resource::DiskInfo::Persistence, set the
> ownership of the volume to that user. Tasks should have to comply to
> ownership of the volume before they can use the volume.
> If user is not specified in Resource::DiskInfo::Persistence, it is
> created with the user mesos-slave process runs as. When the first
> task is launched, it updates the ownership of the persistent volume
> with its user so as to be able to write into that volume.
> Note that tasks can fail to use the volume when they actually try
> to access the volume is that task's ownership is not compatible
> with that of the volume.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 8f024d084189b59bb229c63d20108e7bfe42065f

>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 270d2aa6e06f323bfb6eee3b703a24a600a55871

>   src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/46228/diff/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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