----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13646/#review25312 -----------------------------------------------------------
src/slave/api_refactor/isolators/resource_isolator.hpp <https://reviews.apache.org/r/13646/#comment49689> A member function cannot be both static and virtual, did you intend to have this static? $ cat static_virtual.cpp class Foo { static virtual void foo() { } }; int main() { } $ g++ static_virtual.cpp static_virtual.cpp:2: error: member ‘foo’ cannot be declared both virtual and static src/slave/api_refactor/isolators/resource_isolator.hpp <https://reviews.apache.org/r/13646/#comment49690> I'd be in favor of renaming change to update, given that we're "updating" the ResourceIsolator with the latest resource allocation information. src/slave/api_refactor/isolators/resource_isolator.hpp <https://reviews.apache.org/r/13646/#comment49692> What will this be used for? Why 'managed' instead of an override of 'allocation'? src/slave/api_refactor/isolators/resource_isolator.hpp <https://reviews.apache.org/r/13646/#comment49691> What is this returning? Resources that were once, but are no longer, allocated? Can you add documentation explaining why this would be needed? (Was this because some isolation techniques (e.g. LVM) are unable to shrink the allocation online?). Please add a new diff with documentation instead of replying here. :) - Ben Mahler On Aug. 19, 2013, 4:39 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13646/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2013, 4:39 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: mesos-600 > https://issues.apache.org/jira/browse/mesos-600 > > > Repository: mesos-git > > > Description > ------- > > First pass of new isolation api. See description at MESOS-600. > > Apologies but this was initially submitted to Twitter's internal reviewboard. > I've copied all the comments below: > > [~bmahle > Awesome to see this coming together! > > It looks like the changes to libprocess-ify the isolator is included here. I > commented on that a bit but it's fairly orthogonal to the isolator API > changes needed for pluggable resource isolation. > > (1) As an example, let's say one wants to provide optional PID namespace > isolation. How would they accomplish that using this API? Would that be done > through a {PID,Process}ResourceIsolator? What about other namespaces? > > (2) As a note, let's also try to make sure we don't dig ourselves into a hole > when it comes to having per-executor isolation schemes. This means for > example, using CFS+shares for one executor, and using only shares for another > executor. Simply a note in the code as to how this could be done would be > nice. I hinted at this feature (making resources first class) in > https://issues.apache.org/jira/browse/MESOS-338 to enable per-executor > isolation levels. > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 7:30 p.m.) > I've libprocess-ified the existing Isolator and will post a review shortly. > > 1) Yes, that's what we're thinking. Then you'd have things like a > MountResourceIsolator or UidResourceIsolator. > > 2) I'd like to talk through the per-executor isolation more. I was thinking > this could be exposed (to the isolator) as different resource types - e.g. > 4.0 cpu shares or 2.0 cfs cpu and then a combined cpu isolator would be able > to coordinate both. > Add comment > src/slave/api_refactor/isolator.hpp (Diff revision 1) > public: > 26 > virtual process::Future<Exited> > launch( > I think this should return a future for when the executor starts rather than > exits. > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 7:30 p.m.) > Makes sense! > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 11:46 p.m.) > <<[~czhang] > A different thought on this: > > Looking at cgroup_isolator.cpp, dispatch calls back to slave are either > executorStarted or executorTerminated. One of it can be eliminated by > launch() returning a future that gets fulfilled when an executor starts or > terminates. (Not an expert in libprocess, Is there a way for it to prepare 2 > futures to be fulfilled later?) > > Do we have any other place other than launch() to return a future of when an > executor goes away? If not, launch() could be used for that. executorStarted > still checks the state of the executor anyways, so it shouldn't be so hard to > move logic in executorStarted to before the executor gets started. > [~czhang] > > What about if launch() returned a Started future, kill() returns a Terminated > future and we add wait() which also returns a Terminated future and covers > the executor forking but dying unexpectedly. > Ian Downes 1 week, 2 days ago (Aug. 10, 2013, 12:05 a.m.) > Back to Chi's proposal, can launch() return a Started future which contains a > nested Terminated future? > Eric Biederman 1 week, 2 days ago (Aug. 10, 2013, 12:19 a.m.) > I know I tried that and failed miserably. So at the very least I was missing > the secret sauce to make it work. (Perhaps shared_ptr in the right places?) > > At this point not knowing when the process actually starts isn't particularly > interesting as we can start the timer to timeout on registering the executor > before we call launch, and remove an entire state from the state machine in > slave.cpp. > Benjamin Mahler 1 week, 2 days ago (Aug. 10, 2013, 2:07 a.m.) > Chi: Unfortunately we don't have a notion of this at the moment, one could > return a nested Future as Ian said, but it's a bit clunky. > > Let's focus on the ResourceIsolator changes in this review, making the > Isolator return Futures is an orthogonal change that we can focus on with > Ian's upcoming refactor. > Add comment > src/slave/api_refactor/isolator.hpp (Diff revision 1) > public: > 35 > // Terminate a framework's executor. Resources are NOT automatically > released > 36 > // and should be explicitly released/reduced with change(). > This sounds tricky, what was the motivation for requiring explicit releasing > of resources? > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 7:30 p.m.) > Motivated because we want to keep at least one resource (disk - > directories/lvm/raw) around after an executor finishes. > Add comment > src/slave/api_refactor/isolator.hpp (Diff revision 1) > public: > 45 > virtual process::Future<Resources> > > 46 > change(const UUID& uuid, const Resources& resources) = 0; > Why use the UUID rather than frameworkId / executorId? I think we'd like to > have a consistent way to refer to executors instead. Ditto below and in > ResourceIsolator. > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 7:30 p.m.) > This ties in with resources persisting after an executor terminates. I was > thinking the uuid could be used to differentiate. > Add comment > src/slave/api_refactor/isolator.hpp (Diff revision 1) > public: > 51 > // Get the resource allocation for an executor. > 52 > virtual process::Future<Resources> allocation(const UUID& uuid) = 0; > 53 > 54 > // Get available resources for this isolator. > 55 > virtual process::Future<Resources> totalUsage() = 0; > 56 > 57 > // Get total resources managed by this isolator. > 58 > virtual process::Future<Resources> totalAllocation() = 0; > Is there an anticipated need to add these? Just curious how you anticipate > these will be used. > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 7:30 p.m.) > No, not a strong need and they could be future additions to the api. The > motivation was the possibility of pushing more responsibility for managing > resources down into the isolator and out of the slave proper. > Add comment > src/slave/api_refactor/isolators/resource_isolator.hpp (Diff revision 1) > public: > 14 > // Assign pid to this resource allocation, call in the forked context > We likely do not want to call this in the forked context, was there a need > for that? > Ian Downes 1 week, 2 days ago (Aug. 9, 2013, 7:30 p.m.) > This was motivated from discussions with Eric. Cgroups and most namespaces > support moving pids into a cgroup but some namespaces require processes to > put *themselves* into a namespace. > Eric Biederman 1 week, 2 days ago (Aug. 10, 2013, 12:15 a.m.) > Just to emphasize. > > Moving processes between cgroups from the is a bit iffy, especially from the > outside. > > Namespaces you have to set either at fork time or from the process itself. > > So we can probably kill the pid_t argument as it will always be ::getpid(). > Add comment > src/slave/api_refactor/isolators/resource_isolator.hpp (Diff revision 1) > public: > 20 > // Change the resource, allocating/deallocating them as necessary > 21 > virtual Try<Nothing> change(const UUID& uuid, const Resource& resource) = 0; > My vote would be to "update" rather than "change". > Add comment > > > Diffs > ----- > > src/slave/api_refactor/isolator.hpp PRE-CREATION > src/slave/api_refactor/isolators/dir_disk_isolator.hpp PRE-CREATION > src/slave/api_refactor/isolators/lvm_disk_isolator.hpp PRE-CREATION > src/slave/api_refactor/isolators/resource_isolator.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/13646/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
