Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated July 24, 2017, 11:36 a.m.) Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Reza Motamedi. Changes --- Update flags to reflect correct in htb code to use burst, not cburst. Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs (updated) - include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405 src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a Diff: https://reviews.apache.org/r/59294/diff/6/ Changes: https://reviews.apache.org/r/59294/diff/5-6/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 60203: Introduce HTB class.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60203/ --- (Updated July 24, 2017, 11:36 a.m.) Review request for mesos, Ilya Pronin, Jie Yu, Reza Motamedi, Santhosh Kumar Shanmugham, and Cong Wang. Changes --- Fix libnl call to set rbuffer, not cbuffer. Documentation is incorrect/misleading. The correct buffer to set for sending at ceil rate is the rbuffer, not cbuffer. Repository: mesos Description --- Based heavily on Cong Wang's original review 45605. Introduce support for HTB class and expose configuration. Diffs (updated) - src/linux/routing/queueing/class.hpp PRE-CREATION src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 src/linux/routing/queueing/htb.cpp 464120214b75e2e422c8ea6a57c5654ba77d669f src/linux/routing/queueing/internal.hpp 9fe522ee017c86af8c7b2e518cd0957af08750e4 Diff: https://reviews.apache.org/r/60203/diff/3/ Changes: https://reviews.apache.org/r/60203/diff/2-3/ Testing --- make check Thanks, Ian Downes
Re: Review Request 60203: Introduce HTB class.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60203/ --- (Updated June 30, 2017, 4:01 p.m.) Review request for mesos, Ilya Pronin, Jie Yu, Santhosh Kumar Shanmugham, and Cong Wang. Changes --- Addressed comments. Shuffled some code for cleaner naming. Repository: mesos Description --- Based heavily on Cong Wang's original review 45605. Introduce support for HTB class and expose configuration. Diffs (updated) - src/linux/routing/queueing/class.hpp PRE-CREATION src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 src/linux/routing/queueing/htb.cpp 464120214b75e2e422c8ea6a57c5654ba77d669f src/linux/routing/queueing/internal.hpp 9fe522ee017c86af8c7b2e518cd0957af08750e4 Diff: https://reviews.apache.org/r/60203/diff/2/ Changes: https://reviews.apache.org/r/60203/diff/1-2/ Testing --- make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated June 30, 2017, 4 p.m.) Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu. Changes --- Addressed comments. Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs (updated) - include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405 src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a Diff: https://reviews.apache.org/r/59294/diff/5/ Changes: https://reviews.apache.org/r/59294/diff/4-5/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 60203: Introduce HTB class.
- Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60203/#review178789 --- On June 19, 2017, 12:36 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60203/ > --- > > (Updated June 19, 2017, 12:36 p.m.) > > > Review request for mesos, Ilya Pronin, Jie Yu, Santhosh Kumar Shanmugham, and > Cong Wang. > > > Repository: mesos > > > Description > --- > > Based heavily on Cong Wang's original review 45605. > > Introduce support for HTB class and expose configuration. > > > Diffs > - > > src/linux/routing/queueing/class.hpp PRE-CREATION > src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 > src/linux/routing/queueing/htb.cpp 464120214b75e2e422c8ea6a57c5654ba77d669f > src/linux/routing/queueing/internal.hpp > 9fe522ee017c86af8c7b2e518cd0957af08750e4 > > > Diff: https://reviews.apache.org/r/60203/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Ian Downes > >
Re: Review Request 60203: Introduce HTB class.
> On June 23, 2017, 6:02 a.m., Ilya Pronin wrote: > > src/linux/routing/queueing/internal.hpp > > Lines 595-597 (patched) > > <https://reviews.apache.org/r/60203/diff/1/?file=1753516#file1753516line595> > > > > Why don't we handle this here to be consistent with other `*Class()` > > functions? I read through the error handling chain and for rtnl_class_delete it doesn't appear to check for class existence and return an interpretable error code. I've left the current generic error handling and removed the erroneous comment. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60203/#review178789 ------- On June 19, 2017, 12:36 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60203/ > --- > > (Updated June 19, 2017, 12:36 p.m.) > > > Review request for mesos, Ilya Pronin, Jie Yu, Santhosh Kumar Shanmugham, and > Cong Wang. > > > Repository: mesos > > > Description > --- > > Based heavily on Cong Wang's original review 45605. > > Introduce support for HTB class and expose configuration. > > > Diffs > - > > src/linux/routing/queueing/class.hpp PRE-CREATION > src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 > src/linux/routing/queueing/htb.cpp 464120214b75e2e422c8ea6a57c5654ba77d669f > src/linux/routing/queueing/internal.hpp > 9fe522ee017c86af8c7b2e518cd0957af08750e4 > > > Diff: https://reviews.apache.org/r/60203/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Ian Downes > >
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
> On June 26, 2017, 6:08 a.m., Ilya Pronin wrote: > > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > > Lines 2209-2210 (original), 2301-2302 (patched) > > <https://reviews.apache.org/r/59294/diff/2-4/?file=1731976#file1731976line2339> > > > > Do we expect the helper to hang? Is this a safeguard for the new code > > deployment? Nope, it's not expected to hang but the 10 second timeout is indeed a safety check in case something is amiss. This method is only used during recovery and in tests. > On June 26, 2017, 6:08 a.m., Ilya Pronin wrote: > > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > > Lines 4315-4317 (patched) > > <https://reviews.apache.org/r/59294/diff/2-4/?file=1731976#file1731976line4376> > > > > If we set `ceil` low, it will prevent heavy CPU users' from bursting. > > Wouldn't it be more flexible to make `egress_ceil_limit` flag additive > > instead of absolute? Additive burst makes it more complicated as you need to at least min(rate + ceil, max) or scale ceil with cpu as well. Instead, I think it's simpler to scale rate by cpu and burst naturally becomes progressively less useful for higher cpu. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/#review178796 --- On June 21, 2017, 1:48 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59294/ > --- > > (Updated June 21, 2017, 1:48 p.m.) > > > Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu. > > > Bugs: MESOS-7508 > https://issues.apache.org/jira/browse/MESOS-7508 > > > Repository: mesos > > > Description > --- > > Add support to isolators/port_mapping for optionally scaling egress bandwidth > with CPU and with minimum and maximum limits. > > > Diffs > - > > include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e > src/slave/containerizer/mesos/isolators/network/helper.cpp > 4ed879dca42f85fc9dd7638e763822cf10fa8405 > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > 9d38289c7161d5e931053b587d115684ccc44c94 > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > cd008aaebcd42554a9a81d2b059269546f59c966 > src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c > src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e > src/tests/containerizer/port_mapping_tests.cpp > 16e015a8ac53a4aa5336b60c40228720b5f6910a > > > Diff: https://reviews.apache.org/r/59294/diff/4/ > > > Testing > --- > > # added a new test > $ make check > > > Thanks, > > Ian Downes > >
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated June 21, 2017, 1:48 p.m.) Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu. Changes --- Include egress rate limit, burst rate limit, and burst size in ResourceStatistics. Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs (updated) - include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405 src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a Diff: https://reviews.apache.org/r/59294/diff/4/ Changes: https://reviews.apache.org/r/59294/diff/3-4/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated June 21, 2017, 10:27 a.m.) Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu. Changes --- Use HTB class introduced in review 60203 to avoid shelling out. Included support for setting burst rate (ceil) and bucket size (cburst). This is intentionally still in the model of an egress qdisc inside each container. Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs (updated) - src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405 src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a Diff: https://reviews.apache.org/r/59294/diff/3/ Changes: https://reviews.apache.org/r/59294/diff/2-3/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
> On May 17, 2017, 2:14 p.m., Jie Yu wrote: > > src/slave/flags.cpp > > Lines 770-786 (patched) > > <https://reviews.apache.org/r/59294/diff/1/?file=1719990#file1719990line770> > > > > This sounds like a heuristic. Any justification why this heuristic? > > Wondering if label based solution is better? For instance, the isolator > > will look for a special label of the task/executor. The label specifies the > > egress rate limit which can override the default rate limit. Something > > along this line? > > > > Then, the custom logic can be injected into a label decrorator, rather > > than first class it here? > > Ian Downes wrote: > It's not really a heuristic, it's a simple linear model with min/max. The > major benefit is that it enables more effective allocation of a host's egress > bandwidth without exposing bandwidth as a resource. A fixed egress bandwidth > allocates poorly for either a small number of very large containers > (underutilizing) or a large number of small containers (overcommitting). > Scaling with CPU means a large container can get a larger share of the > bandwidth. > > I thought about a label based solution but this doesn't work well with a > heterogenous cluster. We have a mix of 1G and 10G hosts and we'd like to use > different egress_rate_per_cpu depending on the link speed, e.g., 40 Mbps / > core for 1G and 120 Mbps / core for 10 G. The scheduler doesn't (and > shouldn't) know the specifics of hosts beyond resources so unless we make > bandwidth a first class resource I think the logic should be at the isolator. > Host bandwidth could be exposed via an agent attribute but that's *really* > breaking the resource abstraction. > > Santhosh Kumar Shanmugham wrote: > To add to the point - scaling network bandwidth with CPU is typical in > the cloud and we are just mirroring the same feature here. > > ` Each core is subject to a 2 Gbits/second (Gbps) cap for peak > performance. Each additional core increases the network cap, up to a > theoretical maximum of 16 Gbps for each virtual machine; however, the actual > performance you experience can vary depending on your workload.` > > https://cloud.google.com/compute/docs/networks-and-firewalls > > Jie Yu wrote: > > The scheduler doesn't (and shouldn't) know the specifics of hosts > beyond resources so unless we make bandwidth a first class resource I think > the logic should be at the isolator > > Scheduler is not the one that sets the label. The label decrocator on the > agent will be the one doing that. Since the label decroator lives in the > agent, it can automatically detect the link speed and set the label properly. The label decorator could work but the slaveRunTaskLabelDecorator is only on task start. One of the key benefits of the posted code is that it will update existing containers when this policy is introduced and then in the future if config is changed, all without restarting containers. We're going to deploy this code to large running clusters, incrementally increasing the config values. Rolling the clusters at each increment is not feasible and we don't want out-of-band tooling separately re-configuring. - Ian ----------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/#review175114 --- On May 26, 2017, 11:23 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59294/ > --- > > (Updated May 26, 2017, 11:23 a.m.) > > > Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu. > > > Bugs: MESOS-7508 > https://issues.apache.org/jira/browse/MESOS-7508 > > > Repository: mesos > > > Description > --- > > Add support to isolators/port_mapping for optionally scaling egress bandwidth > with CPU and with minimum and maximum limits. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > 9d38289c7161d5e931053b587d115684ccc44c94 > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > cd008aaebcd42554a9a81d2b059269546f59c966 > src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc > src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 > src/tests/containerizer/port_mapping_tests.cpp > d062f2f6bcf7b44dbcde951cdca23b0a2cd42115 > > > Diff: https://reviews.apache.org/r/59294/diff/2/ > > > Testing > --- > > # added a new test > $ make check > > > Thanks, > > Ian Downes > >
Review Request 60203: Introduce HTB class.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60203/ --- Review request for mesos, Ilya Pronin, Jie Yu, Santhosh Kumar Shanmugham, and Cong Wang. Repository: mesos Description --- Based heavily on Cong Wang's original review 45605. Introduce support for HTB class and expose configuration. Diffs - src/linux/routing/queueing/class.hpp PRE-CREATION src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 src/linux/routing/queueing/htb.cpp 464120214b75e2e422c8ea6a57c5654ba77d669f src/linux/routing/queueing/internal.hpp 9fe522ee017c86af8c7b2e518cd0957af08750e4 Diff: https://reviews.apache.org/r/60203/diff/1/ Testing --- make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated May 26, 2017, 11:23 a.m.) Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu. Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs - src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115 Diff: https://reviews.apache.org/r/59294/diff/2/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated May 26, 2017, 11:22 a.m.) Review request for . Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs (updated) - src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115 Diff: https://reviews.apache.org/r/59294/diff/2/ Changes: https://reviews.apache.org/r/59294/diff/1-2/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- (Updated May 26, 2017, 11:22 a.m.) Review request for . Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs (updated) - src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115 Diff: https://reviews.apache.org/r/59294/diff/2/ Changes: https://reviews.apache.org/r/59294/diff/1-2/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
> On May 17, 2017, 2:14 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > > Lines 586-588 (patched) > > <https://reviews.apache.org/r/59294/diff/1/?file=1719988#file1719988line586> > > > > Instead of shelling out, i'd say we just introduce support in the nl > > library. IN fact, we already have a patch chain starts here to support that > > https://reviews.apache.org/r/45605/ Definitely agree that's a better approach but I'd like to get this change in first. I do plan to revive those reviews so that shaping can be moved out to the host side to enable token sharing but that's a much larger code change and also a much harder deploy: this patch will modify running containers in place with the new policy so is easy to deploy. > On May 17, 2017, 2:14 p.m., Jie Yu wrote: > > src/slave/flags.cpp > > Lines 770-786 (patched) > > <https://reviews.apache.org/r/59294/diff/1/?file=1719990#file1719990line770> > > > > This sounds like a heuristic. Any justification why this heuristic? > > Wondering if label based solution is better? For instance, the isolator > > will look for a special label of the task/executor. The label specifies the > > egress rate limit which can override the default rate limit. Something > > along this line? > > > > Then, the custom logic can be injected into a label decrorator, rather > > than first class it here? It's not really a heuristic, it's a simple linear model with min/max. The major benefit is that it enables more effective allocation of a host's egress bandwidth without exposing bandwidth as a resource. A fixed egress bandwidth allocates poorly for either a small number of very large containers (underutilizing) or a large number of small containers (overcommitting). Scaling with CPU means a large container can get a larger share of the bandwidth. I thought about a label based solution but this doesn't work well with a heterogenous cluster. We have a mix of 1G and 10G hosts and we'd like to use different egress_rate_per_cpu depending on the link speed, e.g., 40 Mbps / core for 1G and 120 Mbps / core for 10 G. The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator. Host bandwidth could be exposed via an agent attribute but that's *really* breaking the resource abstraction. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/#review175114 --- On May 15, 2017, 1:56 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59294/ > --- > > (Updated May 15, 2017, 1:56 p.m.) > > > Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh > Kumar Shanmugham. > > > Bugs: MESOS-7508 > https://issues.apache.org/jira/browse/MESOS-7508 > > > Repository: mesos > > > Description > --- > > Add support to isolators/port_mapping for optionally scaling egress bandwidth > with CPU and with minimum and maximum limits. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > 9d38289c7161d5e931053b587d115684ccc44c94 > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > cd008aaebcd42554a9a81d2b059269546f59c966 > src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd > src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c > src/tests/containerizer/port_mapping_tests.cpp > a528382e8b4831b9c7e8dcc877a5e242909f0cd5 > > > Diff: https://reviews.apache.org/r/59294/diff/1/ > > > Testing > --- > > # added a new test > $ make check > > > Thanks, > > Ian Downes > >
Review Request 59294: Optionally scale egress bandwidth with CPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/ --- Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh Kumar Shanmugham. Bugs: MESOS-7508 https://issues.apache.org/jira/browse/MESOS-7508 Repository: mesos Description --- Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits. Diffs - src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94 src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966 src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c src/tests/containerizer/port_mapping_tests.cpp a528382e8b4831b9c7e8dcc877a5e242909f0cd5 Diff: https://reviews.apache.org/r/59294/diff/1/ Testing --- # added a new test $ make check Thanks, Ian Downes
Re: Review Request 53931: Don't expect an init process in a FreeBSD jail.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53931/#review157872 --- Ship it! Ship It! - Ian Downes On Nov. 19, 2016, 11:13 a.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53931/ > --- > > (Updated Nov. 19, 2016, 11:13 a.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-5645 > https://issues.apache.org/jira/browse/MESOS-5645 > > > Repository: mesos > > > Description > --- > > Don't expect an init process in a FreeBSD jail. > > > Diffs > - > > 3rdparty/stout/tests/os/process_tests.cpp > 4cb3b5fab389492bdc1258a27e821e60aef19dc8 > > Diff: https://reviews.apache.org/r/53931/diff/ > > > Testing > --- > > gmake check > > > Thanks, > > David Forsythe > >
Re: Review Request 53927: Check isJailed in tests that call mknod.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53927/#review157871 --- Ship it! Ship It! - Ian Downes On Nov. 18, 2016, 10:45 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53927/ > --- > > (Updated Nov. 18, 2016, 10:45 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-6612 and MESOS-6613 > https://issues.apache.org/jira/browse/MESOS-6612 > https://issues.apache.org/jira/browse/MESOS-6613 > > > Repository: mesos > > > Description > --- > > Check isJailed in tests that call mknod. > > > Diffs > - > > 3rdparty/stout/tests/os/rmdir_tests.cpp > 9aa4059d589e84f3c377163b1d6d2a278d4130b6 > 3rdparty/stout/tests/os_tests.cpp f4b9ad71b22b5cc70a412c9a6a3e21da67121e17 > > Diff: https://reviews.apache.org/r/53927/diff/ > > > Testing > --- > > gmake check > > > Thanks, > > David Forsythe > >
Re: Review Request 53926: Move isJailed for FreeBSD into utils.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53926/#review157869 --- Ship it! Ship It! - Ian Downes On Nov. 19, 2016, 4:42 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53926/ > --- > > (Updated Nov. 19, 2016, 4:42 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-5644 > https://issues.apache.org/jira/browse/MESOS-5644 > > > Repository: mesos > > > Description > --- > > Move isJailed for FreeBSD into utils. > > > Diffs > - > > 3rdparty/stout/include/stout/tests/utils.hpp > 4e5359c82da4a39d4a7093bbfe95afa14e61f69d > 3rdparty/stout/tests/os_tests.cpp f4b9ad71b22b5cc70a412c9a6a3e21da67121e17 > > Diff: https://reviews.apache.org/r/53926/diff/ > > > Testing > --- > > gmake check on FreeBSD > > > Thanks, > > David Forsythe > >
Re: Review Request 53925: Fix wait macros on FreeBSD.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53925/#review157868 --- Ship it! Ship It! - Ian Downes On Nov. 18, 2016, 10:17 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53925/ > --- > > (Updated Nov. 18, 2016, 10:17 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-6611 > https://issues.apache.org/jira/browse/MESOS-6611 > > > Repository: mesos > > > Description > --- > > Fix wait macros on FreeBSD. > > > Diffs > - > > 3rdparty/stout/include/stout/gtest.hpp > b2f75b6c706df9de68edbac86a1e2dec32a574ed > > Diff: https://reviews.apache.org/r/53925/diff/ > > > Testing > --- > > gmake check gets closer to build completion. > > > Thanks, > > David Forsythe > >
Re: Review Request 53913: Disable sentinel checks for clang on FreeBSD.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53913/#review157866 --- Ship it! Ship It! - Ian Downes On Nov. 18, 2016, 4:11 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53913/ > --- > > (Updated Nov. 18, 2016, 4:11 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-6610 > https://issues.apache.org/jira/browse/MESOS-6610 > > > Repository: mesos > > > Description > --- > > Disable sentinel checks for clang on FreeBSD. > > > Diffs > - > > configure.ac 5380cbc6a7951ede2f883f7045952a3f3434479e > > Diff: https://reviews.apache.org/r/53913/diff/ > > > Testing > --- > > gmake on FreeBSD. > > > Thanks, > > David Forsythe > >
Re: Review Request 53912: Fix xattr for FreeBSD.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53912/#review157865 --- Ship it! Ship It! - Ian Downes On Nov. 18, 2016, 4:10 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53912/ > --- > > (Updated Nov. 18, 2016, 4:10 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-6609 > https://issues.apache.org/jira/browse/MESOS-6609 > > > Repository: mesos > > > Description > --- > > Fix xattr for FreeBSD. > > > Diffs > - > > 3rdparty/stout/include/stout/os/posix/xattr.hpp > 518940fdffab38ad97cf229078c4494fa944e1d8 > > Diff: https://reviews.apache.org/r/53912/diff/ > > > Testing > --- > > gmake on FreeBSD, no check run. > > > Thanks, > > David Forsythe > >
Re: Review Request 53882: Fix configure on FreeBSD.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53882/#review157864 --- Ship it! Ship It! - Ian Downes On Nov. 18, 2016, 4:11 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53882/ > --- > > (Updated Nov. 18, 2016, 4:11 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-6607 > https://issues.apache.org/jira/browse/MESOS-6607 > > > Repository: mesos > > > Description > --- > > Fix configure on FreeBSD. > > > Diffs > - > > configure.ac 5380cbc6a7951ede2f883f7045952a3f3434479e > > Diff: https://reviews.apache.org/r/53882/diff/ > > > Testing > --- > > ../configure > > > Thanks, > > David Forsythe > >
Review Request 53009: Add support for task labels to example no_executor_framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53009/ --- Review request for mesos, Benjamin Mahler and Ilya Pronin. Repository: mesos Description --- Add support for task labels to example no_executor_framework. Diffs - src/examples/no_executor_framework.cpp e82ae9a9ea23c7d181b071f0e3f5071b3856d8a4 Diff: https://reviews.apache.org/r/53009/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 50540: Add systemd watchdog support.
(lines 112 - 113) <https://reviews.apache.org/r/50540/#comment211389> Use stout's `bool strings::contains()` src/tests/linux/systemd_tests.cpp (line 116) <https://reviews.apache.org/r/50540/#comment211391> ditto src/tests/linux/systemd_tests.cpp (lines 122 - 123) <https://reviews.apache.org/r/50540/#comment211390> Does this validate that the service has actually been stopped or just that systemd accepted the stop command? src/tests/linux/systemd_tests.cpp (line 126) <https://reviews.apache.org/r/50540/#comment211392> What happens if there's an assertion during the test!? Because this is outside the test sandbox it'll leak. See earlier comment about writing and referencing this from the sandbox. - Ian Downes On July 27, 2016, 4:38 p.m., Lawrence Wu wrote: > > ----------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50540/ > --- > > (Updated July 27, 2016, 4:38 p.m.) > > > Review request for mesos, David Robinson and Ian Downes. > > > Bugs: MESOS-5376 > https://issues.apache.org/jira/browse/MESOS-5376 > > > Repository: mesos > > > Description > --- > > Add systemd watchdog support. > > > Diffs > - > > configure.ac d2136909b7305498ae901a5ea00133142b77f9e6 > src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 > src/linux/systemd.hpp 91134f1d4b100759e45931bd09ca4e1e1aeaaf8a > src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 > src/slave/main.cpp 4624392d30cf391015dcd63f447fe2414a47a16a > src/tests/linux/systemd_test_helper.hpp PRE-CREATION > src/tests/linux/systemd_test_helper.cpp PRE-CREATION > src/tests/linux/systemd_test_helper_main.cpp PRE-CREATION > src/tests/linux/systemd_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/50540/diff/ > > > Testing > --- > > Tested by sending SIGSTOP to running mesos and verifying via journalctl that > it was killed by the watchdog. > > The test I wrote for this does the following: > - build up a unit file as a string and create a unit file in > /etc/systemd/system/systemd-test-helper.service > - reload the systemd daemon and start the newly discovered helper service > - wait a bit (30s) to make sure the watchdog has had a chance to kill the > service > - use systemctl status systemd-test-helper to check that the service is still > running > - clean up the unit file. > > TODO: create a similar test, but send a SIGSTOP to the service and ensure > that it has been killed by watchdog. > > > Thanks, > > Lawrence Wu > >
Re: Review Request 49273: Helper binary for executors to chroot tasks.
> On June 27, 2016, 4:41 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos-chroot.cpp, lines 107-110 > > <https://reviews.apache.org/r/49273/diff/1/?file=1431194#file1431194line107> > > > > Hum, that reminds me that there's a bug in the current command executor > > impl. Since pivot_root will mutate the mount table, it actually has side > > effects (that's the reason there is a command called pivot_root, and you > > don't have to specify commands, unlike chroot or su)! For command tasks, we > > need to make sure the health check program is also accessible inside the > > chroot. We need to refactor that part to use a libprocess Process instead > > of a new binary. > > > > Ideally, command executor will use the same utility to create user > > task. However, to simply some of the tooling (e.g., mesos exec) for command > > tasks, we need to make sure executor and the actual tasks are in the same > > mount namespace (otherwise, how does the CLI find the namespace handle for > > the task). That makes me wonder that should we make 'unshare' optional here > > (via a flag)? > > > > Also, since binary does more than just chroot. It also set uid/gids, > > chroot, chdir. We need a better name for the subcommand? Ideally, if we can > > merge this impl. with `src/slave/containerizer/mesos/launch.cpp`, that'll > > be awesome (as you can see, most of the logics are the same). > > > > For now, if you feel there're too much work, we can just name it > > `LAUNCH_TASK`. Just to clarify, the requirement for the executor and the health check binary to be in the same mount ns is only for the command executor? - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49273/#review139601 --- On June 27, 2016, 10:09 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49273/ > --- > > (Updated June 27, 2016, 10:09 a.m.) > > > Review request for mesos, Joshua Cohen and Jie Yu. > > > Repository: mesos > > > Description > --- > > Uses the same code as the agent uses for chroot'ing on Linux, i.e., > pivot_root and setting up /dev etc. Intention is that executors (like > Aurora's Thermos) can use it to chroot tasks. > > Currently, the root path is specified as a flag and the remaining arguments > are exec'ed. Joshua has also requested that the root path could be specified > as the first arg. @Jie, thoughts? > > > Diffs > - > > src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 > src/slave/containerizer/mesos-chroot.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/49273/diff/ > > > Testing > --- > > Manual. > > > Thanks, > > Ian Downes > >
Re: Review Request 49273: Helper binary for executors to chroot tasks.
> On July 4, 2016, 10:06 a.m., Jie Yu wrote: > > Please see the test section of https://reviews.apache.org/r/49569/ > > > > Let me know if that's ok or not. Thanks! > > Ian Downes wrote: > Hey, thanks for looking at this. I looked at the review for > `mesos-containerizer launch` and it just optionally creates a new mount ns. > So, to actually "chroot" a task we still need something like the > functionality in this review? Actually, ignore my comment here, I'm reading more of the functionality contained within the launch command and it may be sufficient. :-) - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49273/#review140687 ------- On June 27, 2016, 10:09 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49273/ > --- > > (Updated June 27, 2016, 10:09 a.m.) > > > Review request for mesos, Joshua Cohen and Jie Yu. > > > Repository: mesos > > > Description > --- > > Uses the same code as the agent uses for chroot'ing on Linux, i.e., > pivot_root and setting up /dev etc. Intention is that executors (like > Aurora's Thermos) can use it to chroot tasks. > > Currently, the root path is specified as a flag and the remaining arguments > are exec'ed. Joshua has also requested that the root path could be specified > as the first arg. @Jie, thoughts? > > > Diffs > - > > src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 > src/slave/containerizer/mesos-chroot.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/49273/diff/ > > > Testing > --- > > Manual. > > > Thanks, > > Ian Downes > >
Re: Review Request 49273: Helper binary for executors to chroot tasks.
> On July 4, 2016, 10:06 a.m., Jie Yu wrote: > > Please see the test section of https://reviews.apache.org/r/49569/ > > > > Let me know if that's ok or not. Thanks! Hey, thanks for looking at this. I looked at the review for `mesos-containerizer launch` and it just optionally creates a new mount ns. So, to actually "chroot" a task we still need something like the functionality in this review? - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49273/#review140687 --- On June 27, 2016, 10:09 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49273/ > --- > > (Updated June 27, 2016, 10:09 a.m.) > > > Review request for mesos, Joshua Cohen and Jie Yu. > > > Repository: mesos > > > Description > --- > > Uses the same code as the agent uses for chroot'ing on Linux, i.e., > pivot_root and setting up /dev etc. Intention is that executors (like > Aurora's Thermos) can use it to chroot tasks. > > Currently, the root path is specified as a flag and the remaining arguments > are exec'ed. Joshua has also requested that the root path could be specified > as the first arg. @Jie, thoughts? > > > Diffs > - > > src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 > src/slave/containerizer/mesos-chroot.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/49273/diff/ > > > Testing > --- > > Manual. > > > Thanks, > > Ian Downes > >
Review Request 49273: Helper binary for executors to chroot tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49273/ --- Review request for mesos, Joshua Cohen and Jie Yu. Repository: mesos Description --- Uses the same code as the agent uses for chroot'ing on Linux, i.e., pivot_root and setting up /dev etc. Intention is that executors (like Aurora's Thermos) can use it to chroot tasks. Currently, the root path is specified as a flag and the remaining arguments are exec'ed. Joshua has also requested that the root path could be specified as the first arg. @Jie, thoughts? Diffs - src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 src/slave/containerizer/mesos-chroot.cpp PRE-CREATION Diff: https://reviews.apache.org/r/49273/diff/ Testing --- Manual. Thanks, Ian Downes
Re: Review Request 44475: Improve master slaves metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44475/#review122425 --- Please see my review for https://reviews.apache.org/r/44474/ which describes why I think this is an unexpected fundamental change in how the metrics are counted. - Ian Downes On March 7, 2016, 2:51 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44475/ > --- > > (Updated March 7, 2016, 2:51 p.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-4740 > https://issues.apache.org/jira/browse/MESOS-4740 > > > Repository: mesos > > > Description > --- > > Avoid iterate the list of slaves, instead just maintain some counters. > > > Diffs > - > > src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 > src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f > src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 > src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 > > Diff: https://reviews.apache.org/r/44475/diff/ > > > Testing > --- > > Manual check > > > Thanks, > > Cong Wang > >
Re: Review Request 44474: Improve master tasks metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44474/#review122423 --- Please see the first comment... I think this unexpectedly introduces a fundamental change in how the metrics are counted. src/master/master.cpp (line 6461) <https://reviews.apache.org/r/44474/#comment184411> I'm not familiar with this code but it appears to be changing the behavior substantially. The original code sums across all slaves that are registered at the time of the metric call. The new code tracks counters for the various states when a task on a registered slave changes. IIUC, this is a fundamental change in how the metrics are determined because the slave registration state is considered at different times. For example, a task may change state on a registered slave and it gets counted, then the slave becomes unregistered, then the metric endpoint is queried. Old and new code will give different numbers? If my understanding is correct, then perhaps (somewhat) more performant code could be achieved by maintaining counters at the slave level, and then aggregating the counters from *registered* slaves? src/master/metrics.cpp (lines 38 - 46) <https://reviews.apache.org/r/44474/#comment184414> Please keep the original order which more closely matches the progression of states, i.e., staging before starting. src/master/metrics.cpp (line 193) <https://reviews.apache.org/r/44474/#comment184416> foreach? - Ian Downes On March 7, 2016, 2:51 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44474/ > --- > > (Updated March 7, 2016, 2:51 p.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-4740 > https://issues.apache.org/jira/browse/MESOS-4740 > > > Repository: mesos > > > Description > --- > > Avoid iterate the list of slaves, instead just maintain some counters. > > > Diffs > - > > src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 > src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f > src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 > src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 > > Diff: https://reviews.apache.org/r/44474/diff/ > > > Testing > --- > > Manual check > > > Thanks, > > Cong Wang > >
Re: Review Request 43283: Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1.
> On Feb. 8, 2016, 2:52 p.m., Ian Downes wrote: > > src/linux/perf.cpp, lines 423-426 > > <https://reviews.apache.org/r/43283/diff/1/?file=1237011#file1237011line423> > > > > Hmmm, I'm not satisfied with this. I thought the new output format was > > introduced at a specific kernel version (3.13.0 might not be correct). > > Could you please confirm that the 3.10 on CentOS7.1 has the new unit field > > but other 3.10 kernels don't. > > haosdent huang wrote: > The interesting thing here is I saw `unit` appear since 3.14 > http://lxr.free-electrons.com/source/tools/perf/util/evsel.h?v=3.14#L72 I > think I missing something, let me find out more details about this. > > haosdent huang wrote: > According > https://github.com/torvalds/linux/blob/v3.13/tools/perf/builtin-stat.c#L1190-L1206 > , I think the `perf stat` format for 3.13.0 also is `value,event,cgroup`. > The `unit` added in this commit which contains since 3.14. > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=410136f5dd96b6013fe6d1011b523b1c247e1ccb > Let me try create a virtual machine to verify this. > > haosdent huang wrote: > I try this in > > ``` > # cat /etc/issue > Ubuntu 14.04.3 LTS \n \l > ``` > > Kernel is > > ``` > uname -a > Linux haosdent 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15 03:51:08 UTC > 2014 x86_64 x86_64 x86_64 GNU/Linux > > > The `perf stat` output don't contains unit: > > ``` > perf stat --all-cpus --field-separator , --log-fd 1 --event cycles > --cgroup mesos --event task-clock --cgroup mesos -- sleep 0.25 > ,cycles,mesos > ,task-clock,mesos > ``` Okay, if I'm understanding you correctly the value,unit,event,cgroup format came in 3.14.0, not 3.13.0 as the current code states. Can we not adjust the cases to correctly match the precise kernel versions? - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43283/#review118314 --- On Feb. 13, 2016, 11:59 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43283/ > --- > > (Updated Feb. 13, 2016, 11:59 p.m.) > > > Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett. > > > Bugs: MESOS-4655 > https://issues.apache.org/jira/browse/MESOS-4655 > > > Repository: mesos > > > Description > --- > > Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1. > > > Diffs > - > > src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 > > Diff: https://reviews.apache.org/r/43283/diff/ > > > Testing > --- > > This also fix similar error in > `CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`. > > > Thanks, > > haosdent huang > >
Re: Review Request 43896: Removed the restriction that /tmp needs to be writable in new rootfs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43896/#review120381 --- Fix it, then Ship it! src/linux/fs.cpp (line 600) <https://reviews.apache.org/r/43896/#comment181824> probably unnecessary but since this is specifically for the pivot you could limit the size and make noexec too? src/linux/fs.cpp (line 603) <https://reviews.apache.org/r/43896/#comment181822> Clarify the message that it's a *temporary* tmpfs mount? - Ian Downes On Feb. 23, 2016, 12:12 p.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43896/ > --- > > (Updated Feb. 23, 2016, 12:12 p.m.) > > > Review request for mesos, Gilbert Song, Ian Downes, Jojy Varghese, and > Timothy Chen. > > > Bugs: MESOS-4291 > https://issues.apache.org/jira/browse/MESOS-4291 > > > Repository: mesos > > > Description > --- > > Removed the restriction that /tmp needs to be writable in new rootfs. > > > Diffs > - > > src/linux/fs.cpp 0df194207a66bf4c417349bebcc8912acbf5976e > > Diff: https://reviews.apache.org/r/43896/diff/ > > > Testing > --- > > sudo make check > > > Thanks, > > Jie Yu > >
Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly
> On Dec. 15, 2015, 11:36 a.m., Ian Downes wrote:
> > src/slave/containerizer/docker.cpp, line 838
> > <https://reviews.apache.org/r/33174/diff/2/?file=927472#file927472line838>
> >
> > Docker supports specifying the CFS period and quota to run a container
> > with these flags {{--cpu-period=0}} and {{--cpu-quota=0}}. The correct
> > solution is to set these using Docker itself when it runs the container.
> > Subsequent updates will be done by Mesos in the update() call, as below in
> > this review.
Please add a TODO for this.
> On Dec. 15, 2015, 11:36 a.m., Ian Downes wrote:
> > src/slave/containerizer/docker.cpp, line 976
> > <https://reviews.apache.org/r/33174/diff/2/?file=927472#file927472line976>
> >
> > To reiterate my earlier comment:
> > 1) We don't support changing the cfs period so it only needs to be set
> > once, additional writes are unnecessary but aren't a problem. The
> > MesosContainerizer isolator just writes it on every update rather than
> > determining if it has already written it before.
> > 2) The cfs quota changes depending on the CPU resource value so it
> > definitely does need to be re-written at every update. Again, this is a
> > straight copy-and-paste from the MC cpu isolator.
> >
> > This part of the code looks fine to me. @Tim, do you concur?
@tnachen?
- Ian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33174/#review110547
---
On Dec. 15, 2015, 12:14 p.m., Steve Niemitz wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
>
> (Updated Dec. 15, 2015, 12:14 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Fix for docker containerizer not configuring CFS quotas correctly.
>
> It would be nice to refactor all this isolation code in a way that can be
> shared between all containerizers, as this is basically just copied from the
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
>
>
> Diffs
> -
>
> src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e
>
> Diff: https://reviews.apache.org/r/33174/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33174/#review119701 --- src/slave/containerizer/docker.cpp (line 812) <https://reviews.apache.org/r/33174/#comment181038> Why not call update() here, rather than _update()? It will determine the correct pid, just launched in launchExecutorContainer(). - Ian Downes On Dec. 15, 2015, 12:14 p.m., Steve Niemitz wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33174/ > --- > > (Updated Dec. 15, 2015, 12:14 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen. > > > Bugs: MESOS-2617 > https://issues.apache.org/jira/browse/MESOS-2617 > > > Repository: mesos > > > Description > --- > > Fix for docker containerizer not configuring CFS quotas correctly. > > It would be nice to refactor all this isolation code in a way that can be > shared between all containerizers, as this is basically just copied from the > CgroupsCpushareIsolator, but that's a much bigger undertaking. > > > Diffs > - > > src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e > > Diff: https://reviews.apache.org/r/33174/diff/ > > > Testing > --- > > > Thanks, > > Steve Niemitz > >
Re: Review Request 43730: Added SNMP statistics to v1 mesos.proto too.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43730/#review119687 --- Ship it! Ship It! - Ian Downes On Feb. 18, 2016, 11:15 a.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43730/ > --- > > (Updated Feb. 18, 2016, 11:15 a.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Neil Conway. > > > Repository: mesos > > > Description > --- > > As Neil Conway noticed, we should add these SNMP statistics to v1 mesos.proto > too. > > > Diffs > - > > include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 > > Diff: https://reviews.apache.org/r/43730/diff/ > > > Testing > --- > > make > > > Thanks, > > Cong Wang > >
Re: Review Request 38117: Added per container SNMP statistics.
> On Feb. 18, 2016, 9:49 a.m., Neil Conway wrote: > > Seems like this commit should have updated the `v1` version of > > `mesos.proto`. Ahh, you're probably right. @cwang could you please submit a fix. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38117/#review119643 --- On Feb. 10, 2016, 4:48 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38117/ > --- > > (Updated Feb. 10, 2016, 4:48 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-3365 > https://issues.apache.org/jira/browse/MESOS-3365 > > > Repository: mesos > > > Description > --- > > Added per container SNMP statistics. > > > Diffs > - > > docs/configuration.md b26a058001dd8419b701a3cbea063a9d58b9 > include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > ebf820a2813eef32416f1465eff3f6eea153492f > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > 1c2fbe934baabd1d816018de0c077bc9f63e9d33 > src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d > src/slave/flags.cpp 2ce7f921c9f53f360143b6927d0aaf78a8b958e7 > src/tests/containerizer/port_mapping_tests.cpp > aa9846097feda1a82131b67aa4c782ec3625d049 > > Diff: https://reviews.apache.org/r/38117/diff/ > > > Testing > --- > > ./src/mesos-network-helper statistics --eth0_name=X --enable_snmp_statistics > --pid= > > > Thanks, > > Cong Wang > >
Re: Review Request 43283: Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43283/#review118314 --- src/linux/perf.cpp (lines 423 - 426) <https://reviews.apache.org/r/43283/#comment179540> Hmmm, I'm not satisfied with this. I thought the new output format was introduced at a specific kernel version (3.13.0 might not be correct). Could you please confirm that the 3.10 on CentOS7.1 has the new unit field but other 3.10 kernels don't. - Ian Downes On Feb. 7, 2016, 12:18 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43283/ > --- > > (Updated Feb. 7, 2016, 12:18 a.m.) > > > Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett. > > > Bugs: MESOS-4039 > https://issues.apache.org/jira/browse/MESOS-4039 > > > Repository: mesos > > > Description > --- > > Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1. > > > Diffs > - > > src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 > > Diff: https://reviews.apache.org/r/43283/diff/ > > > Testing > --- > > This also fix similar error in > `CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`. > > > Thanks, > > haosdent huang > >
Re: Review Request 43284: Wait for perf statistics processes exit.
> On Feb. 8, 2016, 2:47 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp, lines
> > 132-134
> > <https://reviews.apache.org/r/43284/diff/1/?file=1237013#file1237013line132>
> >
> > I think you should discard the future and let it do the correct thing
> > -- kill and reap the perf process -- rather than do a blocking await. The
> > perf sample interval is configurable and typical values are 10's of seconds.
> >
> > Note: I'm not sure if the correct behavior is implemented...
Actually, it looks like it might:
```
virtual void initialize()
{
// Stop when no one cares.
promise.future().onDiscard(lambda::bind(
static_cast(terminate), self(), true));
execute();
}
virtual void finalize()
{
// Kill the perf process (if it's still running) by sending
// SIGTERM to the signal handler which will then SIGKILL the
// perf process group created by setupChild.
if (perf.isSome() && perf->status().isPending()) {
kill(perf->pid(), SIGTERM);
}
promise.discard();
}
```
- Ian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43284/#review118312
---
On Feb. 7, 2016, 12:19 a.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43284/
> -----------
>
> (Updated Feb. 7, 2016, 12:19 a.m.)
>
>
> Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett.
>
>
> Bugs: MESOS-4039
> https://issues.apache.org/jira/browse/MESOS-4039
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Wait for perf statistics processes exit.
>
>
> Diffs
> -
>
> src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp
> 65e731886b9e5cac07ae3ad6398faf8f50de5650
> src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp
> 5ef4ae5c468580352cd16e7716b9ca4c0acde659
>
> Diff: https://reviews.apache.org/r/43284/diff/
>
>
> Testing
> ---
>
> Without this patch, when running
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh
> --gtest_filter="PerfEventIsolatorTest.ROOT_CGROUPS_Sample" --verbose
> ```
> , would got this error
> ```
> [--] Global test environment tear-down
> ../../src/tests/environment.cpp:732: Failure
> Failed
> Tests completed with child processes remaining:
> -+- 16501 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests
> --gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose
> |-+- 16580 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests
> --gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose
> | -+- 16582 perf stat --all-cpus --field-separator , --log-fd 1 --event
> cycles --cgroup mesos/239d30bb-f7a1-413b-9d99-0914149d5899 --event task-clock
> --cgroup mesos/239d30bb-f7a1-413b-9d99-0914149d5899 -- sleep 0.25
> | --- 16584 sleep 0.25
> --- 16581 ()
> [==] 1 test from 1 test case ran. (4095 ms total)
> ```
>
> This also fix similar error in
> `MesosContainerizerSlaveRecoveryTest.CGROUPS_ROOT_PerfRollForward` and
> `CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 43284: Wait for perf statistics processes exit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43284/#review118312 --- src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp (lines 132 - 134) <https://reviews.apache.org/r/43284/#comment179538> I think you should discard the future and let it do the correct thing -- kill and reap the perf process -- rather than do a blocking await. The perf sample interval is configurable and typical values are 10's of seconds. Note: I'm not sure if the correct behavior is implemented... - Ian Downes On Feb. 7, 2016, 12:19 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43284/ > --- > > (Updated Feb. 7, 2016, 12:19 a.m.) > > > Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett. > > > Bugs: MESOS-4039 > https://issues.apache.org/jira/browse/MESOS-4039 > > > Repository: mesos > > > Description > --- > > Wait for perf statistics processes exit. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp > 65e731886b9e5cac07ae3ad6398faf8f50de5650 > src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp > 5ef4ae5c468580352cd16e7716b9ca4c0acde659 > > Diff: https://reviews.apache.org/r/43284/diff/ > > > Testing > --- > > Without this patch, when running > ``` > sudo GLOG_v=1 ./bin/mesos-tests.sh > --gtest_filter="PerfEventIsolatorTest.ROOT_CGROUPS_Sample" --verbose > ``` > , would got this error > ``` > [--] Global test environment tear-down > ../../src/tests/environment.cpp:732: Failure > Failed > Tests completed with child processes remaining: > -+- 16501 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests > --gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose > |-+- 16580 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests > --gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose > | -+- 16582 perf stat --all-cpus --field-separator , --log-fd 1 --event > cycles --cgroup mesos/239d30bb-f7a1-413b-9d99-0914149d5899 --event task-clock > --cgroup mesos/239d30bb-f7a1-413b-9d99-0914149d5899 -- sleep 0.25 > | --- 16584 sleep 0.25 > --- 16581 () > [==] 1 test from 1 test case ran. (4095 ms total) > ``` > > This also fix similar error in > `MesosContainerizerSlaveRecoveryTest.CGROUPS_ROOT_PerfRollForward` and > `CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`. > > > Thanks, > > haosdent huang > >
Re: Review Request 42757: Split os::memory() out into platform specific files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42757/#review118301 --- Ship it! Ship It! - Ian Downes On Feb. 5, 2016, 9:35 a.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42757/ > --- > > (Updated Feb. 5, 2016, 9:35 a.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-4504 > https://issues.apache.org/jira/browse/MESOS-4504 > > > Repository: mesos > > > Description > --- > > Split os::memory() out into platform specific files. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/freebsd.hpp > 10346dea8d658ea0a9b658a1d72eee8cc3056da8 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/linux.hpp > f8a4d5ae12cfc2d2bd4a24226e200e1aabe0b8a8 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp > cc9bfe8942db7e62f5d415fc500b78adf2ac9759 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp > 0e897ea15d6b3ed76a8a264726db70980bb9517a > 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp > b47187870d6d8d21425253dac481e28102e8dcc9 > > Diff: https://reviews.apache.org/r/42757/diff/ > > > Testing > --- > > gmake check on FreeBSD and Ubuntu. > > > Thanks, > > David Forsythe > >
Re: Review Request 42735: Make bash scripts portable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42735/#review118297 --- Ship it! Ship It! - Ian Downes On Feb. 5, 2016, 8:51 a.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42735/ > --- > > (Updated Feb. 5, 2016, 8:51 a.m.) > > > Review request for mesos, Artem Harutyunyan and Ian Downes. > > > Bugs: MESOS-4502 > https://issues.apache.org/jira/browse/MESOS-4502 > > > Repository: mesos > > > Description > --- > > Make bash scripts portable. > > > Diffs > - > > support/atexit.sh 90696a2d426ff013d1aaec7800c6dc880bc2a00c > support/coverage.sh df81f9a45d043d506f47e9d6fac6893bf044144a > support/docker_build.sh 55d402e35d6d391fd483c47b69b64c1ff99569d3 > support/hooks/commit-msg d173dfdf380b08a9b349589dfd33c53b3cdccc60 > support/release.sh 633bbace091bddd582b9fcef5d35b331a0f169f0 > support/tag.sh 9d37c81a13cb0c281f4a53884c6c55e09c341d85 > support/vote.sh 218a38512d4dc8e160d975a99c577e4411879eee > > Diff: https://reviews.apache.org/r/42735/diff/ > > > Testing > --- > > > Thanks, > > David Forsythe > >
Re: Review Request 42735: Make commit-msg hook portable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42735/#review116182 --- Can you please fix the other bash Linux-isms in this patch while you're at it? ``` [1350][idownes:mesos]$ git grep "!\/bin\/bash" support/atexit.sh:#!/bin/bash support/coverage.sh:#!/bin/bash support/docker_build.sh:#!/bin/bash support/hooks/commit-msg:#!/bin/bash support/release.sh:#!/bin/bash support/tag.sh:#!/bin/bash support/vote.sh:#!/bin/bash ``` - Ian Downes On Jan. 25, 2016, 12:07 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42735/ > --- > > (Updated Jan. 25, 2016, 12:07 p.m.) > > > Review request for mesos, Artem Harutyunyan and Ian Downes. > > > Bugs: MESOS-4502 > https://issues.apache.org/jira/browse/MESOS-4502 > > > Repository: mesos > > > Description > --- > > Make commit-msg hook portable. > > > Diffs > - > > support/hooks/commit-msg d173dfdf380b08a9b349589dfd33c53b3cdccc60 > > Diff: https://reviews.apache.org/r/42735/diff/ > > > Testing > --- > > > Thanks, > > David Forsythe > >
Re: Review Request 41726: Implement os::memory() for FreeBSD.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41726/#review115510 --- Ship it! Ship It! - Ian Downes On Jan. 7, 2016, 4:52 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41726/ > --- > > (Updated Jan. 7, 2016, 4:52 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-4251 > https://issues.apache.org/jira/browse/MESOS-4251 > > > Repository: mesos > > > Description > --- > > Implement os::memory() for FreeBSD. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp > 4cf693fb7e8c6bb3ad1920ebe90d61f0adb5dc99 > > Diff: https://reviews.apache.org/r/41726/diff/ > > > Testing > --- > > gmake check > > > Thanks, > > David Forsythe > >
Re: Review Request 41911: Added a test case for corrupt packets
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/#review115242
---
src/tests/containerizer/port_mapping_tests.cpp (line 23)
<https://reviews.apache.org/r/41911/#comment176149>
Alphabetize (or comment why this order is needed).
src/tests/containerizer/port_mapping_tests.cpp (line 994)
<https://reviews.apache.org/r/41911/#comment176151>
s/ */* /
This is a strange interface... why not take a `const char* data` and a
`unsigned size`? and then cast the `data` appropriately.
This version from `http://locklessinc.com/articles/tcp_checksum/` is
clearer:
```
unsigned short checksum1(const char *buf, unsigned size)
{
unsigned sum = 0;
int i;
/* Accumulate checksum */
for (i = 0; i < size - 1; i += 2)
{
unsigned short word16 = *(unsigned short *) &buf[i];
sum += word16;
}
/* Handle odd-sized case */
if (size & 1)
{
unsigned short word16 = (unsigned char) buf[i];
sum += word16;
}
/* Fold to get the ones-complement result */
while (sum >> 16) sum = (sum & 0x)+(sum >> 16);
/* Invert to get the negative in ones-complement arithmetic */
return ~sum;
}
```
src/tests/containerizer/port_mapping_tests.cpp (line 995)
<https://reviews.apache.org/r/41911/#comment176154>
Is this is standard IP checksum on the IP header? Please comment as such.
src/tests/containerizer/port_mapping_tests.cpp (line 1012)
<https://reviews.apache.org/r/41911/#comment176150>
s/-/ - /
src/tests/containerizer/port_mapping_tests.cpp (line 1027)
<https://reviews.apache.org/r/41911/#comment176267>
This function is doing a lot, both constructing the packet, opening a
socket and sending the packet. What about splitting this functionality? One
function to construct, another to open/send/close?
src/tests/containerizer/port_mapping_tests.cpp (line 1038)
<https://reviews.apache.org/r/41911/#comment176178>
s/s/socket/
src/tests/containerizer/port_mapping_tests.cpp (line 1047)
<https://reviews.apache.org/r/41911/#comment176182>
s/iph/ipHeader/
src/tests/containerizer/port_mapping_tests.cpp (line 1048)
<https://reviews.apache.org/r/41911/#comment176183>
s/udph/updHeader/
src/tests/containerizer/port_mapping_tests.cpp (line 1050)
<https://reviews.apache.org/r/41911/#comment176180>
What's stopping this writing beyond `datagram`?
src/tests/containerizer/port_mapping_tests.cpp (line 1082)
<https://reviews.apache.org/r/41911/#comment176184>
s/psize/pseudogramSize/
size_t?
src/tests/containerizer/port_mapping_tests.cpp (line 1086)
<https://reviews.apache.org/r/41911/#comment176265>
For multiline, please split all arguments to separate lines.
src/tests/containerizer/port_mapping_tests.cpp (line 1135)
<https://reviews.apache.org/r/41911/#comment176188>
drop the "1" when there's only a single usage.
src/tests/containerizer/port_mapping_tests.cpp (line 1149)
<https://reviews.apache.org/r/41911/#comment176189>
ditto, drop the "1".
src/tests/containerizer/port_mapping_tests.cpp (line 1172)
<https://reviews.apache.org/r/41911/#comment176264>
s/> >/>>/
No need to have space between > chevrons now.
- Ian Downes
On Jan. 14, 2016, 12:03 p.m., Cong Wang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41911/
> ---
>
> (Updated Jan. 14, 2016, 12:03 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Add a test case to ensure no corrupt packet could be delivered to application.
>
>
> Diffs
> -
>
> src/tests/containerizer/port_mapping_tests.cpp
> e3aea53468fa00374320a8b89bdbb64f38e44b01
>
> Diff: https://reviews.apache.org/r/41911/diff/
>
>
> Testing
> ---
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 38117: Export per container SNMP statistics
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38117/#review110563 --- Looks good, I just want to pull that parsing code out so it can be tested. src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 1131 - 1173) <https://reviews.apache.org/r/38117/#comment170501> Can you please pull the parsing code out into a function and add some tests around it? One test could read the host's /proc/net/snmp and test parsing is sucessful, further tests should ensure known values are parsed correctly. - Ian Downes On Dec. 14, 2015, 4:05 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38117/ > --- > > (Updated Dec. 14, 2015, 4:05 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-3365 > https://issues.apache.org/jira/browse/MESOS-3365 > > > Repository: mesos > > > Description > --- > > These stats are those we get by `netstat -s`, they are important for diagnose > networking issues. > > > Diffs > - > > docs/configuration.md c75f56ce849f6960b3b6246bfa6949156a82eabb > include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > cbb94077d46d7b87ffc09b72e02269bc16f25f92 > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > 49e215ba3502bba029956fedfc8bd828c3b4a028 > src/slave/flags.hpp 1250786d1bdc8b312a1912a37ac8aac373dd5ec9 > src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806 > > Diff: https://reviews.apache.org/r/38117/diff/ > > > Testing > --- > > ./src/mesos-network-helper statistics --eth0_name=X --enable_snmp_statistics > --pid= > > > Thanks, > > Cong Wang > >
Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33174/#review110547
---
Initial CFS parameters should be specified to Docker using the appropriate
flags, not tacked onto the end of launch() where we don't yet know the cgroup.
Subsequent updates done by Mesos in update().
src/slave/containerizer/docker.cpp (line 838)
<https://reviews.apache.org/r/33174/#comment170483>
Docker supports specifying the CFS period and quota to run a container with
these flags {{--cpu-period=0}} and {{--cpu-quota=0}}. The correct solution is
to set these using Docker itself when it runs the container. Subsequent updates
will be done by Mesos in the update() call, as below in this review.
src/slave/containerizer/docker.cpp (line 839)
<https://reviews.apache.org/r/33174/#comment170480>
FYI, it would be {{.then([]() { return true; });}} but this piece of code
isn't needed, see previous comment.
src/slave/containerizer/docker.cpp (line 976)
<https://reviews.apache.org/r/33174/#comment170475>
To reiterate my earlier comment:
1) We don't support changing the cfs period so it only needs to be set
once, additional writes are unnecessary but aren't a problem. The
MesosContainerizer isolator just writes it on every update rather than
determining if it has already written it before.
2) The cfs quota changes depending on the CPU resource value so it
definitely does need to be re-written at every update. Again, this is a
straight copy-and-paste from the MC cpu isolator.
This part of the code looks fine to me. @Tim, do you concur?
- Ian Downes
On April 14, 2015, 1:32 p.m., Steve Niemitz wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
>
> (Updated April 14, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Fix for docker containerizer not configuring CFS quotas correctly.
>
> It would be nice to refactor all this isolation code in a way that can be
> shared between all containerizers, as this is basically just copied from the
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
>
>
> Diffs
> -
>
> src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7
>
> Diff: https://reviews.apache.org/r/33174/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 41245: Use ethtool -k lo to check ethtool command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41245/#review109990 --- Ship it! Ship It! - Ian Downes On Dec. 10, 2015, 9:20 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41245/ > --- > > (Updated Dec. 10, 2015, 9:20 p.m.) > > > Review request for mesos, David Robinson, Ian Downes, and Jie Yu. > > > Repository: mesos > > > Description > --- > > David reported that on CentOS5 part of ethtool --version output sends to > stderr instead of stdout, which leads this output in our log. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > 7c1724a4ca5c45bb214e5129743e0644c9ca9661 > > Diff: https://reviews.apache.org/r/41245/diff/ > > > Testing > --- > > Manually start mesos slave > > > Thanks, > > Cong Wang > >
Re: Review Request 41158: Turn off rx checksum offloading for veth in container
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41158/#review109627 --- Ship it! src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 3572) <https://reviews.apache.org/r/41158/#comment169231> s/kernel/the kernel/ src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 3574) <https://reviews.apache.org/r/41158/#comment169230> s/could/to - Ian Downes On Dec. 9, 2015, 3:15 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41158/ > --- > > (Updated Dec. 9, 2015, 3:15 p.m.) > > > Review request for mesos, Ian Downes and Jie Yu. > > > Bugs: MESOS-4105 > https://issues.apache.org/jira/browse/MESOS-4105 > > > Repository: mesos > > > Description > --- > > We noticed that in some cases we delivered some corrupt packets to > applications running in our containers. This is clearly wrong. > > Here is what happens: > > 1) We receive a corrupt packet externally > 2) The hardware driver is able to checksum it and notices it has a bad > checksum > 3) The driver delivers this packet anyway to wait for TCP layer to checksum > it again and then drop it > 4) This packet is moved to a veth interface because it is for a container > 5) Both sides of the veth pair have RX checksum offloading by default > 6) The veth_xmit() marks the packet's checksum as UNNECESSARY since its peer > device has rx checksum offloading > 7) Packet is moved into the container TCP/IP stack > 8) TCP layer is not going to checksum it since it is not necessary > 9) The packet gets delivered to application layer > > > Diffs > - > > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > 89bb36f936417de8169a2442729fbd7c9d60acb7 > > Diff: https://reviews.apache.org/r/41158/diff/ > > > Testing > --- > > 1) Turn rx checksum off manually and the bug is gone > 2) Test this patch and verify rx checksum is turned off as expected. > 3) I don't see any noticable performance issue after turning this off > > > Thanks, > > Cong Wang > >
Re: Review Request 39635: FreeBSD: Enable libprocess build and disable failing test
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39635/#review108381
---
Ship it!
Minor nits on comments. Will make the changes myself when I commit. Thanks!
3rdparty/libprocess/3rdparty/Makefile.am (line 243)
<https://reviews.apache.org/r/39635/#comment167818>
Full stop.
3rdparty/libprocess/configure.ac (line 835)
<https://reviews.apache.org/r/39635/#comment167817>
Full stop.
3rdparty/libprocess/src/tests/http_tests.cpp (line 897)
<https://reviews.apache.org/r/39635/#comment167819>
Full stop.
- Ian Downes
On Nov. 27, 2015, 8:26 p.m., David Forsythe wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39635/
> ---
>
> (Updated Nov. 27, 2015, 8:26 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-1563
> https://issues.apache.org/jira/browse/MESOS-1563
>
>
> Repository: mesos
>
>
> Description
> ---
>
> FreeBSD: Enable libprocess build and disable failing test
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/Makefile.am
> 0adbe539afaf683e4a85582463a2930049a63998
> 3rdparty/libprocess/configure.ac 40801653a7fb9a943dfe33913161d28ef24040c3
> 3rdparty/libprocess/src/config.hpp 8444a6018f9bc911d59a751b5a763f73d86ab1e6
> 3rdparty/libprocess/src/tests/http_tests.cpp
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/39635/diff/
>
>
> Testing
> ---
>
> make check on ubuntu
>
>
> Thanks,
>
> David Forsythe
>
>
Re: Review Request 39634: FreeBSD: Enable mesos build and start fixing some tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39634/#review108377 --- Ship it! Very minor things. I'll just make the changes when I commit the patch. Thanks! configure.ac (line 619) <https://reviews.apache.org/r/39634/#comment167814> End with fullstop. src/Makefile.am (line 1756) <https://reviews.apache.org/r/39634/#comment167816> Can you add a comment that FreeBSD includes the necessary functionality in libc? src/tests/attributes_tests.cpp (line 38) <https://reviews.apache.org/r/39634/#comment167813> These are unrelated to the FreeBSD support so can you split them out? Else, I'll do it when I commit this patch. - Ian Downes On Nov. 27, 2015, 8:26 p.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39634/ > --- > > (Updated Nov. 27, 2015, 8:26 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-1563 > https://issues.apache.org/jira/browse/MESOS-1563 > > > Repository: mesos > > > Description > --- > > FreeBSD: Enable mesos build and start fixing some tests > > > Diffs > - > > configure.ac b30a8d30076f3068fd7d5fc8ccea1982639e998a > src/Makefile.am fd38cfa73d81a98c819378f99a766e2ddb7e1a04 > src/tests/attributes_tests.cpp d8c84d25a37f9cf1b38a97193d5b3b3001fd54ff > src/tests/resources_tests.cpp dbd39cd5a6786682a7b528b6fea37ab78904cf12 > src/tests/values_tests.cpp fb7f982a50df8274ad29dab9e55157c39acdc104 > > Diff: https://reviews.apache.org/r/39634/diff/ > > > Testing > --- > > make check on ubuntu 14.04 > > > Thanks, > > David Forsythe > >
Re: Review Request 38074: Calculate schedule latency with trace events
reviews.apache.org/r/38074/#comment165790>
ditto.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 529)
<https://reviews.apache.org/r/38074/#comment165791>
ditto.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 530)
<https://reviews.apache.org/r/38074/#comment165792>
ditto.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 536 - 537)
<https://reviews.apache.org/r/38074/#comment165800>
Just continue if pid == 0, rather than nesting this conditional block on !=
0.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 539)
<https://reviews.apache.org/r/38074/#comment165793>
ditto.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 541)
<https://reviews.apache.org/r/38074/#comment165801>
Are states enumerated somewhere?
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 544)
<https://reviews.apache.org/r/38074/#comment165802>
newline between blocks
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 554)
<https://reviews.apache.org/r/38074/#comment165804>
sort the latencies in schedLatency here?
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 556)
<https://reviews.apache.org/r/38074/#comment165803>
const string&
grab the cgroup and vector together rather than indexing repeatedly...
```
foreachpair(const string& cgroup,
const vector& latencies,
schedLatency)
```
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 606)
<https://reviews.apache.org/r/38074/#comment165805>
why not call the variable threads?
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 608)
<https://reviews.apache.org/r/38074/#comment165806>
also log the error, {{process.error()}}.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 608 - 609)
<https://reviews.apache.org/r/38074/#comment165815>
Could you just skip rather than failing for all cgroups?
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 611)
<https://reviews.apache.org/r/38074/#comment165810>
as above, suggest stored pid -> cgroup mapping denormalized.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 615)
<https://reviews.apache.org/r/38074/#comment165809>
const string&
The key is an event? If so, it should be named as such.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 619 - 621)
<https://reviews.apache.org/r/38074/#comment165807>
How expensive is this...?
src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 623 - 626)
<https://reviews.apache.org/r/38074/#comment165808>
Users have different kernels, code should determine the version at run time
and act accordingly.
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 633)
<https://reviews.apache.org/r/38074/#comment165811>
Include error message _events.error().
src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 636)
<https://reviews.apache.org/r/38074/#comment165812>
const string&.
I don't think there's another cgroup variable in scope so just use cgroup,
not _cgroup.
- Ian Downes
On Sept. 29, 2015, 5:15 p.m., Cong Wang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38074/
> ---
>
> (Updated Sept. 29, 2015, 5:15 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Finally, calculate schedule latency with the sched trace events, and add it
> to our statistics
>
>
> Diffs
> -
>
> include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999
> src/slave/containerizer/isolators/cgroups/perf_event.hpp
> 1f722ef3ef7ab7fce5542d4affae961d6cec2406
> src/slave/containerizer/isolators/cgroups/perf_event.cpp
> 03035dfbfb84470ba39ed9ecfd1eb73890e3f784
> src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5
> src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
>
> Diff: https://reviews.apache.org/r/38074/diff/
>
>
> Testing
> ---
>
> manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37101: Remove unused sched API's
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37101/#review106954 --- Ship it! Ship It! - Ian Downes On Aug. 24, 2015, 2:30 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37101/ > --- > > (Updated Aug. 24, 2015, 2:30 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > We don't use SCHED_IDLE any more. > > > Diffs > - > > src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db > src/linux/sched.hpp 8cb06531b417a77766ab2b13587393b99a96b0c1 > src/tests/containerizer/sched_tests.cpp > 00723d01cd2cc37410d6f9fdd2de080063b7ccd8 > > Diff: https://reviews.apache.org/r/37101/diff/ > > > Testing > --- > > make > > > Thanks, > > Cong Wang > >
Re: Review Request 39417: Add --egress_flow_classifier_parent flag
> On Oct. 26, 2015, 5:20 p.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, lines 401-405
> > <https://reviews.apache.org/r/39417/diff/1/?file=1100528#file1100528line401>
> >
> > Why not add a 0x prefix if it's not present so you can use numify?
> >
> > if (!strings::startsWith(tokens[0], "0x")) {
> > ...
> > }
>
> Cong Wang wrote:
> This should work too, but I don't feel it is better than my code.
I really think it's necessary to use stout's conversion functions here.
- Ian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39417/#review104119
---
On Oct. 28, 2015, 5:12 p.m., Cong Wang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39417/
> -----------
>
> (Updated Oct. 28, 2015, 5:12 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> When --egress_unique_flow_per_container is enabled, we need to install a flow
> classifier (fq_codel) qdisc on egress side. This flag specifies where to
> install it in the hierarchy. By default, we install it at root. But, for
> example, if you have already installed HTB qdisc at root, you may want this
> to be installed in other place than root, specify an HTB class ID as its
> parent here.
>
>
> Diffs
> -
>
> docs/configuration.md ae9f2612b11447eff92ea85d4191e7011d71b2b2
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> 1911ba6518190cbff6d72b56aaa477d508dbd391
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp 1362dcebef799399739025171696230bded447dd
>
> Diff: https://reviews.apache.org/r/39417/diff/
>
>
> Testing
> ---
>
> Manual tests, with and without a pre-installed HTB qdisc and classes.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39415: Error out when root qdisc already exists
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39415/#review104185 --- Ship it! Ship It! - Ian Downes On Oct. 17, 2015, 5:22 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39415/ > --- > > (Updated Oct. 17, 2015, 5:22 p.m.) > > > Review request for mesos, Ian Downes and Jie Yu. > > > Repository: mesos > > > Description > --- > > When we enable --egress_unique_flow_per_container, we need to install an > fq_codel qdisc on root, but if a qdisc already exists on root, we should not > continue since it could be not fq_codel at all, so just exit with error. > > > Diffs > - > > src/slave/containerizer/isolators/network/port_mapping.cpp > e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef > > Diff: https://reviews.apache.org/r/39415/diff/ > > > Testing > --- > > Manual tests, with a pre-installed HTB qdisc and without. > > > Thanks, > > Cong Wang > >
Re: Review Request 39490: Always create non-IP egress filters
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/#review104183
---
src/slave/containerizer/isolators/network/port_mapping.cpp (line 2431)
<https://reviews.apache.org/r/39490/#comment162473>
Why is this no longer a failure?
src/slave/containerizer/isolators/network/port_mapping.cpp (line 2450)
<https://reviews.apache.org/r/39490/#comment162474>
ditto?
- Ian Downes
On Oct. 20, 2015, 11:57 a.m., Cong Wang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39490/
> ---
>
> (Updated Oct. 20, 2015, 11:57 a.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> When rolling out the new flag --egress_unique_flow_per_container, I noticed,
> on some slaves, only IP egress filters were created as expected, the reset
> were not. Looking at the code, it looks like we skipped the creation if this
> is not the first container we create, this is wrong for this case, because
> egress filters were not created for previous containers yet. We should always
> create them and ignore error if they exist.
>
>
> Diffs
> -
>
> src/slave/containerizer/isolators/network/port_mapping.cpp
> e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39490/diff/
>
>
> Testing
> ---
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39416: Document --egress_unique_flow_per_container in docs/configuration.md
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39416/#review104117 --- docs/configuration.md (lines 1533 - 1534) <https://reviews.apache.org/r/39416/#comment162355> Does it actually create a flow per container or is it really based on the 5 tuple, which will be different for different containers? - Ian Downes On Oct. 17, 2015, 5:24 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39416/ > --- > > (Updated Oct. 17, 2015, 5:24 p.m.) > > > Review request for mesos, Ian Downes and Jie Yu. > > > Repository: mesos > > > Description > --- > > Flag --egress_unique_flow_per_container is documented in help message, but > not in docs/configuration.md. We should document it there too, with more > details. > > > Diffs > - > > docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6 > > Diff: https://reviews.apache.org/r/39416/diff/ > > > Testing > --- > > None. > > > Thanks, > > Cong Wang > >
Re: Review Request 39417: Add --egress_flow_classifier_parent flag
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39417/#review104120 --- Also, is there are way to test the different code paths? - Ian Downes On Oct. 17, 2015, 5:29 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39417/ > --- > > (Updated Oct. 17, 2015, 5:29 p.m.) > > > Review request for mesos, Ian Downes and Jie Yu. > > > Repository: mesos > > > Description > --- > > When --egress_unique_flow_per_container is enabled, we need to install a flow > classifier (fq_codel) qdisc on egress side. This flag specifies where to > install it in the hierarchy. By default, we install it at root. But, for > example, if you have already installed HTB qdisc at root, you may want this > to be installed in other place than root, specify an HTB class ID as its > parent here. > > > Diffs > - > > docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6 > src/slave/containerizer/isolators/network/port_mapping.cpp > e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef > src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 > src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2 > > Diff: https://reviews.apache.org/r/39417/diff/ > > > Testing > --- > > Manual tests, with and without a pre-installed HTB qdisc and classes. > > > Thanks, > > Cong Wang > >
Re: Review Request 39417: Add --egress_flow_classifier_parent flag
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39417/#review104119
---
docs/configuration.md (lines 1545 - 1551)
<https://reviews.apache.org/r/39417/#comment162363>
Can we not infer where to put the classifier from any exisiting classifiers
and only require the user to specify its parent if they want particular
behavior?
src/slave/containerizer/isolators/network/port_mapping.cpp (line 398)
<https://reviews.apache.org/r/39417/#comment162359>
s/Failed tokenize/Failed to tokenize/
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 401 - 405)
<https://reviews.apache.org/r/39417/#comment162360>
Why not add a 0x prefix if it's not present so you can use numify?
if (!strings::startsWith(tokens[0], "0x")) {
...
}
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 407 - 409)
<https://reviews.apache.org/r/39417/#comment162361>
This will get refactored as above, but it's more helpful to specifically
state which token couldn't be parsed.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1415)
<https://reviews.apache.org/r/39417/#comment162364>
kill this newline so the error checking is paired with the Try<>
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1419)
<https://reviews.apache.org/r/39417/#comment162365>
add a newline in here
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1423 - 1424)
<https://reviews.apache.org/r/39417/#comment162366>
Can we verify this somehow? What's the failure mode?
src/slave/flags.cpp (lines 506 - 513)
<https://reviews.apache.org/r/39417/#comment162362>
Why is this description different from the docs?
- Ian Downes
On Oct. 17, 2015, 5:29 p.m., Cong Wang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39417/
> -----------
>
> (Updated Oct. 17, 2015, 5:29 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> When --egress_unique_flow_per_container is enabled, we need to install a flow
> classifier (fq_codel) qdisc on egress side. This flag specifies where to
> install it in the hierarchy. By default, we install it at root. But, for
> example, if you have already installed HTB qdisc at root, you may want this
> to be installed in other place than root, specify an HTB class ID as its
> parent here.
>
>
> Diffs
> -
>
> docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6
> src/slave/containerizer/isolators/network/port_mapping.cpp
> e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2
>
> Diff: https://reviews.apache.org/r/39417/diff/
>
>
> Testing
> ---
>
> Manual tests, with and without a pre-installed HTB qdisc and classes.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39345: Enable build on FreeBSD, start porting components.
> On Oct. 20, 2015, 1:52 a.m., Mesos ReviewBot wrote: > > Bad patch! > > > > Reviews applied: [39345] > > > > Failed command: ./support/apply-review.sh -n -r 39345 > > > > Error: > > 2015-10-20 08:52:33 URL:https://reviews.apache.org/r/39345/diff/raw/ > > [26851/26851] -> "39345.patch" [1] > > Successfully applied: Enable build on FreeBSD, start porting components. > > > > Enable build on FreeBSD, start porting components. > > > > My build steps are: > > > > - Install dependencies from http://mesos.apache.org/gettingstarted/ > > - Install libexecinfo > > - Install clang36 (system clang is 3.4) > > - boostrap > > - ../configure --with-curl=/usr/local --with-apr=/usr/local > > --with-svn=/usr/local CC=clang36 CXX=clang++36 > > - gmake CC=clang36 CXX=clang++36 > > - gmake CC=clang36 CXX=clang++36 check > > > > I disabled one test because I haven't had a chance to debug it and I wanted > > to get a bit further in the test suite. A check run is attached. > > > > > > Review: https://reviews.apache.org/r/39345 > > Checking 20 files using filter > > --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo > > Total errors found: 0 > > ERROR: Commit spanning multiple projects. > > > > Please use separate commits for mesos, libprocess and stout. > > > > Paths grouped by project: > > mesos: > > configure.ac > > src/Makefile.am > > src/slave/containerizer/isolators/posix/disk.cpp > > src/tests/attributes_tests.cpp > > src/tests/resources_tests.cpp > > src/tests/values_tests.cpp > > stout: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/freebsd.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/bootid.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/sendfile.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signals.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/raw/environment.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sysctl.hpp > > 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp > > 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp > > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp > > libprocess: > > 3rdparty/libprocess/3rdparty/Makefile.am > > 3rdparty/libprocess/configure.ac > > 3rdparty/libprocess/src/config.hpp > > 3rdparty/libprocess/src/tests/http_tests.cpp > > Failed to commit patch Looks like most of the failing tests are for non-posix/Linux specific stuff? - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39345/#review103232 ------- On Oct. 20, 2015, 12:45 a.m., David Forsythe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39345/ > --- > > (Updated Oct. 20, 2015, 12:45 a.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: https://issues.apache.org/jira/browse/MESOS-1563 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-1563 > > > Repository: mesos > > > Description > --- > > Enable build on FreeBSD, start porting components. > > My build steps are: > > - Install dependencies from http://mesos.apache.org/gettingstarted/ > - Install libexecinfo > - Install clang36 (system clang is 3.4) > - boostrap > - ../configure --with-curl=/usr/local --with-apr=/usr/local > --with-svn=/usr/local CC=clang36 CXX=clang++36 > - gmake CC=clang36 CXX=clang++36 > - gmake CC=clang36 CXX=clang++36 check > > I disabled one test because I haven't had a chance to debug it and I wanted > to get a bit further in th
Re: Review Request 39345: Enable build on FreeBSD, start porting components.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39345/#review103449
---
Looks good!
I haven't tested this myself yet, so these are preliminary comments.
Please split any reviews into {stout, libprocess, mesos}. Also, please put any
unrelated cleanups into a separate review.
3rdparty/libprocess/3rdparty/Makefile.am (line 128)
<https://reviews.apache.org/r/39345/#comment161502>
s/intialize/Initialize/
3rdparty/libprocess/3rdparty/Makefile.am (line 242)
<https://reviews.apache.org/r/39345/#comment161503>
Can you add a comment that FreeBSD includes all the dynamic linking stuff
in libc.
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp (line 257)
<https://reviews.apache.org/r/39345/#comment161504>
What happens if/when we support IPv6?
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (lines 288 - 290)
<https://reviews.apache.org/r/39345/#comment161505>
IIRC, FreeBSD doesn't often use patch versions, but has in the past. Pretty
sure I started with 4.6.2!
3rdparty/libprocess/3rdparty/stout/include/stout/os/freebsd.hpp (lines 22 - 24)
<https://reviews.apache.org/r/39345/#comment161506>
Alphabetize please.
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp (lines 20 -
25)
<https://reviews.apache.org/r/39345/#comment161507>
Alphabetize please.
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp (lines 84 - 86)
<https://reviews.apache.org/r/39345/#comment161508>
Alphabetize.
3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp (line 28)
<https://reviews.apache.org/r/39345/#comment161509>
When does/will FreeBSD changes its libc version?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (lines 419 - 421)
<https://reviews.apache.org/r/39345/#comment161511>
Could you condition this on security.jail.jailed?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 755)
<https://reviews.apache.org/r/39345/#comment161512>
ditto
3rdparty/libprocess/configure.ac (lines 818 - 820)
<https://reviews.apache.org/r/39345/#comment161501>
Does FreeBSD require this? If not then split the cases.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 872 - 876)
<https://reviews.apache.org/r/39345/#comment161513>
Please add a comment why the test is disabled, as a TODO.
configure.ac (line 985)
<https://reviews.apache.org/r/39345/#comment161500>
Does FreeBSD support all those architectures? I suspect not, so this should
be split out.
src/slave/containerizer/isolators/posix/disk.cpp (line 418)
<https://reviews.apache.org/r/39345/#comment161516>
This is an interesting point... I think we actually care about the usage,
not the apparent size since we're using this for setting quota.
src/tests/attributes_tests.cpp (line 40)
<https://reviews.apache.org/r/39345/#comment161517>
Did this cause a problem running the tests or are you cleaning things up?
- Ian Downes
On Oct. 20, 2015, 12:45 a.m., David Forsythe wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39345/
> -----------
>
> (Updated Oct. 20, 2015, 12:45 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: https://issues.apache.org/jira/browse/MESOS-1563
>
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-1563
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Enable build on FreeBSD, start porting components.
>
> My build steps are:
>
> - Install dependencies from http://mesos.apache.org/gettingstarted/
> - Install libexecinfo
> - Install clang36 (system clang is 3.4)
> - boostrap
> - ../configure --with-curl=/usr/local --with-apr=/usr/local
> --with-svn=/usr/local CC=clang36 CXX=clang++36
> - gmake CC=clang36 CXX=clang++36
> - gmake CC=clang36 CXX=clang++36 check
>
> I disabled one test because I haven't had a chance to debug it and I wanted
> to get a bit further in the test suite. A check run is attached.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/Makefile.am
> 53e83d4905945593e174601a0b791d01824dc34b
> 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp
> dc7c6522b283916b975a77957909f6cdc02944d3
> 3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp
> 9428717fac4655898d7768957f02937592e1a398
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
> e49783a438157706b1be9745436bf666f45cab8b
Re: Review Request 38117: Export per container SNMP statistics
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38117/#review102933
---
include/mesos/mesos.proto (lines 702 - 720)
<https://reviews.apache.org/r/38117/#comment160766>
Are the statistics signed? If not, suggest using uint64 type.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1139)
<https://reviews.apache.org/r/38117/#comment160779>
No need to store all statistics until the end. Key/value lines for a set of
statistics are paired so you can add*Statistics after processing each line
pair. Then you just need a scoped {{hashmap statistics}}.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1140 - 1141)
<https://reviews.apache.org/r/38117/#comment160778>
Clearer to just declare these inside the loop scope (but my suggestions
below obviate them anyway).
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1142)
<https://reviews.apache.org/r/38117/#comment160777>
you don't care about the actual line number, just that you want to toggle
between "key" and "value" lines, suggest something like "bool isKeyLine" and
toggle that?
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1144)
<https://reviews.apache.org/r/38117/#comment160770>
Suggest splitting first on ":" to get the statistics type and keys/values,
then tokenize on " ". You can also validate the type then as well.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1146)
<https://reviews.apache.org/r/38117/#comment160774>
If you split first on ":" then you don't need a separate keys vector.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1150 - 1159)
<https://reviews.apache.org/r/38117/#comment160775>
Assuming you've got the type from splitting on ":" and then you've got the
keys, you can directly insert val into a statistics hashmap after successfully
numifying it, i.e., no need for an intermediate values vector.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1161 - 1165)
<https://reviews.apache.org/r/38117/#comment160780>
This is an unnecessary mix of indexing and foreach looping. Comments above
make this unnecessary but just FYI.
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1162)
<https://reviews.apache.org/r/38117/#comment160783>
This should be a {{const string&}} (if kept).
src/slave/containerizer/isolators/network/port_mapping.cpp (line 1169)
<https://reviews.apache.org/r/38117/#comment160782>
Toggle line type here.
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1171 - 1174)
<https://reviews.apache.org/r/38117/#comment160781>
Do these inside the parsing loop as you parse each line pair for a
statistics type.
- Ian Downes
On Sept. 8, 2015, 2:12 p.m., Cong Wang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38117/
> ---
>
> (Updated Sept. 8, 2015, 2:12 p.m.)
>
>
> Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone.
>
>
> Bugs: MESOS-3365
> https://issues.apache.org/jira/browse/MESOS-3365
>
>
> Repository: mesos
>
>
> Description
> ---
>
> These stats are those we get by `netstat -s`, they are important for diagnose
> networking issues.
>
>
> Diffs
> -
>
> include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c
> src/slave/containerizer/isolators/network/port_mapping.hpp
> 4bca0b81bf69fb4cd75e05aacd02d3e818e32d09
> src/slave/containerizer/isolators/network/port_mapping.cpp
> 34ba2294b0bd7d57aa9de073692a2ea8ec62681d
> src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c
> src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d
>
> Diff: https://reviews.apache.org/r/38117/diff/
>
>
> Testing
> ---
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 38858: Ensured that slave's work_dir is a shared mount in its own peer group when LinuxFilesystemIsolator is used.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38858/#review101011
---
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 88 - 94)
<https://reviews.apache.org/r/38858/#comment158316>
This has appeared in multiple places, perhaps refactor this into a helper?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 121)
<https://reviews.apache.org/r/38858/#comment158317>
Suggestion, refactor to avoid the nesting?
if (workDirMount.isNone() {
// mount it and get the fs::MountInfoTable::Entry
}
// Check it's a slave mount.
// Check it's a shared mount.
src/slave/containerizer/isolators/filesystem/linux.cpp (line 126)
<https://reviews.apache.org/r/38858/#comment158314>
If it's already mounted (so it's in /etc/mtab) there's no reason to shell
out; you can use fs::mount().
src/slave/containerizer/isolators/filesystem/linux.cpp (line 146)
<https://reviews.apache.org/r/38858/#comment158315>
If it's already mounted (so it's in /etc/mtab) there's no reason to shell
out; you can use fs::mount().
src/tests/containerizer/filesystem_isolator_tests.cpp (lines 926 - 930)
<https://reviews.apache.org/r/38858/#comment158318>
This doesn't verify the work_dir *is* bind mounted, just that *if* it is
then it's a shared mount...
src/tests/containerizer/filesystem_isolator_tests.cpp (lines 949 - 953)
<https://reviews.apache.org/r/38858/#comment158319>
Ditto.
- Ian Downes
On Sept. 29, 2015, 12:48 p.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38858/
> ---
>
> (Updated Sept. 29, 2015, 12:48 p.m.)
>
>
> Review request for mesos, Timothy Chen and Jiang Yan Xu.
>
>
> Bugs: MESOS-3539
> https://issues.apache.org/jira/browse/MESOS-3539
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Ensured that slave's work_dir is a shared mount in its own peer group when
> LinuxFilesystemIsolator is used.
>
> This addressed the TODO in the current code.
>
>
> Diffs
> -
>
> src/slave/containerizer/isolators/filesystem/linux.cpp
> 8fa929f12838044762be82287e1b714406d9
> src/tests/containerizer/filesystem_isolator_tests.cpp
> 41d098c7d66eba0befc3708c6660a6a3e5f0d2de
>
> Diff: https://reviews.apache.org/r/38858/diff/
>
>
> Testing
> ---
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38855: Added support for getting shared and slave mount peer group ID.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38855/#review101009
---
Ship it!
src/tests/containerizer/fs_tests.cpp (line 198)
<https://reviews.apache.org/r/38855/#comment158309>
Can you get the mount id before you do the mount and then verify the shared
sibling id is correct? This might be easiest to do with a second self bind
mount so you can find the sibling.
src/tests/containerizer/fs_tests.cpp (line 200)
<https://reviews.apache.org/r/38855/#comment158308>
s/Cleanup/Clean up/
src/tests/containerizer/fs_tests.cpp (line 201)
<https://reviews.apache.org/r/38855/#comment158312>
So the mount won't get cleaned up if the test asserts early?
src/tests/containerizer/fs_tests.cpp (line 241)
<https://reviews.apache.org/r/38855/#comment158311>
Like above, can you verify the master id is correct?
src/tests/containerizer/fs_tests.cpp (line 243)
<https://reviews.apache.org/r/38855/#comment158310>
s/Cleanup/Clean up/
- Ian Downes
On Sept. 29, 2015, 12:09 p.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38855/
> ---
>
> (Updated Sept. 29, 2015, 12:09 p.m.)
>
>
> Review request for mesos, Ian Downes, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3539
> https://issues.apache.org/jira/browse/MESOS-3539
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added support for getting shared and slave mount peer group ID.
>
>
> Diffs
> -
>
> src/linux/fs.hpp f3aa0c2a385527eab5895f5d10c29e3d68e30a49
> src/linux/fs.cpp 8631d892ed6c132d6a9dc2031c2ca040623e9acc
> src/tests/containerizer/fs_tests.cpp
> 34d3c41045373326fb6096031cd1daef0e7b203a
>
> Diff: https://reviews.apache.org/r/38855/diff/
>
>
> Testing
> ---
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/ --- (Updated July 11, 2015, 9:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Addressed comments. Repository: mesos Description --- Add ContainerImage protobuf. Diffs (updated) - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 Diff: https://reviews.apache.org/r/34136/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 11, 2015, 9:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Addressed comments. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs (updated) - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34135: Add filesystem/posix isolator for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated July 11, 2015, 9:46 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Summary (updated) - Add filesystem/posix isolator for persistent volumes. Repository: mesos Description (updated) --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Review Request 36429: Add filesystem/linux isolator for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36429/ --- Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved filesystem/linux from review https://reviews.apache.org/r/34135/ Diffs - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/36429/diff/ Testing --- Thanks, Ian Downes
Review Request 36428: Remove erroneous code for isolator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36428/ --- Review request for mesos, Ben Mahler, Kapil Arya, and Timothy Chen. Repository: mesos Description --- Kapil: this appears to have been introduced in your commit 3a179ce2. Can you please verify this should be removed. Diffs - src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/36428/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated July 11, 2015, 9:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Moved filesystem/linux to separate review. Addressed comments. Repository: mesos Description --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - filesystem/linux (bind mounts, does support container rootfs) The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs (updated) - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Re: Review Request 34908: Rename --docker_sandbox_directory flag for general use.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34908/ --- (Updated July 11, 2015, 9:41 p.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- Rename --docker_sandbox_directory flag for general use. This will require users to update configuration scripts etc if they specify the old flag. Diffs (updated) - docs/configuration.md feee5594c88112f77ce382cb3dd8628653f92d01 src/slave/containerizer/docker.cpp cfb60177fe48ec0eeab12ff392c6c9f89634b92f src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/tests/docker_containerizer_tests.cpp a3da786c1b733e9dd4abf1d02d5dae051393d921 Diff: https://reviews.apache.org/r/34908/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34136: Add ContainerImage protobuf.
> On June 26, 2015, 2:57 p.m., Jiang Yan Xu wrote: > > include/mesos/mesos.proto, lines 1212-1214 > > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1212> > > > > Is it the intention that Image type is **defined** outside MesosInfo > > because DockerInfo can later reference it? > > > > Otherwise it feels more natual to define Image within MesosInfo. > > Vinod Kone wrote: > +1 to put it inside MesosInfo. I wanted it to be outside MesosInfo so it could be used by other container types, i.e., I don't see it as specific to MesosInfo, e.g., we could also support an AppcInfo which contained an Image::APPC. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review89577 ----------- On June 22, 2015, 9:42 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34136/ > --- > > (Updated June 22, 2015, 9:42 a.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Add ContainerImage protobuf. > > > Diffs > - > > include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 > > Diff: https://reviews.apache.org/r/34136/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34136: Add ContainerImage protobuf.
> On June 25, 2015, 3:32 p.m., Paul Brett wrote: > > include/mesos/mesos.proto, line 1221 > > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1221> > > > > Mesos info is optional, but if present can optionally contain an image? > > So what does a mesos present with a MesosInfo message with no image mean? > > Vinod Kone wrote: > yea. why optional? > > Timothy Chen wrote: > I think image can be optional which bypasses the image provisioning. > MesosInfo potentially will contain other information like what DockerInfo > does that is MesosContainerizer specific, so I think this API makes sense to > me. Yep, Tim's comments reflect what I'm thinking. > On June 25, 2015, 3:32 p.m., Paul Brett wrote: > > include/mesos/mesos.proto, line 1202 > > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1202> > > > > Not really an id, more of a signature for the image. Would we be > > better making this optional string sha512hash to allow for the possibility > > that a different signature might be used in the future? > > Vinod Kone wrote: > i'm assuming this is because appc calls it an image id. Correct, this reflects naming in the appc spec. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review89438 --- On June 22, 2015, 9:42 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34136/ > --- > > (Updated June 22, 2015, 9:42 a.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Add ContainerImage protobuf. > > > Diffs > - > > include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 > > Diff: https://reviews.apache.org/r/34136/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34136: Add ContainerImage protobuf.
> On July 1, 2015, 4:54 p.m., Lily Chen wrote: > > include/mesos/mesos.proto, line 1209 > > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1209> > > > > Same issue as when MesosInfo is not present. What happens when there is > > no Appc message present in Image? This is how we represent a union: a required type and then 1 or more optional messages for those types. Code is responsible for verifiying that the optional message is present to match the type. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review90166 ------- On June 22, 2015, 9:42 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34136/ > --- > > (Updated June 22, 2015, 9:42 a.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Add ContainerImage protobuf. > > > Diffs > - > > include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 > > Diff: https://reviews.apache.org/r/34136/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34137: Add support for container image provisioners.
> On June 29, 2015, 12:41 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/containerizer.hpp, lines 319-320 > > <https://reviews.apache.org/r/34137/diff/2/?file=989755#file989755line319> > > > > Why not store ContainerInfo directly? It's optional so I decided to store the required ExecutorInfo and use the container ContainerInfo when appropriate. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review89293 ------- On July 10, 2015, 5:05 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34137/ > --- > > (Updated July 10, 2015, 5:05 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > The MesosContainerizer can optionally provision a root filesystem for the > containers. > > > Diffs > - > > include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/mesos/containerizer.hpp > 3ac2387eabded61c897a5016655aae10cd1bca91 > src/slave/containerizer/mesos/containerizer.cpp > 8c102fb7d1f79ee768cb06de3a976ea12f958712 > src/slave/containerizer/provisioner.hpp PRE-CREATION > src/slave/containerizer/provisioner.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f > src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 > src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 > src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec > src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e > > Diff: https://reviews.apache.org/r/34137/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34137: Add support for container image provisioners.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 10, 2015, 5:05 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Addressed comments. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated July 10, 2015, 5:05 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - filesystem/linux (bind mounts, does support container rootfs) The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
> On June 25, 2015, 12:47 p.m., Jie Yu wrote: > > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 406-407 > > <https://reviews.apache.org/r/34135/diff/2/?file=989747#file989747line406> > > > > Do you need to umount persistent volumes as well? As the comment states, it "notably this includes persistent volume mounts" :-) > On June 25, 2015, 12:47 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/containerizer.cpp, lines 158-166 > > <https://reviews.apache.org/r/34135/diff/2/?file=989751#file989751line158> > > > > Hum... why this logic is under 'if > > (ModuleManager::contains(type)' ? Good catch! No idea why it was there in the first place and why I perpetuated the erroneous code. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/#review84775 ------- On June 22, 2015, 9:41 a.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34135/ > --- > > (Updated June 22, 2015, 9:41 a.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Moved code from Mesos Containerizer to filesystem isolators > - filesystem/posix (symlinks, doesn't support container rootfs) > - filesystem/linux (bind mounts, does support container rootfs) > > The filesystem/posix isolator will be automatically included if no > filesystem/ isolator is specified. > > > Diffs > - > > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION > src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION > src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION > src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION > src/slave/containerizer/linux_launcher.cpp > 8eae258d81229e19f8c587f5e023b1df7deed025 > src/slave/containerizer/mesos/containerizer.cpp > 8c102fb7d1f79ee768cb06de3a976ea12f958712 > > Diff: https://reviews.apache.org/r/34135/diff/ > > > Testing > --- > > existing persistent volumes tests. > > > Thanks, > > Ian Downes > >
Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36380/#review91346
---
src/tests/perf_tests.cpp (line 52)
<https://reviews.apache.org/r/36380/#comment144705>
split args onto separate lines
src/tests/perf_tests.cpp (line 62)
<https://reviews.apache.org/r/36380/#comment144710>
lines? it's 1 or more, how about just calling input?
is debug() a better name for this function?
src/tests/perf_tests.cpp (lines 65 - 69)
<https://reviews.apache.org/r/36380/#comment144709>
use a ternary if here?
```cpp
s << (version.isError() ? "Error:" + version.error()
: version.get());
```
src/tests/perf_tests.cpp (line 104)
<https://reviews.apache.org/r/36380/#comment144706>
I don't think we use this, favoring
```cpp
foreach (const tuple&, input)
```
Is Version hashable? If so, iterating over a Version -> input would be much
cleaner
```cpp
foreachpair(const Version& version, const string& input, inputs)
```
Or, just use the string version and parse the string version inside the
loop?
```cpp
hashmap input {
"2.6.39", "123,cycles\n0.123,task-clock"
}
```
src/tests/perf_tests.cpp (line 157)
<https://reviews.apache.org/r/36380/#comment144714>
Do you need to keep all the logging of the context on failed expectations
after the test has been correctly written? It clutters the code...
src/tests/perf_tests.cpp (line 191)
<https://reviews.apache.org/r/36380/#comment144711>
ditto
src/tests/perf_tests.cpp (line 226)
<https://reviews.apache.org/r/36380/#comment144712>
ditto
- Ian Downes
On July 10, 2015, 1:52 p.m., Paul Brett wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36380/
> -----------
>
> (Updated July 10, 2015, 1:52 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Test cases for performance monitor support of multiple output versions
> depending on kernel version.
>
>
> Diffs
> -
>
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36380/diff/
>
>
> Testing
> ---
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36378/#review91253
---
src/linux/perf.cpp (line 449)
<https://reviews.apache.org/r/36378/#comment144639>
Why a Try<> argument and then a CHECK()? The caller of os::release() should
check the return so either check it in supported() or query and check it in
_supported().
Also, should be const Version&.
src/linux/perf.cpp (lines 468 - 473)
<https://reviews.apache.org/r/36378/#comment144642>
We should document this behavior at os::release() and move this there as an
alternative.
If it's kept in here then make static?
Argument should be named 'version', not 'v'.
Does it need to have such a long function name, particularly if it's local?
Version canonicalize(const Version& version).
src/linux/perf.cpp (line 481)
<https://reviews.apache.org/r/36378/#comment144544>
Suggest moving the TODO to here.
src/linux/perf.cpp (lines 484 - 485)
<https://reviews.apache.org/r/36378/#comment144545>
There's no map, it returns a single Sample?
src/linux/perf.cpp (lines 486 - 488)
<https://reviews.apache.org/r/36378/#comment144645>
This fits on a single line.
src/linux/perf.cpp (lines 490 - 492)
<https://reviews.apache.org/r/36378/#comment144546>
newlines, please ;-)
src/linux/perf.cpp (line 492)
<https://reviews.apache.org/r/36378/#comment144648>
Can you provide links to the documentation/commits that introduced these
changes?
src/linux/perf.cpp (line 508)
<https://reviews.apache.org/r/36378/#comment144549>
Check how patch versions are handled is correct, e.g., this would match
3.12.1 (if it existed) when I think the change was in 3.13.0 so it should be >=
3.13.0
src/linux/perf.cpp (line 524)
<https://reviews.apache.org/r/36378/#comment144649>
newline
src/linux/perf.cpp (line 532)
<https://reviews.apache.org/r/36378/#comment144651>
const T&
src/linux/perf.cpp (line 536)
<https://reviews.apache.org/r/36378/#comment144550>
newline
- Ian Downes
On July 9, 2015, 4:08 p.m., Paul Brett wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------
>
> (Updated July 9, 2015, 4:08 p.m.)
>
>
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Refactor Linux Performance monitor to handle changing 'perf stat' output
> versions depending on kernel version.
>
>
> Diffs
> -
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36378/diff/
>
>
> Testing
> ---
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90798 --- docs/network-isolation.md (line 7) <https://reviews.apache.org/r/36281/#comment143942> "which require to listen" is incorrect "a minimum choose a port in the Linux host ephemeral port range" h, what? I presume you really mean they should bind(0) and use what the kernel tells them to... saying "specific ports" here will be interpreted as 'I want my container to listen to port 80', not that I want to bind some specific port that I can then discover. docs/network-isolation.md (line 25) <https://reviews.apache.org/r/36281/#comment143925> This sentence is not clear, suggest rewording as "> 2.6.39 is advised for debugging purposes but is not required" docs/network-isolation.md (line 27) <https://reviews.apache.org/r/36281/#comment143926> s/development package for libnl3/libnl3 development package/ docs/network-isolation.md (line 48) <https://reviews.apache.org/r/36281/#comment143927> what does "but only assigned ports will be allocated by the kernel mean"? this is not clear. Please also state here that the ephemeral range is split and assigned. docs/network-isolation.md (line 52) <https://reviews.apache.org/r/36281/#comment143930> I think it's potentially confusing to call them short-lived (yes, I know that's historically how they've been used and how wikipedia categorizes them), since applications are free to bind to them as use them for the entirety of the job lifetime. docs/network-isolation.md (line 60) <https://reviews.apache.org/r/36281/#comment143933> You can write directly to `/proc/sys/net/ipv4/ip_local_port_range`. Please state why the reboot is (strongly) advised. docs/network-isolation.md (line 70) <https://reviews.apache.org/r/36281/#comment143935> Is it also recommended that ephemeral_ports per container be power-2 sized and aligned? Can you be precise in the limit on the number of containers? Can you document here the master flag to set a global limit to the number of containers to each slave used as a workaround because ephemeral ports are not exposed to the master. s/packets/packet/ docs/network-isolation.md (line 77) <https://reviews.apache.org/r/36281/#comment143938> Can you state and explain why there's no shaping/limit on ingress? State explicitly that shaping delays traffic and will not drop packets. - Ian Downes On July 7, 2015, 2:54 p.m., Paul Brett wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36281/ > --- > > (Updated July 7, 2015, 2:54 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. > > > Repository: mesos > > > Description > --- > > Document per-container unique egress flows and network queueing statistics. > > > Diffs > - > > docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 > docs/network-isolation.md PRE-CREATION > docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 > > Diff: https://reviews.apache.org/r/36281/diff/ > > > Testing > --- > > Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. > > > Thanks, > > Paul Brett > >
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34141: AppC provsioning backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34141/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Simple copy backend that forms the rootfs by copying each layer. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34141/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34139: AppC image discovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Local and simple discovery only. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34139/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31444/
---
(Updated July 7, 2015, 11:33 a.m.)
Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu, and
James Peach.
Bugs: MESOS-2350
https://issues.apache.org/jira/browse/MESOS-2350
Repository: mesos
Description
---
Optionally take a path that the launch helper should chroot to before exec'ing
the executor. It is assumed that the work directory is mounted to the
appropriate location under the chroot. In particular, the path to the executor
must be relative to the chroot.
Configuration that should be private to the chroot is done during the launch,
e.g. mounting proc and statically configuring basic devices. It is assumed that
other configuration, e.g., preparing the image, mounting in volumes or
persistent resources, is done by the caller.
Mounts can be made to the chroot (e.g., updating the volumes or persistent
resources) and they will propagate in to the container but mounts made inside
the container will not propagate out to the host.
It currently assumes that at least {{chroot}}/tmp is writeable and that mount
points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
This is specific to Linux.
Diffs
-
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263
src/slave/containerizer/mesos/launch.hpp
7c8b535746b5ce9add00afef86fdb6faefb5620e
src/slave/containerizer/mesos/launch.cpp
2f2d60e2011f60ec711d3b29fd2c157e30c83c34
src/tests/launch_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/31444/diff/
Testing
---
Manual testing only so far. This is harder to automate because we need a
self-contained chroot to execute something in... Suggestions welcome.
Thanks,
Ian Downes
Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.
> On June 29, 2015, 4:29 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/launch.cpp, lines 64-65
> > <https://reviews.apache.org/r/31444/diff/7/?file=989735#file989735line64>
> >
> > "must be relative to" is really "is interpreted as relative to" right?
> >
> > Just wanted be sure clarify:
> > 1) Should the user specify an absolute path with a preceding /?
> > 2) The directory path as observed by processes outside the choot jail
> > is `path::join(rootfs, directory)` right?
1) Yes, absolute path. Added this to the description.
2) Yes.
> On June 29, 2015, 4:29 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/launch.cpp, lines 259-260
> > <https://reviews.apache.org/r/31444/diff/7/?file=989735#file989735line259>
> >
> > "This must be an absolute path"
> >
> > As in, if the flags specifies a path without a preceding slash this
> > throws an error?
> >
> > This is not enforced is it?
Actually, it's just interpreted relative to the new root since we chdir() after
chroot() which will change to "/". I clarified the comment.
- Ian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31444/#review89425
---
On June 22, 2015, 9:38 a.m., Ian Downes wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31444/
> ---
>
> (Updated June 22, 2015, 9:38 a.m.)
>
>
> Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu,
> and James Peach.
>
>
> Bugs: MESOS-2350
> https://issues.apache.org/jira/browse/MESOS-2350
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Optionally take a path that the launch helper should chroot to before
> exec'ing the executor. It is assumed that the work directory is mounted to
> the appropriate location under the chroot. In particular, the path to the
> executor must be relative to the chroot.
>
> Configuration that should be private to the chroot is done during the launch,
> e.g. mounting proc and statically configuring basic devices. It is assumed
> that other configuration, e.g., preparing the image, mounting in volumes or
> persistent resources, is done by the caller.
>
> Mounts can be made to the chroot (e.g., updating the volumes or persistent
> resources) and they will propagate in to the container but mounts made inside
> the container will not propagate out to the host.
>
> It currently assumes that at least {{chroot}}/tmp is writeable and that mount
> points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
>
> This is specific to Linux.
>
>
> Diffs
> -
>
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263
> src/slave/containerizer/mesos/launch.hpp
> 7c8b535746b5ce9add00afef86fdb6faefb5620e
> src/slave/containerizer/mesos/launch.cpp
> 2f2d60e2011f60ec711d3b29fd2c157e30c83c34
> src/tests/launch_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31444/diff/
>
>
> Testing
> ---
>
> Manual testing only so far. This is harder to automate because we need a
> self-contained chroot to execute something in... Suggestions welcome.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36115: perf: changed 'parse' interface to allow testing and added tests.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/#review90177
---
src/tests/perf_tests.cpp (lines 76 - 84)
<https://reviews.apache.org/r/36115/#comment143173>
You're testing each version of parse here explicitly rather than testing
that parse() does the right thing for different versions of input. I would
construct different inputs corresponding to different perf versions and then
verify that the *output* had the correct fields and values.
src/tests/perf_tests.cpp (line 87)
<https://reviews.apache.org/r/36115/#comment143172>
```cpp
foreach (const tuple& input, input1) {}
```?
- Ian Downes
On July 1, 2015, 3:44 p.m., Chi Zhang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36115/
> ---
>
> (Updated July 1, 2015, 3:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> ---
>
> perf: changed 'parse' interface to allow testing and added tests.
>
>
> Diffs
> -
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36115/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36114: perf: added another extract function to support the new perf format after v3.12.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36114/#review90170
---
src/linux/perf.cpp (line 473)
<https://reviews.apache.org/r/36114/#comment143171>
Is there some way to have different extract functions without coding a
single, specific kernel version in the name, e.g., even extractv1, extractv2,
... seems preferable
src/linux/perf.cpp (lines 522 - 523)
<https://reviews.apache.org/r/36114/#comment143164>
long ternary operations should be aligned to ?
```cpp
tokens.size() == 6 ? tokens[3]
: PIDS_KEY
```
src/linux/perf.cpp (lines 531 - 532)
<https://reviews.apache.org/r/36114/#comment143170>
I don't understand these conditions, e.g., surely
```
(version > Version(3,12,0)) == (version >= Version(3,0,0) && version >
Version(3, 12, 0)
```
for all possible values of version, unless I'm not understanding how
versions are compared...?
For this type of selection I think it's clearest/simplest to start from the
lowest in the ordering:
```cpp
if (a < x) {
//
} else if (a < y) {
//
} else if (a < z) {
//
} else {
//
}
```
src/linux/perf.cpp (line 549)
<https://reviews.apache.org/r/36114/#comment143163>
Where is the variable `version` defined?
- Ian Downes
On July 1, 2015, 3:44 p.m., Chi Zhang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36114/
> -------
>
> (Updated July 1, 2015, 3:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> ---
>
> perf: added another extract function to support the new perf format after
> v3.12.
>
>
> Diffs
> -
>
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
>
> Diff: https://reviews.apache.org/r/36114/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36113: perf: refactored parse to allow determining an output parsing function based on the runtime version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36113/#review90167 --- src/linux/perf.cpp (lines 471 - 472) <https://reviews.apache.org/r/36113/#comment143159> This is not converting a line, it's converting a vector of tokens. Can you pass in the actual line and tokenize here? src/linux/perf.cpp (lines 493 - 497) <https://reviews.apache.org/r/36113/#comment143161> see below, this shouldn't be used by users directly, instead use an extract(line) wrapper function that determines the perf version and calls the appropriate parser src/linux/perf.cpp (lines 509 - 512) <https://reviews.apache.org/r/36113/#comment143160> Perf version should be handled transparently to the caller, i.e., the user just calls extract(line) and gets the right output. - Ian Downes On July 1, 2015, 3:44 p.m., Chi Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36113/ > --- > > (Updated July 1, 2015, 3:44 p.m.) > > > Review request for mesos, Ian Downes, Paul Brett, and Cong Wang. > > > Bugs: mesos-2834 > https://issues.apache.org/jira/browse/mesos-2834 > > > Repository: mesos > > > Description > --- > > perf: refactored parse to allow determining an output parsing function based > on the runtime version. > > > Diffs > - > > src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 > > Diff: https://reviews.apache.org/r/36113/diff/ > > > Testing > --- > > > Thanks, > > Chi Zhang > >
Re: Review Request 36112: perf: extracted out a 'version' function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36112/#review90168 --- src/linux/perf.cpp (lines 448 - 458) <https://reviews.apache.org/r/36112/#comment143162> See subsequent review, I don't think this needs to be pulled out into a separate function. - Ian Downes On July 1, 2015, 3:43 p.m., Chi Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36112/ > --- > > (Updated July 1, 2015, 3:43 p.m.) > > > Review request for mesos, Ian Downes, Paul Brett, and Cong Wang. > > > Bugs: MESOS-2834 > https://issues.apache.org/jira/browse/MESOS-2834 > > > Repository: mesos > > > Description > --- > > perf: extracted out a 'version' function. > > > Diffs > - > > src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 > > Diff: https://reviews.apache.org/r/36112/diff/ > > > Testing > --- > > > Thanks, > > Chi Zhang > >
Re: Review Request 36014: Fixed stack trace in isolator tests on Linux VM
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36014/#review89775 --- Ship it! Ship It! - Ian Downes On June 29, 2015, 11:58 a.m., Paul Brett wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36014/ > --- > > (Updated June 29, 2015, 11:58 a.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. > > > Bugs: MESOS-2956 > https://issues.apache.org/jira/browse/MESOS-2956 > > > Repository: mesos > > > Description > --- > > Fixed stack trace in isolator tests on Linux VM > > > Diffs > - > > src/tests/isolator_tests.cpp 525a5a88b8ab0681a89c5f7451d9ca9031c793af > > Diff: https://reviews.apache.org/r/36014/diff/ > > > Testing > --- > > sudo make check > > > Thanks, > > Paul Brett > >
Re: Review Request 34835: Add constexpr to C++ whitelist
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34835/#review89559 --- Ship it! docs/mesos-c++-style-guide.md (line 535) <https://reviews.apache.org/r/34835/#comment142184> "uses also converted to `const`" isn't grammatically correct, please revise. docs/mesos-c++-style-guide.md (line 537) <https://reviews.apache.org/r/34835/#comment142185> Isn't this conflating constexpr variables and constexpr functions, i.e., inline is only for constexpr functions? - Ian Downes On June 24, 2015, 2:14 p.m., Paul Brett wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34835/ > --- > > (Updated June 24, 2015, 2:14 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Bernd > Mathiske, Ben Mahler, Dave Lester, Ian Downes, Joerg Schad, Joris Van > Remoortere, Michael Park, Till Toenshoff, and Vinod Kone. > > > Bugs: MESOS-2784 > https://issues.apache.org/jira/browse/MESOS-2784 > > > Repository: mesos > > > Description > --- > > Add constexpr to C++11 whitelist > > > Diffs > - > > docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e > > Diff: https://reviews.apache.org/r/34835/diff/ > > > Testing > --- > > > Thanks, > > Paul Brett > >
Re: Review Request 35585: Updated Isolator to return required namespaces.
> On June 23, 2015, 11:01 a.m., Ian Downes wrote: > > Ship It! For the test error, are we not cleaning out /var/run/mesos/netns (correctly) between tests so we're trying to symlink container1 repeatedly? We should probably be storing these symlinks in a directory the test controls? - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35585/#review89011 --- On June 22, 2015, 10:03 p.m., Kapil Arya wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35585/ > --- > > (Updated June 22, 2015, 10:03 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Niklas Nielsen. > > > Bugs: MESOS-2884 > https://issues.apache.org/jira/browse/MESOS-2884 > > > Repository: mesos > > > Description > --- > > This would enable the MesosContainerizer to pass on a list of namespaces > to LinuxLauncher instead of having LinuxLauncher guess it from the > isolation flags. > > > Diffs > - > > include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 > src/slave/containerizer/isolator.cpp > d51ecc9347cafef90c92a6965e37f417b4929e79 > src/slave/containerizer/isolators/filesystem/shared.hpp > 08c6ffea0e2fada8e0b04f4ab15d8569c5416a8e > src/slave/containerizer/isolators/filesystem/shared.cpp > 5049306dea7b94dfaa4f7c2d8187bbae8c360633 > src/slave/containerizer/isolators/namespaces/pid.hpp > 6b24e2990311d0b09b3f6f2bb4ab29ee83fc > src/slave/containerizer/isolators/namespaces/pid.cpp > c6b28aa70415f4d88b14f0eeff7f20c35018fbdc > src/slave/containerizer/isolators/network/port_mapping.hpp > ee869ce71a68f51d58159c701d3c5d1e502b > src/slave/containerizer/isolators/network/port_mapping.cpp > 137cdc9b064b02e829af857cff7b9023ee7b7b16 > > Diff: https://reviews.apache.org/r/35585/diff/ > > > Testing > --- > > make check > > When I tried to run port-mapping tests as root, I got a bunch of errors. Any > ideas on what might be going on here? > > > ``` > thinkpad:/home/kapil/mesos/netns/build-port-mapping # GLOG_logtostderr=1 > GTEST_v=2 make check GTEST_FILTER="*Port*" > Making check in . > make[1]: Entering directory '/local/mesos/netns/build-port-mapping' > make[1]: Nothing to be done for 'check-am'. > make[1]: Leaving directory '/local/mesos/netns/build-port-mapping' > Making check in 3rdparty > make[1]: Entering directory '/local/mesos/netns/build-port-mapping/3rdparty' > make check-recursive > make[2]: Entering directory '/local/mesos/netns/build-port-mapping/3rdparty' > Making check in libprocess > make[3]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess' > Making check in 3rdparty > make[4]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make check-recursive > make[5]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > Making check in stout > make[6]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > Making check in . > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > make[7]: Nothing to be done for 'check-am'. > make[7]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > Making check in include > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout/include' > make[7]: Nothing to be done for 'check'. > make[7]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout/include' > make[6]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > make[6]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make libgmock.la stout-tests > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make[7]: 'libgmock.la' is up to date. > make[7]: 'stout-tests' is up to date. > make[7]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make check-local > make[
Re: Review Request 35585: Updated Isolator to return required namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35585/#review89011 --- Ship it! Ship It! - Ian Downes On June 22, 2015, 10:03 p.m., Kapil Arya wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35585/ > --- > > (Updated June 22, 2015, 10:03 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Niklas Nielsen. > > > Bugs: MESOS-2884 > https://issues.apache.org/jira/browse/MESOS-2884 > > > Repository: mesos > > > Description > --- > > This would enable the MesosContainerizer to pass on a list of namespaces > to LinuxLauncher instead of having LinuxLauncher guess it from the > isolation flags. > > > Diffs > - > > include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 > src/slave/containerizer/isolator.cpp > d51ecc9347cafef90c92a6965e37f417b4929e79 > src/slave/containerizer/isolators/filesystem/shared.hpp > 08c6ffea0e2fada8e0b04f4ab15d8569c5416a8e > src/slave/containerizer/isolators/filesystem/shared.cpp > 5049306dea7b94dfaa4f7c2d8187bbae8c360633 > src/slave/containerizer/isolators/namespaces/pid.hpp > 6b24e2990311d0b09b3f6f2bb4ab29ee83fc > src/slave/containerizer/isolators/namespaces/pid.cpp > c6b28aa70415f4d88b14f0eeff7f20c35018fbdc > src/slave/containerizer/isolators/network/port_mapping.hpp > ee869ce71a68f51d58159c701d3c5d1e502b > src/slave/containerizer/isolators/network/port_mapping.cpp > 137cdc9b064b02e829af857cff7b9023ee7b7b16 > > Diff: https://reviews.apache.org/r/35585/diff/ > > > Testing > --- > > make check > > When I tried to run port-mapping tests as root, I got a bunch of errors. Any > ideas on what might be going on here? > > > ``` > thinkpad:/home/kapil/mesos/netns/build-port-mapping # GLOG_logtostderr=1 > GTEST_v=2 make check GTEST_FILTER="*Port*" > Making check in . > make[1]: Entering directory '/local/mesos/netns/build-port-mapping' > make[1]: Nothing to be done for 'check-am'. > make[1]: Leaving directory '/local/mesos/netns/build-port-mapping' > Making check in 3rdparty > make[1]: Entering directory '/local/mesos/netns/build-port-mapping/3rdparty' > make check-recursive > make[2]: Entering directory '/local/mesos/netns/build-port-mapping/3rdparty' > Making check in libprocess > make[3]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess' > Making check in 3rdparty > make[4]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make check-recursive > make[5]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > Making check in stout > make[6]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > Making check in . > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > make[7]: Nothing to be done for 'check-am'. > make[7]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > Making check in include > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout/include' > make[7]: Nothing to be done for 'check'. > make[7]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout/include' > make[6]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty/stout' > make[6]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make libgmock.la stout-tests > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make[7]: 'libgmock.la' is up to date. > make[7]: 'stout-tests' is up to date. > make[7]: Leaving directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > make check-local > make[7]: Entering directory > '/local/mesos/netns/build-port-mapping/3rdparty/libprocess/3rdparty' > ./stout-tests > Note: Google Test filter = *Port* > [==] Running 0 tests from 0 test cases. > [==] 0 tests from 0 test cases ran. (0 m
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated June 22, 2015, 9:57 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- - Use protobuf representation of Appc Image Manifest - Implemented recover Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs (updated) - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34908: Rename --docker_sandbox_directory flag for general use.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34908/ --- (Updated June 22, 2015, 9:55 a.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- Rename --docker_sandbox_directory flag for general use. This will require users to update configuration scripts etc if they specify the old flag. Diffs (updated) - docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677 src/slave/containerizer/docker.cpp 00db9811824995fe19a6143aa2bcba3733a93147 src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 src/tests/docker_containerizer_tests.cpp 3a983c6813dab6fa03ccb2c87e1ea71866766d6e Diff: https://reviews.apache.org/r/34908/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34139: AppC image discovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/ --- (Updated June 22, 2015, 9:54 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Flag name changes. Switched to discovery returning at most a single URI. Repository: mesos Description --- Local and simple discovery only. Diffs (updated) - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34139/diff/ Testing --- Thanks, Ian Downes
