Re: Review Request 44934: Updated mesos-execute to add support for Appc.

[Mesos ReviewBot]

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

[Jojy Varghese]

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

[Jojy Varghese]

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

[Tom Runyon]

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

[haosdent huang]

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

[Jojy Varghese]

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

[Jojy Varghese]

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

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123991
---




src/cli/execute.cpp (line 241)


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.

[Jojy Varghese]

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

[Jojy Varghese]

> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > 
> >
> > 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.

[Jojy Varghese]

> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > 
> >
> > 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.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review123969
---




src/cli/execute.cpp (lines 250 - 280)


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

Re: Review Request 44934: Updated mesos-execute to add support for Appc.

[Jojy Varghese]

> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > 
> >
> > 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.

[Jojy Varghese]

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

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44934/#review124370
---




src/cli/execute.cpp (line 317)


cerr << containerInfo.error() << endl;



src/cli/execute.cpp (line 517)


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.

[Jojy Varghese]

> On March 17, 2016, 2:16 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, lines 251-281
> > 
> >
> > 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
> 
>