> 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? > > 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 : )
> 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. - 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 > >
