Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On March 26, 2015, 4:32 p.m., Alexander Rukletsov wrote: 3rdparty/libprocess/src/process.cpp, line 6 https://reviews.apache.org/r/30032/diff/4/?file=834184#file834184line6 One thing captures my attention is how we include C headers. AFAIK, the standard requires to include them like ``` #include ctime #include climits #include cstdio ``` and so on. Could you please create a cleanup newbie JIRA for this? Alexander Rojas wrote: Not very sure about that, what it does by using the c… versions of the headers is to put its functions in the std namespace. Check the C++11 Standard annex *D.6 C standard library headers*: 2 Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3). 3 Example: The header cstdlib assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header stdlib.h assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. —end example. As it reads, using functions from the `cname` versions of the `name.h` headers, may or may not (stdlib implementation specific) require to add the namespace std too all functions and structs provided by the header. Seems we got three options here; - start using the C++ wrappers and allowed for a smooth transition (file by file as needed/touched) - - not sure what the implications here are from an optimizing (linker) point of view - global cleanup and `std` namespace added everywhere - stick with the C variants May be worth a dev-list discussion? - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review77914 --- On March 26, 2015, 9:53 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated March 26, 2015, 9:53 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp 9cf05ac 3rdparty/libprocess/src/process.cpp 67b6b3b 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
On Feb. 18, 2015, 11:38 a.m., Niklas Nielsen wrote: Let's get tests wired up before committing this :) Adam B wrote: Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon. Ben Mahler wrote: Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above. Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa. Adam B wrote: Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields. Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves. But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout. If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical. Adam B wrote: I'm inclined to think that we shouldn't worry too much about the unlikely scenario of a network split in the middle of a rolling upgrade to 0.23 where longer ping timeouts are simultaneously being applied. Procedure should be: upgrade (most of) the cluster, then restart the master(s) with the new longer ping timeout value. I can add a Note to upgrades.md if you like. How does that sound to you @bmahler ? If there are no objections, I'll rebase the patch and update the configuration and upgrades docs. Ben Mahler wrote: Yeah that sounds fine to me (might be prudent to call it out in the upgrade doc or the changelog). Mind reaching out to me directly when this is ready for more reviewing? :) Absolutely. Thanks. :) - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/#review72992 --- On Feb. 19, 2015, 12:10 a.m., Adam B wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/ --- (Updated Feb. 19, 2015, 12:10 a.m.) Review request for mesos, Ben Mahler and Niklas Nielsen. Bugs: MESOS-2110 https://issues.apache.org/jira/browse/MESOS-2110 Repository: mesos Description --- Added new --slave_ping_timeout and --max_slave_ping_timeouts flags to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs) and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5). These can be extended if slaves are expected/allowed to be down for longer than a minute or two. Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage. Beware that this affects recovery from network timeouts as well as actual slave node/process failover. Diffs - src/master/constants.hpp ad3fe81 src/master/constants.cpp d3d0f71 src/master/flags.hpp 51a6059 src/master/master.cpp f10a3cf src/messages/messages.proto 58484ae src/slave/constants.hpp 12d6e92 src/slave/constants.cpp 7868bef src/slave/slave.hpp 91dae10 src/slave/slave.cpp aec9525 src/tests/fault_tolerance_tests.cpp efa5c57 src/tests/partition_tests.cpp eb16a58 src/tests/slave_recovery_tests.cpp 8210c52 src/tests/slave_tests.cpp 153d9d6 Diff: https://reviews.apache.org/r/29507/diff/ Testing --- Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts. Ran unit tests with shorter non-default values for ping timeouts. `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave Thanks, Adam B
Re: Facing issues submit review requests using support/post_review.py
We do accept Pull-Requests https://github.com/apache/mesos/pulls for small patches, especially (markdown) documentation patches; but for larger code changes we prefer the issue tracking, multi-line comments, and incremental diffs provided by ReviewBoard. On Tue, Mar 31, 2015 at 10:55 AM, haosdent haosd...@gmail.com wrote: If it is possible, I suggest use the workflow like https://github.com/apache/spark: Submit push request to github. On Wed, Apr 1, 2015 at 1:49 AM, Vinod Kone vinodk...@apache.org wrote: Are you still having issue with this? I've seen this issue before when my branch depends on a file that was recently committed. The mesos repo used by ReviewBoard is a mirror of the canonical mesos repo and there is sometimes lag in the commits from the latter reaching the former. But the issue usually resolves by itself once the new commits are mirrored. On Sun, Mar 29, 2015 at 9:58 AM, Aditi Dixit aditi96di...@gmail.com wrote: Thanks for the response Alex. I checked the .reviewboardrc file and its parameters were the same that you mentioned. I had also taken care to rebase the so that my commit was over the current master. While trying to submit a patch manually through the ReviewBoard UI, I was getting an error that The file src/master/master.hpp (revision 744886e) was not found in the repository. Is the format of making a patch something other than git format-patch -n ? Regards, Aditi Dixit -- Best Regards, Haosdent Huang
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On March 26, 2015, 4:32 p.m., Alexander Rukletsov wrote: 3rdparty/libprocess/src/process.cpp, line 6 https://reviews.apache.org/r/30032/diff/4/?file=834184#file834184line6 One thing captures my attention is how we include C headers. AFAIK, the standard requires to include them like ``` #include ctime #include climits #include cstdio ``` and so on. Could you please create a cleanup newbie JIRA for this? Alexander Rojas wrote: Not very sure about that, what it does by using the c… versions of the headers is to put its functions in the std namespace. Check the C++11 Standard annex *D.6 C standard library headers*: 2 Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3). 3 Example: The header cstdlib assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header stdlib.h assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. —end example. As it reads, using functions from the `cname` versions of the `name.h` headers, may or may not (stdlib implementation specific) require to add the namespace std too all functions and structs provided by the header. Till Toenshoff wrote: Seems we got three options here; - start using the C++ wrappers and allowed for a smooth transition (file by file as needed/touched) - - not sure what the implications here are from an optimizing (linker) point of view - global cleanup and `std` namespace added everywhere - stick with the C variants May be worth a dev-list discussion? C++11 Standard 17.6.1.2 p4, p8, and reference 177 hint at a preferred form for C++ programs. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review77914 --- On March 26, 2015, 9:53 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated March 26, 2015, 9:53 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp 9cf05ac 3rdparty/libprocess/src/process.cpp 67b6b3b 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On March 26, 2015, 5:32 p.m., Alexander Rukletsov wrote: 3rdparty/libprocess/src/process.cpp, line 6 https://reviews.apache.org/r/30032/diff/4/?file=834184#file834184line6 One thing captures my attention is how we include C headers. AFAIK, the standard requires to include them like ``` #include ctime #include climits #include cstdio ``` and so on. Could you please create a cleanup newbie JIRA for this? Alexander Rojas wrote: Not very sure about that, what it does by using the c… versions of the headers is to put its functions in the std namespace. Check the C++11 Standard annex *D.6 C standard library headers*: 2 Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3). 3 Example: The header cstdlib assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header stdlib.h assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. —end example. As it reads, using functions from the `cname` versions of the `name.h` headers, may or may not (stdlib implementation specific) require to add the namespace std too all functions and structs provided by the header. Till Toenshoff wrote: Seems we got three options here; - start using the C++ wrappers and allowed for a smooth transition (file by file as needed/touched) - - not sure what the implications here are from an optimizing (linker) point of view - global cleanup and `std` namespace added everywhere - stick with the C variants May be worth a dev-list discussion? Alexander Rukletsov wrote: C++11 Standard 17.6.1.2 p4, p8, and reference 177 hint at a preferred form for C++ programs. I think the best would be to add a discussion on the dev list, since changing to the `cname` version would require to prefix every call with the prefix `std::` - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review77914 --- On March 26, 2015, 10:53 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated March 26, 2015, 10:53 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp 9cf05ac 3rdparty/libprocess/src/process.cpp 67b6b3b 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 31676: Added documentation about the use of the LIBPROCESS_DISABLED_ENDPOINTS environment variable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31676/ --- (Updated April 1, 2015, 11:47 a.m.) Review request for mesos, Joerg Schad, Niklas Nielsen, and Till Toenshoff. Changes --- Endpoints disabling doesn't support wildcard matching anymore. Bugs: mesos-2333 https://issues.apache.org/jira/browse/mesos-2333 Repository: mesos Description --- Adds documentation on how to disable endpoints using `LIBPROCESS_DISABLED_ENDPOINTS`. Diffs (updated) - docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b Diff: https://reviews.apache.org/r/31676/diff/ Testing --- Thanks, Alexander Rojas
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On March 26, 2015, 5:32 p.m., Alexander Rukletsov wrote: 3rdparty/libprocess/src/process.cpp, line 6 https://reviews.apache.org/r/30032/diff/4/?file=834184#file834184line6 One thing captures my attention is how we include C headers. AFAIK, the standard requires to include them like ``` #include ctime #include climits #include cstdio ``` and so on. Could you please create a cleanup newbie JIRA for this? Alexander Rojas wrote: Not very sure about that, what it does by using the c… versions of the headers is to put its functions in the std namespace. Check the C++11 Standard annex *D.6 C standard library headers*: 2 Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3). 3 Example: The header cstdlib assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header stdlib.h assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. —end example. As it reads, using functions from the `cname` versions of the `name.h` headers, may or may not (stdlib implementation specific) require to add the namespace std too all functions and structs provided by the header. Till Toenshoff wrote: Seems we got three options here; - start using the C++ wrappers and allowed for a smooth transition (file by file as needed/touched) - - not sure what the implications here are from an optimizing (linker) point of view - global cleanup and `std` namespace added everywhere - stick with the C variants May be worth a dev-list discussion? Alexander Rukletsov wrote: C++11 Standard 17.6.1.2 p4, p8, and reference 177 hint at a preferred form for C++ programs. Alexander Rojas wrote: I think the best would be to add a discussion on the dev list, since changing to the `cname` version would require to prefix every call with the prefix `std::` Preferred is a very different thing from required, as mention originally. And I do agree that I prefer in my private projects to use the `cname` versions. However, I wouldn't do that since it has the capacity of becoming red herring. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review77914 --- On March 26, 2015, 10:53 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated March 26, 2015, 10:53 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp 9cf05ac 3rdparty/libprocess/src/process.cpp 67b6b3b 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Use cname instead of name.h in sources
In Mesos sources we use c-style C headers (stdlib.h), as opposed to c++-style (cstdlib). C++11 Standard 17.6.1.2 p4, p8, and reference 177 hint that cname is a preferred form for C++ programs. However this may require adding std prefix everywhere, since according to the Standard cname headers may not provide its declarations and definitions within the global namespace (C++11 Standard D.5 p3). Basically, we have three options here: 1. Leave the status quo and stick to C-style headers. 2. Create a clean-up JIRA and follow up with the cleanup. 3. Start using cname headers with std prefix and do a smooth transition. Thoughts?
Re: Apache Mesos Community Sync
Reminder: We're having another Mesos Developer Community Sync this Thursday, April 2nd from 3-5pm Pacific. Agenda: https://docs.google.com/document/d/153CUCj5LOJCFAVpdDZC7COJDwKh9RDjxaTA0S7lzwDA/edit?usp=sharing To Join: follow the BlueJeans instructions from the recurring meeting invite at the start of this thread. On Fri, Mar 6, 2015 at 11:11 AM, Vinod Kone vinodk...@apache.org wrote: Hi folks, We are planning to do monthly Mesos community meetings. Tentatively these are scheduled to occur on 1st Thursday of every month at 3 PM PST. See below for details to join the meeting remotely. This is a forum to ask questions/discuss about upcoming features, process etc. Everyone is welcome to join. Feel free to add items to the agenda for the next meeting here https://docs.google.com/document/d/153CUCj5LOJCFAVpdDZC7COJDwKh9RDjxaTA0S7lzwDA/edit?usp=sharing . Cheers, On Thu, Mar 5, 2015 at 11:23 AM, Vinod Kone via Blue Jeans Network inv...@bluejeans.com wrote: [image: Blue Jeans] http://bluejeans.com Vinod Kone vi...@twitter.com has invited you to a video meeting. Meeting Title: Apache Mesos Community Sync Meeting Time: Every 4th week on Thursday • from March 5, 2015 • 3 p.m. PST / 2 hrs Join Meeting https://bluejeans.com/272369669?ll=eng=mrsxmqdnmvzw64zomfygcy3imuxg64th -- Connecting directly from a room system? 1) Dial: 199.48.152.152 or bjn.vc 2) Enter Meeting ID: 272369669 -or- use the pairing code Just want to dial in? (all numbers http://bluejeans.com/premium-numbers ) 1) Direct-dial with my iPhone +14087407256,,#272369669%23,%23 or +1 408 740 7256 +1%20408%20740%207256+1 408 740 7256 +1 888 240 2560 +1%20888%20240%202560+1 888 240 2560 (US Toll Free) +1 408 317 9253 +1%20408%20317%209253+1 408 317 9253 (Alternate Number) 2) Enter Meeting ID: 272369669 -- Description: We will try BlueJeans VC this time for our monthly community sync. If BlueJeans *doesn't* work out we will use the Google Hangout link (https://plus.google.com/hangouts/_/twitter.com/mesos-sync) instead. *Note:* No moderator is required to start this meeting. -- First time joining a Blue Jeans Video Meeting? http://bluejeans.com/support http://bluejeans.com/support?ll=en -- Want to test your video connection? http://bluejeans.com/111 http://bluejeans.com/111?ll=en -- ©Blue Jeans Network 2014
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 9:17 a.m., Adam B wrote: docs/slave-recovery.md, line 71 https://reviews.apache.org/r/32543/diff/2/?file=907123#file907123line71 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? Alexander Rukletsov wrote: Adam, could you please explain what use case do you have in mind and how it is related to slave recovery? Adam B wrote: It's not related to slave recovery necessarily, but to how this KillMode impacts other processes like a custom executor. Some frameworks (like HDFS) have a custom executor that launches task(s) as a separate process/subprocess. If the executor is killed (kill -9, or shutdown by the framework/admin), will this change in KillMode affect whether the executors task subprocesses also get killed? I'm mostly worried about this KillMode change suddenly leaving stranded task processes if/when executors are killed. Alexander Rukletsov wrote: I thought that's exactly why we have containerizers: clean-up all stranded processes. Adam B wrote: Fair enough, when the slave is running. But what if the executor is killed while the slave (thus also the containerizer) is shutdown/recovering? I'm not claiming there's anything necessarily wrong with using this KillMode. I just ask the question to make sure we don't recommend a setting that may fix one issue but cause others. I see your point. I would be surprised if this setting will cause the issue, but let's check: better safe than sorry. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 2:09 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 2:09 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 32583: Marked RunTaskMessage::framework_id as optional.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32583/#review78494 --- Should mention in the Description (commit message) that the new preferred location for the FrameworkID is or will be in FrameworkInfo.id. Would also like for you to replace ambiguous instances of 'it' in the description with the actual field/message to which 'it' refers. src/slave/slave.cpp https://reviews.apache.org/r/32583/#comment127312 CopyFrom? - Adam B On March 31, 2015, 1:28 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32583/ --- (Updated March 31, 2015, 1:28 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2558 https://issues.apache.org/jira/browse/MESOS-2558 Repository: mesos Description --- For this release (N), we still keep setting it (handles older Slaves with newer Master). - Added code to handle it being unset in the Slave (handles older Master with newer Slaves). In the following release (N+1), stop reading/setting it (the previous version would handle the unset case). In release N+2, remove the field altogether (the previous release is not setting/reading it). Diffs - src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32583/diff/ Testing --- make check. TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 9:17 a.m., Adam B wrote: docs/slave-recovery.md, line 71 https://reviews.apache.org/r/32543/diff/2/?file=907123#file907123line71 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? Alexander Rukletsov wrote: Adam, could you please explain what use case do you have in mind and how it is related to slave recovery? Adam B wrote: It's not related to slave recovery necessarily, but to how this KillMode impacts other processes like a custom executor. Some frameworks (like HDFS) have a custom executor that launches task(s) as a separate process/subprocess. If the executor is killed (kill -9, or shutdown by the framework/admin), will this change in KillMode affect whether the executors task subprocesses also get killed? I'm mostly worried about this KillMode change suddenly leaving stranded task processes if/when executors are killed. I thought that's exactly why we have containerizers: clean-up all stranded processes. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 2:09 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 2:09 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 31676: Added documentation about the use of the LIBPROCESS_DISABLED_ENDPOINTS environment variable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31676/#review78502 --- Patch looks great! Reviews applied: [31228, 31676] All tests passed. - Mesos ReviewBot On April 1, 2015, 9:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31676/ --- (Updated April 1, 2015, 9:47 a.m.) Review request for mesos, Joerg Schad, Niklas Nielsen, and Till Toenshoff. Bugs: mesos-2333 https://issues.apache.org/jira/browse/mesos-2333 Repository: mesos Description --- Adds documentation on how to disable endpoints using `LIBPROCESS_DISABLED_ENDPOINTS`. Diffs - docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b Diff: https://reviews.apache.org/r/31676/diff/ Testing --- Thanks, Alexander Rojas
Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/#review78496 --- src/slave/slave.cpp https://reviews.apache.org/r/32585/#comment127315 Hmm... I was actually thinking that this line of code can be removed now that you're always filling in the id in the FrameworkInfo by the time it gets to the slave. However, upon further inspection I see that there is a Slave::Framework::id field that we'll also need to deprecate, ensuring that Slave::Framework::info.id is set, and thus Archive::Framework::framework_info has the frameworkId. - Adam B On March 31, 2015, 1:29 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/ --- (Updated March 31, 2015, 1:29 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- Framework.id() extracts and returns FrameworkID from Framework.info. Diffs - src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32585/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 31228: Added a mechanism for disabling http endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31228/ --- (Updated April 1, 2015, 11:50 a.m.) Review request for mesos, Joerg Schad, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2333 https://issues.apache.org/jira/browse/MESOS-2333 Repository: mesos Description (updated) --- Adds a mechanism for disabling http endpoints (e.g `testprocess/handler1,processname(3)/handler2`). A list of coma separated strings can be provided using the environment variable `LIBPROCESS_DISABLED_ENDPOINTS` which will be read during libprocess initialization. Then, when creating http endpoints (using the method `route`) the endpoint path will be checked against the patterns. If a match is found the endpoint handler will be replaced for a generic once which returns a 403 HTTP Error (Forbidden). Diffs (updated) - 3rdparty/libprocess/include/process/process.hpp 392c74df3e8a122aecd3633dffdeec4bcbd1f097 3rdparty/libprocess/src/process.cpp cf4e36489be2c6aa01e838c1c71383f248deab5b 3rdparty/libprocess/src/tests/process_tests.cpp eb38edce2c483ba7f963a826893a15a075238618 Diff: https://reviews.apache.org/r/31228/diff/ Testing (updated) --- make check Thanks, Alexander Rojas
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 2:17 a.m., Adam B wrote: docs/upgrades.md, line 15 https://reviews.apache.org/r/32543/diff/2/?file=907124#file907124line15 Is there some new behavior in Mesos 0.22 that just caused this issue to start occurring? Or could this have happened with Mesos 0.21 or earlier with the same systemd default setting? If so, this is not an upgrade issue and this note doesn't belong in the upgrades doc. Joerg Schad wrote: It it not related to a release (that is why the issue is described in slave-recovery.md). The idea for including it here as well is to warn people explicitly of this issue when upgrading (where this behavior will likely occur). I can drop it, but personally feel it is quite helpful here. If it's not an issue with upgrading Mesos, then it doesn't belong in the upgrades doc, especially in a section for upgrading to a specific version of Mesos. Please remove. The slave recovery doc seems like the right place for this note. On March 27, 2015, 2:17 a.m., Adam B wrote: docs/slave-recovery.md, line 71 https://reviews.apache.org/r/32543/diff/2/?file=907123#file907123line71 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? Alexander Rukletsov wrote: Adam, could you please explain what use case do you have in mind and how it is related to slave recovery? Adam B wrote: It's not related to slave recovery necessarily, but to how this KillMode impacts other processes like a custom executor. Some frameworks (like HDFS) have a custom executor that launches task(s) as a separate process/subprocess. If the executor is killed (kill -9, or shutdown by the framework/admin), will this change in KillMode affect whether the executors task subprocesses also get killed? I'm mostly worried about this KillMode change suddenly leaving stranded task processes if/when executors are killed. Alexander Rukletsov wrote: I thought that's exactly why we have containerizers: clean-up all stranded processes. Fair enough, when the slave is running. But what if the executor is killed while the slave (thus also the containerizer) is shutdown/recovering? I'm not claiming there's anything necessarily wrong with using this KillMode. I just ask the question to make sure we don't recommend a setting that may fix one issue but cause others. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 7:09 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 7:09 a.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Use cname instead of name.h in sources
I think is important mention the implications. The change doesn’t only imply to change the include headers, but every function, struct and class used from the c++ style headers will need to be prefixed with the std which in turn will messed with the git blame. Not to mention that, while most of the std libraries implementations offer the functions and structures for the c++ style headers on both, the std and the global namespace. This is not required by the standard and can change in any moment causing sudden compilation breaks. In my personal opinion, this is nothing but a cosmetic change which has the potential of becoming a red-herring. As such, I vote for option 1. On 01 Apr 2015, at 12:06, Alex Rukletsov a...@mesosphere.io wrote: In Mesos sources we use c-style C headers (stdlib.h), as opposed to c++-style (cstdlib). C++11 Standard 17.6.1.2 p4, p8, and reference 177 hint that cname is a preferred form for C++ programs. However this may require adding std prefix everywhere, since according to the Standard cname headers may not provide its declarations and definitions within the global namespace (C++11 Standard D.5 p3). Basically, we have three options here: 1. Leave the status quo and stick to C-style headers. 2. Create a clean-up JIRA and follow up with the cleanup. 3. Start using cname headers with std prefix and do a smooth transition. Thoughts?
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 2:17 a.m., Adam B wrote: docs/slave-recovery.md, line 71 https://reviews.apache.org/r/32543/diff/2/?file=907123#file907123line71 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? Alexander Rukletsov wrote: Adam, could you please explain what use case do you have in mind and how it is related to slave recovery? It's not related to slave recovery necessarily, but to how this KillMode impacts other processes like a custom executor. Some frameworks (like HDFS) have a custom executor that launches task(s) as a separate process/subprocess. If the executor is killed (kill -9, or shutdown by the framework/admin), will this change in KillMode affect whether the executors task subprocesses also get killed? I'm mostly worried about this KillMode change suddenly leaving stranded task processes if/when executors are killed. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 7:09 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 7:09 a.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 31267: Added a test allocator module.
On March 27, 2015, 4:36 p.m., Alexander Rojas wrote: src/tests/module.cpp, line 137 https://reviews.apache.org/r/31267/diff/4-5/?file=903021#file903021line137 You may need a `CHECK(modules != NULL)` or a log and return in the NULL case? Alexander Rukletsov wrote: If you follow the invokation path, you'll see that modules is a stack variable (see `initModules()`) and can't be null. I agree this is not clear at the place where we use a pointer to it and may change in the future, but this is more a general question about the design decision that should go to @tillt. Pass by pointer was introduced in Commit: 7ee3b7b672a4d8fee4fe4eb5f0aa2e7e3bf6b049 Review: https://reviews.apache.org/r/31253 Author: Till Toenshoff toensh...@me.com We generally avoid references when mutating on the callee side - that is a style descision which came from google's guidelines if I am not mistaken; see e.g. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Parameter_Ordering - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78070 --- On March 27, 2015, 4:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
On March 27, 2015, 4:36 p.m., Alexander Rojas wrote: src/tests/module.cpp, line 137 https://reviews.apache.org/r/31267/diff/4-5/?file=903021#file903021line137 You may need a `CHECK(modules != NULL)` or a log and return in the NULL case? If you follow the invokation path, you'll see that modules is a stack variable (see `initModules()`) and can't be null. I agree this is not clear at the place where we use a pointer to it and may change in the future, but this is more a general question about the design decision that should go to @tillt. Pass by pointer was introduced in Commit: 7ee3b7b672a4d8fee4fe4eb5f0aa2e7e3bf6b049 Review: https://reviews.apache.org/r/31253 Author: Till Toenshoff toensh...@me.com - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78070 --- On March 27, 2015, 4:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
On March 27, 2015, 4:36 p.m., Alexander Rojas wrote: src/tests/module.cpp, line 137 https://reviews.apache.org/r/31267/diff/4-5/?file=903021#file903021line137 You may need a `CHECK(modules != NULL)` or a log and return in the NULL case? Alexander Rukletsov wrote: If you follow the invokation path, you'll see that modules is a stack variable (see `initModules()`) and can't be null. I agree this is not clear at the place where we use a pointer to it and may change in the future, but this is more a general question about the design decision that should go to @tillt. Pass by pointer was introduced in Commit: 7ee3b7b672a4d8fee4fe4eb5f0aa2e7e3bf6b049 Review: https://reviews.apache.org/r/31253 Author: Till Toenshoff toensh...@me.com Till Toenshoff wrote: We generally avoid references when mutating on the callee side - that is a style descision which came from google's guidelines if I am not mistaken; see e.g. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Parameter_Ordering Actually the above link just menions it briefly, the one Alex just pointed me to is much more explicit: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78070 --- On March 27, 2015, 4:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
On March 24, 2015, 2:49 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, lines 123-126 https://reviews.apache.org/r/31268/diff/4/?file=903030#file903030line123 Could we indent this similar to this example from `src/tests/isolator_tests.cpp`: ``` typedef ::testing::Types CgroupsMemIsolatorProcess, CgroupsCpushareIsolatorProcess, CgroupsPerfEventIsolatorProcess CgroupsIsolatorTypes; ``` ``` typedef ::testing::Types HierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` Alexander Rukletsov wrote: I don't know what is consistent here, `src/tests/cram_md5_authentication_tests.cpp` use the indentation I use. Also, clang-format gives something that is more similar to the way I propose. Michael Park wrote: Yeah I did see the one from `src/tests/cram_md5_authentication_tests.cpp`. I think that one is long enough that we can't do too much better. I would prefer a style that visually separates the list of types with the resulting type. `clang-format` seems to format it as: ``` typedef ::testing::TypesHierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` which also visually separates the list of types and the resulting type so I'm ok with that also. Just explaining exactly what I was pushing for. Having said that, do whatever you feel is right here :) Ok, then let's go for the tool! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review77572 --- On March 27, 2015, 4:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78509 --- Ship it! src/tests/module.cpp https://reviews.apache.org/r/31267/#comment127331 Much better than the old variant - thanks for pointing this out! - Till Toenshoff On March 27, 2015, 4:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review78512 --- Ship it! Ship It! - Till Toenshoff On March 27, 2015, 4:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated April 1, 2015, 2:24 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Niklas' comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs (updated) - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated April 1, 2015, 2:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 1, 2015, 2:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated April 1, 2015, 2:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Alexander's comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 1, 2015, 2:28 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/ --- (Updated April 1, 2015, 2:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest fixture stores the test allocator in turn. Diffs (updated) - src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/31263/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated April 1, 2015, 2:29 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MPark's comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review78525 --- Patch looks great! Reviews applied: [31775, 31776, 31266, 31267, 31262, 31263, 31265, 31268] All tests passed. - Mesos ReviewBot On April 1, 2015, 2:29 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated April 1, 2015, 2:29 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 32657: Refactor wrappers for JSON encoded stats in preparation for greater reuse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32657/ --- (Updated April 1, 2015, 3:39 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor wrappers for JSON encoded stats in preparation for greater reuse Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32657/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32656: Refactor statistics helper out of PortMappingIsolatorTest for easier reuse.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32656/ --- (Updated April 1, 2015, 3:44 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor statistics helper out of PortMappingIsolatorTest for easier reuse. Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32656/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32653: Replace busy look on ready file with a more relaxed loop
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/ --- (Updated April 1, 2015, 3:43 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Replace busy look on ready file with a more relaxed loop Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32653/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32655: Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32655/ --- (Updated April 1, 2015, 3:44 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32655/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32660: Report network isolator statistics on a per container basis (MESOS-2332)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/ --- (Updated April 1, 2015, 3:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Report network isolator statistics on a per container basis (MESOS-2332) Diffs - include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef Diff: https://reviews.apache.org/r/32660/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32659: Pull the common container definitions out of PortIsolatorMappingTest for reuse.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32659/ --- (Updated April 1, 2015, 3:44 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Pull the common container definitions out of PortIsolatorMappingTest for reuse. Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32659/diff/ Testing --- make check Thanks, Paul Brett
Re: bug in 0.22.0?
Hm,
so after replaying some of my logs, I am not entirely sure. This may
be a design decision in the mesos core. Maybe you can help me out
here: If an event is not acked, it gets repeated. This is what happens
right now. If an event never gets acked (e.g. the first TASK_RUNNING),
what will happen if as task falls into TASK_KILLED? Will the slave
still try to deliver the TASK_RUNNING first (and then the TASK_KILLED
once that got acked?)? Or would it discarded the older (TASK_RUNNING)
message which is now superseded?
The messages that I see are:
DEBUG [2015-03-31 19:57:48,971]
com.groupon.mesos.util.HttpProtocolReceiver: Received from
master@10.101.131.128:5050: update {
framework_id {
value: Singularity_local
}
executor_id {
value: MyFirstDeploy-d2913ee6-1427831718108-1-blackbox.group.on-LOCAL.R1.1
}
slave_id {
value: 20150331-113503-2156094730-5050-13210-S0
}
status {
task_id {
value:
MyFirstDeploy-d2913ee6-1427831718108-1-blackbox.group.on-LOCAL.R1.1
}
state: TASK_RUNNING
slave_id {
value: 20150331-113503-2156094730-5050-13210-S0
}
timestamp: 1.427831718964869E9
source: SOURCE_EXECUTOR
}
timestamp: 1.427831718964869E9
uuid: \346H\317\202\361G\022\265\001)\251\037\210\256\234
latest_state: TASK_KILLED
}
which implies, that this is a resend of the original TASK_RUNNING
message but the latest_state is the latest state in time (while the
TASK_RUNNING is still sitting in the message queue).
So, if that is the case, then there are actually is no bug in Mesos.
Once the framework correctly acks the messages, they are delivered
correctly.
-h
On Tue, Mar 31, 2015 at 11:28 AM, Benjamin Mahler
benjamin.mah...@gmail.com wrote:
From the slave logs you posted here:
https://gist.github.com/hgschmie/fc20b361a2cadeba0fbd
The slave received updates for RUNNING followed by KILLED from the executor.
The slave was only forwarding RUNNING to the master as it didn't receive an
ack from the framework. Why do you think that KILLED was being forwarded by
the slave? I don't see that in these logs (note the Forwarding lines).
On Tue, Mar 31, 2015 at 11:20 AM, Niklas Nielsen nik...@mesosphere.io
wrote:
Hi Henning,
It does sound suspicious.
Would you mind capturing this in a JIRA ticket (targeted 0.22.1) so we can
track progress and assign an owner?
Thanks,
Niklas
On 31 March 2015 at 10:00, Henning Schmiedehausen
hschmiedehau...@groupon.com wrote:
Ok, so here is my log file from the slave:
https://gist.github.com/hgschmie/fc20b361a2cadeba0fbd
- Slave shuts down executor:
I0330 16:27:59.187947 18428 slave.cpp:1558] Asked to kill task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local
I0330 16:27:59.239296 18425 slave.cpp:2508] Handling status update
TASK_KILLED (UUID: 7f4fdc95-3a7d-474d-b4f1-b6da45e96396) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local from executor(1)@10.101.131.128:55377
I0330 16:27:59.239320 18425 slave.cpp:4468] Terminating task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT
I0330 16:27:59.240444 18424 status_update_manager.cpp:317] Received
status update TASK_KILLED (UUID: 7f4fdc95-3a7d-474d-b4f1-b6da45e96396)
for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT
of framework Singularity_local
I0330 16:27:59.240613 18425 slave.cpp:2680] Status update manager
successfully handled status update TASK_KILLED (UUID:
7f4fdc95-3a7d-474d-b4f1-b6da45e96396) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local
I0330 16:27:59.240623 18425 slave.cpp:2686] Sending acknowledgement
for status update TASK_KILLED (UUID:
7f4fdc95-3a7d-474d-b4f1-b6da45e96396) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local to executor(1)@10.101.131.128:55377
I0330 16:28:00.443361 18422 slave.cpp:3193] Executor
'MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT' of
framework Singularity_local exited with status 0
Now my framework receives the TASK_KILLED (I see that) but fails to
send the ack message back to mesos. After 30 seconds:
W0330 16:28:30.097679 18426 status_update_manager.cpp:472] Resending
status update TASK_RUNNING (UUID:
ea431985-98de-4447-8668-fda26c95f040) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local
I0330 16:28:30.097704 18426 status_update_manager.cpp:371] Forwarding
update TASK_RUNNING (UUID: ea431985-98de-4447-8668-fda26c95f040) for
task MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT
of framework Singularity_local to the slave
I0330 16:28:30.097761 18426 slave.cpp:2753] Forwarding the update
TASK_RUNNING (UUID: ea431985-98de-4447-8668-fda26c95f040) for task
Re: Review Request 32664: Add port mapping isolator statistics tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/ --- (Updated April 1, 2015, 3:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Add port mapping isolator statistics tests Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32664/diff/ Testing --- make check Thanks, Paul Brett
Re: bug in 0.22.0?
Yes, I am using the jesos code here. I am not planning to use it, this
is just observing. :-)
I guess the comment at
https://github.com/apache/mesos/blob/master/src/messages/messages.proto#L85-L90
pretty much illustrates my problem. I am still stuck at the queue head
(without acks) and the bottom state is already TASK_KILLED.
-h
On Tue, Mar 31, 2015 at 3:49 PM, Benjamin Mahler
benjamin.mah...@gmail.com wrote:
Ah I guess you're using a custom language binding that is speaking the
internal protobuf protocol? 'latest_state' is an internal message field that
is hidden from the scheduler api intentionally, please don't use it.
On Tue, Mar 31, 2015 at 3:30 PM, Henning Schmiedehausen
hschmiedehau...@groupon.com wrote:
Hm,
so after replaying some of my logs, I am not entirely sure. This may
be a design decision in the mesos core. Maybe you can help me out
here: If an event is not acked, it gets repeated. This is what happens
right now. If an event never gets acked (e.g. the first TASK_RUNNING),
what will happen if as task falls into TASK_KILLED? Will the slave
still try to deliver the TASK_RUNNING first (and then the TASK_KILLED
once that got acked?)? Or would it discarded the older (TASK_RUNNING)
message which is now superseded?
The messages that I see are:
DEBUG [2015-03-31 19:57:48,971]
com.groupon.mesos.util.HttpProtocolReceiver: Received from
master@10.101.131.128:5050: update {
framework_id {
value: Singularity_local
}
executor_id {
value:
MyFirstDeploy-d2913ee6-1427831718108-1-blackbox.group.on-LOCAL.R1.1
}
slave_id {
value: 20150331-113503-2156094730-5050-13210-S0
}
status {
task_id {
value:
MyFirstDeploy-d2913ee6-1427831718108-1-blackbox.group.on-LOCAL.R1.1
}
state: TASK_RUNNING
slave_id {
value: 20150331-113503-2156094730-5050-13210-S0
}
timestamp: 1.427831718964869E9
source: SOURCE_EXECUTOR
}
timestamp: 1.427831718964869E9
uuid: \346H\317\202\361G\022\265\001)\251\037\210\256\234
latest_state: TASK_KILLED
}
which implies, that this is a resend of the original TASK_RUNNING
message but the latest_state is the latest state in time (while the
TASK_RUNNING is still sitting in the message queue).
So, if that is the case, then there are actually is no bug in Mesos.
Once the framework correctly acks the messages, they are delivered
correctly.
-h
On Tue, Mar 31, 2015 at 11:28 AM, Benjamin Mahler
benjamin.mah...@gmail.com wrote:
From the slave logs you posted here:
https://gist.github.com/hgschmie/fc20b361a2cadeba0fbd
The slave received updates for RUNNING followed by KILLED from the
executor.
The slave was only forwarding RUNNING to the master as it didn't receive
an
ack from the framework. Why do you think that KILLED was being forwarded
by
the slave? I don't see that in these logs (note the Forwarding lines).
On Tue, Mar 31, 2015 at 11:20 AM, Niklas Nielsen nik...@mesosphere.io
wrote:
Hi Henning,
It does sound suspicious.
Would you mind capturing this in a JIRA ticket (targeted 0.22.1) so we
can
track progress and assign an owner?
Thanks,
Niklas
On 31 March 2015 at 10:00, Henning Schmiedehausen
hschmiedehau...@groupon.com wrote:
Ok, so here is my log file from the slave:
https://gist.github.com/hgschmie/fc20b361a2cadeba0fbd
- Slave shuts down executor:
I0330 16:27:59.187947 18428 slave.cpp:1558] Asked to kill task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local
I0330 16:27:59.239296 18425 slave.cpp:2508] Handling status update
TASK_KILLED (UUID: 7f4fdc95-3a7d-474d-b4f1-b6da45e96396) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local from executor(1)@10.101.131.128:55377
I0330 16:27:59.239320 18425 slave.cpp:4468] Terminating task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT
I0330 16:27:59.240444 18424 status_update_manager.cpp:317] Received
status update TASK_KILLED (UUID:
7f4fdc95-3a7d-474d-b4f1-b6da45e96396)
for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT
of framework Singularity_local
I0330 16:27:59.240613 18425 slave.cpp:2680] Status update manager
successfully handled status update TASK_KILLED (UUID:
7f4fdc95-3a7d-474d-b4f1-b6da45e96396) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local
I0330 16:27:59.240623 18425 slave.cpp:2686] Sending acknowledgement
for status update TASK_KILLED (UUID:
7f4fdc95-3a7d-474d-b4f1-b6da45e96396) for task
MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT of
framework Singularity_local to executor(1)@10.101.131.128:55377
I0330 16:28:00.443361 18422 slave.cpp:3193] Executor
'MyFirstDeploy-a351e465-1427758039385-2-blackbox.group.on-DEFAULT' of
framework Singularity_local
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/#review78526 --- src/slave/containerizer/isolators/posix/disk.cpp https://reviews.apache.org/r/32694/#comment127345 Rather than depending on ``__linux__``, consider adding ``sys/prctl.h`` to ``AC_CHECK_HEADERS`` and using ``#if HAVE_SYS_PRCTL_H``. src/slave/containerizer/isolators/posix/disk.cpp https://reviews.apache.org/r/32694/#comment127344 A minor nitpick, but I think that ``#ifdef PR_SET_PDEATHSIG`` would be better than ``#ifdef __linux__`` since other platforms may implement this prctl() in the future. - James Peach On March 31, 2015, 7:32 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated March 31, 2015, 7:32 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32657: Refactor wrappers for JSON encoded stats in preparation for greater reuse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32657/#review78532 --- Bad patch! Reviews applied: [32654] Failed command: ./support/apply-review.sh -n -r 32654 Error: 2015-04-01 15:54:52 URL:https://reviews.apache.org/r/32654/diff/raw/ [6185/6185] - 32654.patch [1] error: patch failed: src/tests/port_mapping_tests.cpp:400 error: src/tests/port_mapping_tests.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On April 1, 2015, 3:39 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32657/ --- (Updated April 1, 2015, 3:39 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor wrappers for JSON encoded stats in preparation for greater reuse Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32657/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32656: Refactor statistics helper out of PortMappingIsolatorTest for easier reuse.
On March 31, 2015, 11:38 p.m., Chi Zhang wrote: src/tests/port_mapping_tests.cpp, lines 188-192 https://reviews.apache.org/r/32656/diff/1/?file=910499#file910499line188 having no output here isn't expected; why delay reporting it? Having no output here is currently an expected behaviour. If mesos-network-helper is called with an invalid pid (for example, before a namespace has been created) then it generates no output and returns with an error status. I believe it would be better to always generate valid JSON output, with a clear error indication if an error occurs. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32656/#review78432 --- On April 1, 2015, 3:44 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32656/ --- (Updated April 1, 2015, 3:44 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor statistics helper out of PortMappingIsolatorTest for easier reuse. Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32656/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32664: Add port mapping isolator statistics tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/#review78534 --- Bad patch! Reviews applied: [32658, 32659] Failed command: ./support/apply-review.sh -n -r 32659 Error: 2015-04-01 16:14:41 URL:https://reviews.apache.org/r/32659/diff/raw/ [1497/1497] - 32659.patch [1] error: patch failed: src/tests/port_mapping_tests.cpp:100 error: src/tests/port_mapping_tests.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On April 1, 2015, 3:45 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32664/ --- (Updated April 1, 2015, 3:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Add port mapping isolator statistics tests Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32664/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32744: PortMapping: change to not host namespace symlink handles in /var/run/netns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32744/#review78631 --- Patch looks great! Reviews applied: [32744] All tests passed. - Mesos ReviewBot On April 1, 2015, 10:36 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32744/ --- (Updated April 1, 2015, 10:36 p.m.) Review request for mesos, Jie Yu and Cong Wang. Bugs: mesos-2574 https://issues.apache.org/jira/browse/mesos-2574 Repository: mesos Description --- MESOS-2574 Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32744/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 31012: [1/5] cgroups: added tests for memory statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31012/ --- (Updated April 1, 2015, 11:11 p.m.) Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. Summary (updated) - [1/5] cgroups: added tests for memory statistics. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description (updated) --- Added memory statisics test fixure and a test for RSS. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef Diff: https://reviews.apache.org/r/31012/diff/ Testing --- Thanks, Chi Zhang
Review Request 32756: [4/5] Added a memory statistics test for active anonymous memory.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32756/ --- Review request for mesos, Ian Downes and Jie Yu. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description --- Added a memory statistics test for active anonymous memory. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef src/tests/memory_test_helper.hpp 11712d7f378d9426f160d53b0387c698a28a4207 src/tests/memory_test_helper.cpp cdf769b32036c746ec4aa90841ea45c9a4159b51 Diff: https://reviews.apache.org/r/32756/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 31012: [1/5] Added memory statisics test fixure and a test for RSS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31012/ --- (Updated April 1, 2015, 11:13 p.m.) Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. Summary (updated) - [1/5] Added memory statisics test fixure and a test for RSS. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description --- Added memory statisics test fixure and a test for RSS. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef Diff: https://reviews.apache.org/r/31012/diff/ Testing --- Thanks, Chi Zhang
Review Request 32754: [2/5] Added a memory statistics test for page cache.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32754/ --- Review request for mesos. Repository: mesos Description --- Added a memory statistics test for page cache. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef Diff: https://reviews.apache.org/r/32754/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32754: [2/5] Added a memory statistics test for page cache.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32754/ --- (Updated April 1, 2015, 11:14 p.m.) Review request for mesos, Ian Downes and Jie Yu. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description --- Added a memory statistics test for page cache. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef Diff: https://reviews.apache.org/r/32754/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32750: MESOS-2585: Use full width for mesos div.container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32750/#review78625 --- Patch looks great! Reviews applied: [32750] All tests passed. - Mesos ReviewBot On April 1, 2015, 10:22 p.m., Alson Kemp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32750/ --- (Updated April 1, 2015, 10:22 p.m.) Review request for mesos. Bugs: MESOS-2585 https://issues.apache.org/jira/browse/MESOS-2585 Repository: mesos Description --- MESOS-2585: Use full width for mesos div.container Diffs - src/webui/master/static/css/mesos.css 5b1227e9d64757f9fc106e497f7fa3ed72112c10 Diff: https://reviews.apache.org/r/32750/diff/ Testing --- Visual verification. Thanks, Alson Kemp
Re: Review Request 32742: Added command logging for processes running in slave's cgroup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/#review78635 --- Patch looks great! Reviews applied: [32742] All tests passed. - Mesos ReviewBot On April 1, 2015, 10:39 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/ --- (Updated April 1, 2015, 10:39 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2461 https://issues.apache.org/jira/browse/MESOS-2461 Repository: mesos Description --- Added command logging for processes running in slave's cgroup. Diffs - src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 Diff: https://reviews.apache.org/r/32742/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/#review78639 --- Patch looks great! Reviews applied: [32654] All tests passed. - Mesos ReviewBot On April 1, 2015, 11:07 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 1, 2015, 11:07 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs - src/tests/port_mapping_tests.cpp f4124c3 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Review Request 32757: [5/5] Added a memory statistics test for writeback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32757/ --- Review request for mesos, Ian Downes and Jie Yu. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description --- Added a memory statistics test for writeback. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef src/tests/memory_test_helper.hpp 11712d7f378d9426f160d53b0387c698a28a4207 src/tests/memory_test_helper.cpp cdf769b32036c746ec4aa90841ea45c9a4159b51 Diff: https://reviews.apache.org/r/32757/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32653: Replace busy loop on ready file with a more relaxed loop
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/#review78637 --- Patch looks great! Reviews applied: [32653] All tests passed. - Mesos ReviewBot On April 1, 2015, 11 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/ --- (Updated April 1, 2015, 11 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Replace busy loop on ready file with a more relaxed loop Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32653/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32750: MESOS-2585: Use full width for mesos div.container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32750/#review78648 --- Patch looks great! Reviews applied: [32750] All tests passed. - Mesos ReviewBot On April 2, 2015, 12:05 a.m., Alson Kemp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32750/ --- (Updated April 2, 2015, 12:05 a.m.) Review request for mesos. Bugs: MESOS-2585 https://issues.apache.org/jira/browse/MESOS-2585 Repository: mesos Description --- MESOS-2585: Use full width for mesos div.container Diffs - src/webui/master/static/css/mesos.css 5b1227e9d64757f9fc106e497f7fa3ed72112c10 Diff: https://reviews.apache.org/r/32750/diff/ Testing --- Visual verification. Thanks, Alson Kemp
Re: Review Request 31012: cgroups: added tests for memory statistics.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1340
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1340
where is allocateRSSMemory and deallocateRSSMemory, I can't see them in
this review or r30545
now in MemoryTestHelper.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1346
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1346
why do you need to deallocate? shouldn't everything get cleaned up when
the test completes?
I assume that each test is independent...?
MemoryTestHelper doesn't have it anymore.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1364
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1364
+1 the test should be run in a temporary directory that always gets
cleaned up in tear down, see TemporaryDirectoryTest
fixed in the previous patch too; cgroup_tests now a TemporaryDirectoryTest
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1371
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1371
NULL pointer is converted to false, so ASSERT_TRUE(rss) should work
doesn't apply anymore with the use of memory test helper.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1353
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1353
ditto
doesn't apply anymore with the use of memory test helper.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1396
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1396
path doesn't get cleaned up.
TemporaryDirectoryTest now.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, lines 1410-1411
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1410
generally it's cleaner, i.e., avoids extra indentation to structure as:
if (pid == 0) {
// do whatever in the child
// exit or loop
}
// continue in parent
fixed in memory test helper patch.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, line 1444
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1444
why keep trying here but not in the parent...?
memory test helper used subprocess.
On March 24, 2015, 11:34 p.m., Ian Downes wrote:
src/tests/cgroups_tests.cpp, lines 1343-1344
https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1343
(default - delta) stat(rss) (default + delta)
is equivalent, and much more clearly expressed as:
|stat - default| delta
revised the test now this is not needed. wasn't sure about the policy of using
abs function from std libarary.
- Chi
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31012/#review77678
---
On April 1, 2015, 11:09 p.m., Chi Zhang wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31012/
---
(Updated April 1, 2015, 11:09 p.m.)
Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.
Bugs: mesos-2136
https://issues.apache.org/jira/browse/mesos-2136
Repository: mesos
Description
---
cgroups: added tests for memory statistics.
Diffs
-
src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef
Diff: https://reviews.apache.org/r/31012/diff/
Testing
---
Thanks,
Chi Zhang
Re: Review Request 31012: cgroups: added tests for memory statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31012/ --- (Updated April 1, 2015, 11:09 p.m.) Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. Bugs: mesos-2136 https://issues.apache.org/jira/browse/mesos-2136 Repository: mesos Description --- cgroups: added tests for memory statistics. Diffs (updated) - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef Diff: https://reviews.apache.org/r/31012/diff/ Testing --- Thanks, Chi Zhang
Review Request 32755: [3/5] Added a memory statistics test for memory-mapped file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32755/ --- Review request for mesos, Ian Downes and Jie Yu. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description --- Added a memory statistics test for memory-mapped file. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef src/tests/memory_test_helper.hpp 11712d7f378d9426f160d53b0387c698a28a4207 src/tests/memory_test_helper.cpp cdf769b32036c746ec4aa90841ea45c9a4159b51 Diff: https://reviews.apache.org/r/32755/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32750: MESOS-2585: Use full width for mesos div.container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32750/ --- (Updated April 1, 2015, 5:05 p.m.) Review request for mesos. Bugs: MESOS-2585 https://issues.apache.org/jira/browse/MESOS-2585 Repository: mesos Description --- MESOS-2585: Use full width for mesos div.container Diffs (updated) - src/webui/master/static/css/mesos.css 5b1227e9d64757f9fc106e497f7fa3ed72112c10 Diff: https://reviews.apache.org/r/32750/diff/ Testing --- Visual verification. Thanks, Alson Kemp
Re: Review Request 31915: MemIsolator: Improved some statistics naming. (MESOS-2104)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31915/#review78626 --- Patch looks great! Reviews applied: [31914, 31915] All tests passed. - Mesos ReviewBot On April 1, 2015, 10:28 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31915/ --- (Updated April 1, 2015, 10:28 p.m.) Review request for mesos, Ian Downes and Paul Brett. Bugs: mesos-2104 https://issues.apache.org/jira/browse/mesos-2104 Repository: mesos Description --- MemIsolator: Improved some statistics naming. (MESOS-2104) Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 src/slave/containerizer/isolators/cgroups/mem.cpp a7a83ef9ad4726aa139a92fc7f5917ed687d33f5 Diff: https://reviews.apache.org/r/31915/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32757: [5/5] Added a memory statistics test for writeback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32757/#review78642 --- Patch looks great! Reviews applied: [31012, 32754, 32755, 32756, 32757] All tests passed. - Mesos ReviewBot On April 1, 2015, 11:18 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32757/ --- (Updated April 1, 2015, 11:18 p.m.) Review request for mesos, Ian Downes and Jie Yu. Bugs: mesos-2572 https://issues.apache.org/jira/browse/mesos-2572 Repository: mesos Description --- Added a memory statistics test for writeback. Diffs - src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef src/tests/memory_test_helper.hpp 11712d7f378d9426f160d53b0387c698a28a4207 src/tests/memory_test_helper.cpp cdf769b32036c746ec4aa90841ea45c9a4159b51 Diff: https://reviews.apache.org/r/32757/diff/ Testing --- Thanks, Chi Zhang
Review Request 32728: Fixed indentation in mesos.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32728/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Fixed indentation in mesos.proto. Diffs - include/mesos/mesos.proto 0cbee3b188c2f55f6da16d064e1ae9266832d35b Diff: https://reviews.apache.org/r/32728/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32698: Used the argv version of subprocess for linux perf utilities.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32698/#review78545
---
Ship it!
src/linux/perf.cpp
https://reviews.apache.org/r/32698/#comment127389
Not yours but the delimiter doesn't need to be attached to the flag so it's
cleaner to have it as a separate argument now:
{noformat}
-x SEP, --field-separator SEP
print counts using a CSV-style output to make it easy to import directly
into spreadsheets. Columns are separated by the string specified in SEP.
{noformat}
src/linux/perf.cpp
https://reviews.apache.org/r/32698/#comment127388
ditto
- Ian Downes
On March 31, 2015, 1:04 p.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32698/
---
(Updated March 31, 2015, 1:04 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
Bugs: MESOS-2462
https://issues.apache.org/jira/browse/MESOS-2462
Repository: mesos
Description
---
Used the argv version of subprocess for linux perf utilities. See discussion
in MESOS-2462. This is to prepare for the death signal patch.
Diffs
-
src/linux/perf.cpp 863aa4a972289a59f57e93cd06ba2bf9df949fe2
Diff: https://reviews.apache.org/r/32698/diff/
Testing
---
sudo make check
Thanks,
Jie Yu
Re: Review Request 32660: Report network isolator statistics on a per container basis (MESOS-2332)
On April 1, 2015, 6:03 p.m., Ian Downes wrote: src/slave/containerizer/isolators/network/port_mapping.cpp, lines 640-644 https://reviews.apache.org/r/32660/diff/2/?file=911855#file911855line640 Are these statistics not available through a netlink interface, i.e., using libnl? I wasn't expecting to require parsing tc's output. +chzhcn can you comment here? The function that Cong mentioned should do. There is a tc_stat secion in doc/route.txt in libnl code base which has more information. - Chi --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/#review78546 --- On April 1, 2015, 3:45 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/ --- (Updated April 1, 2015, 3:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Report network isolator statistics on a per container basis (MESOS-2332) Diffs - include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef Diff: https://reviews.apache.org/r/32660/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32728: Fixed indentation in mesos.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32728/#review78543 --- Ship it! Thanks! - Jie Yu On April 1, 2015, 5:16 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32728/ --- (Updated April 1, 2015, 5:16 p.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- Fixed indentation in mesos.proto. Diffs - include/mesos/mesos.proto 0cbee3b188c2f55f6da16d064e1ae9266832d35b Diff: https://reviews.apache.org/r/32728/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32586: Removed FrameworkID argument from Slave::_runTask.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32586/ --- (Updated April 1, 2015, 3:35 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- rebased Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- The incoming FrameworkInfo has a valid FrameworkID. Diffs (updated) - src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8 src/tests/mesos.cpp 11e88330dd50913ab00da79054225d113541e83e src/tests/slave_tests.cpp fd09d65bf34136c0959419b451e54105300740c4 Diff: https://reviews.apache.org/r/32586/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32700: Removed FrameworkID from FrameworkState.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32700/ --- (Updated April 1, 2015, 3:35 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- rebased Bugs: MESOS-2559 https://issues.apache.org/jira/browse/MESOS-2559 Repository: mesos Description --- FrameworkState already has FrameworkInfo which will have a valid FrameworkID. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs (updated) - src/slave/containerizer/docker.cpp e83b912c707a3f2687b09a647a9ed248a940ea97 src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9 src/slave/containerizer/mesos/containerizer.cpp fbd1c0a0e5f4f227adb022f0baaa6d2c7e3ad748 src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 src/slave/state.hpp 31dfdd5a4b644f466756a712deded1b025a73c02 src/slave/state.cpp 35ce70b6702473a3100991372f3ba36bcad391c0 src/slave/status_update_manager.cpp fab8c22d46b8ab0a3c3745541ddc650b574bfbd4 Diff: https://reviews.apache.org/r/32700/diff/ Testing --- make check. TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/ --- (Updated April 1, 2015, 3:34 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- rebased Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- Framework.id() extracts and returns FrameworkID from Framework.info. Diffs (updated) - src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32585/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated April 1, 2015, 7:40 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Changes --- Review comments. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs (updated) - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32698: Used the argv version of subprocess for linux perf utilities.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32698/ --- (Updated April 1, 2015, 7:40 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Changes --- Review comments. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Used the argv version of subprocess for linux perf utilities. See discussion in MESOS-2462. This is to prepare for the death signal patch. Diffs (updated) - src/linux/perf.cpp 863aa4a972289a59f57e93cd06ba2bf9df949fe2 Diff: https://reviews.apache.org/r/32698/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 32660: Report network isolator statistics on a per container basis (MESOS-2332)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/#review78564 --- I feel if we use the native libnl api, we could shift a lot of the testing effort to just verifying the library. The test there could be simpler since you don't have to construct the 'mesos cluster' but rather directly verify the link stats after network manipulation (the iperf interface is a very useful addition to generl network testing in the code base!) Then on the testing of port mapping isolator side, we'd only have to care about 'isolator level' stuff such as flag combos, verify that 'usage' code path is through etc. Also I feel like the coding / testing effort here is pretty signifant. Would it be useful to document on the ticket to help achieve some high-level consensus before diving right into it? - Chi Zhang On April 1, 2015, 3:45 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/ --- (Updated April 1, 2015, 3:45 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Report network isolator statistics on a per container basis (MESOS-2332) Diffs - include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef Diff: https://reviews.apache.org/r/32660/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
On April 1, 2015, 3:32 p.m., James Peach wrote: src/slave/containerizer/isolators/posix/disk.cpp, line 21 https://reviews.apache.org/r/32694/diff/2/?file=911373#file911373line21 Rather than depending on ``__linux__``, consider adding ``sys/prctl.h`` to ``AC_CHECK_HEADERS`` and using ``#if HAVE_SYS_PRCTL_H``. I searched our code base. We never used `#if HAVE_SOME_HEADER_H` in our code base. I also feels like we should avoid doing that so that our code does not have a dependency to autoconf (we may change that in the future, say, using CMake, bazel, etc.) On April 1, 2015, 3:32 p.m., James Peach wrote: src/slave/containerizer/isolators/posix/disk.cpp, line 363 https://reviews.apache.org/r/32694/diff/2/?file=911373#file911373line363 A minor nitpick, but I think that ``#ifdef PR_SET_PDEATHSIG`` would be better than ``#ifdef __linux__`` since other platforms may implement this prctl() in the future. We have `#ifdef __linux__ #include sys/prctl.h #endif` for includes, I prefer keeping it the same here:) Thanks for the suggestion anyway! - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/#review78526 --- On March 31, 2015, 7:32 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated March 31, 2015, 7:32 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.
On April 1, 2015, 5:28 a.m., Adam B wrote: src/slave/slave.cpp, line 1043 https://reviews.apache.org/r/32585/diff/1-2/?file=908253#file908253line1043 Hmm... I was actually thinking that this line of code can be removed now that you're always filling in the id in the FrameworkInfo by the time it gets to the slave. However, upon further inspection I see that there is a Slave::Framework::id field that we'll also need to deprecate, ensuring that Slave::Framework::info.id is set, and thus Archive::Framework::framework_info has the frameworkId. I am not sure if I understand this. slave::Framework::id has been removed in this patch. I couldn't find `Slave::Framework` in the codebase. Can you point me to that? - Kapil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/#review78496 --- On March 31, 2015, 4:29 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/ --- (Updated March 31, 2015, 4:29 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- Framework.id() extracts and returns FrameworkID from Framework.info. Diffs - src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32585/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32653: Replace busy look on ready file with a more relaxed loop
On March 31, 2015, 10:42 p.m., Chi Zhang wrote: src/tests/port_mapping_tests.cpp, line 396 https://reviews.apache.org/r/32653/diff/2/?file=910398#file910398line396 how about at least have this function return a future and change call sites to use AWAIT_READY? When this goes into the library, the tests don't have to be changed anymore. More useful, AWAIT_READY supports timeout and has a default timeout time of 10s so you can save the timeout logic here. It'd be great see this in the library code! Good idea but not required for this change. I will make this a TODO so that we can quickly fix the current issue where a failure can cause the test to hang forever. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/#review78421 --- On April 1, 2015, 3:43 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/ --- (Updated April 1, 2015, 3:43 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Replace busy look on ready file with a more relaxed loop Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32653/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32583: Marked RunTaskMessage::framework_id as optional.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32583/ --- (Updated April 1, 2015, 3:34 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- Addressed Adam's concerns. Bugs: MESOS-2558 https://issues.apache.org/jira/browse/MESOS-2558 Repository: mesos Description (updated) --- The new preferred location for FrameworkID is FrameworkInfo.id. This patchset achieves this goal by incrementally deprecating other locations for FrameworkID. Here is a plan to deal with the upgrade path: For this release (N), we still keep setting RunTaskMessage::framework_id - this would handle older Slaves with newer Master. - added code to handle it being unset in the Slave (handles older Master with newer Slaves). In the following release (N+1), stop reading/setting RunTaskMessage::framework_id - the previous version would handle the unset case. In release N+2, remove the field altogether: - the previous release is not setting/reading it. Diffs (updated) - src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32583/diff/ Testing --- make check. TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32584: Do not pass FrameworkID to Framework constructor in Master/Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32584/ --- (Updated April 1, 2015, 3:34 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- rebased. Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- Framework constructor takes FrameworkInfo, which already has a valid FrameworkID. Diffs (updated) - src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32584/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
On April 1, 2015, 12:55 a.m., Ben Mahler wrote: src/slave/containerizer/isolators/posix/disk.cpp, lines 416-419 https://reviews.apache.org/r/32694/diff/2/?file=911373#file911373line416 Can't you just bind directly into `prctl`? E.g. ``` Optionlambda::functionint() setup = None(); #ifdef __linux__ setup = lambda::bind(::prctl(PR_SET_PDEATHSIG, SIGKILL)); #endif TrySubprocess s = subprocess( du, argv, Subprocess::PATH(/dev/null), Subprocess::PIPE(), Subprocess::PIPE(), None(), None(), setup); ``` This has slightly different semantics because if prctl fails, the subprocess won't be exec'ed. I want to make it a best effort semantics. I liked the idea of using lambda here. I tried to use an anonymous struct functor here. But it does not work on gcc-4.4. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/#review78457 --- On March 31, 2015, 7:32 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated March 31, 2015, 7:32 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32660: Report network isolator statistics on a per container basis (MESOS-2332)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/#review78546 --- include/mesos/mesos.proto https://reviews.apache.org/r/32660/#comment127390 s/it's/its/ include/mesos/mesos.proto https://reviews.apache.org/r/32660/#comment127392 why are these doubles and not uint64? src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/32660/#comment127408 Are these statistics not available through a netlink interface, i.e., using libnl? I wasn't expecting to require parsing tc's output. +chzhcn can you comment here? src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/32660/#comment127410 what does iso mean? src/slave/containerizer/isolators/network/port_mapping.cpp https://reviews.apache.org/r/32660/#comment127409 s/delay/delayed/ src/slave/flags.hpp https://reviews.apache.org/r/32660/#comment127395 we've added flags only for potentially expensive queries; do you expect these to be? also, the name is too generic ;-) - Ian Downes On April 1, 2015, 8:45 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/ --- (Updated April 1, 2015, 8:45 a.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Report network isolator statistics on a per container basis (MESOS-2332) Diffs - include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef Diff: https://reviews.apache.org/r/32660/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/#review78596 --- Patch looks great! Reviews applied: [32694] All tests passed. - Mesos ReviewBot On April 1, 2015, 7:40 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated April 1, 2015, 7:40 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32744: PortMapping: change to not host namespace symlink handles in /var/run/netns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32744/#review78601 --- At least the comments above getContainerIdFromSymlink() definition need to change as well. - Cong Wang On April 1, 2015, 9:19 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32744/ --- (Updated April 1, 2015, 9:19 p.m.) Review request for mesos, Jie Yu and Cong Wang. Bugs: mesos-2574 https://issues.apache.org/jira/browse/mesos-2574 Repository: mesos Description --- MESOS-2574 Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32744/diff/ Testing --- Thanks, Chi Zhang
Review Request 32744: PortMapping: change to not host namespace symlink handles in /var/run/netns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32744/ --- Review request for mesos, Jie Yu and Cong Wang. Bugs: mesos-2574 https://issues.apache.org/jira/browse/mesos-2574 Repository: mesos Description --- MESOS-2574 Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32744/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32742: Added command logging for processes running in slave's cgroup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/#review78591 --- src/slave/slave.cpp https://reviews.apache.org/r/32742/#comment127484 ostringstream? Have you considered just building a setstring and using strings::join(\n, commands) below? Then you don't have to deal with the delimiters in the stringstream and the fact that we're left with an extra newline at the end as a result. - Ben Mahler On April 1, 2015, 8:05 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/ --- (Updated April 1, 2015, 8:05 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2461 https://issues.apache.org/jira/browse/MESOS-2461 Repository: mesos Description --- Added command logging for processes running in slave's cgroup. Diffs - src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 Diff: https://reviews.apache.org/r/32742/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32744: PortMapping: change to not host namespace symlink handles in /var/run/netns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32744/ --- (Updated April 1, 2015, 9:19 p.m.) Review request for mesos, Jie Yu and Cong Wang. Bugs: mesos-2574 https://issues.apache.org/jira/browse/mesos-2574 Repository: mesos Description --- MESOS-2574 Diffs (updated) - src/slave/containerizer/isolators/network/port_mapping.hpp 33837b4662959a003c8f38d1e786c6615287a4ff src/slave/containerizer/isolators/network/port_mapping.cpp e691d463515084518c94cdec3fbdf37be4a72977 src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32744/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
On April 1, 2015, 8:48 p.m., Jie Yu wrote: The diff does seem to be correct. Needs one more rev. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/#review78585 --- On April 1, 2015, 9:30 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 1, 2015, 9:30 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 1, 2015, 9:30 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs (updated) - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/#review78585 --- The diff does seem to be correct. - Jie Yu On April 1, 2015, 8:42 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 1, 2015, 8:42 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32653: Replace busy look on ready file with a more relaxed loop
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/ --- (Updated April 1, 2015, 8:54 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Replace busy look on ready file with a more relaxed loop Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32653/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.
On April 1, 2015, 2:28 a.m., Adam B wrote: src/slave/slave.cpp, line 1043 https://reviews.apache.org/r/32585/diff/1-2/?file=908253#file908253line1043 Hmm... I was actually thinking that this line of code can be removed now that you're always filling in the id in the FrameworkInfo by the time it gets to the slave. However, upon further inspection I see that there is a Slave::Framework::id field that we'll also need to deprecate, ensuring that Slave::Framework::info.id is set, and thus Archive::Framework::framework_info has the frameworkId. Kapil Arya wrote: I am not sure if I understand this. slave::Framework::id has been removed in this patch. I couldn't find `Slave::Framework` in the codebase. Can you point me to that? Right you are. Sorry, it was late and I confused `pid` with `id` and thought you only removed the comment, based on ReviewBoard's improper rendering of https://reviews.apache.org/r/32585/diff/1-2/ Both lines removed. Looks good. Dropping. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/#review78496 --- On April 1, 2015, 12:34 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/ --- (Updated April 1, 2015, 12:34 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- Framework.id() extracts and returns FrameworkID from Framework.info. Diffs - src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32585/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 31228: Added a mechanism for disabling http endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31228/#review78574 --- LGTM! - Isabel Jimenez On April 1, 2015, 9:50 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31228/ --- (Updated April 1, 2015, 9:50 a.m.) Review request for mesos, Joerg Schad, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2333 https://issues.apache.org/jira/browse/MESOS-2333 Repository: mesos Description --- Adds a mechanism for disabling http endpoints (e.g `testprocess/handler1,processname(3)/handler2`). A list of coma separated strings can be provided using the environment variable `LIBPROCESS_DISABLED_ENDPOINTS` which will be read during libprocess initialization. Then, when creating http endpoints (using the method `route`) the endpoint path will be checked against the patterns. If a match is found the endpoint handler will be replaced for a generic once which returns a 403 HTTP Error (Forbidden). Diffs - 3rdparty/libprocess/include/process/process.hpp 392c74df3e8a122aecd3633dffdeec4bcbd1f097 3rdparty/libprocess/src/process.cpp cf4e36489be2c6aa01e838c1c71383f248deab5b 3rdparty/libprocess/src/tests/process_tests.cpp eb38edce2c483ba7f963a826893a15a075238618 Diff: https://reviews.apache.org/r/31228/diff/ Testing --- make check Thanks, Alexander Rojas
Review Request 32742: Added command logging for processes running in slave's cgroup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/ --- Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2461 https://issues.apache.org/jira/browse/MESOS-2461 Repository: mesos Description --- Added command logging for processes running in slave's cgroup. Diffs - src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 Diff: https://reviews.apache.org/r/32742/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 1, 2015, 8:42 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs (updated) - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32700: Removed FrameworkID from FrameworkState.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32700/#review78579 --- Patch looks great! Reviews applied: [32583, 32584, 32585, 32586, 32587, 32700] All tests passed. - Mesos ReviewBot On April 1, 2015, 7:35 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32700/ --- (Updated April 1, 2015, 7:35 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2559 https://issues.apache.org/jira/browse/MESOS-2559 Repository: mesos Description --- FrameworkState already has FrameworkInfo which will have a valid FrameworkID. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs - src/slave/containerizer/docker.cpp e83b912c707a3f2687b09a647a9ed248a940ea97 src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9 src/slave/containerizer/mesos/containerizer.cpp fbd1c0a0e5f4f227adb022f0baaa6d2c7e3ad748 src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 src/slave/state.hpp 31dfdd5a4b644f466756a712deded1b025a73c02 src/slave/state.cpp 35ce70b6702473a3100991372f3ba36bcad391c0 src/slave/status_update_manager.cpp fab8c22d46b8ab0a3c3745541ddc650b574bfbd4 Diff: https://reviews.apache.org/r/32700/diff/ Testing --- make check. TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 31228: Added a mechanism for disabling http endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31228/#review78577 --- 3rdparty/libprocess/include/process/process.hpp https://reviews.apache.org/r/31228/#comment127458 Since this function performs an action, it should be named with a verb, e.g. initializeDisabledEndpoints() or setDisabledEndpoints() or updateDisabledEndpoints(). 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/31228/#comment127454 What makes this the right place in initialize() to call this? Why not earlier or later? And initialize(delegate) is a non-static (library-wide?) method calling your new static method. Are there any problems if your method gets called multiple times? Could this even happen? 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/31228/#comment127457 Since disabledHttpEndpoints is a hashset, couldn't you just use contains()? - Adam B On April 1, 2015, 2:50 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31228/ --- (Updated April 1, 2015, 2:50 a.m.) Review request for mesos, Joerg Schad, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2333 https://issues.apache.org/jira/browse/MESOS-2333 Repository: mesos Description --- Adds a mechanism for disabling http endpoints (e.g `testprocess/handler1,processname(3)/handler2`). A list of coma separated strings can be provided using the environment variable `LIBPROCESS_DISABLED_ENDPOINTS` which will be read during libprocess initialization. Then, when creating http endpoints (using the method `route`) the endpoint path will be checked against the patterns. If a match is found the endpoint handler will be replaced for a generic once which returns a 403 HTTP Error (Forbidden). Diffs - 3rdparty/libprocess/include/process/process.hpp 392c74df3e8a122aecd3633dffdeec4bcbd1f097 3rdparty/libprocess/src/process.cpp cf4e36489be2c6aa01e838c1c71383f248deab5b 3rdparty/libprocess/src/tests/process_tests.cpp eb38edce2c483ba7f963a826893a15a075238618 Diff: https://reviews.apache.org/r/31228/diff/ Testing --- make check Thanks, Alexander Rojas
Review Request 32750: MESOS-2585: Use full width for mesos div.container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32750/ --- Review request for mesos. Bugs: MESOS-2585 https://issues.apache.org/jira/browse/MESOS-2585 Repository: mesos Description --- MESOS-2585: Use full width for mesos div.container Diffs - src/webui/master/static/css/mesos.css 5b1227e9d64757f9fc106e497f7fa3ed72112c10 Diff: https://reviews.apache.org/r/32750/diff/ Testing --- Visual verification. Thanks, Alson Kemp
Re: Review Request 32653: Replace busy look on ready file with a more relaxed loop
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/#review78605 --- src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127506 Kill this extra line. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127519 TODO(pbrett): Consider generalizing this function and moving it to a common header. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127505 static bool waitForFileCreation src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127511 s/timeout/duration/ since we have process::Timeout (Clock aware), it's better to avoid confusion here. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127507 60seconds might be too long. Probably change it to 15 seconds so that it's consistent with AWAIT_READY default. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127520 Please include stout/stopwatch.hpp src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32653/#comment127512 s/Seconds(0)/Duration::zero()/ - Jie Yu On April 1, 2015, 8:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32653/ --- (Updated April 1, 2015, 8:54 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Replace busy look on ready file with a more relaxed loop Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32653/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 31914: cgroups: added memsw_usage_in_bytes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31914/ --- (Updated April 1, 2015, 10:27 p.m.) Review request for mesos and Ian Downes. Bugs: mesos-2104 https://issues.apache.org/jira/browse/mesos-2104 Repository: mesos Description --- cgroups: added memsw_usage_in_bytes. Diffs (updated) - src/linux/cgroups.hpp f3a6c50fd9cc078229b80bac233da57bd97cc6fd src/linux/cgroups.cpp df3211a0c25d7a16f36814886d14f81caaef2b9c Diff: https://reviews.apache.org/r/31914/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 31915: MemIsolator: Improved some statistics naming. (MESOS-2104)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31915/ --- (Updated April 1, 2015, 10:28 p.m.) Review request for mesos, Ian Downes and Paul Brett. Bugs: mesos-2104 https://issues.apache.org/jira/browse/mesos-2104 Repository: mesos Description --- MemIsolator: Improved some statistics naming. (MESOS-2104) Diffs (updated) - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 src/slave/containerizer/isolators/cgroups/mem.cpp a7a83ef9ad4726aa139a92fc7f5917ed687d33f5 Diff: https://reviews.apache.org/r/31915/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 31915: MemIsolator: Improved some statistics naming. (MESOS-2104)
On March 17, 2015, 6:53 p.m., Ian Downes wrote: src/slave/containerizer/isolators/cgroups/mem.cpp, line 441 https://reviews.apache.org/r/31915/diff/1/?file=890842#file890842line441 if this conditional on limitSwap? Do you want it more explicit? If swap is not used, option will be none. On March 17, 2015, 6:53 p.m., Ian Downes wrote: include/mesos/mesos.proto, line 454 https://reviews.apache.org/r/31915/diff/1/?file=890841#file890841line454 For protobuf you cannot re-use tags, they must refer to the same field name or, if they were optional, then you can drop them. See Assigning Tags [here|https://developers.google.com/protocol-buffers/docs/proto] change to minimize protobuf modification and added more comments around the area. - Chi --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31915/#review76770 --- On April 1, 2015, 10:28 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31915/ --- (Updated April 1, 2015, 10:28 p.m.) Review request for mesos, Ian Downes and Paul Brett. Bugs: mesos-2104 https://issues.apache.org/jira/browse/mesos-2104 Repository: mesos Description --- MemIsolator: Improved some statistics naming. (MESOS-2104) Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 src/slave/containerizer/isolators/cgroups/mem.cpp a7a83ef9ad4726aa139a92fc7f5917ed687d33f5 Diff: https://reviews.apache.org/r/31915/diff/ Testing --- Thanks, Chi Zhang
