> On March 25, 2015, 11:40 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/cgroups/mem.cpp, lines 481-488 > > <https://reviews.apache.org/r/30546/diff/5/?file=904879#file904879line481> > > > > Can you indent the cases? > > > > Also, there is a bug here, I'll let you find it ;)
:) > On March 25, 2015, 11:40 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/cgroups/mem.cpp, lines 470-474 > > <https://reviews.apache.org/r/30546/diff/5/?file=904879#file904879line470> > > > > It looks like the isolators have become inconsistent about what they > > return for an unknown container (e.g. network isolator is not returning a > > failure for unknown container, but the memory/cpu isolators are). > > > > Much like in `usage`, let's just return a Failure here rather than an > > empty result. The containerizer should understand that it destroyed this > > container and doesn't need the usage() result anymore, right? yeah, took a look at the call chain: I think the usage in all isolators should return a failure for the unknown container case (which port_mapping.cpp currently does not), since MesosContainerizer should not have called usage for destroyed containers. However, it is possible for a container to be destroyed after usage but before _usage. The _collect in slave/monitor.cpp checks for this case and simply returns, which means it does not matter anymore how the downstream components, the containerizer and all the isolators react to the same race? Currently _usage in MesosContainerizer does not check for this case. For isolators, cpu isolator does not have a _usage; port_mapping::_usage does not use containerID so it doesn't matter; I think we can in this patch return failure in MemIsolator::_usage as you suggested and in the future just be consistent on this among isolators. > On March 25, 2015, 11:40 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/cgroups/mem.cpp, lines 679-680 > > <https://reviews.apache.org/r/30546/diff/5/?file=904879#file904879line679> > > > > Do you need to print this? Seems unnecessary? just keep this? 3 log lines each container each slave restart should be fine? > On March 25, 2015, 11:40 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/cgroups/mem.cpp, line 494 > > <https://reviews.apache.org/r/30546/diff/5/?file=904879#file904879line494> > > > > Not returning a Failure in this case (like the file parsing cases in > > `usage()` above) seems to suggest that we're not sure if the pressure > > counters can fail and so we want to proceed with the partial result. > > > > That's good, but why not keep logging the error? (rather than erasing > > the counter upon the first issue we see). Since we're not expecting this to > > be possible (but we're not confident enough about eventfds to make this a > > Failure()) let's just print an ERROR each time we are returning a partial > > result, to make this more obvious in the logs. > > > > This also ends up cleaning up the code, as you no longer need to keep > > an iterator around for erasing. yeah, we should have no less confidence in pressure listening api than reading memory.stat. Changed to just return failure every time. - Chi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30546/#review77820 ----------------------------------------------------------- On March 27, 2015, 3:24 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30546/ > ----------------------------------------------------------- > > (Updated March 27, 2015, 3:24 a.m.) > > > Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. > > > Bugs: MESOS-2136 > https://issues.apache.org/jira/browse/MESOS-2136 > > > Repository: mesos > > > Description > ------- > > MemIsolator: expose memory pressures for containers. > > > Diffs > ----- > > include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 > src/slave/containerizer/isolators/cgroups/mem.hpp > a00f723de61b9bccd76a2948b6d14fde7a993a8d > src/slave/containerizer/isolators/cgroups/mem.cpp > eaeb30164e8ae8c0a703683b7cc0bf1851760c95 > src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e > > Diff: https://reviews.apache.org/r/30546/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
