You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/05/28 23:11:48 UTC
Review Request 70740: Windows: Removed multiple categories of sources
from the build.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
Repository: mesos
Description
-------
This removes some categories of sources from the Windows build,
where it is possible to do so with minimal ifdef-ing.
The features removed are all Linux-specific features that cannot be
feasibly ported to Windows, including:
* Container Storage Interface (CSI)
* Docker image provisioning, specifically related to V2
* Open Container Interface
* Volume GID Manager
Protobufs are excluded where possible, but many of the above categories
of protobufs are interleaved with other protobufs or source code,
which makes exclusion non-trivial. For example, CSI V0 protobufs
cannot be excluded without a large change; or libseccomp is a Linux-only
feature, but its protobufs are now required to build the Mesos
containerizer's protobufs.
Docker image provisioning was semi-trivial to exclude, because the
related components (provisioner & URI fetcher) are somewhat modularized.
Diffs
-----
src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
Diff: https://reviews.apache.org/r/70740/diff/1/
Testing
-------
cmake --build . --target check
This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
Thanks,
Joseph Wu
Re: Review Request 70740: Windows: Removed multiple categories of
sources from the build.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/#review215562
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70740]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 28, 2019, 11:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70740/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 11:11 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This removes some categories of sources from the Windows build,
> where it is possible to do so with minimal ifdef-ing.
>
> The features removed are all Linux-specific features that cannot be
> feasibly ported to Windows, including:
> * Container Storage Interface (CSI)
> * Docker image provisioning, specifically related to V2
> * Open Container Interface
> * Volume GID Manager
>
> Protobufs are excluded where possible, but many of the above categories
> of protobufs are interleaved with other protobufs or source code,
> which makes exclusion non-trivial. For example, CSI V0 protobufs
> cannot be excluded without a large change; or libseccomp is a Linux-only
> feature, but its protobufs are now required to build the Mesos
> containerizer's protobufs.
>
> Docker image provisioning was semi-trivial to exclude, because the
> related components (provisioner & URI fetcher) are somewhat modularized.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
> src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
> src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
> src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
> src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
>
>
> Diff: https://reviews.apache.org/r/70740/diff/1/
>
>
> Testing
> -------
>
> cmake --build . --target check
>
> This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70740: Windows: Removed multiple categories of
sources from the build.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/#review215951
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On May 28, 2019, 4:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70740/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 4:11 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This removes some categories of sources from the Windows build,
> where it is possible to do so with minimal ifdef-ing.
>
> The features removed are all Linux-specific features that cannot be
> feasibly ported to Windows, including:
> * Container Storage Interface (CSI)
> * Docker image provisioning, specifically related to V2
> * Open Container Interface
> * Volume GID Manager
>
> Protobufs are excluded where possible, but many of the above categories
> of protobufs are interleaved with other protobufs or source code,
> which makes exclusion non-trivial. For example, CSI V0 protobufs
> cannot be excluded without a large change; or libseccomp is a Linux-only
> feature, but its protobufs are now required to build the Mesos
> containerizer's protobufs.
>
> Docker image provisioning was semi-trivial to exclude, because the
> related components (provisioner & URI fetcher) are somewhat modularized.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
> src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
> src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
> src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
> src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
>
>
> Diff: https://reviews.apache.org/r/70740/diff/1/
>
>
> Testing
> -------
>
> cmake --build . --target check
>
> This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70740: Windows: Removed multiple categories of
sources from the build.
Posted by Gilbert Song <so...@gmail.com>.
> On June 18, 2019, 4:09 p.m., Gilbert Song wrote:
> > src/uri/fetcher.hpp
> > Lines 43-44 (original), 46-49 (patched)
> > <https://reviews.apache.org/r/70740/diff/1/?file=2146973#file2146973line47>
> >
> > Probably we still want to keep the docker fetche plugin for windows. Otherwise, it is a partial revert for https://issues.apache.org/jira/browse/MESOS-9159
> >
> > Mesos on Windows does not support using a docker image yet, fetching an image is supported. We have three unit tests to verify the work:
> > ```
> > DISABLED_INTERNET_CURL_FetchManifest
> > DISABLED_INTERNET_CURL_FetchImage
> > DISABLED_INTERNET_CURL_InvokeFetchByName
> > ```
> >
> > They were disabled recently due to flakiness. Is the change for URI fetcher just for short term?
We can revisit this later.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/#review215949
-----------------------------------------------------------
On May 28, 2019, 4:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70740/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 4:11 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This removes some categories of sources from the Windows build,
> where it is possible to do so with minimal ifdef-ing.
>
> The features removed are all Linux-specific features that cannot be
> feasibly ported to Windows, including:
> * Container Storage Interface (CSI)
> * Docker image provisioning, specifically related to V2
> * Open Container Interface
> * Volume GID Manager
>
> Protobufs are excluded where possible, but many of the above categories
> of protobufs are interleaved with other protobufs or source code,
> which makes exclusion non-trivial. For example, CSI V0 protobufs
> cannot be excluded without a large change; or libseccomp is a Linux-only
> feature, but its protobufs are now required to build the Mesos
> containerizer's protobufs.
>
> Docker image provisioning was semi-trivial to exclude, because the
> related components (provisioner & URI fetcher) are somewhat modularized.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
> src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
> src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
> src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
> src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
>
>
> Diff: https://reviews.apache.org/r/70740/diff/1/
>
>
> Testing
> -------
>
> cmake --build . --target check
>
> This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70740: Windows: Removed multiple categories of
sources from the build.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/#review215949
-----------------------------------------------------------
src/uri/fetcher.hpp
Lines 43-44 (original), 46-49 (patched)
<https://reviews.apache.org/r/70740/#comment302880>
Probably we still want to keep the docker fetche plugin for windows. Otherwise, it is a partial revert for https://issues.apache.org/jira/browse/MESOS-9159
Mesos on Windows does not support using a docker image yet, fetching an image is supported. We have three unit tests to verify the work:
```
DISABLED_INTERNET_CURL_FetchManifest
DISABLED_INTERNET_CURL_FetchImage
DISABLED_INTERNET_CURL_InvokeFetchByName
```
They were disabled recently due to flakiness. Is the change for URI fetcher just for short term?
- Gilbert Song
On May 28, 2019, 4:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70740/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 4:11 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This removes some categories of sources from the Windows build,
> where it is possible to do so with minimal ifdef-ing.
>
> The features removed are all Linux-specific features that cannot be
> feasibly ported to Windows, including:
> * Container Storage Interface (CSI)
> * Docker image provisioning, specifically related to V2
> * Open Container Interface
> * Volume GID Manager
>
> Protobufs are excluded where possible, but many of the above categories
> of protobufs are interleaved with other protobufs or source code,
> which makes exclusion non-trivial. For example, CSI V0 protobufs
> cannot be excluded without a large change; or libseccomp is a Linux-only
> feature, but its protobufs are now required to build the Mesos
> containerizer's protobufs.
>
> Docker image provisioning was semi-trivial to exclude, because the
> related components (provisioner & URI fetcher) are somewhat modularized.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
> src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
> src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
> src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
> src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
>
>
> Diff: https://reviews.apache.org/r/70740/diff/1/
>
>
> Testing
> -------
>
> cmake --build . --target check
>
> This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70740: Windows: Removed multiple categories of
sources from the build.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On May 28, 2019, 4:48 p.m., Chun-Hung Hsiao wrote:
> > src/CMakeLists.txt
> > Lines 32 (patched)
> > <https://reviews.apache.org/r/70740/diff/1/?file=2146968#file2146968line32>
> >
> > Why are you guarding just the CSI v1 but not v0? They're different from the Mesos v0 vs v1 API protos. They are different APIs.
> >
> > Let's just disable CSI on Windows and guard both for now.
I can try removing this again, to see what exactly fails to link.
> On May 28, 2019, 4:48 p.m., Chun-Hung Hsiao wrote:
> > src/CMakeLists.txt
> > Lines 61 (patched)
> > <https://reviews.apache.org/r/70740/diff/1/?file=2146968#file2146968line67>
> >
> > IIRC Windows uses some v2s2 spec? https://github.com/apache/mesos/blob/fa410f2fb8efb988590f4da2d4cfffbb2ce70637/src/uri/fetchers/docker.cpp#L974
Yeah, I managed to #ifdef the Docker URI fetcher out of the build. That is part of the review (a little bit below).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/#review215558
-----------------------------------------------------------
On May 28, 2019, 4:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70740/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 4:11 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This removes some categories of sources from the Windows build,
> where it is possible to do so with minimal ifdef-ing.
>
> The features removed are all Linux-specific features that cannot be
> feasibly ported to Windows, including:
> * Container Storage Interface (CSI)
> * Docker image provisioning, specifically related to V2
> * Open Container Interface
> * Volume GID Manager
>
> Protobufs are excluded where possible, but many of the above categories
> of protobufs are interleaved with other protobufs or source code,
> which makes exclusion non-trivial. For example, CSI V0 protobufs
> cannot be excluded without a large change; or libseccomp is a Linux-only
> feature, but its protobufs are now required to build the Mesos
> containerizer's protobufs.
>
> Docker image provisioning was semi-trivial to exclude, because the
> related components (provisioner & URI fetcher) are somewhat modularized.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
> src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
> src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
> src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
> src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
>
>
> Diff: https://reviews.apache.org/r/70740/diff/1/
>
>
> Testing
> -------
>
> cmake --build . --target check
>
> This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70740: Windows: Removed multiple categories of
sources from the build.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70740/#review215558
-----------------------------------------------------------
src/CMakeLists.txt
Lines 32 (patched)
<https://reviews.apache.org/r/70740/#comment302287>
Why are you guarding just the CSI v1 but not v0? They're different from the Mesos v0 vs v1 API protos. They are different APIs.
Let's just disable CSI on Windows and guard both for now.
src/CMakeLists.txt
Lines 61 (patched)
<https://reviews.apache.org/r/70740/#comment302288>
IIRC Windows uses some v2s2 spec? https://github.com/apache/mesos/blob/fa410f2fb8efb988590f4da2d4cfffbb2ce70637/src/uri/fetchers/docker.cpp#L974
- Chun-Hung Hsiao
On May 28, 2019, 11:11 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70740/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 11:11 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This removes some categories of sources from the Windows build,
> where it is possible to do so with minimal ifdef-ing.
>
> The features removed are all Linux-specific features that cannot be
> feasibly ported to Windows, including:
> * Container Storage Interface (CSI)
> * Docker image provisioning, specifically related to V2
> * Open Container Interface
> * Volume GID Manager
>
> Protobufs are excluded where possible, but many of the above categories
> of protobufs are interleaved with other protobufs or source code,
> which makes exclusion non-trivial. For example, CSI V0 protobufs
> cannot be excluded without a large change; or libseccomp is a Linux-only
> feature, but its protobufs are now required to build the Mesos
> containerizer's protobufs.
>
> Docker image provisioning was semi-trivial to exclude, because the
> related components (provisioner & URI fetcher) are somewhat modularized.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4
> src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b
> src/slave/containerizer/mesos/provisioner/store.cpp 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/uri_fetcher_tests.cpp c727cc52e82a396fe187a00c8cc3c9e78a919c5d
> src/uri/fetcher.hpp cc4bd93b3b8bcb7803f8f912f4ad9d3cf45a58a9
> src/uri/fetcher.cpp 8db43eb9763f1cf8040db93a1f03aae0fe9ab3c7
>
>
> Diff: https://reviews.apache.org/r/70740/diff/1/
>
>
> Testing
> -------
>
> cmake --build . --target check
>
> This slightly decreases the memory footprint of the build, and allowed my build instance (4GB mem) to proceed beyond some agent files (which is where the Windows CI is also running out of memory). It still ran out of memory when compiling tests however. After giving the instance more memory (8GB), the build succeeds.
>
>
> Thanks,
>
> Joseph Wu
>
>