mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 67728: Fixed orphan container cleanup issue in CNI isolator.
Date Mon, 25 Jun 2018 22:01:47 GMT


> On June 25, 2018, 7:55 p.m., St├ęphane Cottin wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Line 433 (original), 432 (patched)
> > <https://reviews.apache.org/r/67728/diff/1/?file=2044786#file2044786line433>
> >
> >     This test makes sense only if the containerID is removed from orphans during
the _recover call. 
> >     
> >     I may miss something, but it seems _recover() only updates infos, not orphans.

The `containerIdToState.contains(containerId)` above makes sure that the else branch is for
orphan containers (both known or unknown).

The `orphans` parameter to `recover` method are those containers that the containerizer knows
about, but does not tie to any checkpointed executor or standalone container (this is called
"known orphans").

So this check makes sure we only call `cleanup` for unknown orphans (containerizer does not
know about, but isolator notices it from isolator perspective. this is possible for those
pre-1.3 containers, or operator accidentally remove some checkpoint data in the runtime dir).


- Jie


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


On June 25, 2018, 7 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67728/
> -----------------------------------------------------------
> 
> (Updated June 25, 2018, 7 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Sagar Patwardhan.
> 
> 
> Bugs: MESOS-9025
>     https://issues.apache.org/jira/browse/MESOS-9025
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The bug was introduced in https://reviews.apache.org/r/65987/ when we
> want to allow nested container to have a separate network namespace than
> its parent (MESOS-8534).
> 
> The original code misses a `continue` statement after recovering regular
> containers.
> 
> I am surprised that the unit tests didn't catch the bug. We'll add a new
> unit test for catching the regression.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp b6ad4fcf9f1596c07ddeb9bbb134f4619d189671

> 
> 
> Diff: https://reviews.apache.org/r/67728/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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