mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 51865: Added validation for `ContainerInfo`.
Date Mon, 19 Sep 2016 18:43:16 GMT


> On Sept. 19, 2016, 9:24 a.m., Jan Schlicht wrote:
> > src/master/validation.cpp, lines 63-78
> > <https://reviews.apache.org/r/51865/diff/4/?file=1499473#file1499473line63>
> >
> >     This will cause problems with tasks that don't use a container image: While
`ContainerInfo.type` is required, the `DockerInfo` or `MesosInfo` fields are not. If a user
doesn't want to use a container image, he will leave these fields empty and probably set the
type to `MESOS`.
> >     To fix this, the validation should check if the right type is set, when an image
is specified. Like
> >     ```
> >       if (container.has_mesos() && container.type() == ContainerInfo::MESOS)
{
> >         return Error("Expecting 'mesos' to be set for mesos container");
> >       }
> >     ```
> 
> Jan Schlicht wrote:
>     Oops, mistake in the example, should be
>     ```
>     if (container.has_mesos() && container.type() != ContainerInfo::MESOS) {
>       return Error("Expecting 'mesos' to be set for mesos container");
>     }
>     ```
> 
> Alexander Rukletsov wrote:
>     The "union trick" we use in `ContainerInfo` protobuf (https://github.com/apache/mesos/blob/7a5ec49877a2e1be5b053a6b59ed6f32760fbe7a/include/mesos/mesos.proto#L2058)
does not encourage setting type without setting the corresponding field.
>     
>     You say that something like this task definition is valid and possible:
>     ```
>     {
>       "name": "cni-test",
>       "task_id": "task01",
>       "slave_id": 
>      …
>       "container": {
>         "type": "mesos",
>         "network_infos" : [
>           { "name": "network-a" }
>         ]
>       },
>       "command" : {
>         "value": "foo"
>      }
>     ```
>     
>     I would argue, that setting type to `mesos` is misleading. If we allow such task
definitions, we should probably introduce another type, e.g. `NONE`.
> 
> Jie Yu wrote:
>     This is rather unfortunate and was introduced long time ago. I think we cannot apply
this check as it will break existing users.
>     
>     I think 'container.type()' is to tell which containerizer to use. We put all the
common fields in ContainerInfo. YOu can think of 'docker' and 'mesos' fields contain additional
information if needed, but they are optional.
>     
>     I think we should check:
>     ```
>     if (container.has_docker() && container.type() != ContainerInfo::DOCKER)
>     if (container.has_mesos() && container.type() != ContainerInfo::MESOS)
>     ```
>     
>     We should add more documentation here. AlexR, can you own this?

> I think 'container.type()' is to tell which containerizer to use.

But what if we omit `ContainerInfo` altogether? In this case, we do not explicitly tell what
containerizer to use and it is deduced automagically.

I'm happy to drive the doc update, however I would need some help in understanding what the
options are : )


- Alexander


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


On Sept. 16, 2016, 10:19 a.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51865/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2016, 10:19 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6157
>     https://issues.apache.org/jira/browse/MESOS-6157
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
>   src/tests/container_logger_tests.cpp 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
>   src/tests/containerizer/cni_isolator_tests.cpp 0d611c196870b6adabea52a48abcd344c8dad5d1

>   src/tests/containerizer/docker_containerizer_tests.cpp 1671d45171307cda62184505ce1dbadc476abca6

>   src/tests/containerizer/docker_volume_isolator_tests.cpp ca7bffd3b1773a11a4679d114885d3edd977b02b

>   src/tests/containerizer/filesystem_isolator_tests.cpp df4642d2667407b1758ffe2efcfdbf9968cf2c33

>   src/tests/containerizer/isolator_tests.cpp 9bb1e69209f34b18b5b64c3daf5ea26780f2ab74

>   src/tests/master_validation_tests.cpp 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/slave_tests.cpp b44b6fc5627ad97a33be151cb21133da57f3efd8 
> 
> Diff: https://reviews.apache.org/r/51865/diff/
> 
> 
> Testing
> -------
> 
> make check on Mac OS and Linux.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


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