Re: Review Request 57798: Fixed Java V1 Framework unit test for macOS.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57798]

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 March 21, 2017, 1:19 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57798/
> ---
> 
> (Updated March 21, 2017, 1:19 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Gilbert Song.
> 
> 
> Bugs: MESOS-7270
> https://issues.apache.org/jira/browse/MESOS-7270
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Java V1 Framework unit test for macOS. This is the same fix as in 
> https://github.com/apache/mesos/blob/master/src/examples/java/TestFramework.java#L265.
> 
> 
> Diffs
> -
> 
>   src/examples/java/V1TestFramework.java 
> df352ff08c66150483f5be3e52bce150becc7ab2 
> 
> 
> Diff: https://reviews.apache.org/r/57798/diff/1/
> 
> 
> Testing
> ---
> 
> Modified ExamplesTest.V1JavaFramework. Tested under macOS 10.12.3.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 57798: Fixed Java V1 Framework unit test for macOS.

[Chun-Hung Hsiao]

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

Review request for mesos, Anand Mazumdar and Gilbert Song.


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


Repository: mesos


Description
---

Fixed Java V1 Framework unit test for macOS. This is the same fix as in 
https://github.com/apache/mesos/blob/master/src/examples/java/TestFramework.java#L265.


Diffs
-

  src/examples/java/V1TestFramework.java 
df352ff08c66150483f5be3e52bce150becc7ab2 


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


Testing
---

Modified ExamplesTest.V1JavaFramework. Tested under macOS 10.12.3.


Thanks,

Chun-Hung Hsiao

Re: Review Request 57762: Fixed environment duplication in default executor.

[Joseph Wu]

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




src/launcher/executor.cpp
Line 477 (original), 483-487 (patched)


If you mean to remove duplicates, shouldn't you check for both `name` and 
`value` matching?


- Joseph Wu


On March 20, 2017, 8:59 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 20, 2017, 8:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/3/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> Before applying the patch;
> 
> Running 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> will result in the following `stdout` sandbox file:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> After applying the patch;
> 
> The resulting `stdout` sandbox file is as clean as it should be:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-\/executors\/test\/runs\/46f219a3-2772-4d21-89b8-85f1c1ba362e"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_CHECKPOINT","type":"VALUE","

Re: Review Request 54954: Improve the warning when failing to find the executor PID.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [54954]

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 Jan. 3, 2017, 5:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54954/
> ---
> 
> (Updated Jan. 3, 2017, 5:13 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When recovering the run state, if we don't find either the libprocess
> PID or the HTTP marker for the executor, that fact was being logged,
> but there was nothing to identify the executor or container. Add the
> framework, executor and container IDs to the message, and clarify that
> we can't find either marker.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp e64e89678b7d2ab32702edaf5575c92e41fae1e2 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> 
> Diff: https://reviews.apache.org/r/54954/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54954: Improve the warning when failing to find the executor PID.

[Anand Mazumdar]

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




src/slave/state.cpp
Line 545 (original), 546 (patched)


hmm, In this function and elsewhere in general, we return early when there 
is an error. This seems to do so for the "happy" cases too.

Can we keep the original not exists check but move the `LOG(WARNING)` 
inside the `if` conditional?

Something like:
`cpp
if (!os::exists(path)) {
  LOG(WARNING) << ...;
}
```



src/slave/state.cpp
Line 548 (original), 549-553 (patched)


Move this to L544 as per my earlier comment.

Also can you quote `executorId` since it's generated by the scheduler and 
can have spaces. AFAIK, We haven't been consistent with this in the agent code 
too and should do a sweep there too.


- Anand Mazumdar


On Jan. 3, 2017, 5:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54954/
> ---
> 
> (Updated Jan. 3, 2017, 5:13 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When recovering the run state, if we don't find either the libprocess
> PID or the HTTP marker for the executor, that fact was being logged,
> but there was nothing to identify the executor or container. Add the
> framework, executor and container IDs to the message, and clarify that
> we can't find either marker.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp e64e89678b7d2ab32702edaf5575c92e41fae1e2 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> 
> Diff: https://reviews.apache.org/r/54954/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56681: Use glog to log EXIT() messages.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56680, 56681]

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 March 20, 2017, 10:52 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56681/
> ---
> 
> (Updated March 20, 2017, 10:52 a.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Bugs: MESOS-7115
> https://issues.apache.org/jira/browse/MESOS-7115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use glog to log EXIT() messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/exit.hpp 
> 762864afd8f86f7e1f439ef6ea7e3965a5f147d5 
> 
> 
> Diff: https://reviews.apache.org/r/56681/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> **Example output after this change:**
> ```
> [jpeach@jpeach build]$ sudo ./src/mesos-agent --work_dir=/nothing/here 
> --master=phoney:5050
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> I0320 11:01:10.362782   921 main.cpp:368] Build: 2017-03-20 10:43:40 by jpeach
> I0320 11:01:10.362900   921 main.cpp:369] Version: 1.3.0
> I0320 11:01:10.362912   921 main.cpp:376] Git SHA: 
> b0d82241a886e9c6239ce82c5135c4c72c44aca7
> I0320 11:01:10.381340   921 systemd.cpp:238] systemd version `231` detected
> I0320 11:01:10.381404   921 main.cpp:478] Inializing systemd state
> I0320 11:01:10.390058   921 systemd.cpp:326] Started systemd slice 
> `mesos_executors.slice`
> I0320 11:01:10.392071   921 containerizer.cpp:221] Using isolation: 
> posix/cpu,posix/mem,filesystem/posix,network/cni
> I0320 11:01:10.401108   921 linux_launcher.cpp:150] Using 
> /sys/fs/cgroup/freezer as the freezer hierarchy for the Linux launcher
> I0320 11:01:10.402873   921 provisioner.cpp:249] Using default backend 
> 'overlay'
> E0320 11:01:10.426219   921 main.cpp:507] Failed to create a master detector: 
> Failed to parse 'phoney:5050'
> ```
> 
> **Example output before this change:**
> ```
> [jpeach@jpeach build]$ sudo /opt/mesos/sbin//mesos-agent 
> --work_dir=/nothing/here --master=phoney:5050
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> I0320 11:03:43.868015  1188 main.cpp:243] Build: 2017-03-10 10:49:55 by jpeach
> I0320 11:03:43.868135  1188 main.cpp:244] Version: 1.3.0
> I0320 11:03:43.868151  1188 main.cpp:251] Git SHA: 
> 487c667260ec89127e18c77b9e8c8c243cf6ec57
> I0320 11:03:43.886561  1188 systemd.cpp:238] systemd version `231` detected
> I0320 11:03:43.886631  1188 main.cpp:342] Inializing systemd state
> I0320 11:03:43.896606  1188 systemd.cpp:326] Started systemd slice 
> `mesos_executors.slice`
> I0320 11:03:43.899202  1188 containerizer.cpp:221] Using isolation: 
> posix/cpu,posix/mem,filesystem/posix,network/cni
> I0320 11:03:43.911183  1188 linux_launcher.cpp:150] Using 
> /sys/fs/cgroup/freezer as the freezer hierarchy for the Linux launcher
> I0320 11:03:43.913216  1188 provisioner.cpp:249] Using default backend 
> 'overlay'
> Failed to create a master detector: Failed to parse 'phoney:5050'
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57615: Added support for auth tokens to the V1 executor library.

[Greg Mann]

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




src/executor/executor.cpp
Lines 227 (patched)


MESOS_EXECUTOR_AUTHENTICATION_TOKEN ?


- Greg Mann


On March 16, 2017, 7:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57615/
> ---
> 
> (Updated March 16, 2017, 7:13 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jan Schlicht, and Vinod Kone.
> 
> 
> Bugs: MESOS-6304
> https://issues.apache.org/jira/browse/MESOS-6304
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for authentication tokens to the
> V1 default executor library. The tokens are loaded from a
> pre-determined environment variable, if present.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 35378ec4b55ce0427e95f6b868baa6233c9068cd 
> 
> 
> Diff: https://reviews.apache.org/r/57615/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Once MESOS-6999 is resolved, additional tests must be implemented to verify 
> that executor authentication is performed correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57667: Added executor authentication to the docs.

[Greg Mann]

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




docs/authentication.md
Line 92 (original), 92 (patched)


Add text to the Executor HTTP API docs


- Greg Mann


On March 16, 2017, 7:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57667/
> ---
> 
> (Updated March 16, 2017, 7:13 p.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6998
> https://issues.apache.org/jira/browse/MESOS-6998
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added executor authentication to the docs.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 
> 
> 
> Diff: https://reviews.apache.org/r/57667/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.

[Neil Conway]

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

(Updated March 20, 2017, 9:19 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Rebase, tweak comment.


Repository: mesos


Description
---

This commit replaces the sorter's flat list of clients with a tree of
client names; this tree represents the hierarchical relationship between
sorter clients. Each node in the tree contains an (ordered) list of
pointers to child nodes. The tree might contain nodes that do not
correspond directly to sorter clients. For example, adding clients "a/b"
and "c/d" results in the following tree:

root
 -> a
   -> b
 -> c
   -> d

The `sort` member function still only returns one result for each
(active) client in the sorter. This is implemented by ensuring that each
sorter client is associated with a leaf node in the tree. Note that it
is possible for a leaf node to be transformed into an internal node by a
subsequent insertion; to handle this case, we "implicitly" create an
extra child node, which maintains the invariant that each client has a
leaf node. For example, if the client "a/b/x" is added to the tree
above, the result is:

root
 -> a
   -> b
 -> .
 -> x
 -> c
   -> d

The "." leaf node holds the allocation that has been made to the "a/b"
client itself; the "a/b" node holds the sum of all the allocations that
have been made to the subtree rooted at "a/b", which also includes
"a/b/x".


Diffs (updated)
-

  src/master/allocator/sorter/drf/metrics.cpp 
15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/hierarchical_allocator_tests.cpp 
e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
  src/tests/master_allocator_tests.cpp 9f3750215f2b72f6148d0c47cdde6a3f7dfb1b50 
  src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 


Diff: https://reviews.apache.org/r/57254/diff/13/

Changes: https://reviews.apache.org/r/57254/diff/12-13/


Testing
---


Thanks,

Neil Conway

Re: Review Request 57527: Avoided storing weights in the allocator.

[Neil Conway]

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

(Updated March 20, 2017, 9:18 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Tweak comment.


Repository: mesos


Description
---

Previously, DRFSorter only kept track of weights for clients currently
in the sorter; the allocator supplied the current weight when adding a
client to the sorter.

This commit changes the sorter to keep track of all weights, not just
those for the active clients in the sorter. The allocator can now just
pass along role weights to the role sorters, rather than needing to
track them itself.

This commit changes the sorter API.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
f84b0574ce9a392c9528c87b04b01dbb2053cff7 
  src/master/allocator/mesos/hierarchical.cpp 
8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 


Diff: https://reviews.apache.org/r/57527/diff/5/

Changes: https://reviews.apache.org/r/57527/diff/4-5/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 57775: Ensured check's exit code is interpreted correctly on Windows.

[Joseph Wu]

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




src/checks/checker.cpp
Lines 440-443 (original), 440-448 (patched)


I'm guessing there's something in this chain that necessitates this change. 
 Can you explain?

At a glance, these macros should work on Windows:
```
#ifndef WIFEXITED
#define WIFEXITED(x) ((x) != -1)
#endif // WIFEXITED

#ifndef WEXITSTATUS
#define WEXITSTATUS(x) (x & 0xFF)
#endif // WEXITSTATUS
```



src/tests/check_tests.cpp
Lines 747-750 (patched)


I don't recognize this test (so I assume it's been added in this chain)...

Can you assign the TODO to yourself?  Or to me (`josephw`).


- Joseph Wu


On March 20, 2017, 10:02 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57775/
> ---
> 
> (Updated March 20, 2017, 10:02 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Gastón Kleiman, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
> 
> 
> Diff: https://reviews.apache.org/r/57775/diff/2/
> 
> 
> Testing
> ---
> 
> make check on Linux and Windows
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56681: Use glog to log EXIT() messages.

[Benjamin Mahler]

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



Have you seen the feedback 
[here](https://issues.apache.org/jira/browse/MESOS-3465?focusedCommentId=14944383&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14944383)?
 It would be nice to make it clear in the log that the program is exiting IMHO.

- Benjamin Mahler


On March 20, 2017, 5:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56681/
> ---
> 
> (Updated March 20, 2017, 5:52 p.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Bugs: MESOS-7115
> https://issues.apache.org/jira/browse/MESOS-7115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use glog to log EXIT() messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/exit.hpp 
> 762864afd8f86f7e1f439ef6ea7e3965a5f147d5 
> 
> 
> Diff: https://reviews.apache.org/r/56681/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> **Example output after this change:**
> ```
> [jpeach@jpeach build]$ sudo ./src/mesos-agent --work_dir=/nothing/here 
> --master=phoney:5050
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> I0320 11:01:10.362782   921 main.cpp:368] Build: 2017-03-20 10:43:40 by jpeach
> I0320 11:01:10.362900   921 main.cpp:369] Version: 1.3.0
> I0320 11:01:10.362912   921 main.cpp:376] Git SHA: 
> b0d82241a886e9c6239ce82c5135c4c72c44aca7
> I0320 11:01:10.381340   921 systemd.cpp:238] systemd version `231` detected
> I0320 11:01:10.381404   921 main.cpp:478] Inializing systemd state
> I0320 11:01:10.390058   921 systemd.cpp:326] Started systemd slice 
> `mesos_executors.slice`
> I0320 11:01:10.392071   921 containerizer.cpp:221] Using isolation: 
> posix/cpu,posix/mem,filesystem/posix,network/cni
> I0320 11:01:10.401108   921 linux_launcher.cpp:150] Using 
> /sys/fs/cgroup/freezer as the freezer hierarchy for the Linux launcher
> I0320 11:01:10.402873   921 provisioner.cpp:249] Using default backend 
> 'overlay'
> E0320 11:01:10.426219   921 main.cpp:507] Failed to create a master detector: 
> Failed to parse 'phoney:5050'
> ```
> 
> **Example output before this change:**
> ```
> [jpeach@jpeach build]$ sudo /opt/mesos/sbin//mesos-agent 
> --work_dir=/nothing/here --master=phoney:5050
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> I0320 11:03:43.868015  1188 main.cpp:243] Build: 2017-03-10 10:49:55 by jpeach
> I0320 11:03:43.868135  1188 main.cpp:244] Version: 1.3.0
> I0320 11:03:43.868151  1188 main.cpp:251] Git SHA: 
> 487c667260ec89127e18c77b9e8c8c243cf6ec57
> I0320 11:03:43.886561  1188 systemd.cpp:238] systemd version `231` detected
> I0320 11:03:43.886631  1188 main.cpp:342] Inializing systemd state
> I0320 11:03:43.896606  1188 systemd.cpp:326] Started systemd slice 
> `mesos_executors.slice`
> I0320 11:03:43.899202  1188 containerizer.cpp:221] Using isolation: 
> posix/cpu,posix/mem,filesystem/posix,network/cni
> I0320 11:03:43.911183  1188 linux_launcher.cpp:150] Using 
> /sys/fs/cgroup/freezer as the freezer hierarchy for the Linux launcher
> I0320 11:03:43.913216  1188 provisioner.cpp:249] Using default backend 
> 'overlay'
> Failed to create a master detector: Failed to parse 'phoney:5050'
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57527: Avoided storing weights in the allocator.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/allocator/sorter/sorter.hpp
Lines 74-75 (patched)


We should probably also be specific here about what happens when the client 
has not been added to the sorter because the previous semantics were to crash 
or drop calls on unknown clients.

Something like: the sorter will store this weight regardless of whether a 
client under this name has been added.


- Benjamin Mahler


On March 20, 2017, 6:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57527/
> ---
> 
> (Updated March 20, 2017, 6:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, DRFSorter only kept track of weights for clients currently
> in the sorter; the allocator supplied the current weight when adding a
> client to the sorter.
> 
> This commit changes the sorter to keep track of all weights, not just
> those for the active clients in the sorter. The allocator can now just
> pass along role weights to the role sorters, rather than needing to
> track them itself.
> 
> This commit changes the sorter API.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> f84b0574ce9a392c9528c87b04b01dbb2053cff7 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57527/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57784: Updated blog post section in the release guide.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57783, 57784]

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 March 20, 2017, 5:18 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57784/
> ---
> 
> (Updated March 20, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md f86f2f8092ccbac2442a152a588b0c03622cab29 
> 
> 
> Diff: https://reviews.apache.org/r/57784/diff/1/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57713: Added `virtualenv` as a dependency in `getting-started.md`.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 17, 2017, 5:19 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57713/
> ---
> 
> (Updated March 17, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joseph Wu, Michael Park, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-7255
> https://issues.apache.org/jira/browse/MESOS-7255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `virtualenv` as a dependency in `getting-started.md`.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 268eb29551c6cd9cbea50b2c971f97ca74bab18f 
>   docs/windows.md 38275f30638a0197b41f55b454b7a722bea24b3e 
> 
> 
> Diff: https://reviews.apache.org/r/57713/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57700: Modified a test to also check if the data was written to the volume.

[Gilbert Song]

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


Ship it!




Thanks for fixing it, Anand!

- Gilbert Song


On March 16, 2017, 11:49 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57700/
> ---
> 
> (Updated March 16, 2017, 11:49 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possbile that the directory exists but does not
> link to the persistent volume.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> b701e7d2f11af91a94d57db53ac2904e301996f1 
> 
> 
> Diff: https://reviews.apache.org/r/57700/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 57469: Made the default executor populate volume mappings for disk resources.

[Gilbert Song]

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


Fix it, then Ship it!




LGTM!


src/launcher/default_executor.cpp
Lines 344 (patched)


one more space after `foreach`?



src/tests/default_executor_tests.cpp
Lines 1633 (patched)


Curious that the `volume` is converted from `resource` to `resources` 
implicitly?


- Gilbert Song


On March 16, 2017, 11:49 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57469/
> ---
> 
> (Updated March 16, 2017, 11:49 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7225
> https://issues.apache.org/jira/browse/MESOS-7225
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A task can now access any volumes specified in disk resources from
> its own sandbox owing to the default executor populating the mapping
> from the child container sandbox to the parent. Previously, without
> such a mapping it was not possible for the child container to access
> those volumes mounted on the parent container.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
>   src/tests/default_executor_tests.cpp 
> b701e7d2f11af91a94d57db53ac2904e301996f1 
> 
> 
> Diff: https://reviews.apache.org/r/57469/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> The added test uses the new v1 helpers and ideally should not be committed 
> till we do a sweep of this file to update all the tests. That being said the 
> file is already inconsistent though.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 57664: Added the '--executor_secret_key' agent flag.

[Greg Mann]

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

(Updated March 20, 2017, 7:13 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Added a new agent flag, `--executor_secret_key` to allow the
specification of a secret key to be used when generating and
authenticating default executor tokens.


Diffs
-

  docs/configuration.md 9f747406e91840335f97c0166ca75373e47d319b 
  src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
  src/slave/flags.cpp 4637ca62b257921c1e1f7019864d968f07fca8a1 


Diff: https://reviews.apache.org/r/57664/diff/2/


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 57789: Updated the style guide with escape rules for IDs.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 20, 2017, 6:41 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57789/
> ---
> 
> (Updated March 20, 2017, 6:41 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 18f716b3dbcb6ae2538e437facc98c2befc73cf8 
> 
> 
> Diff: https://reviews.apache.org/r/57789/diff/1/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57762: Fixed environment duplication in default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57762]

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 March 20, 2017, 3:59 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 20, 2017, 3:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/3/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> Before applying the patch;
> 
> Running 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> will result in the following `stdout` sandbox file:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> After applying the patch;
> 
> The resulting `stdout` sandbox file is as clean as it should be:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-\/executors\/test\/runs\/46f219a3-2772-4d21-89b8-85f1c1ba362e"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_CHECKPOINT","type":"V

Review Request 57789: Updated the style guide with escape rules for IDs.

[Alexander Rukletsov]

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

Review request for mesos and Greg Mann.


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/c++-style-guide.md 18f716b3dbcb6ae2538e437facc98c2befc73cf8 


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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 57788: Changed allocator to skip batch allocate on weight change.

[Neil Conway]

> On March 20, 2017, 6:26 p.m., Benjamin Mahler wrote:
> > Hm.. seems like there was no test for this "optimization" if the tests 
> > pass, did you do a scan of the allocator unit tests to be sure we're not 
> > leaving a stale test lying around?

The change was introduced here, which didn't include any tests: 
https://reviews.apache.org/r/41597/


- Neil


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


On March 20, 2017, 6:14 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57788/
> ---
> 
> (Updated March 20, 2017, 6:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Benjamin Mahler, and 
> Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the weight is changed for one or more roles, the allocator will no
> longer do a batch allocation. The weight change will be reflected in the
> next allocation, which should be sufficient.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
> 
> 
> Diff: https://reviews.apache.org/r/57788/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57788: Changed allocator to skip batch allocate on weight change.

[Benjamin Mahler]

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


Ship it!




Hm.. seems like there was no test for this "optimization" if the tests pass, 
did you do a scan of the allocator unit tests to be sure we're not leaving a 
stale test lying around?

- Benjamin Mahler


On March 20, 2017, 6:14 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57788/
> ---
> 
> (Updated March 20, 2017, 6:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Benjamin Mahler, and 
> Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the weight is changed for one or more roles, the allocator will no
> longer do a batch allocation. The weight change will be reflected in the
> next allocation, which should be sufficient.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
> 
> 
> Diff: https://reviews.apache.org/r/57788/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57743: Updated the agent to generate executor secrets.

[Greg Mann]

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

(Updated March 20, 2017, 6:15 p.m.)


Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

This patch updates the agent code to generate executor
authentication tokens when executor authentication is
enabled. For now, the generated `Secret` objects must
be of `VALUE` type, and they're passed directly into the
executor environment.


Diffs (updated)
-

  src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb 
  src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b 


Diff: https://reviews.apache.org/r/57743/diff/2/

Changes: https://reviews.apache.org/r/57743/diff/1-2/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 57527: Avoided storing weights in the allocator.

[Neil Conway]

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

(Updated March 20, 2017, 6:15 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Rebase.


Repository: mesos


Description
---

Previously, DRFSorter only kept track of weights for clients currently
in the sorter; the allocator supplied the current weight when adding a
client to the sorter.

This commit changes the sorter to keep track of all weights, not just
those for the active clients in the sorter. The allocator can now just
pass along role weights to the role sorters, rather than needing to
track them itself.

This commit changes the sorter API.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
f84b0574ce9a392c9528c87b04b01dbb2053cff7 
  src/master/allocator/mesos/hierarchical.cpp 
8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57787: Factor out common validation in agent's 'run()' code path.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar, Alexander Rojas, Jie Yu, and Vinod 
Kone.


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


Repository: mesos


Description
---

This patch factors out code that is common to `Slave::_run()`
and `Slave::__run()` into a new member function helper,
`Slave::validateFrameworkAndTasks()`.


Diffs
-

  src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb 
  src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b 


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


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 57527: Avoided storing weights in the allocator.

[Neil Conway]

> On March 18, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1329-1355 (original), 1329-1337 (patched)
> > 
> >
> > Hm.. this seems somewhat snuck in to this patch. It's not clear if we 
> > should even be triggering an allocation here, my inclination is no, since 
> > the weight update will be reflected in the next allocation and there's 
> > nothing here that warrants an immediate allocation.
> > 
> > In any case, can you pull it out in front of this review?

Split this into https://reviews.apache.org/r/57788/ and removed the allocation.


> On March 18, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/sorter.hpp
> > Lines 74-78 (patched)
> > 
> >
> > I guess I was more curious as to why we don't support it, is it because 
> > we don't support removing unsetting weights from the mesos api? If so, wow 
> > :) and also, shouldn't we just support it here in anticipation? Seems 
> > pretty straightforward to me to just avoid this confusion:
> > 
> > ```
> >   virtual void setWeight(const std::string& client, double weight) = 0;
> >   virtual void unsetWeight(const std::string& client) = 0;
> > ```

Right -- we don't support it in the sorter because we don't support it in the 
allocator; we don't support it in the allocator because we don't allow users to 
delete/unset weights. I agree this should be improved (I've griped about it in 
the past :) -- I opened https://issues.apache.org/jira/browse/MESOS-7267 to 
track this.

My preference would be to do nothing for now -- if/when we add support for 
removing weights, I'd prefer to do it all at once then. Supporting removing 
weights in some places but not others seems more confusing to me.


- Neil


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


On March 17, 2017, 1:12 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57527/
> ---
> 
> (Updated March 17, 2017, 1:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, DRFSorter only kept track of weights for clients currently
> in the sorter; the allocator supplied the current weight when adding a
> client to the sorter.
> 
> This commit changes the sorter to keep track of all weights, not just
> those for the active clients in the sorter. The allocator can now just
> pass along role weights to the role sorters, rather than needing to
> track them itself.
> 
> This commit changes the sorter API.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> f84b0574ce9a392c9528c87b04b01dbb2053cff7 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57527/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 57788: Changed allocator to skip batch allocate on weight change.

[Neil Conway]

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

Review request for mesos, Adam B, Benjamin Bannier, Benjamin Mahler, and 
Michael Park.


Repository: mesos


Description
---

When the weight is changed for one or more roles, the allocator will no
longer do a batch allocation. The weight change will be reflected in the
next allocation, which should be sufficient.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 


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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56681: Use glog to log EXIT() messages.

[James Peach]

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

(Updated March 20, 2017, 5:52 p.m.)


Review request for mesos and haosdent huang.


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


Repository: mesos


Description
---

Use glog to log EXIT() messages.


Diffs (updated)
-

  3rdparty/stout/include/stout/exit.hpp 
762864afd8f86f7e1f439ef6ea7e3965a5f147d5 


Diff: https://reviews.apache.org/r/56681/diff/2/

Changes: https://reviews.apache.org/r/56681/diff/1-2/


Testing (updated)
---

make check (Fedora 25)

**Example output after this change:**
```
[jpeach@jpeach build]$ sudo ./src/mesos-agent --work_dir=/nothing/here 
--master=phoney:5050
WARNING: Logging before InitGoogleLogging() is written to STDERR
I0320 11:01:10.362782   921 main.cpp:368] Build: 2017-03-20 10:43:40 by jpeach
I0320 11:01:10.362900   921 main.cpp:369] Version: 1.3.0
I0320 11:01:10.362912   921 main.cpp:376] Git SHA: 
b0d82241a886e9c6239ce82c5135c4c72c44aca7
I0320 11:01:10.381340   921 systemd.cpp:238] systemd version `231` detected
I0320 11:01:10.381404   921 main.cpp:478] Inializing systemd state
I0320 11:01:10.390058   921 systemd.cpp:326] Started systemd slice 
`mesos_executors.slice`
I0320 11:01:10.392071   921 containerizer.cpp:221] Using isolation: 
posix/cpu,posix/mem,filesystem/posix,network/cni
I0320 11:01:10.401108   921 linux_launcher.cpp:150] Using 
/sys/fs/cgroup/freezer as the freezer hierarchy for the Linux launcher
I0320 11:01:10.402873   921 provisioner.cpp:249] Using default backend 'overlay'
E0320 11:01:10.426219   921 main.cpp:507] Failed to create a master detector: 
Failed to parse 'phoney:5050'
```

**Example output before this change:**
```
[jpeach@jpeach build]$ sudo /opt/mesos/sbin//mesos-agent 
--work_dir=/nothing/here --master=phoney:5050
WARNING: Logging before InitGoogleLogging() is written to STDERR
I0320 11:03:43.868015  1188 main.cpp:243] Build: 2017-03-10 10:49:55 by jpeach
I0320 11:03:43.868135  1188 main.cpp:244] Version: 1.3.0
I0320 11:03:43.868151  1188 main.cpp:251] Git SHA: 
487c667260ec89127e18c77b9e8c8c243cf6ec57
I0320 11:03:43.886561  1188 systemd.cpp:238] systemd version `231` detected
I0320 11:03:43.886631  1188 main.cpp:342] Inializing systemd state
I0320 11:03:43.896606  1188 systemd.cpp:326] Started systemd slice 
`mesos_executors.slice`
I0320 11:03:43.899202  1188 containerizer.cpp:221] Using isolation: 
posix/cpu,posix/mem,filesystem/posix,network/cni
I0320 11:03:43.911183  1188 linux_launcher.cpp:150] Using 
/sys/fs/cgroup/freezer as the freezer hierarchy for the Linux launcher
I0320 11:03:43.913216  1188 provisioner.cpp:249] Using default backend 'overlay'
Failed to create a master detector: Failed to parse 'phoney:5050'
```


Thanks,

James Peach

Re: Review Request 56680: Use the EXIT() macro more consistently in agent startup.

[James Peach]

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

(Updated March 20, 2017, 5:50 p.m.)


Review request for mesos and haosdent huang.


Summary (updated)
-

Use the EXIT() macro more consistently in agent startup.


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


Repository: mesos


Description (updated)
---

Use the EXIT() macro more consistently in agent startup.


Diffs (updated)
-

  src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b 


Diff: https://reviews.apache.org/r/56680/diff/2/

Changes: https://reviews.apache.org/r/56680/diff/1-2/


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 57474: Added test for authorization of hierarchical roles.

[Benjamin Bannier]

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



LGTM but for one non-functional issue.

I believe it might make sense to combine this into one change set with the 
previous patch since we e.g., likely wouldn't be interested in backporting. 
Maybe @adam-mesos has some idea.


src/tests/authorization_tests.cpp
Lines 1171 (patched)


The blocks you add here should probably be documented in the same fat style 
currently used in this file.

Here and elsewhere in this patch.


- Benjamin Bannier


On March 13, 2017, 4:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated March 13, 2017, 4:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 57784: Updated blog post section in the release guide.

[Alexander Rukletsov]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/release-guide.md f86f2f8092ccbac2442a152a588b0c03622cab29 


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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Review Request 57783: Updated website for 1.1.1 release.

[Alexander Rukletsov]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  site/data/releases.yml 4bffe4df60bd0c7da68b94c7d680569494931a04 
  site/source/blog/2017-03-14-mesos-1-1-1-released.md PRE-CREATION 


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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 57696: Overloaded `<<` for `CheckStatusInfo`.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/common/type_utils.cpp
Lines 478 (patched)


I'd remove the commas after the check types.


- Gastón Kleiman


On March 20, 2017, 11:56 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57696/
> ---
> 
> (Updated March 20, 2017, 11:56 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 62187432093b7a5172a831a8782a8097d42a441a 
>   include/mesos/v1/mesos.hpp f5e08790c8abf3df4ed780a929335abd1dc97db6 
>   src/common/type_utils.cpp 1fce5221906d4e17eaee181c4486b6bde148d8e7 
>   src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
>   src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 
>   src/v1/mesos.cpp 3d9281dcbf806fd5440b4a10bed5f99fc89f08ce 
> 
> 
> Diff: https://reviews.apache.org/r/57696/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57775: Ensured check's exit code is interpreted correctly on Windows.

[Gastón Kleiman]

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



LGTM, but I have no clue about Windows, so I defer the "Ship It!" to someone 
who knows a bit more about Windows. =)

- Gastón Kleiman


On March 20, 2017, 2:29 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57775/
> ---
> 
> (Updated March 20, 2017, 2:29 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
> 
> 
> Diff: https://reviews.apache.org/r/57775/diff/2/
> 
> 
> Testing
> ---
> 
> make check on Linux and Windows
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57775: Ensured check's exit code is interpreted correctly on Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57773, 56449, 57694, 57695, 56215, 56217, 57696, 56213, 
56218, 57774, 57775]

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 March 20, 2017, 2:29 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57775/
> ---
> 
> (Updated March 20, 2017, 2:29 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
> 
> 
> Diff: https://reviews.apache.org/r/57775/diff/2/
> 
> 
> Testing
> ---
> 
> make check on Linux and Windows
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57695: Kept TaskInfo beyond first scheduler ack in default executor.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp
Lines 221 (patched)


s/acknowledgement/status update acknowledgement/

I haven't tested this, but it seems like we will log a warning for each 
terminal status update? I can imagine that being a bit confusing.



src/launcher/default_executor.cpp
Line 274 (original), 288 (patched)


do you mean `WAIT_NESTED_CONTAINER` here?


- Gastón Kleiman


On March 20, 2017, 11:55 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57695/
> ---
> 
> (Updated March 20, 2017, 11:55 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.
> 
> 
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of maintaining a separate collection for unacknowledged tasks,
> we augment internal `Container` struct by the corresponding `TaskInfo`
> and `acknowledged` flag. This way we are still able to find all
> unacknowledged tasks (slightly less efficiently as before since now
> we have to iterate through all tasks), but also keep `TaskInfo`'s
> beyond receiving the first acknowledgement.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
> 
> 
> Diff: https://reviews.apache.org/r/57695/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57762: Fixed environment duplication in default executor.

[Till Toenshoff]

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

(Updated March 20, 2017, 3:59 p.m.)


Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
Joseph Wu.


Changes
---

adressed comments - thanks a bunch for the review Benjamin!


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


Repository: mesos


Description (updated)
---

Fixed environment duplication in default executor.


Diffs (updated)
-

  src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 


Diff: https://reviews.apache.org/r/57762/diff/3/

Changes: https://reviews.apache.org/r/57762/diff/2-3/


Testing
---

make check + functional testing..

Before applying the patch;

Running 
```
$ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' --command='sleep 
1000' --master=127.0.0.1:5050
```
will result in the following `stdout` sandbox file:
```
Received SUBSCRIBED event
Subscribed executor on lobomacpro2.fritz.box
Received LAUNCH event
Starting task test
/Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
--help="false" 
--launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
 
1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type":"
 
VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT_E
 
NCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
Forked command at 2376
Overwriting environment variable 'key1', original: 'value1', new: 'value1'
```

After applying the patch;

The resulting `stdout` sandbox file is as clean as it should be:
```
Received SUBSCRIBED event
Subscribed executor on lobomacpro2.fritz.box
Received LAUNCH event
Starting task test
/Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
--help="false" 
--launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
 
1000"},"environment":{"variables":[{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-\/executors\/test\/runs\/46f219a3-2772-4d21-89b8-85f1c1ba362e"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_S
 
ANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-\/executors\/test\/runs\/46f219a3-2772-4d21-89b8-85f1c1ba362e"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type":"VALUE","value":"5secs"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"DUALCASE"

Re: Review Request 57473: Added support for authorization of Hierachical roles.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/authorizer/local/authorizer.cpp
Lines 62-67 (patched)


I am still not convinced this abstraction is needed from looking at only at 
this patch. All it does is move one `string` comparison from `approved` time to 
ACL build time, but we still do a lot of string comparisons at `approved` time 
when matching. I also don't see a lot of potential for reuse as we already 
reuse a lot of code via `createHierarchicalACLs`.

I believe this could be replaced by a simple string check at approved time 
(possibly encapsulated in a function).



src/authorizer/local/authorizer.cpp
Lines 401-407 (patched)


Let's sort these alphanumerically.



src/authorizer/local/authorizer.cpp
Lines 607 (patched)


I believe we resolved this as `+1`.



src/authorizer/local/authorizer.cpp
Lines 612-632 (patched)


Let's sort these.



src/authorizer/local/authorizer.cpp
Lines 750 (patched)


`matches`.


- Benjamin Bannier


On March 17, 2017, 12:54 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> ---
> 
> (Updated March 17, 2017, 12:54 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
> 
> 
> Diff: https://reviews.apache.org/r/57473/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57694: Added a warning if default executor gets an unknown acknowledgement.

[Gastón Kleiman]

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




src/launcher/default_executor.cpp
Lines 202-203 (patched)


We might want to wrap the `uiid` in single quotes.


- Gastón Kleiman


On March 20, 2017, 11:54 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57694/
> ---
> 
> (Updated March 20, 2017, 11:54 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
> 
> 
> Diff: https://reviews.apache.org/r/57694/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57694: Added a warning if default executor gets an unknown acknowledgement.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On March 20, 2017, 11:54 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57694/
> ---
> 
> (Updated March 20, 2017, 11:54 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
> 
> 
> Diff: https://reviews.apache.org/r/57694/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56449: Moved health checker closer to container in default executor.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On March 20, 2017, 11:54 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> ---
> 
> (Updated March 20, 2017, 11:54 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.
> 
> 
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
> 
> 
> Diff: https://reviews.apache.org/r/56449/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57773: Improved logging on a fatal condition in command executor.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On March 20, 2017, 11:50 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57773/
> ---
> 
> (Updated March 20, 2017, 11:50 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 
> 
> 
> Diff: https://reviews.apache.org/r/57773/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57472: Removing deprecated ACLs `SetQuota` and `RemoveQuota`.

[Benjamin Bannier]

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




docs/upgrades.md
Line 46 (original), 46 (patched)


This should be `1.3.x`.


- Benjamin Bannier


On March 17, 2017, 12:09 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57472/
> ---
> 
> (Updated March 17, 2017, 12:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7259
> https://issues.apache.org/jira/browse/MESOS-7259
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The ACLs `SetQuota` and `RemoveQuota` where marked for deprecation
> more than six months ago, where they were replaced for the more
> convenient `UpdateQuota` ACL. The deprecation cycle for these actions
> is finally due while at the same time removing will make easier to
> implement the hierarchical role feature in a generic way.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 0a4e1dd52f9197151c5ef4318579ffa4e9bcf0ee 
>   docs/upgrades.md e7f95aadeee330b3a9dbafc0f716c13df1bc8252 
>   include/mesos/authorizer/acls.proto 
> e75e1879435f1c2bce47a86e9feebf9d051e969b 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67 
>   src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 
>   src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 
> 
> 
> Diff: https://reviews.apache.org/r/57472/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57775: Ensured check's exit code is interpreted correctly on Windows.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 2:29 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 


Diff: https://reviews.apache.org/r/57775/diff/2/

Changes: https://reviews.apache.org/r/57775/diff/1-2/


Testing
---

make check on Linux and Windows


Thanks,

Alexander Rukletsov

Re: Review Request 57774: Printed a note that docker executor does not support general checks.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 2:29 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/docker/executor.cpp b652a99340f6bbbef2dbdd3a6e10333f32762c0f 
  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 


Diff: https://reviews.apache.org/r/57774/diff/2/

Changes: https://reviews.apache.org/r/57774/diff/1-2/


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 56218: Added a check test for default executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 2:29 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 


Diff: https://reviews.apache.org/r/56218/diff/7/

Changes: https://reviews.apache.org/r/56218/diff/6-7/


Testing
---

Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
make check


Thanks,

Alexander Rukletsov

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 2:28 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
  src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 


Diff: https://reviews.apache.org/r/56213/diff/9/

Changes: https://reviews.apache.org/r/56213/diff/8-9/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 57762: Fixed environment duplication in default executor.

[Till Toenshoff]

> On March 20, 2017, 11:43 a.m., Benjamin Bannier wrote:
> > src/launcher/executor.cpp
> > Lines 476-477 (patched)
> > 
> >
> > `ValueSecret` seems to do the job of `Environment::Variable` which 
> > causes a lot of manual duplicate validation and conversions in this patch. 
> > I believe if you'd use a map of `(string, Environment::Variable)` the patch 
> > could be simplified, e.g., something along the lines of
> > 
> > hashmap environment;
> > 
> > foreachpair (const string& name, const string& value, 
> > os::environment()) {
> >   Environment::Variable variable;
> >   variable.set_name(name);
> >   variable.set_type(Environment::Variable::VALUE);
> >   variable.set_value(value);
> >   environment.put(name, variable);
> > }
> > 
> > if (taskEnvironment.isSome()) {
> >   foreach (
> >   const Environment::Variable& variable,
> >   taskEnvironment->variables()) {
> > environment[variable.name()] = variable;
> >   }
> > }
> > 
> > if (command.has_environment()) {
> >   foreach (
> >   const Environment::Variable& variable,
> >   command.environment().variables()) {
> > environment[variable.name()] = variable;
> >   }
> > }
> > 
> > Environment launchEnvironment;
> > foreachvalue (const Environment::Variable& value, environment) {
> >   launchEnvironment.add_variables()->CopyFrom(value);
> > }

Awesome Benjamin - this is a perfect simplification - thanks so much.


- Till


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


On March 20, 2017, 4:03 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 20, 2017, 4:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/2/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> Before applying the patch;
> 
> Running 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> will result in the following `stdout` sandbox file:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-

Re: Review Request 57762: Fixed environment duplication in default executor.

[Till Toenshoff]

> On March 20, 2017, 11:46 a.m., Benjamin Bannier wrote:
> > I believe we should also have a test for this functionality.

I will come up with one - its a TODO for now.


- Till


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


On March 20, 2017, 4:03 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 20, 2017, 4:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/2/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> Before applying the patch;
> 
> Running 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> will result in the following `stdout` sandbox file:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> After applying the patch;
> 
> The resulting `stdout` sandbox file is as clean as it should be:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-\/executors\/test\/runs\/46f219a3-2772-4d21-89b8-85f1c1ba362e"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-

Re: Review Request 57768: Added a test to check /roles endpoint of master includes quota info.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57767, 57768]

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 March 20, 2017, 8:55 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57768/
> ---
> 
> (Updated March 20, 2017, 8:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6995
> https://issues.apache.org/jira/browse/MESOS-6995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to check /roles endpoint of master includes quota info.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp 1d58d7b37be23d30a37d405c762b274e4e53b409 
> 
> 
> Diff: https://reviews.apache.org/r/57768/diff/1/
> 
> 
> Testing
> ---
> 
> added a test `RoleTest.RolesEndpointContainsQuota`
> 
> make check on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 57775: Ensured check's exit code is interpreted correctly on Windows.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 


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


Testing
---

make check on Linux and Windows


Thanks,

Alexander Rukletsov

Review Request 57774: Printed a note that docker executor does not support general checks.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/docker/executor.cpp b652a99340f6bbbef2dbdd3a6e10333f32762c0f 
  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 


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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 56218: Added a check test for default executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:57 a.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 


Diff: https://reviews.apache.org/r/56218/diff/6/

Changes: https://reviews.apache.org/r/56218/diff/5-6/


Testing
---

Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
make check


Thanks,

Alexander Rukletsov

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:56 a.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
  src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 


Diff: https://reviews.apache.org/r/56213/diff/8/

Changes: https://reviews.apache.org/r/56213/diff/7-8/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 57696: Overloaded `<<` for `CheckStatusInfo`.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:56 a.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/type_utils.hpp 62187432093b7a5172a831a8782a8097d42a441a 
  include/mesos/v1/mesos.hpp f5e08790c8abf3df4ed780a929335abd1dc97db6 
  src/common/type_utils.cpp 1fce5221906d4e17eaee181c4486b6bde148d8e7 
  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
  src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 
  src/v1/mesos.cpp 3d9281dcbf806fd5440b4a10bed5f99fc89f08ce 


Diff: https://reviews.apache.org/r/57696/diff/3/

Changes: https://reviews.apache.org/r/57696/diff/2-3/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56217: Added support for general checks to default executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:56 a.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 


Diff: https://reviews.apache.org/r/56217/diff/5/

Changes: https://reviews.apache.org/r/56217/diff/4-5/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56215: Reused previous task status to generate a new one in default executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:55 a.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Sometimes when a new task status update is generated in the executor,
we have to make sure specific data is duplicated from the previous
task status to, e.g., avoid shadowing of these data during
reconciliation. For instance, consider a check status being sent;
in this status update we must include the latest known health
information.

This patch also refactors `update()` routine into two separate calls:
`createTaskStatus()` which is responsible for creating a task status
from scratch and `forward()`, which is responsible for forwarding task
status updates to the agent.


Diffs (updated)
-

  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 


Diff: https://reviews.apache.org/r/56215/diff/7/

Changes: https://reviews.apache.org/r/56215/diff/6-7/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 57694: Added a warning if default executor gets an unknown acknowledgement.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:54 a.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 


Diff: https://reviews.apache.org/r/57694/diff/3/

Changes: https://reviews.apache.org/r/57694/diff/2-3/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 57695: Kept TaskInfo beyond first scheduler ack in default executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:55 a.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Instead of maintaining a separate collection for unacknowledged tasks,
we augment internal `Container` struct by the corresponding `TaskInfo`
and `acknowledged` flag. This way we are still able to find all
unacknowledged tasks (slightly less efficiently as before since now
we have to iterate through all tasks), but also keep `TaskInfo`'s
beyond receiving the first acknowledgement.


Diffs (updated)
-

  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 


Diff: https://reviews.apache.org/r/57695/diff/2/

Changes: https://reviews.apache.org/r/57695/diff/1-2/


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56449: Moved health checker closer to container in default executor.

[Alexander Rukletsov]

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

(Updated March 20, 2017, 11:54 a.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.


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


Repository: mesos


Description
---

With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.


Diffs (updated)
-

  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 


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

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 57773: Improved logging on a fatal condition in command executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/executor.cpp bd3c0cf6ae2329ed30d6ab4b2ab3e7a90a46 


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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 57762: Fixed environment duplication in default executor.

[Benjamin Bannier]

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



I believe we should also have a test for this functionality.

- Benjamin Bannier


On March 20, 2017, 5:03 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 20, 2017, 5:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/2/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> Before applying the patch;
> 
> Running 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> will result in the following `stdout` sandbox file:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> After applying the patch;
> 
> The resulting `stdout` sandbox file is as clean as it should be:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-\/executors\/test\/runs\/46f219a3-2772-4d21-89b8-85f1c1ba362e"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS
 
_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619

Re: Review Request 57763: Fixed environment overwrite warning to not leak possibly sensitive data.

[Benjamin Bannier]

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



We already discussed this offline, and I still (maybe even stronger) feel that 
cases of duplicate env vars with differing values should be errors, dispite the 
fact that in the current implementation Mesos makes sure that isolators' 
`prepare` are called in deterministic order (there is no guarantee on the 
interface level). We should strongly consider tightening the behavior here if 
we do not have to support the current behavior for e.g., backwards 
compatibility reasons.

Env vars potentially mix user input from `CommandInfo` with output from 
isolators configured by an operator. Conflicting values cause settings by 
either the user or an isolator to be silently dropped. The first case would 
provide a bad UX, while the second one could be a configuration bug or worse 
from incompatible isolators. Diagnosing either of these through non-fatal log 
messages seems to do to little. As a user it would not be enough to just 
manually make sure none of these non-fatal errors are reported in `stdout` (not 
even `stderr`) when developing the task since an administrator might later on 
introduce introduce isolators making use of already taken user variables which 
would now be be broken by the task's setup. As an operator I wouldn't even be 
able to see these messages a user might not care about since they are reported 
in the task's output, not e.g., the agent log.

It we'd just rejected tasks with conflicting env settings outright, the 
situation would improve for both users and operators. If we'd go that route I 
believe it would make sense to add duplicate detection to `validateEnvironment` 
and directly invoke it here.

- Benjamin Bannier


On March 20, 2017, 3:51 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57763/
> ---
> 
> (Updated March 20, 2017, 3:51 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7264
> https://issues.apache.org/jira/browse/MESOS-7264
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 8658525b00e78bed9227d6d400eae2cf25dd 
> 
> 
> Diff: https://reviews.apache.org/r/57763/diff/1/
> 
> 
> Testing
> ---
> 
> make check & functional testing...
> 
> ```
> ./src/mesos-execute --name="test" --env='{"key1":"value1"}' --command='sleep 
> 1000' --master=127.0.0.1:5050
> ```
> 
> Within the contents of `stdout` before applying this:
> ```
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> After applying this there is no mention of the actually duplicate but equally 
> valued variable anymore. Note, this is before applying 
> https://reviews.apache.org/r/57762.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57764: Removed containerizer flag logging to prevent leak of sensitive data.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/launcher/posix/executor.cpp
Line 146 (original), 146-147 (patched)


I personally would be confused by `[***]` which to me looks more special 
than needed. What about just `...`?


- Benjamin Bannier


On March 20, 2017, 4:15 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57764/
> ---
> 
> (Updated March 20, 2017, 4:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7265
> https://issues.apache.org/jira/browse/MESOS-7265
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.cpp 59e7c0c7737e96059f117899eb4d9e69f2c8369d 
> 
> 
> Diff: https://reviews.apache.org/r/57764/diff/1/
> 
> 
> Testing
> ---
> 
> make check & functional testing...
> 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> 
> `stdout` before applying this:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> after applying this:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> Running /Users/till/Development/mesos-private/build/src/mesos-containerizer 
> launch [***]
> Forked command at 21513
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57762: Fixed environment duplication in default executor.

[Benjamin Bannier]

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




src/launcher/executor.cpp
Lines 476-477 (patched)


`ValueSecret` seems to do the job of `Environment::Variable` which causes a 
lot of manual duplicate validation and conversions in this patch. I believe if 
you'd use a map of `(string, Environment::Variable)` the patch could be 
simplified, e.g., something along the lines of

hashmap environment;

foreachpair (const string& name, const string& value, 
os::environment()) {
  Environment::Variable variable;
  variable.set_name(name);
  variable.set_type(Environment::Variable::VALUE);
  variable.set_value(value);
  environment.put(name, variable);
}

if (taskEnvironment.isSome()) {
  foreach (
  const Environment::Variable& variable,
  taskEnvironment->variables()) {
environment[variable.name()] = variable;
  }
}

if (command.has_environment()) {
  foreach (
  const Environment::Variable& variable,
  command.environment().variables()) {
environment[variable.name()] = variable;
  }
}

Environment launchEnvironment;
foreachvalue (const Environment::Variable& value, environment) {
  launchEnvironment.add_variables()->CopyFrom(value);
}


- Benjamin Bannier


On March 20, 2017, 5:03 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 20, 2017, 5:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bd3c0cf 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/2/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> Before applying the patch;
> 
> Running 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> will result in the following `stdout` sandbox file:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'ke

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

> On March 14, 2017, 6:24 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/check_tests.cpp
> > Lines 74 (patched)
> > 
> >
> > As the resident PowerShell "expert" (heh!), I tested this (and the 
> > second one) and they do what you're expecting.
> > 
> > Just a couple notes:
> > 
> > You might be wanting to use `Test-Path "somepath"` instead, which 
> > returns a boolean for existence of that path, so you can do `if (Test-Path 
> > "somepath")` instead of the gunk around checking `$?`. Also, semicolons are 
> > unnecssary in PowerShell except to separate multiple commands on one line. 
> > If you want it to be _perfect_ we try to prefer the `Camel-Case` of cmdlet 
> > names, but it's whatever.
> 
> Vinod Kone wrote:
> Thanks Andrew for the reply.
> 
> @Alex Do you want to update accordingly?

Sure.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/56213/diff/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56218: Added a check test for default executor.

[Alexander Rukletsov]

> On March 17, 2017, 11:07 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 854 (patched)
> > 
> >
> > can you add a TODO to cleanup this file as follows:
> > 
> > 1) Move variables closer to their usage
> > 2) Do not use evolve/devolve but use v1 variables/helpers wherever 
> > possible
> > 
> > please take a look at default_executor_tests.cpp for an example.

I've decided not to add a TODO, but cleaned up things now.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
> 
> 
> Diff: https://reviews.apache.org/r/56218/diff/5/
> 
> 
> Testing
> ---
> 
> Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57765: Remove and Update ARP and ICMP filters using handle.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57765]

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 March 20, 2017, 6:20 a.m., Santhosh Kumar Shanmugham wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57765/
> ---
> 
> (Updated March 20, 2017, 6:20 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7266
> https://issues.apache.org/jira/browse/MESOS-7266
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make the port_mapping isolator play nice with external
> components by updating and removing filters that are only
> created by Mesos. This is achieved by finding the handle
> of the tc filters and using it when updating/removing
> filters to limit the scope of the change.
> 
> This change will need an updated libnl
> which has been extended to provide support for querying
> `rtnl_act` objects.
> 
> Note: This change needs an update to the libnl library,
> which is being worked on here - 
> https://github.com/thom311/libnl/commit/9a1a71039439e81dba4c05b81eb061d632c2e7c7
> I will update the notes when the libnl change is committed and released.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/filter/action.hpp 
> 3e1e7016aa643fc4dbbdd0f67be471908f4573c8 
>   src/linux/routing/filter/basic.hpp 67d99a48a0d32b6bb573384ed9f370c6c24615f9 
>   src/linux/routing/filter/basic.cpp c0e0cbb38bd3e76f7f0cab03b5db674e13ec660d 
>   src/linux/routing/filter/icmp.hpp bc0aed0f4fc7c48f06cece14aa9ec43d7fa36bc4 
>   src/linux/routing/filter/icmp.cpp 68a1c3486e3fceb7110769240fbca97e3325ef31 
>   src/linux/routing/filter/internal.hpp 
> dc4b8f9f97a07411ac5697cb7f04282f87507034 
>   src/linux/routing/filter/ip.cpp 7283008c99f3cfbd43a851cf21f7c9523639cd99 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> dfa71fb6fe867c2f34451ca9f3e7f7f62402133c 
>   src/tests/containerizer/routing_tests.cpp 
> d05b3b11dc5fcd54d956990f252509865168e6b6 
> 
> 
> Diff: https://reviews.apache.org/r/57765/diff/1/
> 
> 
> Testing
> ---
> 
> TBD
> 
> 
> Thanks,
> 
> Santhosh Kumar Shanmugham
> 
>

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

> On Feb. 15, 2017, 1:41 p.m., Gastón Kleiman wrote:
> > src/tests/check_tests.cpp
> > Lines 620-625 (patched)
> > 
> >
> > This has the potential of being flaky on a very busy box.
> > 
> > A more robust approach could set the grace period to MAX_INT, and make 
> > the health check pass iff a file created by the check exists.

Or does not depend on which update comes first. I'll add a TODO for now, which 
we can fix once we actually see this test being flaky.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/56213/diff/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56218: Added a check test for default executor.

[Alexander Rukletsov]

> On March 17, 2017, 11:07 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 846-848 (patched)
> > 
> >
> > dont you want to do these tests for HTTP as well?

I think doing the same for HTTP would be redundant: shadowing, timing out is 
not related to the check type. However, in order to ensure these things are 
indeed unrealated, we should better introduce a mocked checker.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
> 
> 
> Diff: https://reviews.apache.org/r/56218/diff/5/
> 
> 
> Testing
> ---
> 
> Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 62-63 (patched)
> > 
> >
> > i don't see tests for these in this review. if they are added in the 
> > latter part of the chain, please don't mention them here. or add a TODO in 
> > front of them.

Moved them to the next review.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/56213/diff/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 73 (patched)
> > 
> >
> > Any particular reason for using v1 scheduler API? The general 
> > recommendation has been to use v0 scheduler API unless you are testing v1 
> > features. The main reason being most users are still using v0 scheduler API.
> 
> Alexander Rukletsov wrote:
> My original intention was to use unversioned HTTP API. However, I didn't 
> figure out how to do this and I suspect that it is not even possible. Is it 
> correct, that there are two choices? 
> 1) use older driver-based API
> 2) use newer v1 HTTP API
> 
> Is the recommendation then to use older driver-based API in tests?
> 
> Vinod Kone wrote:
> yes, the recommendation is to use v0 scheduler API unless either the rest 
> of the tests in the file are all using v1 scheduler API or you are testing v1 
> scheduler API functionality.

In this file, we will be testing default executor as well, which only supports 
v1. Hence, for consistency, I decided to use V1 for all the tests in the file.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/56213/diff/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

> On Feb. 10, 2017, 2:42 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Lines 177 (patched)
> > 
> >
> > s/agent/slave/ 
> > 
> > here and everywhere else.
> > 
> > we are not doing this change yet.
> 
> Gastón Kleiman wrote:
> We do this in `health_check_tests.cpp`:
> 
> ```
> $ grep "agent = " health_check_tests.cpp | wc -l
>   15
> $ grep "slave = " health_check_tests.cpp | wc -l
>0
> $
> ```
> 
> Vinod Kone wrote:
> I think we should fix `health_check_tests.cpp` as well then.
> 
> Gastón Kleiman wrote:
> I used `health_check_tests.cpp` just as an example, but we're using this 
> in a lot of places:
> 
> ```
> $ git grep "agent[^ ]* =" | cut -d: -f1 | sort | uniq
> api_tests.cpp
> authentication_tests.cpp
> containerizer/cgroups_isolator_tests.cpp
> containerizer/docker_containerizer_tests.cpp
> containerizer/io_switchboard_tests.cpp
> containerizer/memory_pressure_tests.cpp
> fault_tolerance_tests.cpp
> health_check_tests.cpp
> hierarchical_allocator_tests.cpp
> hook_tests.cpp
> master_allocator_tests.cpp
> master_tests.cpp
> master_validation_tests.cpp
> mesos.cpp
> mesos.hpp
> metrics_tests.cpp
> oversubscription_tests.cpp
> partition_tests.cpp
> reconciliation_tests.cpp
> reservation_endpoints_tests.cpp
> reservation_tests.cpp
> slave_authorization_tests.cpp
> slave_recovery_tests.cpp
> slave_tests.cpp
> sorter_tests.cpp
> $ git grep "agent[^ ]* =" | wc -l
>  207
> $
> ```
> 
> Alexander Rukletsov wrote:
> The policy I'm sticking to is to use `agent` in all new or being modified 
> non-user facing code. I'd suggest we keep `agent` here and update in other 
> places as we go.
> 
> Vinod Kone wrote:
> I spot checked some of these files, and looks like the code has been 
> updated recently to rename `slaveFlags` to `agentFlags` (which I suspect is 
> what the grep above is capturing) which is unfortunate. I think this arose 
> from a single commit that Alex Clemmer did.
> 
> It's my fault to not clearly articulate on dev list how phase 2 of slave 
> to agent rename should be done. But the policy that I've been suggesting to 
> my reviwees, is to not make a file locally inconsistent. IOW, if the rest of 
> the file is calling it `agent` use `agent`, otherwise stick to `slave`. I 
> think this applies to any such change not just slave to agent rename. The 
> reason being, a new dev coming to a file and adding a new test, would be very 
> confused if a file has a mix of old and new style.
> 
> So, if you really want to use `agent` here, I recommend to do a sweep to 
> update this file to do it across all tests (in a dependent review of course).
> 
> Does that make sense?

This is a new file and it only uses `agent*` for variable names. Hence I 
suggest we keep it as is without renaming everything back to `slave*`.


- Alexander


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


On March 16, 2017, 4:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56213/
> ---
> 
> (Updated March 16, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/56213/diff/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 57768: Added a test to check /roles endpoint of master includes quota info.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added a test to check /roles endpoint of master includes quota info.


Diffs
-

  src/tests/role_tests.cpp 1d58d7b37be23d30a37d405c762b274e4e53b409 


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


Testing
---

added a test `RoleTest.RolesEndpointContainsQuota`

make check on Ubuntu 14.04


Thanks,

Jay Guo

Review Request 57767: Added quota information to /roles endpoint of master.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added quota information to /roles endpoint of master.


Diffs
-

  src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3 
  src/common/http.cpp ce32ff36ee58b19f2cb11d80e69ab1ff007e75ef 
  src/master/http.cpp 862b68ff2e8846d6fb968556a6612ec7dab2a61a 


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


Testing
---

see next review in this chain


Thanks,

Jay Guo

Re: Review Request 56680: Apply glog to agent exit messages.

[Jiang Yan Xu]

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



Thanks for the cleanup for these. I think we are currently being inconsistent 
in the usage of CHECK/LOG(FATAL) vs EXIT, especially in handling local 
filesystem operation failures. This is mostly due to historical reaons I think: 
we used to just simply CHECK_SOME everywhere and later changed individual 
places / started to use EXIT for local filesystem failures.

To keep the rules simple we should probably consistently use EXIT when we 
expect that things can possibly fail and only use CHECK/LOG(FATAL) when it's 
unexpected and result of local logic and thus would benefit from the backtrace 
in investigation.

For filesystem operations when we change something for consistency, let's 
change the ones with CHECK/CHECK_SOME to EXIT.


src/slave/slave.cpp
Lines 248-250 (original), 248-251 (patched)


In places like this where we replace EXIT with a LOG(ERROR) and exit, I 
feel we don't need it anymore if we have /r/56681/?



src/slave/slave.cpp
Lines 413-414 (original), 427-429 (patched)


Ditto (all occurrences above)



src/slave/slave.cpp
Lines 418-419 (original), 433-439 (patched)


As I mentioned in the review header, it's probably good to use EXIT.

So +1 on this.



src/slave/slave.cpp
Lines 423-424 (original), 443-445 (patched)


Keep EXIT.



src/slave/slave.cpp
Lines 442-444 (original), 463-466 (patched)


Ditto.



src/slave/slave.cpp
Lines 496-497 (original), 522-524 (patched)


Ditto here and occurrences above.



src/slave/slave.cpp
Line 514 (original), 540-541 (patched)


+1 on replacing LOG(FATAL) with EXIT but s/EXIT_SUCCESS/EXIT_FAILURE/ ?



src/slave/slave.cpp
Lines 773-775 (original), 800-803 (patched)


Keep EXIT.



src/slave/slave.cpp
Lines 788-789 (original), 817-819 (patched)


Ditto here and the occurrence above.



src/slave/slave.cpp
Line 914 (original), 944 (patched)


External conditions can result in this failure (which doesn't suggest 
anything unexpected) so EXIT makes more sense.



src/slave/slave.cpp
Lines 1013-1015 (original), 1043-1045 (patched)


Module creation could fail due to incorrect config EXIT with the error 
message makes more sense.



src/slave/slave.cpp
Line 1084 (original), 1114 (patched)


Keep EXIT.



src/slave/slave.cpp
Line 1182 (original), 1212 (patched)


Keep EXIT. This situation although unexpected, is likely not a local error 
that could benefit from the stacktrace.



src/slave/slave.cpp
Line 1229 (original), 1259 (patched)


Ditto.



src/slave/slave.cpp
Lines 2923-2940 (original), 2953-2964 (patched)


Instead of changing EXIT to CHECK_SOME, we could change CHECK_SOME to EXIT. 
The practice of using of `CHECK_SOME` on checkpoints is before started to use 
EXIT for more graceful exit messages. Inconsistency does exist for this case 
elsewhere but to make this method more consistency let's follow the general 
principle of not printing backtrace in cases where failures are due to external 
conditions.



src/slave/slave.cpp
Lines 5546-5552 (original), 5570-5577 (patched)


Keep EXIT.


- Jiang Yan Xu


On Feb. 14, 2017, 1:05 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56680/
> ---
> 
> (Updated Feb. 14, 2017, 1:05 p.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Bugs: MESOS-7115
> https://issues.apache.org/jira/browse/MESOS-7115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> EXIT(...) doesn't format messages like glog so it's less easy to
> tell where a message came from and when it was emitted. For agent all
> initialization messages, perform a LOG(ERROR) followe by an exit. For
> other agent exit scenarios, use LOG(FATAL) or the equivalent CHECK().
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ebba8e16bc9ec45781183

Re: Review Request 57764: Removed containerizer flag logging to prevent leak of sensitive data.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57764]

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 March 20, 2017, 3:15 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57764/
> ---
> 
> (Updated March 20, 2017, 3:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7265
> https://issues.apache.org/jira/browse/MESOS-7265
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.cpp 59e7c0c7737e96059f117899eb4d9e69f2c8369d 
> 
> 
> Diff: https://reviews.apache.org/r/57764/diff/1/
> 
> 
> Testing
> ---
> 
> make check & functional testing...
> 
> ```
> $ ./src/mesos-execute --name="test" --env='{"key1":"value1"}' 
> --command='sleep 1000' --master=127.0.0.1:5050
> ```
> 
> `stdout` before applying this:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> /Users/till/Development/mesos-private/build/src/mesos-containerizer launch 
> --help="false" 
> --launch_info="{"command":{"environment":{"variables":[{"name":"key1","type":"VALUE","value":"value1"}]},"shell":true,"value":"sleep
>  
> 1000"},"environment":{"variables":[{"name":"BIN_SH","type":"VALUE","value":"xpg4"},{"name":"DUALCASE","type":"VALUE","value":"1"},{"name":"DYLD_LIBRARY_PATH","type":"VALUE","value":"\/Users\/till\/Development\/mesos-private\/build\/src\/.libs"},{"name":"LIBPROCESS_PORT","type":"VALUE","value":"0"},{"name":"MESOS_AGENT_ENDPOINT","type":"VALUE","value":"192.168.178.20:5051"},{"name":"MESOS_CHECKPOINT","type":"VALUE","value":"0"},{"name":"MESOS_DIRECTORY","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_EXECUTOR_ID","type":"VALUE","value":"test"},{"name":"MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD","type"
 
:"VALUE","value":"5secs"},{"name":"MESOS_FRAMEWORK_ID","type":"VALUE","value":"c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-"},{"name":"MESOS_HTTP_COMMAND_EXECUTOR","type":"VALUE","value":"0"},{"name":"MESOS_SANDBOX","type":"VALUE","value":"\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"MESOS_SLAVE_ID","type":"VALUE","value":"816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0"},{"name":"MESOS_SLAVE_PID","type":"VALUE","value":"slave(1)@192.168.178.20:5051"},{"name":"PATH","type":"VALUE","value":"\/usr\/local\/sbin:\/usr\/local\/bin:\/usr\/sbin:\/usr\/bin:\/sbin:\/bin"},{"name":"PWD","type":"VALUE","value":"\/private\/tmp\/mesos\/slaves\/816619b6-f5ce-42d6-ad6b-2ef2001adc0a-S0\/frameworks\/c84f9ee5-8a6b-4f6c-9fc0-154bceb3c615-\/executors\/test\/runs\/025c1d1a-6886-4b42-9313-0b1a411d5e39"},{"name":"SHLVL","type":"VALUE","value":"0"},{"name":"__CF_USER_TEXT
 
_ENCODING","type":"VALUE","value":"0x1F5:0x0:0x0"},{"name":"key1","type":"VALUE","value":"value1"},{"name":"key1","type":"VALUE","value":"value1"}]}}"
> Forked command at 2376
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> after applying this:
> ```
> Received SUBSCRIBED event
> Subscribed executor on lobomacpro2.fritz.box
> Received LAUNCH event
> Starting task test
> Running /Users/till/Development/mesos-private/build/src/mesos-containerizer 
> launch [***]
> Forked command at 21513
> Overwriting environment variable 'key1', original: 'value1', new: 'value1'
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>