----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48527/#review137667 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 93) <https://reviews.apache.org/r/48527/#comment202843> Should this be an `Option` ? Since, not all containers would have lable set? Also, why have a separate field here if it's already contained in `NetworkInfo`? src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 95) <https://reviews.apache.org/r/48527/#comment202844> Don't need this comment. src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 137) <https://reviews.apache.org/r/48527/#comment202846> This could be an `Option` since not all `NetworkInfo` would have their `labels` set. `Labels` is an optional field in `NetworkInfo`. Also, the signature could be: process::Future<Nothing> attach( const ContainerID& containerId, const std::string& networkName, const std::string& netNsHandle, const TaskID& taskId const Option<hashmap<std::string, std::string>>& labels = None()); src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 566) <https://reviews.apache.org/r/48527/#comment202845> This is not required. We could just do : const mesos::TaskID taskId = containerConfig.task_info().task_id(); src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 937) <https://reviews.apache.org/r/48527/#comment202847> Should this be "Read CNI configuration from disk" ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 955) <https://reviews.apache.org/r/48527/#comment202853> Can we add a comment here for the purposed of the `args` key in the CNI config. Maybe a brife description and point to a github issue if reader is interested in understanding the purpose of the `args` key? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 960) <https://reviews.apache.org/r/48527/#comment202852> Should there be an `else` clause ? What happens if the `labels` key exists? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 967) <https://reviews.apache.org/r/48527/#comment202849> We should make these keys ("labels", "task-id", "args") as `constexpr` in spec.hpp. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 986) <https://reviews.apache.org/r/48527/#comment202850> Is there a guarantee that the plugin would be waiting for the isolator to `write` into the PIPE ? Also, this can potentially block the `network/cni` isolator if the plugin is not fast enough in reading the pipe. Can we follow the existing idiom of writing mutated JSON into a temp file? Probably checkpoint the JSON into the containers checkpoint DIR and set the stdinof the plugin to the checkpointed JSON config. This would also serve the dual purpose of checkpointing the mutated JSON to be used when deleteing the container. - Avinash sridharan On June 14, 2016, 7:55 p.m., Dan Osborne wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48527/ > ----------------------------------------------------------- > > (Updated June 14, 2016, 7:55 p.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Add NetworkInfo.labels to CNI Network before passing to CNI plugin. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp > 09890cedf2e7a1846bd1cb250e117be1680a1b80 > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 106ff35320f37b2e75ed60381de1406459e6d515 > > Diff: https://reviews.apache.org/r/48527/diff/ > > > Testing > ------- > > > Thanks, > > Dan Osborne > >
