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

Reply via email to