mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 70581: Add flag to decouple docker runtime.
Date Wed, 15 May 2019 11:51:46 GMT


> On May 15, 2019, 6:16 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 230-235 (patched)
> > <https://reviews.apache.org/r/70581/diff/4/?file=2145030#file2145030line230>
> >
> >     s/ignore_docker_runtime_isolator/docker_ignore_runtime/
> >     
> >     And erasing `docker/runtime` from `isolations` seems a bit weird to me, should
we instead add a flag (like `--ignore-docker-image-manifest`) and let `docker/runtime` isolator
handle it (i.e., if it is true the isolator will just do nothing)? Or maybe this should be
a configuration per container? Like add a field in `ContainerInfo`.

> s/ignore_docker_runtime_isolator/docker_ignore_runtime/

Agreed.

> And erasing docker/runtime from isolations seems a bit weird to me, should we instead
add a flag (like --ignore-docker-image-manifest) and
> let docker/runtime isolator handle it (i.e., if it is true the isolator will just do
nothing)? Or maybe this should be a configuration per
> container? Like add a field in ContainerInfo.

We did go back and forth on this a bit. The documentation is pretty clear in a number of places
that `docker/runtime` is required and it is the isolator that deals with the Docker runtime
config. So in the end I thought that it was clearer to link the flag name to the isolator
name quite explicitly. Once you do that, then the implementation follows naturally. Whether
we drop the isolator or whether the isolator remains but takes no action is an implementation
detail that is invisible to an operator.

I would have prefered that we just allow the `docker/runtime` isolator the be absent, but
we felt constrained by backwards compatibility a bit here.

It did not occur to me to make this a per-image flag. In the future, this could still be added.
We could change the `docker/runtime` isolator to check the `ContainerInfo` flag as you suggest.
However, our reason for needing this is that we have to retain compatibility with an existing
PaaS implementation, so I think that the need for this as a container configuration is limited.


- James


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


On May 14, 2019, 6:51 p.m., Jacob Janco wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70581/
> -----------------------------------------------------------
> 
> (Updated May 14, 2019, 6:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-9760
>     https://issues.apache.org/jira/browse/MESOS-9760
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Docker runtime isolator propagates manifest configuration metadata.
> This is not always desirable, e.g. a customer may want to ignore the
> defined WORKDIR/ENV defined in the manifest. This patch decouples
> isolator work from the selection of Docker as an image provider.
> 
> https://issues.apache.org/jira/browse/MESOS-9760
> 
> 
> Diffs
> -----
> 
>   docs/configuration/agent.md 04d7114e92c189afd04a8889b3ab853ade9deec6 
>   docs/upgrades.md ece8462b9950626166c832d3b554ad51f68f42e1 
>   src/slave/containerizer/mesos/containerizer.cpp c4a6827c5574586475152b1dc31572d6e1821a82

>   src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a

>   src/slave/flags.hpp 09921cb6172202b5c1d2f8d03f9ccaeb3d0e8c94 
>   src/slave/flags.cpp 49a350f9e13409493fa9612c53d9d58b62122371 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1c5d72091d570001ad543e342ab0b9128ffc01ae

> 
> 
> Diff: https://reviews.apache.org/r/70581/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>


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