mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Budnik <abud...@mesosphere.com>
Subject Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.
Date Tue, 22 May 2018 15:46:25 GMT


> On May 21, 2018, 1:21 p.m., Qian Zhang wrote:
> > src/slave/containerizer/composing.cpp
> > Line 361 (original), 360-365 (patched)
> > <https://reviews.apache.org/r/66668/diff/4/?file=2021203#file2021203line361>
> >
> >     So besides removing the container from the `containers_` map, we also eliminate
a call to `ComposingContainerizerProcess::destroy()`, My understanding is we assume the underlying
containerizers will always do its own destroy to clean up its internal data, so here in composing
containerizer we just need to remove the container from the `containers_` map, right?

>we assume the underlying containerizers will always do its own destroy to clean up its
internal data, so here in composing containerizer we just need to remove the container from
the containers_ map, right?


Yes, correct
https://github.com/apache/mesos/blob/d7d7cfbc3e5609fc9a4e8de8203a6ecb11afeac7/src/slave/containerizer/containerizer.hpp#L129-L130


> On May 21, 2018, 1:21 p.m., Qian Zhang wrote:
> > src/slave/containerizer/composing.cpp
> > Lines 396-400 (original)
> > <https://reviews.apache.org/r/66668/diff/4/?file=2021203#file2021203line401>
> >
> >     Previously, in the case that `destroy-in-progress stopped an launch-in-progress`,
the `destroy()` will return `ContainerTermination()` which is set here. But now with this
implementation, it will return `None()` which is set by the underlying containerizer. This
seems not correct to me, in this case, the destroy should considered as succeeded (return
`ContainerTermination()`) rather than failed (return `None()`).

Before https://reviews.apache.org/r/66510/ patch was committed, `destroy()` returned `Future<bool>`.
There was a reason to set a `true` to the `destroyed` promise, because we successfully destroyed
a container (in the case that destroy-in-progress stopped an launch-in-progress).

>the destroy should considered as succeeded (return ContainerTermination()) rather than
failed (return None()).

At the moment I don't think that a client should interpret the result of `destroy()` as failed/succeeded
relying on whether or not `Option<ContainerTermination>` is empty. If there was no actual
attempt to launch a container using an underlying containerizer, why do we need to return
some virtual `ContainerTermination`?


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66668/#review203490
-----------------------------------------------------------


On April 17, 2018, 3:23 p.m., Andrei Budnik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66668/
> -----------------------------------------------------------
> 
> (Updated April 17, 2018, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-8712
>     https://issues.apache.org/jira/browse/MESOS-8712
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, we stored `destroyed` promise for each container and used
> it to guarantee that `destroy()` returns a non-empty value when the
> destroy-in-progress stops an launch-in-progress using the next
> containerizer. Since `wait()` and `destroy()` return the same
> `ContainerTermination` value when called with the same ContainerID
> argument, we can remove `destroyed` promise and add callbacks to
> clean up `containers_` map instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/composing.cpp 1fb79f53b48154ecbd3b0165b6a8002c871e6dce 
> 
> 
> Diff: https://reviews.apache.org/r/66668/diff/5/
> 
> 
> Testing
> -------
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message