> 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");
>     }
>     ```

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


- 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