Re: Review Request 69009: Stout: Added a sync option for `write` and `rename`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69009/#review209509 --- Ship it! Ship It! - Andrew Schwartzmeyer On Oct. 12, 2018, 4:53 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69009/ > --- > > (Updated Oct. 12, 2018, 4:53 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and > Jan Schlicht. > > > Bugs: MESOS-9281 > https://issues.apache.org/jira/browse/MESOS-9281 > > > Repository: mesos > > > Description > --- > > This patch adds an option for the caller to sync the file and directory > contents to the disk after a write to prevent filesystem inconsistency > against reboots. > > > Diffs > - > > 3rdparty/stout/include/stout/os/posix/rename.hpp > 9cff6db16c8a4e5a79bf0082e18a1133bd287796 > 3rdparty/stout/include/stout/os/windows/rename.hpp > 523912ac3bf315f70f542e8eab7d2d02249909b4 > 3rdparty/stout/include/stout/os/write.hpp > cd35285a9595004bac627abf687588050aef8161 > 3rdparty/stout/include/stout/protobuf.hpp > 1d03e1e3a8dd642f7239d777fb04759caf100a8b > > > Diff: https://reviews.apache.org/r/69009/diff/2/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68899: Updated the MesosCon 2018 location.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68899/#review209157 --- Ship it! Ship It! - Andrew Schwartzmeyer On Oct. 2, 2018, 11:59 a.m., Gastón Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68899/ > --- > > (Updated Oct. 2, 2018, 11:59 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Updated the MesosCon 2018 location. > > > Diffs > - > > site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md > 09cf1a85f5d497c421d96c97bb855b9a486ccc0d > > > Diff: https://reviews.apache.org/r/68899/diff/1/ > > > Testing > --- > > Built the website using `site/mesos-website-dev.sh`, see the attached > screenshot. > > > File Attachments > > > Blog post screenshot > > https://reviews.apache.org/media/uploaded/files/2018/10/02/ddbc84c9-ff2f-44f0-b68e-c6bc9070b11a__screenshot.png > > > Thanks, > > Gastón Kleiman > >
Re: Review Request 68826: Fixed bug in `verify-reviews` due to mismatched types.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68826/ --- (Updated Sept. 24, 2018, 11:27 a.m.) Review request for mesos, Benjamin Bannier and Vinod Kone. Bugs: MESOS-9253 https://issues.apache.org/jira/browse/MESOS-9253 Repository: mesos Description --- Because Python is not type-safe, we encountered a bug in the code executed on non-Windows platforms that was expecting `output` to be a normal Python string instead of a Python byte string (with encoded content). To fix this, we now always decode the bytes into a string, so that the logic afterwards only has one type to deal with. Diffs - support/verify-reviews.py 72b7eb5b9baaf8eaa352b55dad55e62881d87323 Diff: https://reviews.apache.org/r/68826/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68826: Fixed bug in `verify-reviews` due to mismatched types.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68826/ --- Review request for mesos, Benjamin Bannier and Vinod Kone. Repository: mesos Description --- Because Python is not type-safe, we encountered a bug in the code executed on non-Windows platforms that was expecting `output` to be a normal Python string instead of a Python byte string (with encoded content). To fix this, we now always decode the bytes into a string, so that the logic afterwards only has one type to deal with. Diffs - support/verify-reviews.py 72b7eb5b9baaf8eaa352b55dad55e62881d87323 Diff: https://reviews.apache.org/r/68826/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68723: Fixed `verify-reviews.py` script to make `-o` optional.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68723/#review208646 --- support/verify-reviews.py Line 253 (original), 246 (patched) <https://reviews.apache.org/r/68723/#comment292751> Up to your use of it if you just want silence here instead. - Andrew Schwartzmeyer On Sept. 14, 2018, 12:39 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68723/ > --- > > (Updated Sept. 14, 2018, 12:39 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9232 > https://issues.apache.org/jira/browse/MESOS-9232 > > > Repository: mesos > > > Description > --- > > The semantics changed when the ReviewBot scripts were upstreamed; this > makes the `out_file` parameter optional, writing to stdout if not set. > > > Diffs > - > > support/verify-reviews.py ce9a35962e82e88648a19d0e8772ba0217ef229d > > > Diff: https://reviews.apache.org/r/68723/diff/1/ > > > Testing > --- > > Tested with `--skip-verify` on the CLI; argument is no longer required. > > > Thanks, > > Andrew Schwartzmeyer > >
Review Request 68723: Fixed `verify-reviews.py` script to make `-o` optional.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68723/ --- Review request for mesos and Benjamin Bannier. Bugs: MESOS-9232 https://issues.apache.org/jira/browse/MESOS-9232 Repository: mesos Description --- The semantics changed when the ReviewBot scripts were upstreamed; this makes the `out_file` parameter optional, writing to stdout if not set. Diffs - support/verify-reviews.py ce9a35962e82e88648a19d0e8772ba0217ef229d Diff: https://reviews.apache.org/r/68723/diff/1/ Testing --- Tested with `--skip-verify` on the CLI; argument is no longer required. Thanks, Andrew Schwartzmeyer
Re: Review Request 68625: Added 'MesosCon 2018 CFP is now closed!' blog post.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68625/ --- (Updated Sept. 4, 2018, 10 p.m.) Review request for mesos, Gastón Kleiman and Joerg Schad. Changes --- Fixed capitalization of 'Bay Area' as it is a proper noun when used together. Repository: mesos Description --- Added 'MesosCon 2018 CFP is now closed!' blog post. Diffs (updated) - site/source/blog/2018-09-05-mesoscon-2018-cfp-is-now-closed.md PRE-CREATION Diff: https://reviews.apache.org/r/68625/diff/2/ Changes: https://reviews.apache.org/r/68625/diff/1-2/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68625: Added 'MesosCon 2018 CFP is now closed!' blog post.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68625/ --- Review request for mesos, Gastón Kleiman and Joerg Schad. Repository: mesos Description --- Added 'MesosCon 2018 CFP is now closed!' blog post. Diffs - site/source/blog/2018-09-05-mesoscon-2018-cfp-is-now-closed.md PRE-CREATION Diff: https://reviews.apache.org/r/68625/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68420: Added /files API test for reserved query characters.
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote: > > src/tests/files_tests.cpp > > Lines 331-332 (patched) > > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331> > > > > This is a little odd to read without the context, this tests reserved > > characters by using `%`, but we could use anything other reserved character > > to acheive the same test, e.g. `+` seems the simplest: > > > > ``` > > // We use a reserved character for query parameters `+` to > > // ensure it is encoded and decoded correctly. > > const string filename = "foo+bar"; > > ``` > > > > If you'd like to stick with using `%`, probably we should just say: > > > > ``` > > // We use a reserved character for query parameters `%` to > > // ensure it is encoded and decoded correctly. > > const string filename = "foo%3Abar"; > > ``` > > > > And not do line 334 as it's irrelevant to this test > > Andrew Schwartzmeyer wrote: > You couldn't use `+` because it wouldn't have the same problem: the > second decode would not turn `+` into something else (this is why we haven't > run into this problem before with other characters, even a literal `:`). The > problem is that `%3A` _can_ be decoded, and so decoding more times than > encoding causes it to erroneously become `:`. The number of encodes and > decodes must be balanced. > > Also, line 334 is relevant to this test: it's demonstrating that the > query can be erroneously decoded too many times. > > Andrew Schwartzmeyer wrote: > I explained why thits is explicitly an entire percent-encoding, and am > considering this fixed. Thanks! Okay I am going to assume this was "fix it, then ship it" as you marked the other two as ship it, and I'd like to get at least the tests and libprocess fix in. - Andrew ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68420/#review207830 --- On Aug. 28, 2018, 10:40 a.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68420/ > --- > > (Updated Aug. 28, 2018, 10:40 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-9168 > https://issues.apache.org/jira/browse/MESOS-9168 > > > Repository: mesos > > > Description > --- > > Due to a bug in the HTTP client in libprocess (MESOS-9168), the > use of reserved query characters in paths did not work when using > the HTTP client in libprocess. This adds a test to ensure that > the /files API correctly handles reserved query characters in > file paths. > > > Diffs > - > > src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9 > > > Diff: https://reviews.apache.org/r/68420/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 68619: Defaulted support scripts to Python 3.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68619/#review208316 --- Ship it! Ship It! - Andrew Schwartzmeyer On Sept. 4, 2018, 4:41 a.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68619/ > --- > > (Updated Sept. 4, 2018, 4:41 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Kevin > Klues. > > > Bugs: MESOS-8770 > https://issues.apache.org/jira/browse/MESOS-8770 > > > Repository: mesos > > > Description > --- > > Use the new Python 3 support scripts by default, removing the deprecated > ones using Python 2. The new scripts offer feature-parity and more, they > require Python 3.6 or newer in order to run. > > > Diffs > - > > docs/windows.md 2edab6dc0d982d106a1e1d8bb0ab9da3c3482390 > support/README.md 0eae4d36ae2531b6f44f2a2b5da863e1431e360a > support/apply-reviews.py de22b7033a574562c59700914cde38cc5496d3e0 > support/build-virtualenv 90e1fc8210aa1cfc16ee9f28c4638193ef127849 > support/check-python3.py 898e5436ee2d0e2747a2720233fc13ec7c40f469 > support/generate-endpoint-help.py 8b38f739d544df64643d98ad8cfcaf9acbac4b87 > support/hooks/post-rewrite 81f345411e958aaffeca7d1b0a67d2bb561b1dad > support/hooks/pre-commit ac0ff24b49846dc5d6d2bdd04b4ac861a8745805 > support/jsonurl.py c9cf1d99c4433315b69c4945423f985a85951d8d > support/mesos-gtest-runner.py 9cabbdf901c8eb4df47b6b0ee46f48c56c0aa160 > support/mesos-split.py f83986db26dd640a8a91ec7a76c7a9ff69f61159 > support/mesos-style.py 7dbf96abd114c83a20d703e033943c38fa12b505 > support/pip-requirements.txt e3fbcc217827c480ab3b011a95973b67102a5bf7 > support/post-reviews.py 94ece857f15a8dc70680f226d1779bdfd11b50ff > support/push-commits.py b10e802f8b6ff0307e21a34f31c7f11b31d521b0 > support/python3/apply-reviews.py 92ad85945fb97e6ae3e21b3b9ab53c885158417e > support/python3/common.py > support/python3/generate-endpoint-help.py > 2db9e14b2749af958c714b4c4c1d8d1504fcafb0 > support/python3/get-review-ids.py > support/python3/jsonurl.py ffa3e362b31dcbbcbf4975d9bb41c074df4823b0 > support/python3/mesos-gtest-runner.py > 9cf72af246a2fabf2cb57ed42136eeaaa1495f20 > support/python3/mesos-split.py 0a77c257386ffe576abd12f59f926640836ad900 > support/python3/mesos-style.py c201038e99be88f5767fd0bdd0179186b566bef7 > support/python3/post-build-result.py > support/python3/post-reviews.py feb7d51d2457912f9996049e6d36b58323456d12 > support/python3/push-commits.py 7be67e3b3756cd152b0d8b5eebaa0a032aab9a88 > support/python3/test-upgrade.py 3a7bcac3c1a9ed8c923b639394258fe02e346b4f > support/python3/verify-reviews.py 14e5f9bf512732cb6705e7484efd86e2838287bb > support/test-upgrade.py b7c66123b9fbd0b660186fa8667de0acaf8bcaab > support/verify-reviews.py 3d8c67e0e161c24a25f0e89b34cef2099931038f > > > Diff: https://reviews.apache.org/r/68619/diff/1/ > > > Testing > --- > > This commit is mainly moving the files from `support/python3/` to `support/`. > I have fixed all the paths so that `support/python3` is not used anymore and > replaced by `support/`. I have tested `post-reviews.py` and `mesos-style.py` > to post this review request. > > > Thanks, > > Armand Grillet > >
Re: Review Request 68420: Added /files API test for reserved query characters.
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote: > > src/tests/files_tests.cpp > > Lines 331-332 (patched) > > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331> > > > > This is a little odd to read without the context, this tests reserved > > characters by using `%`, but we could use anything other reserved character > > to acheive the same test, e.g. `+` seems the simplest: > > > > ``` > > // We use a reserved character for query parameters `+` to > > // ensure it is encoded and decoded correctly. > > const string filename = "foo+bar"; > > ``` > > > > If you'd like to stick with using `%`, probably we should just say: > > > > ``` > > // We use a reserved character for query parameters `%` to > > // ensure it is encoded and decoded correctly. > > const string filename = "foo%3Abar"; > > ``` > > > > And not do line 334 as it's irrelevant to this test > > Andrew Schwartzmeyer wrote: > You couldn't use `+` because it wouldn't have the same problem: the > second decode would not turn `+` into something else (this is why we haven't > run into this problem before with other characters, even a literal `:`). The > problem is that `%3A` _can_ be decoded, and so decoding more times than > encoding causes it to erroneously become `:`. The number of encodes and > decodes must be balanced. > > Also, line 334 is relevant to this test: it's demonstrating that the > query can be erroneously decoded too many times. I explained why thits is explicitly an entire percent-encoding, and am considering this fixed. Thanks! - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68420/#review207830 --- On Aug. 28, 2018, 10:40 a.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68420/ > --- > > (Updated Aug. 28, 2018, 10:40 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-9168 > https://issues.apache.org/jira/browse/MESOS-9168 > > > Repository: mesos > > > Description > --- > > Due to a bug in the HTTP client in libprocess (MESOS-9168), the > use of reserved query characters in paths did not work when using > the HTTP client in libprocess. This adds a test to ensure that > the /files API correctly handles reserved query characters in > file paths. > > > Diffs > - > > src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9 > > > Diff: https://reviews.apache.org/r/68420/diff/2/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 68420: Added /files API test for reserved query characters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68420/ --- (Updated Aug. 28, 2018, 10:40 a.m.) Review request for mesos and Benjamin Mahler. Changes --- Updated with better explanation and a new test name and commit message. Summary (updated) - Added /files API test for reserved query characters. Bugs: MESOS-9168 https://issues.apache.org/jira/browse/MESOS-9168 Repository: mesos Description (updated) --- Due to a bug in the HTTP client in libprocess (MESOS-9168), the use of reserved query characters in paths did not work when using the HTTP client in libprocess. This adds a test to ensure that the /files API correctly handles reserved query characters in file paths. Diffs (updated) - src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9 Diff: https://reviews.apache.org/r/68420/diff/2/ Changes: https://reviews.apache.org/r/68420/diff/1-2/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68532: Windows: Ported `OsTest.Children, KillTree, ProcessExists`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68532/ --- (Updated Aug. 27, 2018, 3:26 p.m.) Review request for mesos, Akash Gupta, Joseph Wu, and Radhika Jandhyala. Changes --- Fixed failing test. Bugs: MESOS-5813 https://issues.apache.org/jira/browse/MESOS-5813 Repository: mesos Description --- These tests were (partially) ported by implementing logic similar to the what's used on POSIX with `os::Fork` and `os::Exec` (which will not be implemented on Windows). Diffs (updated) - 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c Diff: https://reviews.apache.org/r/68532/diff/3/ Changes: https://reviews.apache.org/r/68532/diff/2-3/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68532: Windows: Ported `OsTest.Children, KillTree, ProcessExists`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68532/ --- Review request for mesos, Akash Gupta, Joseph Wu, and Radhika Jandhyala. Bugs: MESOS-5813 https://issues.apache.org/jira/browse/MESOS-5813 Repository: mesos Description --- These tests were (partially) ported by implementing logic similar to the what's used on POSIX with `os::Fork` and `os::Exec` (which will not be implemented on Windows). Diffs - 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c Diff: https://reviews.apache.org/r/68532/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68530: Windows: Enabled `RemoveWithContinueOnError` test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68530/ --- Review request for mesos, Akash Gupta, Joseph Wu, and Radhika Jandhyala. Bugs: MESOS-5813 https://issues.apache.org/jira/browse/MESOS-5813 Repository: mesos Description --- This test "just worked" after previous fixes. Diffs - 3rdparty/stout/tests/os/rmdir_tests.cpp 3ca4099300c6288eb453ef04eb97d299adc5880d Diff: https://reviews.apache.org/r/68530/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68531: Windows: Ported `OsTest.Environment`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68531/ --- Review request for mesos, Akash Gupta, Joseph Wu, and Radhika Jandhyala. Bugs: MESOS-5813 https://issues.apache.org/jira/browse/MESOS-5813 Repository: mesos Description --- This required replacing `os::raw::environment()` inline as it does not exist on Windows. Furthermore, it was also discovered that the documentation is incorrect, and that sometimes environment variables start with an equal sign. The fix is to simply look for the first equal sign excluding the first character, otherwise we end up with empty keys. Diffs - 3rdparty/stout/include/stout/os/windows/environment.hpp 40eee59ac0b73ebe71b120e540ba7eace672ff4c 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c Diff: https://reviews.apache.org/r/68531/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68529: Windows: Added notes for permanently disabled tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68529/ --- Review request for mesos, Akash Gupta, Joseph Wu, and Radhika Jandhyala. Bugs: MESOS-5813 https://issues.apache.org/jira/browse/MESOS-5813 Repository: mesos Description --- Windows: Added notes for permanently disabled tests. Diffs - 3rdparty/stout/tests/os/filesystem_tests.cpp 09d0a40a74a293dcf6eecde2a443281e4b8d9fe8 3rdparty/stout/tests/os/rmdir_tests.cpp 3ca4099300c6288eb453ef04eb97d299adc5880d 3rdparty/stout/tests/os/strerror_tests.cpp 6a69d52a5b9ae93f3393ab17a3282ecb30ad90b0 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c Diff: https://reviews.apache.org/r/68529/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image manifest.
> On Aug. 23, 2018, 10:45 p.m., Jie Yu wrote: > > src/uri/fetchers/docker.cpp > > Lines 99 (patched) > > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line99> > > > > Can you explain a bit why you need to do this? > > > > this sounds to me that we need to make `URI` streaming function to be > > aware windows, instead of doing it adhoc here? Yes, that might be a good future refactor, but I don't think it's worth blocking these patches. We can add a `TODO(andschwa)` here to move this logic into `URI` (we had to do something similar coming from URIs with `path::from_uri` for the fetcher, it's reasonable to do it the other way too now). > On Aug. 23, 2018, 10:45 p.m., Jie Yu wrote: > > src/uri/fetchers/docker.cpp > > Lines 235 (patched) > > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line235> > > > > Can you check with Andy. Let's be consisent on doing encoding for path > > on windows. If we use percentage encoding in other places, let's be > > consistent I don't think this is worth blocking on right now; let's put in a `TODO(andschwa)` to reconcile with https://reviews.apache.org/r/68297/ as there is still ongoing dev-list dicussion on what and how we want to percent-encode. Nothing will take an immediate dependency on this, as this is "infrastructure" code working towards full support of the UCR. It can (and likely will) be modified in the future. > On Aug. 23, 2018, 10:45 p.m., Jie Yu wrote: > > src/uri/fetchers/docker.cpp > > Lines 735-736 (patched) > > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line735> > > > > Any reason we don't return a `Failure` here? Yes, there was an explanation in a prior iteration. IIRC this is because if the schema 2 isn't fetched, the protocol is that we fall back to schema 1. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68454/#review207849 --- On Aug. 23, 2018, 3:44 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68454/ > --- > > (Updated Aug. 23, 2018, 3:44 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, > Jie Yu, Joseph Wu, and Qian Zhang. > > > Bugs: MESOS-9159 > https://issues.apache.org/jira/browse/MESOS-9159 > > > Repository: mesos > > > Description > --- > > DockerFetcher now fetches both V2S1 and V2S2 manifests to save on > disk when agent is running on Windows. Linux part of the code in > agent is unchanged. In addition to fetching from DockerHub, > DockerFetcher now supports fetching from foreign URLs provided in > V2S2 Docker image manifest. > > > Diffs > - > > 3rdparty/stout/include/stout/path.hpp > ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7 > 3rdparty/stout/tests/path_tests.cpp > 452865b919c0d3644eb0ece0e17e402318aaff41 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/68454/diff/2/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 68420: Added regression test for encoded queries over the `/files` API.
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 326 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line326>
> >
> > How about: `ReservedQueryCharacter`
It's not quite the same. This isn't testing any arbitray reserved character
existing in the query; it's explicitly testing a percent-encoded sequence in
the query. How about: `QueryWithEncodedSequence`?
> On Aug. 23, 2018, 1:02 p.m., Benjamin Mahler wrote:
> > src/tests/files_tests.cpp
> > Lines 331-332 (patched)
> > <https://reviews.apache.org/r/68420/diff/1/?file=2074999#file2074999line331>
> >
> > This is a little odd to read without the context, this tests reserved
> > characters by using `%`, but we could use anything other reserved character
> > to acheive the same test, e.g. `+` seems the simplest:
> >
> > ```
> > // We use a reserved character for query parameters `+` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo+bar";
> > ```
> >
> > If you'd like to stick with using `%`, probably we should just say:
> >
> > ```
> > // We use a reserved character for query parameters `%` to
> > // ensure it is encoded and decoded correctly.
> > const string filename = "foo%3Abar";
> > ```
> >
> > And not do line 334 as it's irrelevant to this test
You couldn't use `+` because it wouldn't have the same problem: the second
decode would not turn `+` into something else (this is why we haven't run into
this problem before with other characters, even a literal `:`). The problem is
that `%3A` _can_ be decoded, and so decoding more times than encoding causes it
to erroneously become `:`. The number of encodes and decodes must be balanced.
Also, line 334 is relevant to this test: it's demonstrating that the query can
be erroneously decoded too many times.
- Andrew
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/#review207830
---
On Aug. 17, 2018, 6:20 p.m., Andrew Schwartzmeyer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68420/
> ---
>
> (Updated Aug. 17, 2018, 6:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9168
> https://issues.apache.org/jira/browse/MESOS-9168
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This test currently fails with:
>
> ```
> src\tests\files_tests.cpp(346): error: Value of: (response)->status
> Actual: "404 Not Found"
> Expected: OK().status
> Which is: "200 OK"
> Body: ""
> src\tests\files_tests.cpp(347): error: Value of: (response)->body
> Actual: ""
> Expected: stringify(expected)
> Which is: "{\"data\":\"body\"}"
> ```
>
> This is due to libprocess decoding the query in both `http::get()` and
> in the `DataDecoder`. However, libprocess only encodes once. When the
> user has a query with a literal ASCII escape sequence, it is expected
> that it should only need to be encoded once. That is `foo%3Abar`
> should be represented in the query as `foo%253Abar`. Because
> libprocess decodes twice, but currently only encodes once, this query
> erroneously becomes `foo:bar`. The added test demonstrates how this
> breaks something such as the `files` server.
>
>
> Diffs
> -
>
> src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
>
>
> Diff: https://reviews.apache.org/r/68420/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 68301: Changed `DEFAULT_EXECUTOR_INFO` to use `default:id`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68301/ --- (Updated Aug. 23, 2018, 11:26 a.m.) Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Changes --- Rebased. Repository: mesos Description --- By adding a `:` to the default executor ID used by the tests, this thoroughly exercises MESOS-9109. Diffs (updated) - src/tests/containerizer/volume_host_path_isolator_tests.cpp 81bf72e869d36edb162b121f9e84a53d2096dae3 src/tests/gc_tests.cpp 4f288cfd70cc53b4064bced96c3c5d6377c1c421 src/tests/health_check_tests.cpp 7544b2c20cb271655ad41c5d8f71739c26e6c638 src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 src/tests/master_validation_tests.cpp ec4fd13724a44a86ad655fb2a8affbb9f476834f src/tests/mesos.hpp 75c5fae8ed30f64c973b1cc290f8540a072cb8a8 src/tests/slave_authorization_tests.cpp 061e230b52805c4260e93ac83734aed5454ea3b5 src/tests/slave_tests.cpp 9597067799aaedf9d1c9d797454bb4bdf240cde1 src/tests/upgrade_tests.cpp de3911c77f9e40398672889cf43c25d36a6fb274 Diff: https://reviews.apache.org/r/68301/diff/4/ Changes: https://reviews.apache.org/r/68301/diff/3-4/ Testing --- This should have _almost_ all the tests pass, except for: ``` [ FAILED ] 9 tests, listed below: [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTaskGroupOnTaskFailure/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.KillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "docker,mesos" ``` Due to long-path issues with `SetCurrentDirectory` that cannot be worked around except via regedit + manifest. Thanks, Andrew Schwartzmeyer
Re: Review Request 68422: Added regression test for `http::internal::encode(Request)`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68422/ --- (Updated Aug. 23, 2018, 11:26 a.m.) Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Changes --- Updated per comments. Bugs: MESOS-9168 https://issues.apache.org/jira/browse/MESOS-9168 Repository: mesos Description --- This function is supposed to be encoding the query parameters, but currently fails to do so, resulting in: ``` libprocess\src\tests\http_tests.cpp(655): error: Value of: read.get() Expected: starts with "GET /?path=C%3A%5Cfoo%5Cbar%253Abaz" Actual: "GET /?path=C:\foo\bar%3Abaz HTTP/1.1\r\nConnection: close\r\nHost: mesos.apache.org\r\nContent-Length: 0\r\n\r\n" ``` The query parameter is going over the wire unencoded. Diffs (updated) - 3rdparty/libprocess/src/tests/http_tests.cpp 89d3bb2f91efe97591fe3f43e947413cf8b61f8c Diff: https://reviews.apache.org/r/68422/diff/2/ Changes: https://reviews.apache.org/r/68422/diff/1-2/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68319: Put binaries into root folder in libprocess.
> On Aug. 20, 2018, 11:18 a.m., Andrew Schwartzmeyer wrote: > > I have to ask: why? I've personally thought it would be nicer to remove > > this rather unexpected logic (from a CMake standpoint) from Mesos. Why add > > it to stout and libprocess? > > Benjamin Bannier wrote: > Thanks for engaging. The main idea would be to make the various binaries > easier to run, e.g., when manually executing tests. IMO the alternative to > the patches proposed here would be to all put them into a shared, _global_ > folder -- many projects use bin/ for that. > > Andrew Schwartzmeyer wrote: > We could do that in an `install` step, which seems to be CMake's > "canonical" method of aggregating the binaries. > > Re: manually executing tests: there is also `ctest`. So you can use ` > ctest -R Stout` to run the `StoutTests` regardless of where it is (which is > nice because you also don't have to remember that it is `stout-tests.exe` on > Windows). Though passing like `--gtest_filter` etc. I'm not so sure about... > > Benjamin Bannier wrote: > Fair enough. I thought this might be useful, e.g., when running tests > with parallel test runners or for the mentioned filtering, but I agree with > you that adding this extra complexity to the build setup isn't really worth > it (it might even confused multiconfig generators). > > As an example, llvm _does_ put produced binaries into a dedicated file, > but they have a dedicated function to deal with multiconfig generators > instead of directly setting this global variable. > > I'll drop the chain. I do like where you were going with it though. What we should do is get rid of this part of cruft from the Mesos CMake files, and make sure `make install` gives us a folder of stuff we want (though I imagine actually that we would want to ignore tests). - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68319/#review207624 --- On Aug. 20, 2018, 2:53 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68319/ > ------- > > (Updated Aug. 20, 2018, 2:53 a.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Repository: mesos > > > Description > --- > > Put binaries into root folder in libprocess. > > > Diffs > - > > 3rdparty/libprocess/CMakeLists.txt 5bba8e2c59ea3d7c37d186a2273bcaad6ffb5c46 > 3rdparty/libprocess/src/tests/CMakeLists.txt > a03a77eb5e289b4daac0bbd414dc17c8acc848dc > > > Diff: https://reviews.apache.org/r/68319/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 68329: Windows: Made `libwinio` the default option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68329/ --- (Updated Aug. 20, 2018, 3:11 p.m.) Review request for mesos, Akash Gupta, Benjamin Mahler, and Joseph Wu. Changes --- Added missing depedency (post-reviews on Windows is weird sometimes still). Bugs: MESOS-9084 https://issues.apache.org/jira/browse/MESOS-9084 Repository: mesos Description --- Note that we now add `io_tests.cpp` unconditionally, which will fail/hang with libevent. However, this is because libevent is buggy, and should not be used. We would prefer the tests not pass with libevent enabled, rather than appear to pass because the failing tests are excluded, giving the false impression that it works correctly. Diffs - 3rdparty/libprocess/src/CMakeLists.txt 19fa9809c2298ea68649702b94a0c75d806caba3 3rdparty/libprocess/src/tests/CMakeLists.txt a03a77eb5e289b4daac0bbd414dc17c8acc848dc Diff: https://reviews.apache.org/r/68329/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68398: Added `fs::used` helper API to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68398/#review207631 --- 3rdparty/stout/include/stout/windows/fs.hpp Lines 57 (patched) <https://reviews.apache.org/r/68398/#comment291076> `UNIMPLEMENTED()`. Should cause a compiler warning if used in a code-path on Windows. - Andrew Schwartzmeyer On Aug. 16, 2018, 4:51 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68398/ > --- > > (Updated Aug. 16, 2018, 4:51 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, > and Jiang Yan Xu. > > > Bugs: MESOS-5158 > https://issues.apache.org/jira/browse/MESOS-5158 > > > Repository: mesos > > > Description > --- > > Added a `fs::used` API that returns the amount of used space on a > given filesystem. > > > Diffs > - > > 3rdparty/stout/include/stout/posix/fs.hpp > 269a4f50f1df8d68be9e11030f885cf2c254c9d8 > 3rdparty/stout/include/stout/windows/fs.hpp > cc9a56012bd17cf962c8bc6d59ae221fcf19ecba > 3rdparty/stout/tests/os_tests.cpp b80c34e8e84cab68de7c843c7eafefbd84c3328c > > > Diff: https://reviews.apache.org/r/68398/diff/1/ > > > Testing > --- > > sudo make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 68396: Updated the ::pipe() system calls to pipe2 in posix subprocess.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68396/#review207629
---
3rdparty/libprocess/src/posix/subprocess.cpp
Line 66 (original), 68 (patched)
<https://reviews.apache.org/r/68396/#comment291075>
Oh that's a lot cleaner than `.get()[0]`... gotta go fix Windows now.
Though you can replace the declaration + assignments with just:
`return OutputFileDescriptors{pipefd->at(0), pipefd->at(1)};` and then
(after I use `at`) it'll look just like windows/subprocess.cpp!
Actually... it would end up identical save for the arguments sent to
`os::pipe`. Time for some consolidation?
- Andrew Schwartzmeyer
On Aug. 17, 2018, 3:36 p.m., Gilbert Song wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68396/
> ---
>
> (Updated Aug. 17, 2018, 3:36 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin
> Mahler, Jie Yu, James Peach, and Qian Zhang.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Updated the ::pipe() system calls to pipe2 in posix subprocess.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/src/posix/subprocess.cpp
> 01e3272fccda6ff66e1629fb10d22d8c4967b22a
>
>
> Diff: https://reviews.apache.org/r/68396/diff/1/
>
>
> Testing
> ---
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 68319: Put binaries into root folder in libprocess.
> On Aug. 20, 2018, 11:18 a.m., Andrew Schwartzmeyer wrote: > > I have to ask: why? I've personally thought it would be nicer to remove > > this rather unexpected logic (from a CMake standpoint) from Mesos. Why add > > it to stout and libprocess? > > Benjamin Bannier wrote: > Thanks for engaging. The main idea would be to make the various binaries > easier to run, e.g., when manually executing tests. IMO the alternative to > the patches proposed here would be to all put them into a shared, _global_ > folder -- many projects use bin/ for that. We could do that in an `install` step, which seems to be CMake's "canonical" method of aggregating the binaries. Re: manually executing tests: there is also `ctest`. So you can use ` ctest -R Stout` to run the `StoutTests` regardless of where it is (which is nice because you also don't have to remember that it is `stout-tests.exe` on Windows). Though passing like `--gtest_filter` etc. I'm not so sure about... - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68319/#review207624 --- On Aug. 20, 2018, 2:53 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68319/ > --- > > (Updated Aug. 20, 2018, 2:53 a.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Repository: mesos > > > Description > --- > > Put binaries into root folder in libprocess. > > > Diffs > - > > 3rdparty/libprocess/CMakeLists.txt 5bba8e2c59ea3d7c37d186a2273bcaad6ffb5c46 > 3rdparty/libprocess/src/tests/CMakeLists.txt > a03a77eb5e289b4daac0bbd414dc17c8acc848dc > > > Diff: https://reviews.apache.org/r/68319/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 68429: Fixed paths to 3rdparty test executable under Windows.
> On Aug. 20, 2018, 5:54 a.m., Mesos Reviewbot Windows wrote: > > FAIL: Some of the unit tests failed. Please check the relevant logs. > > > > Reviews applied: `['68318', '68319', '68429']` > > > > Failed command: `Start-MesosCITesting` > > > > All the build artifacts available at: > > http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2196/mesos-review-68429 > > > > Relevant logs: > > > > - > > [stout-tests.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2196/mesos-review-68429/logs/stout-tests.log): > > > > ``` > > 'D:\DCOS\mesos\3rdparty\stout\tests\Debug\stout-tests.exe' is not > > recognized as an internal or external command, > > operable program or batch file. > > ``` > > > > - > > [libprocess-tests.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2196/mesos-review-68429/logs/libprocess-tests.log): > > > > ``` > > 'D:\DCOS\mesos\3rdparty\libprocess\src\tests\Debug\libprocess-tests.exe' is > > not recognized as an internal or external command, > > operable program or batch file. > > ``` If you want to fix this: https://github.com/Microsoft/mesos-jenkins/blob/master/Mesos/start-windows-build.ps1 Also, I don't think editted file is still in use... except maybe by an ASF machine? Ping Joseph. We tried to delete it and something stopped us. https://reviews.apache.org/r/65441/ - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68429/#review207612 --- On Aug. 20, 2018, 4:56 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68429/ > --- > > (Updated Aug. 20, 2018, 4:56 a.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Repository: mesos > > > Description > --- > > Fixed paths to 3rdparty test executable under Windows. > > > Diffs > - > > support/windows-build.bat 49732c4baae3bdcdf8254ded03e7dab961b74b2e > > > Diff: https://reviews.apache.org/r/68429/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 68319: Put binaries into root folder in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68319/#review207624 --- I have to ask: why? I've personally thought it would be nicer to remove this rather unexpected logic (from a CMake standpoint) from Mesos. Why add it to stout and libprocess? - Andrew Schwartzmeyer On Aug. 20, 2018, 2:53 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68319/ > --- > > (Updated Aug. 20, 2018, 2:53 a.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Repository: mesos > > > Description > --- > > Put binaries into root folder in libprocess. > > > Diffs > - > > 3rdparty/libprocess/CMakeLists.txt 5bba8e2c59ea3d7c37d186a2273bcaad6ffb5c46 > 3rdparty/libprocess/src/tests/CMakeLists.txt > a03a77eb5e289b4daac0bbd414dc17c8acc848dc > > > Diff: https://reviews.apache.org/r/68319/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 68301: Changed `DEFAULT_EXECUTOR_INFO` to use `default:id`.
> On Aug. 17, 2018, 7:22 p.m., Mesos Reviewbot Windows wrote: > > FAIL: Some of the unit tests failed. Please check the relevant logs. > > > > Reviews applied: `['68420', '68422', '68375', '68297', '68374', '68301']` > > > > Failed command: `Start-MesosCITesting` > > > > All the build artifacts available at: > > http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2192/mesos-review-68301 > > > > Relevant logs: > > > > - > > [mesos-tests.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2192/mesos-review-68301/logs/mesos-tests.log): > > > > ``` > > I0818 02:22:05.121134 28916 hier[ OK ] > > IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (693 ms) > > [--] 1 test from IsolationFlag/MemoryIsolatorTest (713 ms total) > > > > [--] Global test environment tear-down > > [==] 1030 tests from 101 test cases ran. (598964 ms total) > > [ PASSED ] 1020 tests. > > [ FAILED ] 10 tests, listed below: > > [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTask/0, where > > GetParam() = "mesos" > > [ FAILED ] > > MesosContainerizer/DefaultExecutorTest.KillTaskGroupOnTaskFailure/0, where > > GetParam() = "mesos" > > [ FAILED ] > > MesosContainerizer/DefaultExecutorTest.CommitSuicideOnKillTask/0, where > > GetParam() = "mesos" > > [ FAILED ] MesosContainerizer/DefaultExecutorTest.MaxCompletionTime/0, > > where GetParam() = "mesos" > > [ FAILED ] MesosContainerizer/DefaultExecutorTest.TaskWithFileURI/0, > > where GetParam() = "mesos" > > [ FAILED ] > > ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.KillTask/0, > > where GetParam() = "docker,mesos" > > [ FAILED ] > > ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.KillTaskGroupOnTaskFailure/0, > > where GetParam() = "docker,mesos" > > [ FAILED ] > > ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.CommitSuicideOnKillTask/0, > > where GetParam() = "docker,mesos" > > [ FAILED ] > > ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.MaxCompletionTime/0, > > where GetParam() = "docker,mesos" > > [ FAILED ] > > ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.TaskWithFileURI/0, > > where GetParam() = "docker,mesos" > > > > 10 FAILED TESTS > > YOU HAVE 222 DISABLED TESTS > > > > archical.cpp:795] Agent 2ca394f1-f5a9-4188-add6-1beb8e603e46-S0 deactivated > > I0818 02:22:05.123139 28916 containerizer.cpp:2416] Destroying container > > 1cf96023-8016-42bf-9209-66b4f8dd101e in RUNNING state > > I0818 02:22:05.123139 28916 containerizer.cpp:3030] Transitioning the state > > of container 1cf96023-8016-42bf-9209-66b4f8dd101e from RUNNING to DESTROYING > > I0818 02:22:05.124130 28916 launcher.cpp:166] Asked to destroy container > > 1cf96023-8016-42bf-9209-66b4f8dd101e > > I0818 02:22:05.182164 28280 containerizer.cpp:2869] Container > > 1cf96023-8016-42bf-9209-66b4f8dd101e has exited > > I0818 02:22:05.212131 28068 master.cpp:1093] Master terminating > > I0818 02:22:05.214131 4480 hierarchical.cpp:637] Removed agent > > 2ca394f1-f5a9-4188-add6-1beb8e603e46-S0 > > I0818 02:22:05.592200 5000 process.cpp:926] Stopped the socket accept loop > > ``` These are expected due to long path issues on the host. We could (a) change `TMP` in the tests (not very nice) or (b) change `TMP` on the build machines themselves, which would work. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68301/#review207579 --- On Aug. 17, 2018, 6:19 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68301/ > --- > > (Updated Aug. 17, 2018, 6:19 p.m.) > > > Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > By adding a `:` to the default executor ID used by the tests, this > thoroughly exercises MESOS-9109. > > > Diffs > - > > src/tests/containerizer/volume_host_path_isolator_tests.cpp > 81bf72e869d36edb162b121f9e84a53d2096dae3 > src/tests/gc_tests.cpp 4f288c
Review Request 68420: Added regression test for encoded queries over the `/files` API.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68420/
---
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
---
This test currently fails with:
```
src\tests\files_tests.cpp(346): error: Value of: (response)->status
Actual: "404 Not Found"
Expected: OK().status
Which is: "200 OK"
Body: ""
src\tests\files_tests.cpp(347): error: Value of: (response)->body
Actual: ""
Expected: stringify(expected)
Which is: "{\"data\":\"body\"}"
```
This is due to libprocess decoding the query in both `http::get()` and
in the `DataDecoder`. However, libprocess only encodes once. When the
user has a query with a literal ASCII escape sequence, it is expected
that it should only need to be encoded once. That is `foo%3Abar`
should be represented in the query as `foo%253Abar`. Because
libprocess decodes twice, but currently only encodes once, this query
erroneously becomes `foo:bar`. The added test demonstrates how this
breaks something such as the `files` server.
Diffs
-
src/tests/files_tests.cpp d09279077ec704167991933349ee943ab44d0aa9
Diff: https://reviews.apache.org/r/68420/diff/1/
Testing
---
Thanks,
Andrew Schwartzmeyer
Re: Review Request 68375: Fixed encoding behavior of `http::internal::encode(Request)`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68375/ --- (Updated Aug. 17, 2018, 6:19 p.m.) Review request for mesos and Benjamin Mahler. Summary (updated) - Fixed encoding behavior of `http::internal::encode(Request)`. Repository: mesos Description (updated) --- Fixed encoding behavior of `http::internal::encode(Request)`. Diffs (updated) - 3rdparty/libprocess/src/http.cpp e9d439280a10c49e2ac7e0acfbb8f5934e0eb53b Diff: https://reviews.apache.org/r/68375/diff/2/ Changes: https://reviews.apache.org/r/68375/diff/1-2/ Testing --- Just posting the last of the chain that currently passes on my machine (with a shorter path set for `$TMP`). Needs more tests and testing. Thanks, Andrew Schwartzmeyer
Re: Review Request 68301: Changed `DEFAULT_EXECUTOR_INFO` to use `default:id`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68301/ --- (Updated Aug. 17, 2018, 6:19 p.m.) Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Summary (updated) - Changed `DEFAULT_EXECUTOR_INFO` to use `default:id`. Repository: mesos Description (updated) --- By adding a `:` to the default executor ID used by the tests, this thoroughly exercises MESOS-9109. Diffs (updated) - src/tests/containerizer/volume_host_path_isolator_tests.cpp 81bf72e869d36edb162b121f9e84a53d2096dae3 src/tests/gc_tests.cpp 4f288cfd70cc53b4064bced96c3c5d6377c1c421 src/tests/health_check_tests.cpp 7544b2c20cb271655ad41c5d8f71739c26e6c638 src/tests/master_tests.cpp 5accfbb8681ab2d903cca13b0ef05a3bdce9b1fe src/tests/master_validation_tests.cpp ec4fd13724a44a86ad655fb2a8affbb9f476834f src/tests/mesos.hpp 75c5fae8ed30f64c973b1cc290f8540a072cb8a8 src/tests/slave_authorization_tests.cpp 061e230b52805c4260e93ac83734aed5454ea3b5 src/tests/slave_tests.cpp 9597067799aaedf9d1c9d797454bb4bdf240cde1 src/tests/upgrade_tests.cpp de3911c77f9e40398672889cf43c25d36a6fb274 Diff: https://reviews.apache.org/r/68301/diff/3/ Changes: https://reviews.apache.org/r/68301/diff/2-3/ Testing --- This should have _almost_ all the tests pass, except for: ``` [ FAILED ] 9 tests, listed below: [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTaskGroupOnTaskFailure/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.KillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "docker,mesos" ``` Due to long-path issues with `SetCurrentDirectory` that cannot be worked around except via regedit + manifest. Thanks, Andrew Schwartzmeyer
Review Request 68422: Added regression test for `http::internal::encode(Request)`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68422/ --- Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Repository: mesos Description --- This function is supposed to be encoding the query parameters, but currently fails to do so, resulting in: ``` libprocess\src\tests\http_tests.cpp(655): error: Value of: read.get() Expected: starts with "GET /?path=C%3A%5Cfoo%5Cbar%253Abaz" Actual: "GET /?path=C:\foo\bar%3Abaz HTTP/1.1\r\nConnection: close\r\nHost: mesos.apache.org\r\nContent-Length: 0\r\n\r\n" ``` The query parameter is going over the wire unencoded. Diffs - 3rdparty/libprocess/src/tests/http_tests.cpp 89d3bb2f91efe97591fe3f43e947413cf8b61f8c Diff: https://reviews.apache.org/r/68422/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 68297: Windows: Fixed a bug when `executorId` contains a `:`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68297/ --- (Updated Aug. 17, 2018, 6:18 p.m.) Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Bugs: MESOS-9109 https://issues.apache.org/jira/browse/MESOS-9109 Repository: mesos Description (updated) --- Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their `executorId`, but this is a reserved character on Windows, and when its used as part of the path on disk results in an OS error. Unfortunately, we need to remain backward-compatible with existing frameworks, so now we escape `:` as `%3A` when writing to disk, and unescape it when parsing. Diffs (updated) - src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d Diff: https://reviews.apache.org/r/68297/diff/3/ Changes: https://reviews.apache.org/r/68297/diff/2-3/ Testing --- ``` [--] Global test environment tear-down [==] 1008 tests from 98 test cases ran. (446260 ms total) [ PASSED ] 1008 tests. ``` Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it). Thanks, Andrew Schwartzmeyer
Review Request 68375: WIP: Added missing encode. Needs tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68375/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description --- WIP: Added missing encode. Needs tests. Diffs - 3rdparty/libprocess/src/http.cpp e9d439280a10c49e2ac7e0acfbb8f5934e0eb53b Diff: https://reviews.apache.org/r/68375/diff/1/ Testing --- Just posting the last of the chain that currently passes on my machine (with a shorter path set for `$TMP`). Needs more tests and testing. Thanks, Andrew Schwartzmeyer
Re: Review Request 68301: WIP: Tests with `default:id`. Not for merging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68301/ --- (Updated Aug. 15, 2018, 8:13 p.m.) Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Repository: mesos Description (updated) --- Need to also add tests for a `taskId` with a `:`. Diffs (updated) - src/tests/gc_tests.cpp 4f288cfd70cc53b4064bced96c3c5d6377c1c421 src/tests/master_validation_tests.cpp ec4fd13724a44a86ad655fb2a8affbb9f476834f src/tests/mesos.hpp 75c5fae8ed30f64c973b1cc290f8540a072cb8a8 src/tests/slave_tests.cpp 9597067799aaedf9d1c9d797454bb4bdf240cde1 Diff: https://reviews.apache.org/r/68301/diff/2/ Changes: https://reviews.apache.org/r/68301/diff/1-2/ Testing --- This should have _almost_ all the tests pass, except for: ``` [ FAILED ] 9 tests, listed below: [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTaskGroupOnTaskFailure/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.KillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "docker,mesos" ``` Due to long-path issues with `SetCurrentDirectory` that cannot be worked around except via regedit + manifest. Thanks, Andrew Schwartzmeyer
Review Request 68374: Windows: Fixed a bug when `taskID` contains a `:`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68374/ --- Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Repository: mesos Description --- Windows: Fixed a bug when `taskID` contains a `:`. Diffs - src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 Diff: https://reviews.apache.org/r/68374/diff/1/ Testing --- TODO Thanks, Andrew Schwartzmeyer
Re: Review Request 68297: Windows: Fixed a bug when `executorId` contains a `:`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68297/ --- (Updated Aug. 15, 2018, 8:12 p.m.) Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Changes --- Switched to `%3A`. TODO: Maybe use `process::http::encode` here instead. Bugs: MESOS-9109 https://issues.apache.org/jira/browse/MESOS-9109 Repository: mesos Description --- Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their `executorId`, but this is a reserved character on Windows, and when its use as part of the path on disk results in an OS error. Unfortunately, we need to remain backward-compatible with existing frameworks, so now we escape `:` as `_COLON_` when writing to disk, and unescape it when parsing. Note that we explicitly do not escape to `%3A` (the ASCII equivalent), because otherwise it is impossible to query for the file over HTTP, as libprocess helpfully (but not in this case) pre-emptively decodes it to `:` and so queries for the incorrect path. Diffs (updated) - src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d Diff: https://reviews.apache.org/r/68297/diff/2/ Changes: https://reviews.apache.org/r/68297/diff/1-2/ Testing --- ``` [--] Global test environment tear-down [==] 1008 tests from 98 test cases ran. (446260 ms total) [ PASSED ] 1008 tests. ``` Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it). Thanks, Andrew Schwartzmeyer
Re: Review Request 67505: Refactored verify-reviews.py to use commons.py and argparse.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67505/#review207251 --- Ship it! Ship It! - Andrew Schwartzmeyer On Aug. 7, 2018, 10:37 a.m., Dragos Schebesch wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67505/ > --- > > (Updated Aug. 7, 2018, 10:37 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Armand Grillet. > > > Repository: mesos > > > Description > --- > > Refactored verify-reviews.py to use commons.py and argparse. > > > Diffs > - > > support/python3/verify-reviews.py 2e925908ffb59dbcdfe99691c5bdbc36a3b7d855 > > > Diff: https://reviews.apache.org/r/67505/diff/5/ > > > Testing > --- > > Sample run on this review request: > ``` > ./support/python3/verify-reviews.py -u -p -r 67505 file -o > test.txt > > 07-18-18_11:09:22 - Running support/python3/verify-reviews.py > 0 review requests need verification > ``` > > > Thanks, > > Dragos Schebesch > >
Re: Review Request 67504: Added support script to post build results.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67504/#review207243 --- Ship it! Ship It! - Andrew Schwartzmeyer On Aug. 7, 2018, 10:37 a.m., Dragos Schebesch wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67504/ > --- > > (Updated Aug. 7, 2018, 10:37 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Armand Grillet. > > > Repository: mesos > > > Description > --- > > Added support script to post build results. > > > Diffs > - > > support/python3/post-build-result.py PRE-CREATION > > > Diff: https://reviews.apache.org/r/67504/diff/5/ > > > Testing > --- > > Sample test run that can be seen on this review request: > ``` > ./python3/post-build-result.py -r 67504 -u -p -m 'dummy' -o > http://dummy.com/artifact > Posting to review request: https://reviews.apache.org/r/67504 > dummy > > All the build artifacts available at: http://dummy.com/artifact > ``` > > > Thanks, > > Dragos Schebesch > >
Re: Review Request 67503: Added support helper for fetching review ids.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67503/#review207242 --- Ship it! Ship It! - Andrew Schwartzmeyer On Aug. 7, 2018, 10:36 a.m., Dragos Schebesch wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67503/ > --- > > (Updated Aug. 7, 2018, 10:36 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Armand Grillet. > > > Repository: mesos > > > Description > --- > > Added support helper for fetching review ids. > > > Diffs > - > > support/python3/get-review-ids.py PRE-CREATION > > > Diff: https://reviews.apache.org/r/67503/diff/5/ > > > Testing > --- > > Sample run on this review request id: > ``` > ./get-review-ids.py -r 67503 > Dependent review: https://reviews.apache.org/api/review-requests/67502/ > 67502 > > 67503 > ``` > > > Thanks, > > Dragos Schebesch > >
Re: Review Request 67502: Refactored ReviewBoard API functionality into separate module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67502/#review207241 --- Fix it, then Ship it! support/python3/common.py Lines 153-154 (patched) <https://reviews.apache.org/r/67502/#comment290552> I actually disagree with the TODO here: we often use an empty diff (but a new iteration of the review) to purposefully trigger the Review Bot. - Andrew Schwartzmeyer On Aug. 7, 2018, 10:36 a.m., Dragos Schebesch wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67502/ > --- > > (Updated Aug. 7, 2018, 10:36 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Armand Grillet. > > > Repository: mesos > > > Description > --- > > Refactored ReviewBoard API functionality into separate module. > > > Diffs > - > > support/python3/common.py PRE-CREATION > > > Diff: https://reviews.apache.org/r/67502/diff/5/ > > > Testing > --- > > For example, to call the api on a specific review_url, with some data, we > would use the following code: > > ``` > ReviewBoardHandler().api(review_url, data) > ``` > > > Thanks, > > Dragos Schebesch > >
Review Request 68331: Windows: Made `libwinio` the default option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68331/ --- Review request for mesos, Akash Gupta, Benjamin Mahler, and Joseph Wu. Bugs: MESOS-9084 https://issues.apache.org/jira/browse/MESOS-9084 Repository: mesos Description --- With no options specified, Windows will now build with the `libwinio` (native Windows Thread API eventing library) by default (similar to POSIX defaulting to `libev` without options). To use `libevent` (NOT recommended), it must be explicitly enabled, and will emit an appropriate warning. Furthermore, the tests which were previously excluded when `libevent` was enabled are now enabled on Windows unconditionally. This serves two reasons: (1) it simplifies the build logic and (2) by failing with `libevent` it demonstrates why it is not recommended (and avoids giving false impressions). Diffs - 3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c cmake/CompilationConfigure.cmake 10cacfb99e2cff1ddd2285ae441730f61182e06d docs/windows.md f0f12fc81ed664528cf7496cf75e1cb11eee6013 src/tests/CMakeLists.txt fed072a8a761c6b9e65b2c75f2e92facd8353f7c Diff: https://reviews.apache.org/r/68331/diff/1/ Testing --- Built with and without `-DENABLE_LIBEVENT` on Windows. Thanks, Andrew Schwartzmeyer
Review Request 68329: Windows: Made `libwinio` the default option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68329/ --- Review request for mesos, Akash Gupta, Benjamin Mahler, and Joseph Wu. Bugs: MESOS-9084 https://issues.apache.org/jira/browse/MESOS-9084 Repository: mesos Description --- Note that we now add `io_tests.cpp` unconditionally, which will fail/hang with libevent. However, this is because libevent is buggy, and should not be used. We would prefer the tests not pass with libevent enabled, rather than appear to pass because the failing tests are excluded, giving the false impression that it works correctly. Diffs - 3rdparty/libprocess/src/CMakeLists.txt 19fa9809c2298ea68649702b94a0c75d806caba3 3rdparty/libprocess/src/tests/CMakeLists.txt a03a77eb5e289b4daac0bbd414dc17c8acc848dc Diff: https://reviews.apache.org/r/68329/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 67932: Windows: Extracted file layers using `wclayer` from `hcsshim`.
> On Aug. 10, 2018, 7:41 p.m., Mesos Reviewbot Windows wrote: > > FAIL: Mesos binaries failed to build. > > > > Reviews applied: `['67931', '67930', '67984', '67932']` > > > > Failed command: `cmake.exe --build . --config Release -- /maxcpucount` > > > > All the build artifacts available at: > > http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2135/mesos-review-67932 Seems like a dependency problem. All the tests built and ran and passed. But the "binaries" target(?) failed with: ``` CUSTOMBUILD : error : downloading 'https://github.com/mesos/3rdparty/raw/master/wclayer.exe' failed [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : The requested URL returned error : 503 first byte timeout [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : error : downloading 'https://github.com/mesos/3rdparty/raw/master/wclayer.exe' failed [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : The requested URL returned error : 503 first byte timeout [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : error : downloading 'https://github.com/mesos/3rdparty/raw/master/wclayer.exe' failed [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : The requested URL returned error : 503 first byte timeout [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : error : downloading 'https://github.com/mesos/3rdparty/raw/master/wclayer.exe' failed [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : The requested URL returned error : 503 first byte timeout [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : error : downloading 'https://github.com/mesos/3rdparty/raw/master/wclayer.exe' failed [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : The requested URL returned error : 502 Bad Gateway [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : error : downloading 'https://github.com/mesos/3rdparty/raw/master/wclayer.exe' failed [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] CUSTOMBUILD : The requested URL returned error : 502 Bad Gateway [D:\DCOS\mesos\3rdparty\wclayer-WIP.vcxproj] ``` - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review207103 --- On Aug. 6, 2018, 11:46 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated Aug. 6, 2018, 11:46 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, > Jie Yu, Joseph Wu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so we use > `wclayer` instead. Note that the folder generated from extraction also > cannot be deleted by `rmdir`, so the GC is also changed to use > `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > > > Diff: https://reviews.apache.org/r/67932/diff/8/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 68303: Fixed gRPC compilation for non-SSL build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68303/#review207098 --- Ship it! Tested with `libssl-dev` uninstalled; repro'ed without, built with. Thanks! - Andrew Schwartzmeyer On Aug. 10, 2018, 3:17 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68303/ > --- > > (Updated Aug. 10, 2018, 3:17 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier. > > > Bugs: MESOS-9149 > https://issues.apache.org/jira/browse/MESOS-9149 > > > Repository: mesos > > > Description > --- > > Fixed gRPC compilation for non-SSL build. > > > Diffs > - > > 3rdparty/grpc-1.10.0.patch 98ae6af2b82dbad42cabf7f19dbbb3539c2dd6dc > 3rdparty/grpc.md 8d0e189e0d7a212e5ebd73f53adceb26457735e8 > > > Diff: https://reviews.apache.org/r/68303/diff/1/ > > > Testing > --- > > `make check` on a fresh Ubuntu 16.04 box. > > > Thanks, > > Chun-Hung Hsiao > >
Review Request 68301: WIP: Tests with `default:id`. Not for merging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68301/ --- Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Repository: mesos Description --- WIP: Tests with `default:id`. Not for merging. Diffs - src/tests/master_validation_tests.cpp ec4fd13724a44a86ad655fb2a8affbb9f476834f src/tests/mesos.hpp 75c5fae8ed30f64c973b1cc290f8540a072cb8a8 src/tests/slave_tests.cpp 9597067799aaedf9d1c9d797454bb4bdf240cde1 Diff: https://reviews.apache.org/r/68301/diff/1/ Testing --- This should have _almost_ all the tests pass, except for: ``` [ FAILED ] 9 tests, listed below: [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.KillTaskGroupOnTaskFailure/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "mesos" [ FAILED ] MesosContainerizer/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.KillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.CommitSuicideOnKillTask/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.MaxCompletionTime/0, where GetParam() = "docker,mesos" [ FAILED ] ROOT_DOCKER_DockerAndMesosContainerizers/DefaultExecutorTest.TaskWithFileURI/0, where GetParam() = "docker,mesos" ``` Due to long-path issues with `SetCurrentDirectory` that cannot be worked around except via regedit + manifest. Thanks, Andrew Schwartzmeyer
Review Request 68297: Windows: Fixed a bug when `executorId` contains a `:`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68297/ --- Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Joseph Wu. Bugs: MESOS-9109 https://issues.apache.org/jira/browse/MESOS-9109 Repository: mesos Description --- Per MESOS-9109, some frameworks, such as Chronos, use a `:` in their `executorId`, but this is a reserved character on Windows, and when its use as part of the path on disk results in an OS error. Unfortunately, we need to remain backward-compatible with existing frameworks, so now we escape `:` as `_COLON_` when writing to disk, and unescape it when parsing. Note that we explicitly do not escape to `%3A` (the ASCII equivalent), because otherwise it is impossible to query for the file over HTTP, as libprocess helpfully (but not in this case) pre-emptively decodes it to `:` and so queries for the incorrect path. Diffs - src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 src/tests/paths_tests.cpp 4808402f789ebc783b308c03b8e412ba22bf9f5d Diff: https://reviews.apache.org/r/68297/diff/1/ Testing --- ``` [--] Global test environment tear-down [==] 1008 tests from 98 test cases ran. (446260 ms total) [ PASSED ] 1008 tests. ``` Note that this is without changing the used-everywhere default executor ID of `default` to `default:id`, which I also tested, but required changes of asserts (and caused long path issues only solvable by changing the machine's registry, so I'd rather not include it). Thanks, Andrew Schwartzmeyer
Re: Review Request 68265: Built gRPC support in mesos-tidy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68265/#review207000 --- Ship it! ``` make[1]: Leaving directory '/BUILD' No mesos-tidy violations found. ``` - Andrew Schwartzmeyer On Aug. 7, 2018, 4:56 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68265/ > --- > > (Updated Aug. 7, 2018, 4:56 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Michael > Park. > > > Repository: mesos > > > Description > --- > > Built gRPC support in mesos-tidy. > > > Diffs > - > > support/mesos-tidy/Dockerfile 66adbd006490d4f6b2f10d77b612fe82f8ef6141 > support/mesos-tidy/entrypoint.sh bb6344c01a01e35d32741f1886ef2029194e126b > > > Diff: https://reviews.apache.org/r/68265/diff/1/ > > > Testing > --- > > `support/mesos-tidy.sh` > > > Thanks, > > Chun-Hung Hsiao > >
Review Request 68263: Fixed Windows build break due to missing lambda capture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68263/ --- Review request for mesos and James Peach. Repository: mesos Description --- Since no default capture semantics were specified, `messageCount` was not captured which MSVC refused to compile. Interestingly, clang emits a warning if `messageCount` is captured explicitly, so we compromise with an implicit value capture and explicit reference capture of `q`. Diffs - 3rdparty/libprocess/src/tests/benchmarks.cpp 16b3bca44ca3e9a5fddd93fa02b9a3021c696fda Diff: https://reviews.apache.org/r/68263/diff/1/ Testing --- Windows build, cquery (clang) did not emit warning. Thanks, Andrew Schwartzmeyer
Review Request 68238: Fixed `mesos-style.py` for Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68238/ --- Review request for mesos and Armand Grillet. Repository: mesos Description --- Due to an unfortunate platform difference, the `virtualenv` subfolder is `Script` instead of `bin` on Windows. Some hacky inline scripting was done to mimic the shell script `build-virtualenv` to build the `virtualenv` on Windows. Diffs - support/python3/mesos-style.py 12fd33061fcb2cb04349ec1138844ea712610259 Diff: https://reviews.apache.org/r/68238/diff/1/ Testing --- Can commit using normal hooks on Windows. Thanks, Andrew Schwartzmeyer
Re: Review Request 68176: Fixed gRPC release build on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68176/#review206846 --- Ship it! Ship It! - Andrew Schwartzmeyer On Aug. 2, 2018, 4:06 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68176/ > --- > > (Updated Aug. 2, 2018, 4:06 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph > Wu. > > > Bugs: MESOS-8395 > https://issues.apache.org/jira/browse/MESOS-8395 > > > Repository: mesos > > > Description > --- > > The gRPC library was by default built by CMake with the "Debug" > configuration on Windows, even if Mesos is built with the "Release" > configuration. As a result, gRPC would try to link to the "Debug" > protobuf library and end up with a failure. This patch fixes this > problem by explicitly specifying the configuration in gRPC's build > command. > > > Diffs > - > > 3rdparty/CMakeLists.txt 8300088aaec5afd5ea92268757235d8561cda916 > > > Diff: https://reviews.apache.org/r/68176/diff/1/ > > > Testing > --- > > Built and tested using VS on Windows > Built and tested using VS on Windows with `--config Release` > Built and tested using Ninja on Linux > Built and tested using Makefile on Linux > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 67933: Replaced `` with `` in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67933/#review206761 --- This was trivial and Windows only so I shipped it. - Andrew Schwartzmeyer On July 16, 2018, 2:43 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67933/ > --- > > (Updated July 16, 2018, 2:43 p.m.) > > > Review request for mesos, Akash Gupta, John Kordich, Joseph Wu, and Radhika > Jandhyala. > > > Repository: mesos > > > Description > --- > > The intent is that `os.hpp` includes the correct OS-specific header, > `windows/os.hpp` or `posix/os.hpp`, automatically. They should never > be included manually. > > > Diffs > - > > 3rdparty/stout/include/stout/os/windows/kill.hpp > bd944357bd741d62aea78c839d87b80853e4a852 > 3rdparty/stout/include/stout/os/windows/rm.hpp > 1c66e3e6fa20116866842378077e10052c147289 > 3rdparty/stout/include/stout/windows/net.hpp > d70f967773d23672f53dd7dd6123434f5e81eeb0 > > > Diff: https://reviews.apache.org/r/67933/diff/1/ > > > Testing > --- > > Old patch I found on my computer that I figured I should post. `ninja tests ; > ctest -V` on Windows. > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67931/#review206696 --- Ship it! I'll clean up for Mesos whitespace/formatting/commenting etc., and double-check the schema, but otherwise LGTM - Andrew Schwartzmeyer On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67931/ > --- > > (Updated July 31, 2018, 2:41 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Support parsing schema 2 and fetching blob with multiple URLs as > specified in schema 2. Update `curl` version to 7.61.0 because of bug > encountered in version 7.57.0. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 > include/mesos/docker/v2_2.hpp PRE-CREATION > include/mesos/docker/v2_2.proto PRE-CREATION > include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 > src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 > src/slave/containerizer/mesos/containerizer.cpp > 98129d006cda9b65804b518619b6addc8990410a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 > src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 > src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/67931/diff/4/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67930: Get tests ready for Windows UCR development.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67930/#review206695 --- Ship it! I'll clean up for Mesos whitespace/formatting/commenting etc., but otherwise LGTM. - Andrew Schwartzmeyer On July 31, 2018, 2:42 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67930/ > --- > > (Updated July 31, 2018, 2:42 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Enabled Internet test environment on Windows. Disabled Internet > HealthCheckTests on Windows, since they require complete development. > Modified DockerFetcherPluginTest to fetch `microsoft/nanoserver` for > more extensive test for fetcher on Windows. > > > Diffs > - > > src/tests/environment.cpp 3b84c0a413193badbbdb6d3ee71d74f3ab85c90b > src/tests/health_check_tests.cpp 34102e2e3ff90c194bea83ae8a702181121d6522 > src/tests/uri_fetcher_tests.cpp 816d8ec2503ea79d069c062dfa2f01224b263bf6 > > > Diff: https://reviews.apache.org/r/67930/diff/4/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67984: Windows: Added CMake logic to download and "install" `wclayer.exe`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67984/#review206694 --- Ship it! I'll make sure this gets committed with original authorship (me); I thought RB would preserve it, but apparently not, I guess it takes it from the user submitting. Oh well. - Andrew Schwartzmeyer On July 31, 2018, 2:42 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67984/ > --- > > (Updated July 31, 2018, 2:42 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Windows: Added CMake logic to download and "install" `wclayer.exe`. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > src/slave/CMakeLists.txt e5fe32a265e310f568139127ee6e5f441590985c > > > Diff: https://reviews.apache.org/r/67984/diff/2/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206693 --- Ship it! I'll clean up for Mesos whitespace/formatting/commenting etc., but otherwise LGTM. - Andrew Schwartzmeyer On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 31, 2018, 2:41 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/4/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68111/#review206689 --- Ship it! Tested sponsor email, it works! - Andrew Schwartzmeyer On July 31, 2018, 9:34 a.m., Gastón Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68111/ > --- > > (Updated July 31, 2018, 9:34 a.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Added 'MesosCon 2018 CFP is now open!' blog post. > > > Diffs > - > > site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION > > > Diff: https://reviews.apache.org/r/68111/diff/4/ > > > Testing > --- > > Used `./mesos-website-dev.sh` to verify that the website is properly rendered. > > > Thanks, > > Gastón Kleiman > >
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206688 --- src/common/command_utils.cpp Line 186 (original), 186 (patched) <https://reviews.apache.org/r/67932/#comment289772> :) - Andrew Schwartzmeyer On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 31, 2018, 2:41 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/4/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
> On July 30, 2018, 4:39 p.m., Andrew Schwartzmeyer wrote: > > include/mesos/uri/fetcher.hpp > > Lines 101 (patched) > > <https://reviews.apache.org/r/67931/diff/3/?file=2061467#file2061467line101> > > > > I am not convinced we need to be passing this as a shared_ptr, wouldn't > > const-ref be just fine? Spoke offline; the reason this is the right choice is that this vector of strings is later used in several async lambdas. So by sharing a pointer to the vector instead, we utilize the ref counting of `shared_ptr` to avoid having to copy the vector for each lambda, and the size of the vector is ~10KB, so it's reasonable to want to avoid this. > On July 30, 2018, 4:39 p.m., Andrew Schwartzmeyer wrote: > > src/uri/fetchers/docker.cpp > > Lines 1016-1017 (patched) > > <https://reviews.apache.org/r/67931/diff/3/?file=2061476#file2061476line1020> > > > > Are we just skipping a failed blob here and trying to process the rest? > > Does that make sense to do? > > Liangyu Zhao wrote: > Yes, the code will try each url until one succeeds. If all fail, the > fetch will be considered failed. I'm not sure I understand why that works. Are these multiple URLs for the same blob? I would have that any single failed fetch would result in a total failure. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67931/#review206622 --- On July 30, 2018, 3:40 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67931/ > ------- > > (Updated July 30, 2018, 3:40 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Support parsing schema 2 and fetching blob with multiple URLs as > specified in schema 2. Update `curl` version to 7.61.0 because of bug > encountered in version 7.57.0. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 > include/mesos/docker/v2_2.hpp PRE-CREATION > include/mesos/docker/v2_2.proto PRE-CREATION > include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 > src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 > src/slave/containerizer/mesos/containerizer.cpp > 98129d006cda9b65804b518619b6addc8990410a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 > src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 > src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/67931/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
> On July 30, 2018, 5:30 p.m., Andrew Schwartzmeyer wrote: > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > > Line 381 (original), 388 (patched) > > <https://reviews.apache.org/r/67932/diff/3/?file=2061482#file2061482line388> > > > > Did we change behavior here? This is in a loop, so the original code > > was constantly adding to the front, whereas now we're adding the back (IMHO > > original code smells a bit funny). > > Liangyu Zhao wrote: > Nope, in line 469, I reverse the vector. It's just not cool to > continuously add stuff at front in vector. Ah, I see. Can you update the location of the comment above now? I agree, it's not cool to always prepend to a vector. > On July 30, 2018, 5:30 p.m., Andrew Schwartzmeyer wrote: > > src/slave/containerizer/mesos/provisioner/docker/store.cpp > > Lines 379-380 (patched) > > <https://reviews.apache.org/r/67932/diff/3/?file=2061483#file2061483line379> > > > > I take it that the `wclayer` command in `wclayer_remove` is supposed to > > remove the directory? > > Liangyu Zhao wrote: > Yeah, there is some privilege issues that `rmdir` does not work here. To > remove the directory, we have to use `wclayer`. That's fine; please add a comment though to explain the discrepancy with the Linux code. > On July 30, 2018, 5:30 p.m., Andrew Schwartzmeyer wrote: > > src/slave/containerizer/mesos/provisioner/docker/store.cpp > > Lines 490 (patched) > > <https://reviews.apache.org/r/67932/diff/3/?file=2061483#file2061483line490> > > > > Last I remember, `flags.docker_store_dir` was some Linux path; did we > > get that fixed (or am I misremembering)? > > Liangyu Zhao wrote: > I believe it has already been fixed. I have run the agent and log this. > It turns out to be a Windows path. Yup, looks like it: `path::join(os::temp(), "mesos", "store", "docker")`. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206625 --- On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 18, 2018, 2:27 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67916: Patched Google Test with upstream bugfix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67916/ --- (Updated July 31, 2018, 11:02 a.m.) Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff. Changes --- Used original upstream patch as GNU Patch is capable of applying it, though `git cherry-pick` (my original method) could not. Bugs: MESOS-8990 https://issues.apache.org/jira/browse/MESOS-8990 Repository: mesos Description (updated) --- Per MESOS-8990, our Google Test dependency needs a patch from upstream, https://github.com/google/googletest/pull/1620, in order to continue building with the next version of MSVC (and potentially other compilers). This patch file was generated with `git format-patch` for the commit `f66ab00704cd47e4e63ef6d425ca14b9192aaebb`. Additionally, we need to define `GTEST_LANG_CXX11=1` when including the GoogleTest headers such that the patch is used. Diffs (updated) - 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 3rdparty/googletest-release-1.8.0.patch PRE-CREATION Diff: https://reviews.apache.org/r/67916/diff/3/ Changes: https://reviews.apache.org/r/67916/diff/2-3/ Testing --- Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes: ``` # Set the default standard to C++11 for all targets. -set(CMAKE_CXX_STANDARD 11) -set(CMAKE_CXX_STANDARD_REQUIRED ON) +# set(CMAKE_CXX_STANDARD 11) +# set(CMAKE_CXX_STANDARD_REQUIRED ON) # Do not use, for example, `-std=gnu++11`. -set(CMAKE_CXX_EXTENSIONS OFF) +# set(CMAKE_CXX_EXTENSIONS OFF) +add_compile_options(/std:c++latest) +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING) +add_definitions(-D_HAS_AUTO_PTR_ETC=1) +add_definitions(-D_HAS_TR1_NAMESPACE=1) +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING) +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING) ``` After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch. Thanks, Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
> On July 31, 2018, 1:27 a.m., Benjamin Bannier wrote: > > LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using > > the original upstream patch would be great if possible. Ben, this did use the oiginal upsteam patch. I cloned the repo, checked out the tag we bundle, and cherry-picked the upstream patch from master, then created a patch file from the applied diff. The commit in master does not apply to 1.8.0 perfectly, so I can't just turn it into a patch, it had to be applied and conflicts fixed. I'm closing the issue again, because this is the original upstream patch, or as close as possible as we can get. If you have another way to do it, I'm all ears! - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67916/#review206647 --- On July 30, 2018, 11:50 a.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67916/ > --- > > (Updated July 30, 2018, 11:50 a.m.) > > > Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff. > > > Bugs: MESOS-8990 > https://issues.apache.org/jira/browse/MESOS-8990 > > > Repository: mesos > > > Description > --- > > Per MESOS-8990, our Google Test dependency needs a patch from > upstream, https://github.com/google/googletest/pull/1620, in order to > continue building with the next version of MSVC (and potentially other > compilers). > > This patch file was generated by cherry-picking `f66ab00` from > `master` onto `release-1.8.0` in the Google Test repo, and resolving > the merge conflict. > > Additionally, we need to define `GTEST_LANG_CXX11=1` when including > the GoogleTest headers such that the patch is used. > > > Diffs > - > > 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 > 3rdparty/googletest-release-1.8.0.patch PRE-CREATION > > > Diff: https://reviews.apache.org/r/67916/diff/2/ > > > Testing > --- > > Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following > (temporary) CMake changes: > > ``` > # Set the default standard to C++11 for all targets. > -set(CMAKE_CXX_STANDARD 11) > -set(CMAKE_CXX_STANDARD_REQUIRED ON) > +# set(CMAKE_CXX_STANDARD 11) > +# set(CMAKE_CXX_STANDARD_REQUIRED ON) > # Do not use, for example, `-std=gnu++11`. > -set(CMAKE_CXX_EXTENSIONS OFF) > +# set(CMAKE_CXX_EXTENSIONS OFF) > +add_compile_options(/std:c++latest) > +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING) > +add_definitions(-D_HAS_AUTO_PTR_ETC=1) > +add_definitions(-D_HAS_TR1_NAMESPACE=1) > +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING) > +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING) > ``` > > After adding the necessary compile interface definition, `ninja tests` > successfully built on Windows with this patch applied, after not building > before the patch. > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 68016: Added libseccomp to the build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68016/#review206629 --- cmake/CompilationConfigure.cmake Lines 538-541 (patched) <https://reviews.apache.org/r/68016/#comment289680> What targets need `ENABLE_SECCOMP_ISOLATOR` defined? Presumably this is new code, so we don't have to leave this as a to-do. (And I'm guessing just `libmesos PRIVATE`?) - Andrew Schwartzmeyer On July 23, 2018, 9:17 a.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68016/ > --- > > (Updated July 23, 2018, 9:17 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, James > Peach, and Qian Zhang. > > > Bugs: MESOS-9032 > https://issues.apache.org/jira/browse/MESOS-9032 > > > Repository: mesos > > > Description > --- > > This library is needed to implement Seccomp syscall filtering in the > Mesos containerizer. This patch introduces `seccomp-isolator` build > flag, which is used to include or exclude sources related to Seccomp > from the build. Since Seccomp is a Linux-specific feature, the flag > is disabled by default. Enabling `seccomp-isolator` means either: > > 1. Compiling and linking against the bundled version of libseccomp from >sources (default). > > 2. Linking against the libseccomp installed in the OS, >if `--with-libseccomp` build flag is provided. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > 3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 > cmake/CompilationConfigure.cmake f80c265d117c70acda95da6b943cdd3e4e477daa > configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 > > > Diff: https://reviews.apache.org/r/68016/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206625 --- src/common/command_utils.cpp Lines 186 (patched) <https://reviews.apache.org/r/67932/#comment289668> ? src/common/command_utils.cpp Lines 188-191 (patched) <https://reviews.apache.org/r/67932/#comment289669> Trying to think of a cleaner way to do this... src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Line 381 (original), 388 (patched) <https://reviews.apache.org/r/67932/#comment289670> Did we change behavior here? This is in a loop, so the original code was constantly adding to the front, whereas now we're adding the back (IMHO original code smells a bit funny). src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Line 416 (original), 433-455 (patched) <https://reviews.apache.org/r/67932/#comment289672> Goodness, but it looks like the best we can do here. src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Line 430 (original), 469 (patched) <https://reviews.apache.org/r/67932/#comment289675> Why do we have to do this now? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 379-380 (patched) <https://reviews.apache.org/r/67932/#comment289678> I take it that the `wclayer` command in `wclayer_remove` is supposed to remove the directory? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 490 (patched) <https://reviews.apache.org/r/67932/#comment289679> Last I remember, `flags.docker_store_dir` was some Linux path; did we get that fixed (or am I misremembering)? - Andrew Schwartzmeyer On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 18, 2018, 2:27 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67930: Get tests ready for Windows UCR development.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67930/#review206624 --- Fix it, then Ship it! src/tests/uri_fetcher_tests.cpp Lines 281-283 (patched) <https://reviews.apache.org/r/67930/#comment289667> nit: `static constexpr char DOCKER_IMAGE[]` (like above) - Andrew Schwartzmeyer On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67930/ > --- > > (Updated July 18, 2018, 2:27 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Enabled Internet test environment on Windows. Disabled Internet > HealthCheckTests on Windows, since they require complete development. > Modified DockerFetcherPluginTest to fetch `microsoft/nanoserver` for > more extensive test for fetcher on Windows. > > > Diffs > - > > src/tests/environment.cpp 3b84c0a413193badbbdb6d3ee71d74f3ab85c90b > src/tests/health_check_tests.cpp 34102e2e3ff90c194bea83ae8a702181121d6522 > src/tests/uri_fetcher_tests.cpp 816d8ec2503ea79d069c062dfa2f01224b263bf6 > > > Diff: https://reviews.apache.org/r/67930/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67931/#review206622 --- 3rdparty/CMakeLists.txt Lines 721-736 (original), 721-736 (patched) <https://reviews.apache.org/r/67931/#comment289655> Note for others: this was forced by an upstream build change in curl. include/mesos/docker/spec.hpp Lines 40-42 (patched) <https://reviews.apache.org/r/67931/#comment289656> +1 include/mesos/uri/fetcher.hpp Lines 101 (patched) <https://reviews.apache.org/r/67931/#comment289657> I am not convinced we need to be passing this as a shared_ptr, wouldn't const-ref be just fine? src/Makefile.am Lines 692-697 (original), 695-703 (patched) <https://reviews.apache.org/r/67931/#comment289658> Whitespace I'll fix up when committing... src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 467-505 (patched) <https://reviews.apache.org/r/67931/#comment289661> I believe you could accomplish what you're doing here with `goto fail/succeed` by refactoring this into a function (or even an in-place lambda) where the `goto`s would turn into `return`s. But I'm not sure if it'd be any cleaner. I have nothing against `goto` when used right, and this seems reasonable. src/uri/fetcher.hpp Line 46 (original), 43 (patched) <https://reviews.apache.org/r/67931/#comment289654> This can go. src/uri/fetchers/docker.cpp Lines 791-818 (patched) <https://reviews.apache.org/r/67931/#comment289665> Hey that worked out nicely. src/uri/fetchers/docker.cpp Lines 1016-1017 (patched) <https://reviews.apache.org/r/67931/#comment289666> Are we just skipping a failed blob here and trying to process the rest? Does that make sense to do? - Andrew Schwartzmeyer On July 30, 2018, 3:40 p.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67931/ > --- > > (Updated July 30, 2018, 3:40 p.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Support parsing schema 2 and fetching blob with multiple URLs as > specified in schema 2. Update `curl` version to 7.61.0 because of bug > encountered in version 7.57.0. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 > include/mesos/docker/v2_2.hpp PRE-CREATION > include/mesos/docker/v2_2.proto PRE-CREATION > include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 > src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 > src/slave/containerizer/mesos/containerizer.cpp > 98129d006cda9b65804b518619b6addc8990410a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 > src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 > src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/67931/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
omBuild target) -> > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > > > 172 Warning(s) > > 12 Error(s) > > > > Time Elapsed 00:05:08.45 > > ``` > > Andrew Schwartzmeyer wrote: > We will need to get this taken care of; which first requires pushing the > new cURL tarball to https://github.com/mesos/3rdparty/ This is in, so give this review an update and we'll see if the CI passes. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206119 --- On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 18, 2018, 2:27 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
> On July 18, 2018, 3:10 a.m., Mesos Reviewbot Windows wrote: > > Bad review! > > > > Error: > > Circular dependency detected for review 67931. Please fix the 'depends_on' > > field. Did this get fixed? - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206197 --- On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 18, 2018, 2:27 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
> On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote: > > include/mesos/docker/spec.hpp > > Lines 124-126 (patched) > > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line124> > > > > I think this and the commit before need to be re-ordered, as this > > commit is introducing logic required and used by the previous commit. I think you fixed this? > On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote: > > include/mesos/docker/spec.hpp > > Lines 141 (patched) > > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line141> > > > > Dumb question, but is there an S1/S2 for V1 as well? > > Liangyu Zhao wrote: > I don't think so. There is, but it's not in use, so I'll drop this. > On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote: > > include/mesos/docker/spec.hpp > > Lines 141-164 (patched) > > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line141> > > > > I'm not sure, but would this be better as a variant? > > > > ``` > > std::unique_ptr > v2_2::ImageManifest>> manifest; > > ``` > > > > (Declared at point of use.) > > > > P.S. Why do we need pointers? > > Liangyu Zhao wrote: > I haven't used the `std::Variant` before. I think it is a better choice. > > The reason why I used pointers is trying to avoid copying the object. > This is just a minor optimization of efficiency. Avoid pointers whenever possible; the minor optimization isn't worth the potential bug cost. Plus with move semantics the optimization probably won't even matter. > On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote: > > src/uri/fetcher.cpp > > Lines 88 (patched) > > <https://reviews.apache.org/r/67931/diff/1/?file=2059712#file2059712line90> > > > > Why does this function throw away `urls`? > > Liangyu Zhao wrote: > This is just a default parent class function. Currently, only > `DockerFetcherPulgin` supports multiple URLs fetch, so it overrides this > function. As for other plugins, this is the default way of handling extra > URLs. Commented at `include\mesos\uri\fetcher.hpp` line 89. Gotcha. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67931/#review206157 --- On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote: > > ------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67931/ > --- > > (Updated July 18, 2018, 2:27 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Support parsing schema 2 and fetching blob with multiple URLs as > specified in schema 2. Update `curl` version to 7.61.0 because of bug > encountered in version 7.57.0. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 > include/mesos/docker/v2_2.hpp PRE-CREATION > include/mesos/docker/v2_2.proto PRE-CREATION > include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 > src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 > src/slave/containerizer/mesos/containerizer.cpp > 98129d006cda9b65804b518619b6addc8990410a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 > src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 > src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/67931/diff/3/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67916: Patched Google Test with upstream bugfix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67916/ --- (Updated July 30, 2018, 11:50 a.m.) Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff. Changes --- Fixed build by adding compile interface definition. Bugs: MESOS-8990 https://issues.apache.org/jira/browse/MESOS-8990 Repository: mesos Description (updated) --- Per MESOS-8990, our Google Test dependency needs a patch from upstream, https://github.com/google/googletest/pull/1620, in order to continue building with the next version of MSVC (and potentially other compilers). This patch file was generated by cherry-picking `f66ab00` from `master` onto `release-1.8.0` in the Google Test repo, and resolving the merge conflict. Additionally, we need to define `GTEST_LANG_CXX11=1` when including the GoogleTest headers such that the patch is used. Diffs (updated) - 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4 3rdparty/googletest-release-1.8.0.patch PRE-CREATION Diff: https://reviews.apache.org/r/67916/diff/2/ Changes: https://reviews.apache.org/r/67916/diff/1-2/ Testing (updated) --- Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes: ``` # Set the default standard to C++11 for all targets. -set(CMAKE_CXX_STANDARD 11) -set(CMAKE_CXX_STANDARD_REQUIRED ON) +# set(CMAKE_CXX_STANDARD 11) +# set(CMAKE_CXX_STANDARD_REQUIRED ON) # Do not use, for example, `-std=gnu++11`. -set(CMAKE_CXX_EXTENSIONS OFF) +# set(CMAKE_CXX_EXTENSIONS OFF) +add_compile_options(/std:c++latest) +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING) +add_definitions(-D_HAS_AUTO_PTR_ETC=1) +add_definitions(-D_HAS_TR1_NAMESPACE=1) +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING) +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING) ``` After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch. Thanks, Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67916/ --- (Updated July 30, 2018, 11:02 a.m.) Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff. Changes --- Does not fix MESOS-8990 (yet). Bugs: MESOS-8990 https://issues.apache.org/jira/browse/MESOS-8990 Repository: mesos Description --- Per MESOS-8990, our Google Test dependency needs a patch from upstream, https://github.com/google/googletest/pull/1620, in order to continue building with the next version of MSVC (and potentially other compilers). This patch file was generated by cherry-picking `f66ab00` from `master` onto `release-1.8.0` in the Google Test repo, and resolving the merge conflict. Diffs - 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 3rdparty/googletest-release-1.8.0.patch PRE-CREATION Diff: https://reviews.apache.org/r/67916/diff/1/ Testing (updated) --- Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes: ``` # Set the default standard to C++11 for all targets. -set(CMAKE_CXX_STANDARD 11) -set(CMAKE_CXX_STANDARD_REQUIRED ON) +# set(CMAKE_CXX_STANDARD 11) +# set(CMAKE_CXX_STANDARD_REQUIRED ON) # Do not use, for example, `-std=gnu++11`. -set(CMAKE_CXX_EXTENSIONS OFF) +# set(CMAKE_CXX_EXTENSIONS OFF) +add_compile_options(/std:c++latest) +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING) +add_definitions(-D_HAS_AUTO_PTR_ETC=1) +add_definitions(-D_HAS_TR1_NAMESPACE=1) +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING) +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING) ``` Unfortunately, it still repro'ed with this patch applied too, so I need to figure out what we missed. Thanks, Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
> On July 23, 2018, 12:35 a.m., Benjamin Bannier wrote: > > > It is not yet known if the Autotools system will also need this patch. Do > > > we want it added there anyway? > > > > I'd vote for enabling this on all platforms to simplify the build setup. Ah, but that doesn't simplify it, it complicates it :P (I've never added a patch to Autotools). > On July 23, 2018, 12:35 a.m., Benjamin Bannier wrote: > > 3rdparty/googletest-release-1.8.0.patch > > Lines 1 (patched) > > <https://reviews.apache.org/r/67916/diff/1/?file=2059402#file2059402line1> > > > > This patch has landed upstream so let's just use the patch from > > upstream `f66ab00704cd47e4e63ef6d425ca14b9192aaebb` instead of referencing > > a PR. If we do that we should also update our commit message. > > > > `f66ab00704c` applied cleanly with some fuzziness for me, so if > > possible just use the actual upstream patch. I believe that I did cherry-pick the merged upstream patch. This diff was generated from the cherry-picked commit, which is why the hash is not `f66ab00704cd47e4e63ef6d425ca14b9192aaebb`. > On July 23, 2018, 12:35 a.m., Benjamin Bannier wrote: > > 3rdparty/googletest-release-1.8.0.patch > > Lines 1-10 (patched) > > <https://reviews.apache.org/r/67916/diff/1/?file=2059402#file2059402line1> > > > > Thanks for including the git metadata with the patch! This is much > > easier to maintain than bare diffs we have in many other patches. The commit hash isn't useful since it was a cherry-pick, but the message I thought was nice to have :) I think I just generated with `git format-patch`... how else do people generate patches? - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67916/#review206332 --- On July 13, 2018, 2:04 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67916/ > --- > > (Updated July 13, 2018, 2:04 p.m.) > > > Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff. > > > Bugs: MESOS-8990 > https://issues.apache.org/jira/browse/MESOS-8990 > > > Repository: mesos > > > Description > --- > > Per MESOS-8990, our Google Test dependency needs a patch from > upstream, https://github.com/google/googletest/pull/1620, in order to > continue building with the next version of MSVC (and potentially other > compilers). > > This patch file was generated by cherry-picking `f66ab00` from > `master` onto `release-1.8.0` in the Google Test repo, and resolving > the merge conflict. > > > Diffs > - > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/googletest-release-1.8.0.patch PRE-CREATION > > > Diff: https://reviews.apache.org/r/67916/diff/1/ > > > Testing > --- > > Clean build on Windows using CMake (which is the only place this patch > currently applies). It is not yet known if the Autotools system will also > need this patch. Do we want it added there anyway? > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 67751: Add missing files to CMake build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67751/ --- (Updated July 27, 2018, 3:26 p.m.) Review request for mesos, Benjamin Bannier, Benjamin Mahler, James Peach, and Joseph Wu. Changes --- Fix to not break on Windows. Bugs: MESOS-8994 https://issues.apache.org/jira/browse/MESOS-8994 Repository: mesos Description (updated) --- This patch adds stubs for the Python executor and scheduler, adds support for `mesos-resolve`, adds conditional support for the old `mesos-cli` when `ENABLE_NEW_CLI` is not set and not on Windows, and fixes some ordering oddities. Diffs (updated) - src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 src/Makefile.am 71f9052f60fc65d8183faa7ab9764d3e6388ddc9 src/cli/CMakeLists.txt 7b2abf2fe14888ec1da11414189f71da972ac427 src/python/executor/CMakeLists.txt PRE-CREATION src/python/scheduler/CMakeLists.txt PRE-CREATION src/tests/CMakeLists.txt 695b6f543847d4270f7004a1768919904975d7a4 Diff: https://reviews.apache.org/r/67751/diff/3/ Changes: https://reviews.apache.org/r/67751/diff/2-3/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68094: Add port mapping and network ports isolators sources to CMake.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68094/ --- Review request for mesos, Benjamin Bannier, Benjamin Mahler, James Peach, and Joseph Wu. Bugs: MESOS-8994 and MESOS-9118 https://issues.apache.org/jira/browse/MESOS-8994 https://issues.apache.org/jira/browse/MESOS-9118 Repository: mesos Description --- This adds the relevant options, but with a fatal message if they're used, as we do not yet check for their dependency. This also adds the `mesos-network-helper` executable as used by the port mapping isolator. Diffs - cmake/CompilationConfigure.cmake f80c265d117c70acda95da6b943cdd3e4e477daa src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 src/slave/containerizer/mesos/CMakeLists.txt ba1f92fe7dd59c34c6dee0bc7ecf6f1b5160eee8 src/tests/CMakeLists.txt 695b6f543847d4270f7004a1768919904975d7a4 Diff: https://reviews.apache.org/r/68094/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 68093: Add XFS disk isolator files to CMake.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68093/ --- Review request for mesos, Benjamin Bannier, Benjamin Mahler, James Peach, and Joseph Wu. Bugs: MESOS-8994 and MESOS-9117 https://issues.apache.org/jira/browse/MESOS-8994 https://issues.apache.org/jira/browse/MESOS-9117 Repository: mesos Description --- This adds an option to turn on the isolator, but that results in a fatal error because we do not yet check for the required headers and libraries. Diffs - cmake/CompilationConfigure.cmake f80c265d117c70acda95da6b943cdd3e4e477daa src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 src/tests/CMakeLists.txt 695b6f543847d4270f7004a1768919904975d7a4 Diff: https://reviews.apache.org/r/68093/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 67751: Add missing files to CMake build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67751/ --- (Updated July 27, 2018, 12:10 p.m.) Review request for mesos, Benjamin Bannier, Benjamin Mahler, James Peach, and Joseph Wu. Changes --- Split out unrelated changes and addressed review. Summary (updated) - Add missing files to CMake build. Bugs: MESOS-8994 https://issues.apache.org/jira/browse/MESOS-8994 Repository: mesos Description (updated) --- This patch adds stubs for the Python executor and scheduler, adds support for `mesos-resolve`, adds conditional support for the old `mesos-cli` when `ENABLE_NEW_CLI` is not set, and fixes some ordering oddities. Diffs (updated) - src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 src/Makefile.am 71f9052f60fc65d8183faa7ab9764d3e6388ddc9 src/cli/CMakeLists.txt 7b2abf2fe14888ec1da11414189f71da972ac427 src/python/executor/CMakeLists.txt PRE-CREATION src/python/scheduler/CMakeLists.txt PRE-CREATION src/tests/CMakeLists.txt 695b6f543847d4270f7004a1768919904975d7a4 Diff: https://reviews.apache.org/r/67751/diff/2/ Changes: https://reviews.apache.org/r/67751/diff/1-2/ Testing (updated) --- Thanks, Andrew Schwartzmeyer
Review Request 67983: Enabled `TimeTest.Now` on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67983/ --- Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Repository: mesos Description --- While the timer resolution on Windows does not support 10 microseconds, it does support 1000 microseconds, which is still fast enough for a unit test. Diffs - 3rdparty/libprocess/src/tests/time_tests.cpp 08ddb56f1789f400b8cd072c53e885c759f13ddc Diff: https://reviews.apache.org/r/67983/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67980/
---
(Updated July 19, 2018, 1:47 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu,
Liangyu Zhao, and Radhika Jandhyala.
Changes
---
Added bug.
Bugs: MESOS-5817 and MESOS-7527
https://issues.apache.org/jira/browse/MESOS-5817
https://issues.apache.org/jira/browse/MESOS-7527
Repository: mesos
Description
---
Several of these worked as-is, and the two firewall tests were fixed
by replace `path::join` (which joined with ``) with
`strings::join("/")`.
Diffs
-
3rdparty/libprocess/src/tests/process_tests.cpp
8baf60d8bbb675e26fea5e76c825ef73fedbc629
Diff: https://reviews.apache.org/r/67980/diff/1/
Testing
---
`ctest -V` on Windows
Thanks,
Andrew Schwartzmeyer
Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67980/
---
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu,
Liangyu Zhao, and Radhika Jandhyala.
Bugs: MESOS-5817
https://issues.apache.org/jira/browse/MESOS-5817
Repository: mesos
Description
---
Several of these worked as-is, and the two firewall tests were fixed
by replace `path::join` (which joined with ``) with
`strings::join("/")`.
Diffs
-
3rdparty/libprocess/src/tests/process_tests.cpp
8baf60d8bbb675e26fea5e76c825ef73fedbc629
Diff: https://reviews.apache.org/r/67980/diff/1/
Testing
---
`ctest -V` on Windows
Thanks,
Andrew Schwartzmeyer
Review Request 67979: Windows: Documented why the `RemoteLinkLeak` test is not enabled.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67979/ --- Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Bugs: MESOS-9093 https://issues.apache.org/jira/browse/MESOS-9093 Repository: mesos Description --- Windows: Documented why the `RemoteLinkLeak` test is not enabled. Diffs - 3rdparty/libprocess/src/tests/process_tests.cpp 8baf60d8bbb675e26fea5e76c825ef73fedbc629 Diff: https://reviews.apache.org/r/67979/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 67978: Windows: Enabled `RemoteLink` tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67978/ --- Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Bugs: MESOS-5941 https://issues.apache.org/jira/browse/MESOS-5941 Repository: mesos Description --- This resolves MESOS-5941. Note that on Windows, `os::killtree()` is only valid for processes specifically spawned in a group via a job object, otherwise it will fail (and the failure is not being checked here). Since the `linkee` process only ever spawns the single OS process `test-linkee`, it is safe to switch to `os::kill()` which can correctly kill it. Diffs - 3rdparty/libprocess/src/tests/process_tests.cpp 8baf60d8bbb675e26fea5e76c825ef73fedbc629 Diff: https://reviews.apache.org/r/67978/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 67977: Fixed `test-linkee` logic in `ProcessRemoteLinkTest::SetUp()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67977/ --- Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Bugs: MESOS-5941 and MESOS-9097 https://issues.apache.org/jira/browse/MESOS-5941 https://issues.apache.org/jira/browse/MESOS-9097 Repository: mesos Description --- Several problems existed on Windows here: * The `BUILD_DIR` path has forward slashes (probably fine, but wrong). * The executable must be named correctly, `.exe` and all. * We should assert that `test-linkee` exists. * The shell should not be used, otherwise `'(62)'` will resolve to an unknown UPID inside `test-linkee` because of the single quotes. Diffs - 3rdparty/libprocess/src/tests/process_tests.cpp 8baf60d8bbb675e26fea5e76c825ef73fedbc629 Diff: https://reviews.apache.org/r/67977/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Review Request 67976: Windows: Added `nullptr` checks when using `libwinio_loop` pointer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67976/ --- Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Bugs: MESOS-9097 https://issues.apache.org/jira/browse/MESOS-9097 Repository: mesos Description --- It was discovered that the `Socket` constructor could dereference a null pointer (by way of `prepare_async()`) if the Windows IOCP event loop had not yet been initialized. So now we check for its initialization before each dereference, and return an error or fatal log event. In order to ensure that it is initialized in `test-linkee`, we call `process::initialize()`. This should be fixed in the future, per MESOS-9097. Diffs - 3rdparty/libprocess/src/tests/test_linkee.cpp cc482717290f72a5fd95fe745ac01893c0ce41f8 3rdparty/libprocess/src/windows/event_loop.cpp 0050ff0d87fdd01bf37742233fcd38b02f284ff3 3rdparty/libprocess/src/windows/io.cpp 1f9adde36192b673d7051549295a0f403be8e718 Diff: https://reviews.apache.org/r/67976/diff/1/ Testing --- Thanks, Andrew Schwartzmeyer
Re: Review Request 67951: Added optional `path_separator` parameter to `Path` constructor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67951/ --- (Updated July 19, 2018, 1:41 p.m.) Review request for mesos, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Changes --- Removed const member so that it can be default copy constructed. Bugs: MESOS-5817 and MESOS-5904 https://issues.apache.org/jira/browse/MESOS-5817 https://issues.apache.org/jira/browse/MESOS-5904 Repository: mesos Description --- This defaults to `os::PATH_SEPARATOR` and so by default retains the previous behavior. However, now `Path` can be arbitrarily used with, e.g., URLs on Windows by providing `/` as the separator. Diffs (updated) - 3rdparty/stout/include/stout/path.hpp 27438d31617b3b78bf3d4deffd25c93340610e8d Diff: https://reviews.apache.org/r/67951/diff/3/ Changes: https://reviews.apache.org/r/67951/diff/2-3/ Testing --- `stout-tests` and `libprocess-tests` passed on Windows and Ubuntu Thanks, Andrew Schwartzmeyer
Re: Review Request 67952: Windows: Ported remaining tests in the `HTTPTest` suite.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67952/ --- (Updated July 17, 2018, 5:52 p.m.) Review request for mesos, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Changes --- Manually add dependency because apparently the script didn't. Bugs: MESOS-5904 and MESOS-8564 https://issues.apache.org/jira/browse/MESOS-5904 https://issues.apache.org/jira/browse/MESOS-8564 Repository: mesos Description --- These were enabled by fixing the use of `Path()` in `process.cpp` to explicitly use '/' as the separator, as it's being used to manipulate URLs, not filesystem paths, and previously this failed due the Windows path separator being '\'. Diffs - 3rdparty/libprocess/src/process.cpp 7c0a0bc0c1e50354b6da219032ac830cbeec0a0d 3rdparty/libprocess/src/tests/http_tests.cpp acbd6973829411652fc5d57ef473c0d8ba9cd5b4 Diff: https://reviews.apache.org/r/67952/diff/1/ Testing --- `stout-tests` and `libprocess-tests` passed on Windows and Ubuntu Thanks, Andrew Schwartzmeyer
Review Request 67951: Added optional `path_separator` parameter to `Path` constructor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67951/ --- Review request for mesos, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Bugs: MESOS-5904 and MESOS-8564 https://issues.apache.org/jira/browse/MESOS-5904 https://issues.apache.org/jira/browse/MESOS-8564 Repository: mesos Description --- This defaults to `os::PATH_SEPARATOR` and so by default retains the previous behavior. However, now `Path` can be arbitrarily used with, e.g., URLs on Windows by providing `/` as the separator. Diffs - 3rdparty/stout/include/stout/path.hpp 27438d31617b3b78bf3d4deffd25c93340610e8d Diff: https://reviews.apache.org/r/67951/diff/1/ Testing --- `stout-tests` and `libprocess-tests` passed on Windows and Ubuntu Thanks, Andrew Schwartzmeyer
Review Request 67952: Windows: Ported remaining tests in the `HTTPTest` suite.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67952/ --- Review request for mesos, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, and Radhika Jandhyala. Bugs: MESOS-5904 and MESOS-8564 https://issues.apache.org/jira/browse/MESOS-5904 https://issues.apache.org/jira/browse/MESOS-8564 Repository: mesos Description --- These were enabled by fixing the use of `Path()` in `process.cpp` to explicitly use '/' as the separator, as it's being used to manipulate URLs, not filesystem paths, and previously this failed due the Windows path separator being '\'. Diffs - 3rdparty/libprocess/src/process.cpp 7c0a0bc0c1e50354b6da219032ac830cbeec0a0d 3rdparty/libprocess/src/tests/http_tests.cpp acbd6973829411652fc5d57ef473c0d8ba9cd5b4 Diff: https://reviews.apache.org/r/67952/diff/1/ Testing --- `stout-tests` and `libprocess-tests` passed on Windows and Ubuntu Thanks, Andrew Schwartzmeyer
Re: Review Request 67932: Use `wclayer` from `hcsshim` to extract file layers.
omBuild target) -> > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : error : downloading > > 'https://github.com/mesos/3rdparty/raw/master/curl-7.60.0.tar.gz' failed > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > CUSTOMBUILD : The requested URL returned error : 404 Not Found > > [D:\DCOS\mesos\3rdparty\curl-7.60.0.vcxproj] > > > > 172 Warning(s) > > 12 Error(s) > > > > Time Elapsed 00:05:08.45 > > ``` We will need to get this taken care of; which first requires pushing the new cURL tarball to https://github.com/mesos/3rdparty/ - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67932/#review206119 --- On July 16, 2018, 11:35 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67932/ > --- > > (Updated July 16, 2018, 11:35 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The `tar` command cannot work successfully on Windows, so use `wclayer` > instead. Note that the folder generated from extraction also cannot be > deleted by `rmdir`, so the GC is also changed to use `wclayer remove`. > > > Diffs > - > > src/common/command_utils.hpp 525f9c1c37b74f7e4cc71fdc8d52944226998ddc > src/common/command_utils.cpp 7dfcc9ff74bcf044d47b803ebc42cf63fba89d17 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 85aad25ac8ecfda125be85fb46d882c3982f3930 > > > Diff: https://reviews.apache.org/r/67932/diff/1/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67931/#review206157 --- There is still more to review... I'll get to it. include/mesos/docker/spec.hpp Lines 124-126 (patched) <https://reviews.apache.org/r/67931/#comment289057> I think this and the commit before need to be re-ordered, as this commit is introducing logic required and used by the previous commit. include/mesos/docker/spec.hpp Lines 141 (patched) <https://reviews.apache.org/r/67931/#comment289058> Dumb question, but is there an S1/S2 for V1 as well? include/mesos/docker/spec.hpp Lines 141-164 (patched) <https://reviews.apache.org/r/67931/#comment289060> I'm not sure, but would this be better as a variant? ``` std::unique_ptr> manifest; ``` (Declared at point of use.) P.S. Why do we need pointers? include/mesos/uri/fetcher.hpp Lines 84-96 (patched) <https://reviews.apache.org/r/67931/#comment289059> Maybe add to this comment (and the one below) that this is used by the V2_S2 schema. src/docker/spec.cpp Lines 390-426 (patched) <https://reviews.apache.org/r/67931/#comment289061> I think you'd get all of this for "free" out of a variant. src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 467 (patched) <https://reviews.apache.org/r/67931/#comment289062> Why do we need a `shared_ptr` here? src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 473 (patched) <https://reviews.apache.org/r/67931/#comment289064> Heyyy what is this... src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 490 (patched) <https://reviews.apache.org/r/67931/#comment289063> Why are we making this on the heap and using a pointer? src/uri/fetcher.cpp Line 56 (original), 56 (patched) <https://reviews.apache.org/r/67931/#comment289065> We can delete this now! Also, add `MESOS-8570` to the "bug" field of this review, and let's get you a JIRA account so I can assign it to you. src/uri/fetcher.cpp Lines 88 (patched) <https://reviews.apache.org/r/67931/#comment289066> Why does this function throw away `urls`? - Andrew Schwartzmeyer On July 16, 2018, 11:35 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67931/ > ------- > > (Updated July 16, 2018, 11:35 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Support parsing schema 2 and fetching blob with multiple URLs as > specified in schema 2. Update `curl` version to 7.60.0 because of bug > encountered in version 7.57.0. > > > Diffs > - > > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 > include/mesos/docker/v2_2.hpp PRE-CREATION > include/mesos/docker/v2_2.proto PRE-CREATION > include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 > src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 > src/slave/containerizer/mesos/containerizer.cpp > 98129d006cda9b65804b518619b6addc8990410a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 > src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 > src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/67931/diff/1/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Re: Review Request 67488: Updated CLI to Python 3.
> On July 16, 2018, 1:32 p.m., Andrew Schwartzmeyer wrote: > > Did you have an automated way of finding where we should apply `override` > > in existing code? I'd like to see if there's any Windows-only code/tests we > > should fix too. > > Armand Grillet wrote: > What do you mean by `override`? I have applied `2to3` on the CLI codebase > to find what needed to be updated. I haven't run `mesos-cli-tests` on a > Windows machine, that should tell us if we need to fix something else for > that operating system. Wrong review... I think I was copy-pasting and had a couple tabs open. Sorry! - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67488/#review206125 --- On July 6, 2018, 6:56 a.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67488/ > --- > > (Updated July 6, 2018, 6:56 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, > and Kevin Klues. > > > Bugs: MESOS-8955 > https://issues.apache.org/jira/browse/MESOS-8955 > > > Repository: mesos > > > Description > --- > > The build tools are also up to date thus the CLI can still be built > using Autotools and CMake. No features have been added to the CLI. > > > Diffs > - > > cmake/CompilationConfigure.cmake 61387d77b12a17571a31430db3ca1fe0bbb66a21 > configure.ac 66cc28a5a34949bcadc038551249f3781ea9d45b > src/Makefile.am db42e71d90ff2066e104f4b9c269c5e78a9a6ada > src/python/cli_new/CMakeLists.txt ef8da70757e2721f4ac1bee46d0b5d95e81298ca > src/python/cli_new/README.md 3d646e91a8c7c72d4ee1b1180454e5f587295053 > src/python/cli_new/bin/main.py 53130383d8ca2ed40c97224b3a6e98aa6b6b107c > src/python/cli_new/bootstrap fb6fbc449a970ccf960914ed910204f3984ea61f > src/python/cli_new/lib/cli/config.py > 6f92622725d8a042a2a728fd38c977ac690ef6be > src/python/cli_new/lib/cli/docopt.py > 86a4e9c74326fb80cc59487113f07358dd96960d > src/python/cli_new/lib/cli/http.py 03d6031cb3273575f41d4d06d9a409f74488a16b > src/python/cli_new/lib/cli/plugins/agent/main.py > 59280ece8ebd00bb96df3675b6356a26cc48a2c0 > src/python/cli_new/lib/cli/plugins/task/main.py > cc6cff56c71262729a8870017bef2e97636abe5a > src/python/cli_new/lib/cli/tests/base.py > 89360e6cac5ca910044a5a82fab7237510edee7f > src/python/cli_new/lib/cli/tests/tests.py > 79e1036f6d11c63884091fe43672607b03955c1a > src/python/cli_new/lib/cli/util.py 307b22293a9c7199ad7088dfd0db6dff83a08ac8 > src/python/cli_new/tests/main.py acf2e0868555da0eb1c1cee7fb30b1e80783f1e1 > src/python/cli_new/tox.ini 58ca3807e3d6096296b4cd09a5cec32b32444d91 > src/python/lib/tox.ini 05b633e837fa39a36fb2c5a0778513ce743099db > support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43 > support/python3/mesos-style.py 350ef909e3e7a1c927140cf4475547d704ac2ad5 > > > Diff: https://reviews.apache.org/r/67488/diff/12/ > > > Testing > --- > > Testing done on Fedora 25 with `python` being Python 2.7, `python3` being > Python 3.5 and `python36` being Python 3.6. > > > For Autotools: > > ``` > $ ./bootstrap > $ mkdir build > $ cd build > $ PYTHON_3=python36 ../configure --enable-new-cli --disable-java > $ make check > ``` > > For CMake: > > ``` > $ ./bootstrap > $ mkdir build > $ cd build > $ cmake .. -DENABLE_NEW_CLI=1 -DPYTHON=python36 > $ cmake --build . -- -j16 > $ ./src/mesos > Mesos CLI > > Usage: > mesos (-h | --help) > mesos --version > mesos [...] > > Options: > -h --help Show this screen. > --version Show version info. > > Commands: > agent Interacts with the Mesos agents > config Interacts with the Mesos CLI configuration file > taskInteracts with the tasks running in a Mesos cluster > > See 'mesos help ' for more information on a specific command. > $ cmake --build . --target tests -- -j16 > $ ctest -R CLI > Test project /home/agrillet/apache-mesos/build > Start 4: CLITests > 1/1 Test #4: CLITests . Passed3.63 sec > > 100% tests passed, 0 tests failed out of 1 > ``` > > Checked that the the CLI tests were run, that the content of the directory > build/src/cli was as expected, and that build/src/mesos was correctly running. > > > Thanks, > > Armand Grillet > >
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67931/#review206134 --- Ah, sorry, I will get the rest of this reviewed tomorrow. 3rdparty/cmake/Versions.cmake Lines 7-8 (original), 7-8 (patched) <https://reviews.apache.org/r/67931/#comment289034> It looks like cURL has since posted 7.61.0, so as long as we're updating this, let's update to 7.61.0, and get it added to https://github.com/mesos/3rdparty/ so we can get the ReviewBot to pass. (We spoke offine so I know you're already doing this, just posting it for posterity.) src/CMakeLists.txt Lines 170 (patched) <https://reviews.apache.org/r/67931/#comment289038> I'm not certain we can just add it to `AGENT_SRC` as that'll also add it for POSIX systems. Instead, like how it's added to `LINUX_SRC`, let's also add it to `WIN32_SRC`. I would suggest, take this existing snippet in this file (which shouldn't exist given the existence of `WIN32_SRC`: ``` if (WIN32) list(APPEND AGENT_SRC slave/containerizer/mesos/isolators/windows/cpu.cpp slave/containerizer/mesos/isolators/windows/mem.cpp) else () ``` and instead make: ``` set(WIN32)SRC slave/containerizer/mesos/isolators/docker/runtime.cpp slave/containerizer/mesos/isolators/windows/cpu.cpp slave/containerizer/mesos/isolators/windows/mem.cpp ... ) ``` src/CMakeLists.txt Lines 424-427 (original), 426-427 (patched) <https://reviews.apache.org/r/67931/#comment289039> With this being true, we can just move `uri/fetchers/docker.cpp` to the point where we're setting `URI_SRC` to begin with. src/uri/fetchers/docker.cpp Lines 102-105 (patched) <https://reviews.apache.org/r/67931/#comment289041> I think there's a `strings::replace` to do this. - Andrew Schwartzmeyer On July 16, 2018, 11:35 a.m., Liangyu Zhao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67931/ > --- > > (Updated July 16, 2018, 11:35 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Support parsing schema 2 and fetching blob with multiple URLs as > specified in schema 2. Update `curl` version to 7.60.0 because of bug > encountered in version 7.57.0. > > > Diffs > - > > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 > include/mesos/docker/v2_2.hpp PRE-CREATION > include/mesos/docker/v2_2.proto PRE-CREATION > include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b > src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 > src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07 > src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 > src/slave/containerizer/mesos/containerizer.cpp > 98129d006cda9b65804b518619b6addc8990410a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 > src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 > src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5 > src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 > > > Diff: https://reviews.apache.org/r/67931/diff/1/ > > > Testing > --- > > > Thanks, > > Liangyu Zhao > >
Review Request 67933: Replaced `` with `` in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67933/ --- Review request for mesos, Akash Gupta, John Kordich, Joseph Wu, and Radhika Jandhyala. Repository: mesos Description --- The intent is that `os.hpp` includes the correct OS-specific header, `windows/os.hpp` or `posix/os.hpp`, automatically. They should never be included manually. Diffs - 3rdparty/stout/include/stout/os/windows/kill.hpp bd944357bd741d62aea78c839d87b80853e4a852 3rdparty/stout/include/stout/os/windows/rm.hpp 1c66e3e6fa20116866842378077e10052c147289 3rdparty/stout/include/stout/windows/net.hpp d70f967773d23672f53dd7dd6123434f5e81eeb0 Diff: https://reviews.apache.org/r/67933/diff/1/ Testing --- Old patch I found on my computer that I figured I should post. `ninja tests ; ctest -V` on Windows. Thanks, Andrew Schwartzmeyer
Re: Review Request 67869: Add use of `override` to the Mesos C++ style guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67869/#review206133 --- Ship it! As mentioned in my reply to Ben, I am _for_ this change, as I believe that if we care about it, we need to call it out in addition to its presence in the Google Style Guide (that thing is enormous!). - Andrew Schwartzmeyer On July 11, 2018, 10:03 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67869/ > --- > > (Updated July 11, 2018, 10:03 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, > Mesos Reviewbot, Till Toenshoff, and Zhitao Li. > > > Bugs: MESOS-9065 > https://issues.apache.org/jira/browse/MESOS-9065 > > > Repository: mesos > > > Description > --- > > Add an explicit guideline to the Mesos C++ style guide that the use of > `override` keyword is required. Update cpplint and clang-tidy tooling > to encourage this. > > > Diffs > - > > docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea > support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d > support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43 > > > Diff: https://reviews.apache.org/r/67869/diff/2/ > > > Testing > --- > > Manual. The `cpplint` change doesn't appear to enforce the use of `override`. > > > Thanks, > > James Peach > >
Re: Review Request 67868: Apply the `override` keyword to Mesos.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67868/#review206131 --- Ship it! Ship It! - Andrew Schwartzmeyer On July 11, 2018, 10:03 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67868/ > --- > > (Updated July 11, 2018, 10:03 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, > Mesos Reviewbot, Till Toenshoff, and Zhitao Li. > > > Bugs: MESOS-9065 > https://issues.apache.org/jira/browse/MESOS-9065 > > > Repository: mesos > > > Description > --- > > Apply the `override` keyword to Mesos. > > > Diffs > - > > include/mesos/executor.hpp 6a9e6fc6b1ab94be2ade9a96de4fb11364b46a13 > include/mesos/scheduler.hpp d4cc96e86b27cfe2e3a3de448fd0f5e06b1564df > include/mesos/state/in_memory.hpp 3fcf4620b7252c632ca9416d06a5bd1b9ed5a35a > include/mesos/state/leveldb.hpp 265e08b58ea4a50458587f884a98e24cdc5d1aa2 > include/mesos/state/log.hpp 54cbdc8d67ef9dfc47b321d5837e09dd56920c94 > include/mesos/state/protobuf.hpp c15696ab79b61f5487ee4a849d62b34b91ca1614 > include/mesos/state/zookeeper.hpp fee93989c74db031ff2343e06eb25c5a7b88510f > include/mesos/v1/executor.hpp 9a2eb454b0d5d5dce7af03e3100dfd7ae078b5a1 > include/mesos/v1/scheduler.hpp b1dfbb1454b550c8a2cd018b1ca9e3533b9171f7 > include/mesos/zookeeper/group.hpp 7983e50ba9d240f19a2b3ae02d7fa8339e77f2fa > include/mesos/zookeeper/watcher.hpp > b2961a54eae74c5810df5d085a39652991685930 > src/authentication/cram_md5/authenticatee.hpp > d77b2108c83951f0495b67f6494fb7410592e848 > src/authentication/cram_md5/authenticatee.cpp > d43dedf1ec86d3f25811718101ff273a7c0d7333 > src/authentication/cram_md5/authenticator.hpp > a68b2cf43fb5e45bdc2073a9031b72b6f7a1c1bc > src/authentication/cram_md5/authenticator.cpp > 0aad3aa9a6a2c70a6e98fc75a3443ea340d0404d > src/authorizer/local/authorizer.hpp > 794443ae9e0ec66845bf611d45d6a037ec96ddf4 > src/authorizer/local/authorizer.cpp > 056b1723f5ec34271f1afe3ad810bc498db8f28e > src/checks/checker_process.hpp 6364af8f0bfbdf7f6596dc0bf32049806bd2ba83 > src/cli/execute.cpp 89e5130784e6f18850a743c4e0219245f7658ff3 > src/common/http.hpp 76e8f71b5687031344612e1742d3bb50040db758 > src/common/recordio.hpp 88e789c7f65c7ac2a97473d656ab15b3803cc886 > src/docker/executor.cpp d9f5a736c1cb28a55db1b4fdc793bc706c7cb1fe > src/examples/balloon_executor.cpp 5281db1718f3db462d40d65dfa9ce0e3d9adcc34 > src/examples/balloon_framework.cpp cbc25e45258b05a645363d4229c0a701baa5481c > src/examples/disk_full_framework.cpp > dfbdddae7f50d314e67db59300e5140b0476f807 > src/examples/docker_no_executor_framework.cpp > 0cbda07872fcf7db091c17ef49f5bcea6d80e660 > src/examples/dynamic_reservation_framework.cpp > e8d8d09dbda95f77e4b7dbbc8e031cd962e3a45f > src/examples/example_module_impl.cpp > b84ff73ba96999072a03461f46717c2f104ae9ff > src/examples/load_generator_framework.cpp > 3ae25b221185ec6bd4bd84c478be9a511d763162 > src/examples/long_lived_executor.cpp > 21b511d1d6d0793569ebdb2ecc5c9bc37f3de632 > src/examples/long_lived_framework.cpp > 0e4e3d00f86ce810ecc273baa4c7d5d124a1a29d > src/examples/no_executor_framework.cpp > dc75d7802e43164293c5d68ae78eb7fba6deba64 > src/examples/persistent_volume_framework.cpp > 43375503f1bdb74fba7994f73beb5b5486026e73 > src/examples/test_anonymous_module.cpp > c41b7fc6d5feb7760603df1cda8cc0697a815324 > src/examples/test_csi_plugin.cpp 9c4da8811cc260bcf3bccfea3036a7964cb75697 > src/examples/test_csi_user_framework.cpp > 91212e990af711633f7b890b0c9e10587f9efe7c > src/examples/test_executor.cpp d9b7cdd7dbaf61bc54d7490ab9e7f188ea45f571 > src/examples/test_framework.cpp aaf952c20cd3d722d00504228c297c053079bf50 > src/examples/test_hook_module.cpp 9b9d75c2bb81bf80e8edb919a04e0854e858f49d > src/examples/test_http_executor.cpp > fb9209f630c9c0fa6c1e866eeaf178c560372edc > src/examples/test_http_framework.cpp > 9ea3c6f35bf6464b4c6cfa7ba2eaf4febc168396 > src/exec/exec.cpp ca4f065af18934a59161ad0ee63daba59beeb115 > src/executor/executor.cpp ab67caefd82f46eacd33221270b2c408ef70cd17 > src/executor/v0_v1executor.hpp ef9bae6ab75279814a5ba9081aa663107195d8e6 > src/executor/v0_v1executor.cpp 086cfc74c820657cb3dd7d531f2de44a97832782 > src/files/files.cpp 4b8713a2783ec4c4205af071b6cfff3397ec4c1d > src/java/jni/org_
Re: Review Request 67867: Apply the `override` keyword to libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67867/#review206129 --- Ship it! Ship It! - Andrew Schwartzmeyer On July 11, 2018, 10:09 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67867/ > --- > > (Updated July 11, 2018, 10:09 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, > Mesos Reviewbot, Till Toenshoff, and Zhitao Li. > > > Bugs: MESOS-9065 > https://issues.apache.org/jira/browse/MESOS-9065 > > > Repository: mesos > > > Description > --- > > Apply the `override` keyword to libprocess. > > > Diffs > - > > 3rdparty/libprocess/README.md 9846ecea45f742da74dd39d1701b5177d2f3f125 > 3rdparty/libprocess/examples/example.cpp > 0db2d03bccb7b4910acfd7e08248f022bc74b6ef > 3rdparty/libprocess/include/process/async.hpp > 116c90c01655cd1df67616d9c927f6769f5c92da > 3rdparty/libprocess/include/process/collect.hpp > 5263f0134808a0161b16b3b45871f711b0f5cb45 > 3rdparty/libprocess/include/process/event.hpp > ec64eb786b6cf30bf03ba07a30c6e44d2db9173a > 3rdparty/libprocess/include/process/filter.hpp > 3f4a8272c451e333ebf9ff79c5f0fee329a8450a > 3rdparty/libprocess/include/process/firewall.hpp > 0a7b985380893820d0193f7b2eb657ee8c07430e > 3rdparty/libprocess/include/process/gmock.hpp > b5ead1c2ecacdb468a319c98edd2ffc831a9f74c > 3rdparty/libprocess/include/process/grpc.hpp > 0ff8184791277ed02b30481b352be60f10743428 > 3rdparty/libprocess/include/process/gtest.hpp > 79ea2b1f6a2a16ff9c5aae281c900e50940877bf > 3rdparty/libprocess/include/process/help.hpp > 3393b71c484bb8c55c6a4cd2a27dcb798e841a80 > 3rdparty/libprocess/include/process/limiter.hpp > b4b6a7bb48aa19fa40c0ea9f8759b8f553f43ca6 > 3rdparty/libprocess/include/process/logging.hpp > aad7ce8caa193e6aff1a295fef899b49fcfce7b0 > 3rdparty/libprocess/include/process/metrics/counter.hpp > 15aeeb5710636d4e11b862faee50fd6ea4d1cb07 > 3rdparty/libprocess/include/process/metrics/metrics.hpp > f9b72029b2c85826c91b1d7656b0af94dc87010c > 3rdparty/libprocess/include/process/metrics/pull_gauge.hpp > 5c2a227425f31c439bd48b58171d4038a7f04e5f > 3rdparty/libprocess/include/process/metrics/push_gauge.hpp > 5c39846172fc46c3022e9aa420460f5b0fb6db2e > 3rdparty/libprocess/include/process/metrics/timer.hpp > 0a9c0227c457c6c81a59f65f901a5464ee00983d > 3rdparty/libprocess/include/process/process.hpp > c36df991b6a2c120ab0562e8ff907f9fbf8630d1 > 3rdparty/libprocess/include/process/profiler.hpp > 74890ae7b10c15b77ef1d112a7ebfb3a45b29ed2 > 3rdparty/libprocess/include/process/protobuf.hpp > 5a75a83d294e2d14f81503b113724893c0e140f3 > 3rdparty/libprocess/include/process/reap.hpp > 628bc0a6ec59bfe6bfa7c54906ea266ac31a71a7 > 3rdparty/libprocess/include/process/run.hpp > 0d282c989b8ddeb9ddb31a050cd52afe01623a66 > 3rdparty/libprocess/include/process/sequence.hpp > 24712b16953347eb05a28ad50fbdfad17bf64f19 > 3rdparty/libprocess/include/process/system.hpp > 81ded8af1015fe3ecd578ed98759b66f917ee264 > 3rdparty/libprocess/src/encoder.hpp > 70b5ec479e90c0eb6ac729b465739b581729a956 > 3rdparty/libprocess/src/http.cpp dc38716960da98b861ee69ae310068e2db1245bb > 3rdparty/libprocess/src/http_proxy.hpp > 5b6e7e8786ed9eab50cd4c2cfdec455c92d72eca > 3rdparty/libprocess/src/memory_profiler.hpp > 617f8de27949f2e536e53c122d7269f2a919814b > 3rdparty/libprocess/src/poll_socket.hpp > eebd7182e433adabf6bb20c41d6ecf1863adf992 > 3rdparty/libprocess/src/process.cpp > b3bd0286e09dcb9d0867679c6a509c34e309a72e > 3rdparty/libprocess/src/tests/benchmarks.cpp > 604122a2f33d7abfab92631d9eac33b6753e846b > 3rdparty/libprocess/src/tests/http_tests.cpp > 5429034e9f4a1a6a4c4dc53a9f7e01dca119ea24 > 3rdparty/libprocess/src/tests/metrics_tests.cpp > a5b41ed3f4491244656222681f443e7ed55393c2 > 3rdparty/libprocess/src/tests/process_tests.cpp > 2f8a8234aa1cb6128a85254efc81868c19e6fdac > 3rdparty/libprocess/src/tests/profiler_tests.cpp > 995bd02f6ecce484cd9b2aca355c2707d73d40b2 > > > Diff: https://reviews.apache.org/r/67867/diff/3/ > > > Testing > --- > > make check (Fedora 28) > > The compliation database was generated using the method in > `support/mesos-tidy/entrypoint.sh`. I also used `-DENABLE_GRPC=YES`, > `-DENABLE_JEMALLOC_ALLOCATOR` and `-DENABLE_JAVA`. I wasn't able to get the > SSL support to build on F28, and not sure how the Python support works in > cmake. > > > Thanks, > > James Peach > >
Re: Review Request 67869: Add use of `override` to the Mesos C++ style guide.
> On July 16, 2018, 1:31 p.m., Benjamin Bannier wrote: > > docs/c++-style-guide.md > > Lines 651-662 (patched) > > <https://reviews.apache.org/r/67869/diff/2/?file=2058877#file2058877line651> > > > > Like I already mentioned on the mailing list, I am still not a fan of > > adding redundant information which is already in the Google style guide > > here. That way we'd ask ideal contributors to read the Google style guide, > > our style guide, and figure out where any the delta is. IMHO it cannot be reasonably expected that a dev is going to read the entire Google Style Guide. I refer to it plenty often, but if we think this part is _important_, then I think that it is on us to call it out by adding it here. Sure, the _ideal_ contributor thinks about every possible syntactical rule or formatting rule and tries to find the answer as they write it (I do this, but I'm crazy, and still get it wrong a lot), but realistically I think the most we can expect is that this guide is read, and hopefully it sinks in as "gotchas" that we care most about. - Andrew --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67869/#review206124 --- On July 11, 2018, 10:03 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67869/ > --- > > (Updated July 11, 2018, 10:03 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, > Mesos Reviewbot, Till Toenshoff, and Zhitao Li. > > > Bugs: MESOS-9065 > https://issues.apache.org/jira/browse/MESOS-9065 > > > Repository: mesos > > > Description > --- > > Add an explicit guideline to the Mesos C++ style guide that the use of > `override` keyword is required. Update cpplint and clang-tidy tooling > to encourage this. > > > Diffs > - > > docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea > support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d > support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43 > > > Diff: https://reviews.apache.org/r/67869/diff/2/ > > > Testing > --- > > Manual. The `cpplint` change doesn't appear to enforce the use of `override`. > > > Thanks, > > James Peach > >
Re: Review Request 67488: Updated CLI to Python 3.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67488/#review206125 --- Ship it! Did you have an automated way of finding where we should apply `override` in existing code? I'd like to see if there's any Windows-only code/tests we should fix too. - Andrew Schwartzmeyer On July 6, 2018, 6:56 a.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67488/ > --- > > (Updated July 6, 2018, 6:56 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, > and Kevin Klues. > > > Bugs: MESOS-8955 > https://issues.apache.org/jira/browse/MESOS-8955 > > > Repository: mesos > > > Description > --- > > The build tools are also up to date thus the CLI can still be built > using Autotools and CMake. No features have been added to the CLI. > > > Diffs > - > > cmake/CompilationConfigure.cmake 61387d77b12a17571a31430db3ca1fe0bbb66a21 > configure.ac 66cc28a5a34949bcadc038551249f3781ea9d45b > src/Makefile.am db42e71d90ff2066e104f4b9c269c5e78a9a6ada > src/python/cli_new/CMakeLists.txt ef8da70757e2721f4ac1bee46d0b5d95e81298ca > src/python/cli_new/README.md 3d646e91a8c7c72d4ee1b1180454e5f587295053 > src/python/cli_new/bin/main.py 53130383d8ca2ed40c97224b3a6e98aa6b6b107c > src/python/cli_new/bootstrap fb6fbc449a970ccf960914ed910204f3984ea61f > src/python/cli_new/lib/cli/config.py > 6f92622725d8a042a2a728fd38c977ac690ef6be > src/python/cli_new/lib/cli/docopt.py > 86a4e9c74326fb80cc59487113f07358dd96960d > src/python/cli_new/lib/cli/http.py 03d6031cb3273575f41d4d06d9a409f74488a16b > src/python/cli_new/lib/cli/plugins/agent/main.py > 59280ece8ebd00bb96df3675b6356a26cc48a2c0 > src/python/cli_new/lib/cli/plugins/task/main.py > cc6cff56c71262729a8870017bef2e97636abe5a > src/python/cli_new/lib/cli/tests/base.py > 89360e6cac5ca910044a5a82fab7237510edee7f > src/python/cli_new/lib/cli/tests/tests.py > 79e1036f6d11c63884091fe43672607b03955c1a > src/python/cli_new/lib/cli/util.py 307b22293a9c7199ad7088dfd0db6dff83a08ac8 > src/python/cli_new/tests/main.py acf2e0868555da0eb1c1cee7fb30b1e80783f1e1 > src/python/cli_new/tox.ini 58ca3807e3d6096296b4cd09a5cec32b32444d91 > src/python/lib/tox.ini 05b633e837fa39a36fb2c5a0778513ce743099db > support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43 > support/python3/mesos-style.py 350ef909e3e7a1c927140cf4475547d704ac2ad5 > > > Diff: https://reviews.apache.org/r/67488/diff/12/ > > > Testing > --- > > Testing done on Fedora 25 with `python` being Python 2.7, `python3` being > Python 3.5 and `python36` being Python 3.6. > > > For Autotools: > > ``` > $ ./bootstrap > $ mkdir build > $ cd build > $ PYTHON_3=python36 ../configure --enable-new-cli --disable-java > $ make check > ``` > > For CMake: > > ``` > $ ./bootstrap > $ mkdir build > $ cd build > $ cmake .. -DENABLE_NEW_CLI=1 -DPYTHON=python36 > $ cmake --build . -- -j16 > $ ./src/mesos > Mesos CLI > > Usage: > mesos (-h | --help) > mesos --version > mesos [...] > > Options: > -h --help Show this screen. > --version Show version info. > > Commands: > agent Interacts with the Mesos agents > config Interacts with the Mesos CLI configuration file > taskInteracts with the tasks running in a Mesos cluster > > See 'mesos help ' for more information on a specific command. > $ cmake --build . --target tests -- -j16 > $ ctest -R CLI > Test project /home/agrillet/apache-mesos/build > Start 4: CLITests > 1/1 Test #4: CLITests . Passed3.63 sec > > 100% tests passed, 0 tests failed out of 1 > ``` > > Checked that the the CLI tests were run, that the content of the directory > build/src/cli was as expected, and that build/src/mesos was correctly running. > > > Thanks, > > Armand Grillet > >
Re: Review Request 67922: Removed unused `TestStore` mock.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67922/#review206123 --- Ship it! Ship It! - Andrew Schwartzmeyer On July 15, 2018, 5:36 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67922/ > --- > > (Updated July 15, 2018, 5:36 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Gilbert Song, and Jie Yu. > > > Repository: mesos > > > Description > --- > > Removed unused `TestStore` mock. > > > Diffs > - > > src/Makefile.am 9c13eaaa8bd62f10539ebbb6ab16c7303dabfcc1 > src/tests/containerizer/store.hpp 0589be698d3bf835293fbe7ea817a11a7bb91f67 > > > Diff: https://reviews.apache.org/r/67922/diff/1/ > > > Testing > --- > > make check (Fedora 28) > > `TestStore` usage was removed in > [r/52539](https://reviews.apache.org/r/52539). > > > Thanks, > > James Peach > >
Review Request 67916: Patched Google Test with upstream bugfix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67916/ --- Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff. Bugs: MESOS-8990 https://issues.apache.org/jira/browse/MESOS-8990 Repository: mesos Description --- Per MESOS-8990, our Google Test dependency needs a patch from upstream, https://github.com/google/googletest/pull/1620, in order to continue building with the next version of MSVC (and potentially other compilers). This patch file was generated by cherry-picking `f66ab00` from `master` onto `release-1.8.0` in the Google Test repo, and resolving the merge conflict. Diffs - 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 3rdparty/googletest-release-1.8.0.patch PRE-CREATION Diff: https://reviews.apache.org/r/67916/diff/1/ Testing --- Clean build on Windows using CMake (which is the only place this patch currently applies). It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway? Thanks, Andrew Schwartzmeyer
