> On Sept. 19, 2016, 11: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? > > Alexander Rukletsov wrote: > > 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 : ) > > Jie Yu wrote: > > But what if we omit ContainerInfo altogether? In this case, we do not > explicitly tell what containerizer to use and it is deduced automagically. > > If ContainerInfo is not set, we assume it'll be handled by > MesosContainerizer. It's like saying the default container runtime is > MesosContainerizer. > > Qian Zhang wrote: > It seems this check has broken the existing users: > https://issues.apache.org/jira/browse/MESOS-6208
That ticket was created by me. I should've linked to this RR from there, will do it now. - Jan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51865/#review149445 ----------------------------------------------------------- On Sept. 16, 2016, 12:19 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51865/ > ----------------------------------------------------------- > > (Updated Sept. 16, 2016, 12:19 p.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 > >
