----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55334/#review186159 -----------------------------------------------------------
could you rebase this chain? sorry for the delay, but thank you, Zhitao! src/slave/containerizer/mesos/containerizer.cpp Lines 689 (patched) <https://reviews.apache.org/r/55334/#comment262616> log the containerId? src/slave/containerizer/mesos/containerizer.cpp Lines 695 (patched) <https://reviews.apache.org/r/55334/#comment262619> should we use `WARNING` here? since everytime the containerizer recovers, it might log a bunch of warning if there are many legacy containers running. src/slave/containerizer/mesos/containerizer.cpp Lines 696 (patched) <https://reviews.apache.org/r/55334/#comment262620> do we still plan to back fill? src/slave/containerizer/mesos/containerizer.cpp Lines 755 (patched) <https://reviews.apache.org/r/55334/#comment262622> ditto. src/slave/containerizer/mesos/containerizer.cpp Lines 792-794 (patched) <https://reviews.apache.org/r/55334/#comment262637> identify this log for both executor container and nested container? src/slave/containerizer/mesos/containerizer.cpp Line 1050 (original), 1088 (patched) <https://reviews.apache.org/r/55334/#comment262639> this may cause segfault, right? we should consider address this TODO in a separate patch in favor of checkpointing the `ContainerConfig`. src/slave/containerizer/mesos/containerizer.cpp Lines 1114-1117 (patched) <https://reviews.apache.org/r/55334/#comment262612> How about: Checkpoint the `ContainerConfig` which includes all information to launch a container. Critical information (e.g., `ContainerInfo`) can be used for tracking container image usage. src/slave/containerizer/mesos/containerizer.cpp Lines 1130 (patched) <https://reviews.apache.org/r/55334/#comment262617> quote the path? src/slave/containerizer/mesos/containerizer.cpp Line 1191 (original), 1247 (patched) <https://reviews.apache.org/r/55334/#comment262613> add a `CHECK_SOME()` in the very beginning of `prepare()` funtion? src/slave/containerizer/mesos/containerizer.cpp Line 1486 (original), 1545 (patched) <https://reviews.apache.org/r/55334/#comment262614> fix the indentation (extra space)? src/slave/containerizer/mesos/paths.cpp Lines 438 (patched) <https://reviews.apache.org/r/55334/#comment262609> Move below `getContainerTermination()`? src/slave/containerizer/mesos/paths.cpp Lines 449 (patched) <https://reviews.apache.org/r/55334/#comment262610> quote the `path`? and add `containerId`? src/slave/containerizer/mesos/paths.cpp Lines 458 (patched) <https://reviews.apache.org/r/55334/#comment262611> permutate the space to the line above? mind fix getContainerTermination() as well? - Gilbert Song On Aug. 1, 2017, 10:10 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55334/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2017, 10:10 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu. > > > Bugs: MESOS-6894 > https://issues.apache.org/jira/browse/MESOS-6894 > > > Repository: mesos > > > Description > ------- > > This patch includes the following change: > - Checkpointed `ContainerConfig` used to launch a container; > - Added helper function to read checkpointed `ContainerConfig`; > - Recovered `ContainerConfig`. > - Make `ContainerConfig` Option in `Container` struct to indicate whether > recovered. > > > Diffs > ----- > > src/slave/containerizer/mesos/containerizer.hpp > fd586306f7a6d3d2cdb58481f0ef4906de8d0f88 > src/slave/containerizer/mesos/containerizer.cpp > 6f100b516f2750732acaed693f7023b1e44b39d9 > src/slave/containerizer/mesos/paths.hpp > 12ae7c7329f9e8d334cb32c30cc38465bddc046c > src/slave/containerizer/mesos/paths.cpp > 0c61c20c345a327ec469b382558aaeed0280e754 > > > Diff: https://reviews.apache.org/r/55334/diff/7/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
