mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Janco <jjanco....@gmail.com>
Subject Re: Review Request 70581: Add flag to decouple docker runtime.
Date Wed, 15 May 2019 21:50:30 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`.
> 
> James Peach wrote:
>     > 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.
> 
> Qian Zhang wrote:
>     I think what we usually do in Mesos code is an agent flag for agent-wide configuration
and a protobuf message field for per container configuration. I agree that we can implement
the per container configuration later, but for the agent flag I would suggest to let the `docker/runtime`
isolatior handle it rather than using the flag to drop the isolator because in future when
we add the per containner configuration, the implemenation would be nature and easy to read
since all the codes are in a single place (the isolator) rather than in multiple places (like
the containerizer and store).

This seems reasonable - I'll move this into the isolator.


- Jacob


-----------------------------------------------------------
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