You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2016/03/16 22:21:24 UTC
Review Request 44934: Updated mesos-execute to add support for Appc.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
Updated mesos-execute to add support for Appc.
Diffs
-----
src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
Diff: https://reviews.apache.org/r/44934/diff/
Testing
-------
Tested with various Appc images.
Thanks,
Jojy Varghese
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review126224
-----------------------------------------------------------
Just FYI, this needs to be rebased as the current mesos-executor is using HTTP API now.
- Guangya Liu
On 三月 20, 2016, 4:19 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated 三月 20, 2016, 4:19 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review124448
-----------------------------------------------------------
Ship it!
Ship It!
- haosdent huang
On March 20, 2016, 4:19 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 20, 2016, 4:19 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review126648
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45259, 44934]
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 1, 2016, 8:05 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 8:05 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/
-----------------------------------------------------------
(Updated April 1, 2016, 8:05 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
rebased.
Bugs: MESOS-4978
https://issues.apache.org/jira/browse/MESOS-4978
Repository: mesos
Description
-------
Updated mesos-execute to add support for Appc.
Diffs (updated)
-----
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/44934/diff/
Testing
-------
Tested with various Appc images.
Thanks,
Jojy Varghese
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On March 22, 2016, 5:10 p.m., Tom Runyon wrote:
> > It would be helpful to update docs/container-image.md to include example appc deployments using mesos-execute.
Created MESOS-5006.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review124834
-----------------------------------------------------------
On March 20, 2016, 4:19 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 20, 2016, 4:19 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Tom Runyon <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review124834
-----------------------------------------------------------
It would be helpful to update docs/container-image.md to include example appc deployments using mesos-execute.
- Tom Runyon
On March 20, 2016, 4:19 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 20, 2016, 4:19 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/
-----------------------------------------------------------
(Updated March 20, 2016, 4:19 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4978
https://issues.apache.org/jira/browse/MESOS-4978
Repository: mesos
Description
-------
Updated mesos-execute to add support for Appc.
Diffs (updated)
-----
src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
Diff: https://reviews.apache.org/r/44934/diff/
Testing
-------
Tested with various Appc images.
Thanks,
Jojy Varghese
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review124370
-----------------------------------------------------------
src/cli/execute.cpp (line 317)
<https://reviews.apache.org/r/44934/#comment186890>
cerr << containerInfo.error() << endl;
src/cli/execute.cpp (line 517)
<https://reviews.apache.org/r/44934/#comment186887>
do not need period
- Guangya Liu
On 三月 18, 2016, 5:39 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated 三月 18, 2016, 5:39 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/
-----------------------------------------------------------
(Updated March 18, 2016, 5:39 p.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4978
https://issues.apache.org/jira/browse/MESOS-4978
Repository: mesos
Description
-------
Updated mesos-execute to add support for Appc.
Diffs
-----
src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
Diff: https://reviews.apache.org/r/44934/diff/
Testing
-------
Tested with various Appc images.
Thanks,
Jojy Varghese
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/
-----------------------------------------------------------
(Updated March 18, 2016, 5:26 p.m.)
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
Updated mesos-execute to add support for Appc.
Diffs (updated)
-----
src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
Diff: https://reviews.apache.org/r/44934/diff/
Testing
-------
Tested with various Appc images.
Thanks,
Jojy Varghese
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check this?
> >
> > What about make the logic as this? This will involve less code change and the logic may be more clear?
> >
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> > old logic here;
> > }
> >
> > if (appcImage.isSome() && containerizer == "mesos") {
> > ContainerInfo containerInfo;
> > containerInfo.set_type(ContainerInfo::MESOS);
> > ContainerInfo::MesosInfo mesosInfo;
> >
> > Image::Appc appc;
> >
> > appc.set_name(appcImage.get());
> >
> > // TODO(jojy): Labels are hard coded right now.
> > // Consider adding label flags for customization.
> > Label arch;
> > arch.set_key("arch");
> > arch.set_value("amd64");
> >
> > Label os;
> > os.set_key("os");
> > os.set_value("linux");
> >
> > Labels labels;
> > labels.add_labels()->CopyFrom(os);
> > labels.add_labels()->CopyFrom(arch);
> >
> > appc.mutable_labels()->CopyFrom(labels);
> >
> > Image mesosImage;
> >
> > mesosImage.set_type(Image::APPC);
> > mesosImage.mutable_appc()->CopyFrom(appc);
> >
> > mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >
> > containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> >
> > task.mutable_container()->CopyFrom(containerInfo);
> > }
>
> Jojy Varghese wrote:
> Then we will have to repeat the `if (containerizer == "mesos")` for both dockerImage case and appcImage case. By seperating the logic by cotainerizer, we now can add more image types into the "mesos" containerizer block (say OCI image).
>
> Guangya Liu wrote:
> Yes, that's also my concern when I open this issue, I think it is ok to keep your current logic.
>
> The logic of checking `containerizer.empty()` should be removed?
We could remove that but the idea is that the block can be cleanly separated out now to a method (say `handleContainerizer`) that handles the containerizers.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123991
-----------------------------------------------------------
On March 17, 2016, 6:54 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 17, 2016, 6:54 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Guangya Liu <gy...@gmail.com>.
> On 三月 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check this?
> >
> > What about make the logic as this? This will involve less code change and the logic may be more clear?
> >
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> > old logic here;
> > }
> >
> > if (appcImage.isSome() && containerizer == "mesos") {
> > ContainerInfo containerInfo;
> > containerInfo.set_type(ContainerInfo::MESOS);
> > ContainerInfo::MesosInfo mesosInfo;
> >
> > Image::Appc appc;
> >
> > appc.set_name(appcImage.get());
> >
> > // TODO(jojy): Labels are hard coded right now.
> > // Consider adding label flags for customization.
> > Label arch;
> > arch.set_key("arch");
> > arch.set_value("amd64");
> >
> > Label os;
> > os.set_key("os");
> > os.set_value("linux");
> >
> > Labels labels;
> > labels.add_labels()->CopyFrom(os);
> > labels.add_labels()->CopyFrom(arch);
> >
> > appc.mutable_labels()->CopyFrom(labels);
> >
> > Image mesosImage;
> >
> > mesosImage.set_type(Image::APPC);
> > mesosImage.mutable_appc()->CopyFrom(appc);
> >
> > mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >
> > containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> >
> > task.mutable_container()->CopyFrom(containerInfo);
> > }
>
> Jojy Varghese wrote:
> Then we will have to repeat the `if (containerizer == "mesos")` for both dockerImage case and appcImage case. By seperating the logic by cotainerizer, we now can add more image types into the "mesos" containerizer block (say OCI image).
Yes, that's also my concern when I open this issue, I think it is ok to keep your current logic.
The logic of checking `containerizer.empty()` should be removed?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123991
-----------------------------------------------------------
On 三月 17, 2016, 6:54 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated 三月 17, 2016, 6:54 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check this?
> >
> > What about make the logic as this? This will involve less code change and the logic may be more clear?
> >
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> > old logic here;
> > }
> >
> > if (appcImage.isSome() && containerizer == "mesos") {
> > ContainerInfo containerInfo;
> > containerInfo.set_type(ContainerInfo::MESOS);
> > ContainerInfo::MesosInfo mesosInfo;
> >
> > Image::Appc appc;
> >
> > appc.set_name(appcImage.get());
> >
> > // TODO(jojy): Labels are hard coded right now.
> > // Consider adding label flags for customization.
> > Label arch;
> > arch.set_key("arch");
> > arch.set_value("amd64");
> >
> > Label os;
> > os.set_key("os");
> > os.set_value("linux");
> >
> > Labels labels;
> > labels.add_labels()->CopyFrom(os);
> > labels.add_labels()->CopyFrom(arch);
> >
> > appc.mutable_labels()->CopyFrom(labels);
> >
> > Image mesosImage;
> >
> > mesosImage.set_type(Image::APPC);
> > mesosImage.mutable_appc()->CopyFrom(appc);
> >
> > mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >
> > containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> >
> > task.mutable_container()->CopyFrom(containerInfo);
> > }
>
> Jojy Varghese wrote:
> Then we will have to repeat the `if (containerizer == "mesos")` for both dockerImage case and appcImage case. By seperating the logic by cotainerizer, we now can add more image types into the "mesos" containerizer block (say OCI image).
>
> Guangya Liu wrote:
> Yes, that's also my concern when I open this issue, I think it is ok to keep your current logic.
>
> The logic of checking `containerizer.empty()` should be removed?
>
> Jojy Varghese wrote:
> We could remove that but the idea is that the block can be cleanly separated out now to a method (say `handleContainerizer`) that handles the containerizers.
cleaned up/refactored the flow to make it easy to follow.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123991
-----------------------------------------------------------
On March 18, 2016, 5:26 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 5:26 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check this?
> >
> > What about make the logic as this? This will involve less code change and the logic may be more clear?
> >
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> > old logic here;
> > }
> >
> > if (appcImage.isSome() && containerizer == "mesos") {
> > ContainerInfo containerInfo;
> > containerInfo.set_type(ContainerInfo::MESOS);
> > ContainerInfo::MesosInfo mesosInfo;
> >
> > Image::Appc appc;
> >
> > appc.set_name(appcImage.get());
> >
> > // TODO(jojy): Labels are hard coded right now.
> > // Consider adding label flags for customization.
> > Label arch;
> > arch.set_key("arch");
> > arch.set_value("amd64");
> >
> > Label os;
> > os.set_key("os");
> > os.set_value("linux");
> >
> > Labels labels;
> > labels.add_labels()->CopyFrom(os);
> > labels.add_labels()->CopyFrom(arch);
> >
> > appc.mutable_labels()->CopyFrom(labels);
> >
> > Image mesosImage;
> >
> > mesosImage.set_type(Image::APPC);
> > mesosImage.mutable_appc()->CopyFrom(appc);
> >
> > mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >
> > containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> >
> > task.mutable_container()->CopyFrom(containerInfo);
> > }
Then we will have to repeat the `if (containerizer == "mesos")` for both dockerImage case and appcImage case. By seperating the logic by cotainerizer, we now can add more image types into the "mesos" containerizer block (say OCI image).
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123991
-----------------------------------------------------------
On March 17, 2016, 6:54 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 17, 2016, 6:54 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123991
-----------------------------------------------------------
src/cli/execute.cpp (line 241)
<https://reviews.apache.org/r/44934/#comment186347>
The default value of `containerizer` is `mesos`, do we need to check this?
What about make the logic as this? This will involve less code change and the logic may be more clear?
// Do not touch old dockerImage logic.
if (dockerImage.isSome()) {
old logic here;
}
if (appcImage.isSome() && containerizer == "mesos") {
ContainerInfo containerInfo;
containerInfo.set_type(ContainerInfo::MESOS);
ContainerInfo::MesosInfo mesosInfo;
Image::Appc appc;
appc.set_name(appcImage.get());
// TODO(jojy): Labels are hard coded right now.
// Consider adding label flags for customization.
Label arch;
arch.set_key("arch");
arch.set_value("amd64");
Label os;
os.set_key("os");
os.set_value("linux");
Labels labels;
labels.add_labels()->CopyFrom(os);
labels.add_labels()->CopyFrom(arch);
appc.mutable_labels()->CopyFrom(labels);
Image mesosImage;
mesosImage.set_type(Image::APPC);
mesosImage.mutable_appc()->CopyFrom(appc);
mesosInfo.mutable_image()->CopyFrom(mesosImage);
containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
task.mutable_container()->CopyFrom(containerInfo);
}
- Guangya Liu
On 三月 17, 2016, 6:54 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated 三月 17, 2016, 6:54 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/
-----------------------------------------------------------
(Updated March 17, 2016, 6:54 a.m.)
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
Updated mesos-execute to add support for Appc.
Diffs (updated)
-----
src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
Diff: https://reviews.apache.org/r/44934/diff/
Testing
-------
Tested with various Appc images.
Thanks,
Jojy Varghese
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On March 17, 2016, 2:16 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, lines 251-281
> > <https://reviews.apache.org/r/44934/diff/1/?file=1301837#file1301837line251>
> >
> > What is the behavior of this if with mesos containerizer but without a docker/appc image?
> >
> > The current logic is a bit different from previous logic, the mesos execute will not construct the containerInfo if no docker image.
Thanks for the review ! Updated the patch.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123969
-----------------------------------------------------------
On March 16, 2016, 9:21 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated March 16, 2016, 9:21 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 44934: Updated mesos-execute to add support for
Appc.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123969
-----------------------------------------------------------
src/cli/execute.cpp (lines 250 - 280)
<https://reviews.apache.org/r/44934/#comment186298>
What is the behavior of this if with mesos containerizer but without a docker/appc image?
The current logic is a bit different from previous logic, the mesos execute will not construct the containerInfo if no docker image.
- Guangya Liu
On 三月 16, 2016, 9:21 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
>
> (Updated 三月 16, 2016, 9:21 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated mesos-execute to add support for Appc.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e
>
> Diff: https://reviews.apache.org/r/44934/diff/
>
>
> Testing
> -------
>
> Tested with various Appc images.
>
>
> Thanks,
>
> Jojy Varghese
>
>