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

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?


- Jie


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