> On May 23, 2016, 7:56 p.m., Benjamin Mahler wrote: > > src/slave/containerizer/containerizer.hpp, lines 64-68 > > <https://reviews.apache.org/r/47707/diff/1/?file=1391149#file1391149line64> > > > > Hm.. it seems more like the default set of resources would not include > > the flags. > > > > The current function is a little more than just the default resources, > > it is the flag provided resources along with defaults injected in for any > > that were omitted. > > > > Could we instead provide just the defaults (per the suggested signature > > above) and have containerizer implementations inject defaults with the help > > of this function? > > > > As an example: > > > > ``` > > // Provides a means for containerizer implementation to use a > > // consistent set of default resources when resources are not > > // specified via the '--resources' flag. The containerizer may > > // choose its own default value by probing the system, however > > // this helper provides a standard set of defaults for: > > // > > // ["cpus", "mem", "disk", "ports"] > > // > > // A containerizer is free to choose alternative defaults for these > > // resources, but this increases the likelihood that containerizers > > // disagree about the amount of resources to manage when used within > > // the composing containerizer. > > static Try<Resources> defaultResources(); > > ``` > > > > This means we could augment the Containerizer::resources and > > Isolator::resources to do two things: > > > > (1) Get informed about the resources provided via flags, and > > (2) Return additional resources that the containerizer or isolator > > wishes to manage. > > > > Something like: > > > > ``` > > // Informs the containerizer / isolator about the resources > > // that are explicitly specified. The containerizer / isolator > > // returns a subset of the resources that it will manage. The > > // containerizer / isolator may also return additional resources > > // that it wishes to manage if they were not specified. > > // For example, a GPU isolator, seeing that "gpus" is unspecified > > // may probe the system and decide to manage some GPUs (i.e. it > > // will return these GPUs from this function). > > Future<Resources> manage(const Resources& provided); > > ``` > > Kevin Klues wrote: > We could do this (though it feels a little less intuitive and less > flexible than allowing arbitrary flags to be passed in). We are also > unnecessarily enumerating all of the `cpu, mem, disk, ports` available on the > system even if the flags would indicate to override them. Is the assumption > that the actual class implementing `manage()` would need to find a way to get > at the flags itself if it relies on them? How would the default > implementation of `manage()` work? The whole goal of it was to avoid having > to push new logic down into each existing containerizer (which your proposal > will require in order to do the flags parsing somehwere to override the > defaults).
Also, how do you envision this working for `defaultResources()` not taking flags, when it relies on `flags.work_dir` (for disk) and `flags.default_role` (for all default resources when setting their role). - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47707/#review134414 ----------------------------------------------------------- On May 23, 2016, 12:45 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47707/ > ----------------------------------------------------------- > > (Updated May 23, 2016, 12:45 a.m.) > > > Review request for mesos, Benjamin Mahler and Jie Yu. > > > Bugs: MESOS-5256 > https://issues.apache.org/jira/browse/MESOS-5256 > > > Repository: mesos > > > Description > ------- > > Previously, the `Containerizer` class contained a static `resources()` > function for enumerating all of the resources available on an agent. > This made it hard to add new resources to this enumeration that were, > for example, containerizer-specific or tied to a new isolator module > loaded at runtime. > > This commit refactors the resource enumeration logic to allow each > containerizer to enumerate resources itself. It does this by adding a > new virtual `resources()` function, whose default implementation > includes the same logic as the old static version of `resources()`. > Individual containerizers simply overwrite this function to do their > own enumeration if they want to. > > Similar logic exists to push resource enumeration down into isolators > as well. As such, the logic for the Nvidia GPU resource enumeration has > been moved down into the Nvidia GPU isolator instead of being > hard-coded into the mesos containerizer itself. > > > Diffs > ----- > > include/mesos/slave/isolator.hpp 4be8c2bb409052e2e07138483408209384f41e23 > src/slave/containerizer/composing.hpp > f3eebd19bc9e6b3b8a969a2ad967b3e2909e0ee4 > src/slave/containerizer/composing.cpp > 15d059f0bbda4e8cb93c65c09327dde1e34d3e7b > src/slave/containerizer/containerizer.hpp > ff78b4d0fd4a3b862f6019fc757c16b7367cd3cf > src/slave/containerizer/containerizer.cpp > faa0c789dda8a6f36fdb6217b0bae270b6b8f2e2 > src/slave/containerizer/mesos/containerizer.hpp > a1a00020668f6da8d611f26e5637afffc87d09ba > src/slave/containerizer/mesos/containerizer.cpp > 75e5a32a3e70ec60a6800e21a621673184ea0956 > src/slave/containerizer/mesos/isolator.hpp > bacd86af42d16cb7c9b6622dfb298dcaa7007b75 > src/slave/containerizer/mesos/isolator.cpp > 253ff3cea8aff3e7a3051fb5a763cc081f455f18 > src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.hpp > 502204650192d5ea44aa631eac8eb37e051843f0 > src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp > 8f81cb79c10261670efc9eaa8614751854f53806 > src/slave/slave.cpp ce0e7b1f1d17c3b82d835b0a6296ed7b1e9eeac1 > > Diff: https://reviews.apache.org/r/47707/diff/ > > > Testing > ------- > > make -j check ( which fails on one test -- see > https://reviews.apache.org/r/47708/ ) > > > Thanks, > > Kevin Klues > >
