You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@cn.ibm.com> on 2016/04/05 07:39:38 UTC
Review Request 45731: Introduced '--networks' flag to mesos-execute.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/
-----------------------------------------------------------
Review request for mesos, Avinash sridharan and Jie Yu.
Bugs: MESOS-5117
https://issues.apache.org/jira/browse/MESOS-5117
Repository: mesos
Description
-------
Introduced '--networks' flag to mesos-execute.
Diffs (updated)
-----
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/45731/diff/
Testing (updated)
-------
Ran the following command:
`sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
The created container successfully joined the CNI networks `net1` and `net2`.
Thanks,
Qian Zhang
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127048
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45731]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 5, 2016, 5:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:39 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127962
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45731]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 9, 2016, 2:53 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 2:53 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp df93e92252addaa794898ba95604eb2f91284b87
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On April 12, 2016, 6:53 a.m., Jie Yu wrote:
> > src/cli/execute.cpp, line 511
> > <https://reviews.apache.org/r/45731/diff/4/?file=1338061#file1338061line511>
> >
> > You should set DockerInfo network mode as well to 'USER'?
Yes, thanks for catching this!
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review128276
-----------------------------------------------------------
On April 9, 2016, 10:53 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 10:53 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp df93e92252addaa794898ba95604eb2f91284b87
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review128276
-----------------------------------------------------------
src/cli/execute.cpp (line 488)
<https://reviews.apache.org/r/45731/#comment191703>
use tokenize instead.
src/cli/execute.cpp (line 504)
<https://reviews.apache.org/r/45731/#comment191704>
Ditto on using tokenize.
src/cli/execute.cpp (line 509)
<https://reviews.apache.org/r/45731/#comment191705>
You should set DockerInfo network mode as well to 'USER'?
- Jie Yu
On April 9, 2016, 2:53 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 2:53 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp df93e92252addaa794898ba95604eb2f91284b87
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review128649
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45731]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 13, 2016, 5:37 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 5:37 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp 306779f5227248a9f1d67b6c50854ec47dd59cd5
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/
-----------------------------------------------------------
(Updated April 13, 2016, 1:37 p.m.)
Review request for mesos, Avinash sridharan and Jie Yu.
Bugs: MESOS-5117
https://issues.apache.org/jira/browse/MESOS-5117
Repository: mesos
Description
-------
Introduced '--networks' flag to mesos-execute.
Diffs (updated)
-----
src/cli/execute.cpp 306779f5227248a9f1d67b6c50854ec47dd59cd5
Diff: https://reviews.apache.org/r/45731/diff/
Testing
-------
Ran the following command:
`sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
The created container successfully joined the CNI networks `net1` and `net2`.
Thanks,
Qian Zhang
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/
-----------------------------------------------------------
(Updated April 9, 2016, 10:53 a.m.)
Review request for mesos, Avinash sridharan and Jie Yu.
Bugs: MESOS-5117
https://issues.apache.org/jira/browse/MESOS-5117
Repository: mesos
Description
-------
Introduced '--networks' flag to mesos-execute.
Diffs (updated)
-----
src/cli/execute.cpp df93e92252addaa794898ba95604eb2f91284b87
Diff: https://reviews.apache.org/r/45731/diff/
Testing
-------
Ran the following command:
`sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
The created container successfully joined the CNI networks `net1` and `net2`.
Thanks,
Qian Zhang
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127311
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45731]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 6, 2016, 7:56 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 7:56 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/
-----------------------------------------------------------
(Updated April 6, 2016, 3:56 p.m.)
Review request for mesos, Avinash sridharan and Jie Yu.
Bugs: MESOS-5117
https://issues.apache.org/jira/browse/MESOS-5117
Repository: mesos
Description
-------
Introduced '--networks' flag to mesos-execute.
Diffs (updated)
-----
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/45731/diff/
Testing
-------
Ran the following command:
`sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
The created container successfully joined the CNI networks `net1` and `net2`.
Thanks,
Qian Zhang
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127079
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45731]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 5, 2016, 9:13 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 9:13 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 6, 2016, 12:46 a.m., Avinash sridharan wrote:
> > src/cli/execute.cpp, line 356
> > <https://reviews.apache.org/r/45731/diff/2/?file=1325791#file1325791line356>
> >
> > It would much simpler to have a single if conditional here as follows:
> > if (networks.isSome()) {
> > ContainerInfo* containerInfo = task.mutable_container();
> >
> > foreach (const string& network,
> > strings::split(networks.get(), ",")) {
> > containerInfo->add_network_infos()->set_name(network);
> > }
> > }
>
> Qian Zhang wrote:
> But we need to have different logic to handle the flag `--networks` for `MesosContainerizer` and `DockerContainerizer`, i.e., for `DockerContainerizer`, we need to make sure user is not allowed to set multiple networks, but for `MesosContainerizer`, we do not need to make sure that. So I'd like to handle `--networks` separately.
This should be straightforward. At the end of the loop :
if (containerInfo->network_info_size() > 1 && containerizer == "docker) {
return Error(....)
}
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127266
-----------------------------------------------------------
On April 6, 2016, 7:56 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 7:56 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On April 6, 2016, 8:46 a.m., Avinash sridharan wrote:
> > src/cli/execute.cpp, line 356
> > <https://reviews.apache.org/r/45731/diff/2/?file=1325791#file1325791line356>
> >
> > It would much simpler to have a single if conditional here as follows:
> > if (networks.isSome()) {
> > ContainerInfo* containerInfo = task.mutable_container();
> >
> > foreach (const string& network,
> > strings::split(networks.get(), ",")) {
> > containerInfo->add_network_infos()->set_name(network);
> > }
> > }
But we need to have different logic to handle the flag `--networks` for `MesosContainerizer` and `DockerContainerizer`, i.e., for `DockerContainerizer`, we need to make sure user is not allowed to set multiple networks, but for `MesosContainerizer`, we do not need to make sure that. So I'd like to handle `--networks` separately.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127266
-----------------------------------------------------------
On April 5, 2016, 5:13 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:13 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127266
-----------------------------------------------------------
src/cli/execute.cpp (line 354)
<https://reviews.apache.org/r/45731/#comment190484>
It would much simpler to have a single if conditional here as follows:
if (networks.isSome()) {
ContainerInfo* containerInfo = task.mutable_container();
foreach (const string& network,
strings::split(networks.get(), ",")) {
containerInfo->add_network_infos()->set_name(network);
}
}
- Avinash sridharan
On April 5, 2016, 9:13 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 9:13 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/
-----------------------------------------------------------
(Updated April 5, 2016, 5:13 p.m.)
Review request for mesos, Avinash sridharan and Jie Yu.
Bugs: MESOS-5117
https://issues.apache.org/jira/browse/MESOS-5117
Repository: mesos
Description
-------
Introduced '--networks' flag to mesos-execute.
Diffs (updated)
-----
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/45731/diff/
Testing
-------
Ran the following command:
`sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
The created container successfully joined the CNI networks `net1` and `net2`.
Thanks,
Qian Zhang
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 5, 2016, 6:25 a.m., Gilbert Song wrote:
> > src/cli/execute.cpp, line 152
> > <https://reviews.apache.org/r/45731/diff/1/?file=1325657#file1325657line152>
> >
> > Should we mention that an image has to be provided && for mesos containerizer only?
Why do we need an image for this? Also, the `name` field is used to configure user-defined networks for `DockerContainerizer` as well (MESOS-4369).
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127043
-----------------------------------------------------------
On April 5, 2016, 5:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:39 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 5, 2016, 6:25 a.m., Gilbert Song wrote:
> > src/cli/execute.cpp, line 152
> > <https://reviews.apache.org/r/45731/diff/1/?file=1325657#file1325657line152>
> >
> > Should we mention that an image has to be provided && for mesos containerizer only?
>
> Avinash sridharan wrote:
> Why do we need an image for this? Also, the `name` field is used to configure user-defined networks for `DockerContainerizer` as well (MESOS-4369).
>
> Qian Zhang wrote:
> Agree with Avinash, a container without image can also join the specified networks. However Gilbert's comment reveals some potential issues in `mesos-execute`.
>
> The currently logic (https://github.com/apache/mesos/blob/0.28.0/src/cli/execute.cpp#L233:L263) in `mesos-execute` to launch task is:
> 1. Check if `--docker_image` is specified.
> 2. If it is specfied, then construct `ContainerInfo` based on `--containerizer` which can be `mesos` or `docker`.
>
> The problems I see with this logic are:
> 1. If `--docker_image` is not specified, then we will NOT construct `ContainerInfo` at all. This will not be correct when we introduce this `--networks` flag, because `mesos-execute` should be able to launch a container joins some networks but without an image.
> 2. If `--docker_image` is not specified and `--containerizer` is `docker`, then the result is we will launch a task with `MesosContainerizer` which is not expected since what user specifies is `DockerContainerizer`. And if `MesosContainerizer` is not enabled in agent (e.g., start agent with `--containerizers=docker`), then the container will be failed to create with an error like: `None of the enabled containerizers (docker) could create a container for the provided TaskInfo/ExecutorInfo message`. So I think in this case, we should guard it in `mesos-execute` rather than leaving it to agent, i.e., `mesos-execute` should exit with an error message to let user know if s/he choose `--containerizer=docker`, then s/he must specify `--docker_image` as well.
>
> To resolve the above issues, I have updated the patch with a new logic, please review it and let me know for any comments.
>
> Avinash sridharan wrote:
> Had a discussion with Gilbert. Qian, thanks for the explanation. Based on this I think Gilbert's comments are valid, since we can't use the --networks attribute without the image. We should probably throw an error in --networks is specified but --docker_image is not specified.
Please ignore my last comment, it was a bit hasty. I think the original comment still makes sense. We can launch containers on CNI networks without images.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127043
-----------------------------------------------------------
On April 6, 2016, 7:56 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 7:56 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On April 5, 2016, 2:25 p.m., Gilbert Song wrote:
> > src/cli/execute.cpp, line 152
> > <https://reviews.apache.org/r/45731/diff/1/?file=1325657#file1325657line152>
> >
> > Should we mention that an image has to be provided && for mesos containerizer only?
>
> Avinash sridharan wrote:
> Why do we need an image for this? Also, the `name` field is used to configure user-defined networks for `DockerContainerizer` as well (MESOS-4369).
Agree with Avinash, a container without image can also join the specified networks. However Gilbert's comment reveals some potential issues in `mesos-execute`.
The currently logic (https://github.com/apache/mesos/blob/0.28.0/src/cli/execute.cpp#L233:L263) in `mesos-execute` to launch task is:
1. Check if `--docker_image` is specified.
2. If it is specfied, then construct `ContainerInfo` based on `--containerizer` which can be `mesos` or `docker`.
The problems I see with this logic are:
1. If `--docker_image` is not specified, then we will NOT construct `ContainerInfo` at all. This will not be correct when we introduce this `--networks` flag, because `mesos-execute` should be able to launch a container joins some networks but without an image.
2. If `--docker_image` is not specified and `--containerizer` is `docker`, then the result is we will launch a task with `MesosContainerizer` which is not expected since what user specifies is `DockerContainerizer`. And if `MesosContainerizer` is not enabled in agent (e.g., start agent with `--containerizers=docker`), then the container will be failed to create with an error like: `None of the enabled containerizers (docker) could create a container for the provided TaskInfo/ExecutorInfo message`. So I think in this case, we should guard it in `mesos-execute` rather than leaving it to agent, i.e., `mesos-execute` should exit with an error message to let user know if s/he choose `--containerizer=docker`, then s/he must specify `--docker_image` as well.
To resolve the above issues, I have updated the patch with a new logic, please review it and let me know for any comments.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127043
-----------------------------------------------------------
On April 5, 2016, 1:39 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 1:39 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 5, 2016, 6:25 a.m., Gilbert Song wrote:
> > src/cli/execute.cpp, line 152
> > <https://reviews.apache.org/r/45731/diff/1/?file=1325657#file1325657line152>
> >
> > Should we mention that an image has to be provided && for mesos containerizer only?
>
> Avinash sridharan wrote:
> Why do we need an image for this? Also, the `name` field is used to configure user-defined networks for `DockerContainerizer` as well (MESOS-4369).
>
> Qian Zhang wrote:
> Agree with Avinash, a container without image can also join the specified networks. However Gilbert's comment reveals some potential issues in `mesos-execute`.
>
> The currently logic (https://github.com/apache/mesos/blob/0.28.0/src/cli/execute.cpp#L233:L263) in `mesos-execute` to launch task is:
> 1. Check if `--docker_image` is specified.
> 2. If it is specfied, then construct `ContainerInfo` based on `--containerizer` which can be `mesos` or `docker`.
>
> The problems I see with this logic are:
> 1. If `--docker_image` is not specified, then we will NOT construct `ContainerInfo` at all. This will not be correct when we introduce this `--networks` flag, because `mesos-execute` should be able to launch a container joins some networks but without an image.
> 2. If `--docker_image` is not specified and `--containerizer` is `docker`, then the result is we will launch a task with `MesosContainerizer` which is not expected since what user specifies is `DockerContainerizer`. And if `MesosContainerizer` is not enabled in agent (e.g., start agent with `--containerizers=docker`), then the container will be failed to create with an error like: `None of the enabled containerizers (docker) could create a container for the provided TaskInfo/ExecutorInfo message`. So I think in this case, we should guard it in `mesos-execute` rather than leaving it to agent, i.e., `mesos-execute` should exit with an error message to let user know if s/he choose `--containerizer=docker`, then s/he must specify `--docker_image` as well.
>
> To resolve the above issues, I have updated the patch with a new logic, please review it and let me know for any comments.
Had a discussion with Gilbert. Qian, thanks for the explanation. Based on this I think Gilbert's comments are valid, since we can't use the --networks attribute without the image. We should probably throw an error in --networks is specified but --docker_image is not specified.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127043
-----------------------------------------------------------
On April 5, 2016, 9:13 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 9:13 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On April 5, 2016, 2:25 p.m., Gilbert Song wrote:
> > src/cli/execute.cpp, line 187
> > <https://reviews.apache.org/r/45731/diff/1/?file=1325657#file1325657line187>
> >
> > Could you put it below `_dockerImage`? Thanks.
Can you please elaborate why we should put it below `_dockerImage`? The reason I put it below `_containerizer` is that I put the flag `Option<string> networks;` (L169) below `string containerizer;`, my idea was to keep them consistent in the code.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127043
-----------------------------------------------------------
On April 5, 2016, 1:39 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 1:39 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127043
-----------------------------------------------------------
src/cli/execute.cpp (line 152)
<https://reviews.apache.org/r/45731/#comment190179>
Should we mention that an image has to be provided && for mesos containerizer only?
src/cli/execute.cpp (line 187)
<https://reviews.apache.org/r/45731/#comment190178>
Could you put it below `_dockerImage`? Thanks.
src/cli/execute.cpp (line 468)
<https://reviews.apache.org/r/45731/#comment190180>
ditto.
src/cli/execute.cpp (line 612)
<https://reviews.apache.org/r/45731/#comment190182>
ditto.
- Gilbert Song
On April 4, 2016, 10:39 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 10:39 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On April 5, 2016, 2:26 p.m., Avinash sridharan wrote:
> > src/cli/execute.cpp, line 322
> > <https://reviews.apache.org/r/45731/diff/1/?file=1325657#file1325657line322>
> >
> > We should support this for `DockerContainerizer` as well, since MESOS-4369 is going use the `name` field to set user-defined networks in `DockerContainerizer` as well. We however need to make sure that for `DockerContainerizer` user is not allowed to set multiple networks.
Agree!
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127044
-----------------------------------------------------------
On April 5, 2016, 5:13 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:13 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 45731: Introduced '--networks' flag to
mesos-execute.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45731/#review127044
-----------------------------------------------------------
src/cli/execute.cpp (line 322)
<https://reviews.apache.org/r/45731/#comment190183>
We should support this for `DockerContainerizer` as well, since MESOS-4369 is going use the `name` field to set user-defined networks in `DockerContainerizer` as well. We however need to make sure that for `DockerContainerizer` user is not allowed to set multiple networks.
- Avinash sridharan
On April 5, 2016, 5:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:39 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced '--networks' flag to mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45731/diff/
>
>
> Testing
> -------
>
> Ran the following command:
> `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test --docker_image=library/busybox --command="sleep 30" --shell=true --networks=net1,net2`
>
> The created container successfully joined the CNI networks `net1` and `net2`.
>
>
> Thanks,
>
> Qian Zhang
>
>