Re: Review Request 62518: Fixed default executor handling of nested container status.

[James Peach]

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

(Updated Oct. 12, 2017, 3:57 p.m.)


Review request for mesos, Andrei Budnik and Anand Mazumdar.


Bugs: MESOS-8080
https://issues.apache.org/jira/browse/MESOS-8080


Repository: mesos


Description
---

The default executor was not handling a missing nested container
exit status correctly. It was assuming the protobuf accessor was
returning an Option rather than explicitly checking whether the
`exit_status` field was present in the message.

Added the explicit check for the `exit_status` field, and always
propagated an appropriate message into the status update, even
when the `exit_status` is absent. Added some documentation of
the `exit_status` field to the protobuf definition files.


Diffs
-

  include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
  include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
  src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 


Diff: https://reviews.apache.org/r/62518/diff/4/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Andrei Budnik]

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


Ship it!




Ship It!

- Andrei Budnik


On Sept. 25, 2017, 3:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 25, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Sept. 25, 2017, 11:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 25, 2017, 11:21 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 62518 was successfully built and tested.

Reviews applied: `['62518']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62518

- Mesos Reviewbot Windows


On Sept. 25, 2017, 3:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 25, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62518]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Sept. 25, 2017, 8:51 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 25, 2017, 8:51 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[James Peach]

> On Sept. 25, 2017, 12:42 a.m., Qian Zhang wrote:
> > Why do we need to change those .py files? I do not see you mentioned it in 
> > the commit message.

AFAICT `post-reviews` gets confused if you `git fetch` without doing a `git 
pull` and you end up including spurious changes. Removed them.


- James


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


On Sept. 22, 2017, 11:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 22, 2017, 11:22 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/3/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[James Peach]

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

(Updated Sept. 25, 2017, 3:21 p.m.)


Review request for mesos, Andrei Budnik and Anand Mazumdar.


Repository: mesos


Description
---

The default executor was not handling a missing nested container
exit status correctly. It was assuming the protobuf accessor was
returning an Option rather than explicitly checking whether the
`exit_status` field was present in the message.

Added the explicit check for the `exit_status` field, and always
propagated an appropriate message into the status update, even
when the `exit_status` is absent. Added some documentation of
the `exit_status` field to the protobuf definition files.


Diffs (updated)
-

  include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
  include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
  src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 


Diff: https://reviews.apache.org/r/62518/diff/4/

Changes: https://reviews.apache.org/r/62518/diff/3-4/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Qian Zhang]

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



Why do we need to change those .py files? I do not see you mentioned it in the 
commit message.


include/mesos/agent/agent.proto
Lines 337 (patched)


Kill the "be".



include/mesos/v1/agent/agent.proto
Lines 337 (patched)


Dito.



src/launcher/default_executor.cpp
Line 822 (original), 820-824 (patched)


Can we change these code to the below? And then we do not need to set 
`taskState` to `TASK_FAILED` when declaring it in the line 800.
```
} else {
  taskState = TASK_FAILED;
  message = "Command terminated with unknown status";
}
```


- Qian Zhang


On Sept. 23, 2017, 7:22 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 23, 2017, 7:22 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
>   src/python/cli_new/lib/cli/plugins/base.py 
> e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c 
>   src/python/cli_new/lib/cli/tests/base.py 
> 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9 
>   src/python/cli_new/lib/cli/tests/constants.py 
> 1398701b167eae397730afc5b1fab6a21e723266 
>   src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e 
>   support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62518]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Sept. 22, 2017, 11:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 22, 2017, 11:22 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
>   src/python/cli_new/lib/cli/plugins/base.py 
> e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c 
>   src/python/cli_new/lib/cli/tests/base.py 
> 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9 
>   src/python/cli_new/lib/cli/tests/constants.py 
> 1398701b167eae397730afc5b1fab6a21e723266 
>   src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e 
>   support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62518: Fixed default executor handling of nested container status.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 62518 was successfully built and tested.

Reviews applied: `['62518']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62518

- Mesos Reviewbot Windows


On Sept. 22, 2017, 11:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> ---
> 
> (Updated Sept. 22, 2017, 11:22 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
> 
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
>   src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
>   src/python/cli_new/lib/cli/plugins/base.py 
> e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c 
>   src/python/cli_new/lib/cli/tests/base.py 
> 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9 
>   src/python/cli_new/lib/cli/tests/constants.py 
> 1398701b167eae397730afc5b1fab6a21e723266 
>   src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e 
>   support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb 
> 
> 
> Diff: https://reviews.apache.org/r/62518/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 62518: Fixed default executor handling of nested container status.

[James Peach]

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

Review request for mesos, Andrei Budnik and Anand Mazumdar.


Repository: mesos


Description
---

The default executor was not handling a missing nested container
exit status correctly. It was assuming the protobuf accessor was
returning an Option rather than explicitly checking whether the
`exit_status` field was present in the message.

Added the explicit check for the `exit_status` field, and always
propagated an appropriate message into the status update, even
when the `exit_status` is absent. Added some documentation of
the `exit_status` field to the protobuf definition files.


Diffs
-

  include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
  include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
  src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0 
  src/python/cli_new/lib/cli/plugins/base.py 
e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c 
  src/python/cli_new/lib/cli/tests/base.py 
33c67c1161d9c9fd57aa5f44b04746e3740e1dd9 
  src/python/cli_new/lib/cli/tests/constants.py 
1398701b167eae397730afc5b1fab6a21e723266 
  src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e 
  support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb 


Diff: https://reviews.apache.org/r/62518/diff/1/


Testing
---

make check (Fedora 26)


Thanks,

James Peach