Review Request 71121: Added a test `ROOT_NonePrivateIPCModeWithShmSize`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71121/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added a test `ROOT_NonePrivateIPCModeWithShmSize`. Diffs - src/tests/containerizer/isolator_tests.cpp 14feaed50d4e71e2dfaba21880cd67f53584366f Diff: https://reviews.apache.org/r/71121/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 70887: Added `PendingFutureTracker` class for tracking pending futures.
> On July 16, 2019, 3:11 p.m., Gilbert Song wrote: > > src/common/future_tracker.hpp > > Lines 101-105 (patched) > > <https://reviews.apache.org/r/70887/diff/4/?file=2154856#file2154856line101> > > > > probably we want to add some comments to explain these parameters. > > > > Other than `name`, another para confuses me a bit is `component`. > > > > In next patch, we have `containerizer` as component and > > `isolator::method` as name. How do we define component and name for the > > following case: > > ``` > > mesos_containerizer->provisioner->docker_store->local_puller->pull()``` Do we really need both `name` and `component`? Can we just keep one? I think the `name` like `linux/seccomp::prepare` already give us enough info, why do we need `containerizer` as `component` here? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70887/#review216641 --- On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70887/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Benjamin Mahler, Gilbert Song, James Peach, Meng > Zhu, and Qian Zhang. > > > Bugs: MESOS-9837 > https://issues.apache.org/jira/browse/MESOS-9837 > > > Repository: mesos > > > Description > --- > > This patch introduces a mechanism for tracking pending futures. > This feature allows detection of hanging operations, which get > stuck on a blocking operation or asynchronously. However, this > feature does not provide any mechanism for tracking pending > promises, because `Promise` objects might not be accessible in > various cases. Thereby, we introduce a new class that can be > used to track pending futures, so it might facilitate debugging > of stuck issues. > > > Diffs > - > > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/common/future_tracker.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70887/diff/4/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70889: Wrapped isolators in `IsolatorTracker`.
> On July 16, 2019, 3:29 p.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/containerizer.cpp > > Lines 550-552 (patched) > > <https://reviews.apache.org/r/70889/diff/2/?file=2154668#file2154668line550> > > > > seems like we are going to have one more libprocess process for each > > isolator/module. we may have some concerns on perf. > > > > would you mind to collect some perf data for w/o isolatorTracker and > > compare? > > > > A quick method to test it out is to use mesos-executor and launch 100 > > containers in one single task group. I think the performance overhead should be acceptable. Yes, we add a libprocess `PendingFutureTrackerProcess`, but for each isolator's method, we just need to dispatch a call (i.e., `PendingFutureTrackerProcess::addFuture`) to that libprocess but no need to wait for that libprocess to complete that call, so the performance overhead are just some functions calls but no extra thread switches. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70889/#review216643 --- On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70889/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Gilbert Song and Qian Zhang. > > > Bugs: MESOS-9841 > https://issues.apache.org/jira/browse/MESOS-9841 > > > Repository: mesos > > > Description > --- > > This patch wraps every isolator in instance of `IsolatorTracker` class. > If an isolator gets stuck in some operation, `pendingFutures` will > return the isolator name, method name along with its arguments such as > `containerId`, `pid`, etc. > > > Diffs > - > > src/slave/containerizer/containerizer.hpp > d33c65cd3fae9bb4d347681cb00dbdb25ecf57b8 > src/slave/containerizer/containerizer.cpp > 5ce0d9c4bfa707495ea644c90fa6e35eb0fe25cd > src/slave/containerizer/mesos/containerizer.hpp > 558e412b28a84ae0ca1bf05b596540072bb216d6 > src/slave/containerizer/mesos/containerizer.cpp > c9a369bcdbfc1676912430c3e84fa567bbd7f766 > src/slave/main.cpp ef5ea02f169427d9913b807664bf3073d72cd9b6 > src/tests/cluster.hpp c04ee14beafe350568a295c8258566aa3704028a > src/tests/cluster.cpp b43f806eab096b7d4e86e2b5d4b81d6baecb0ee5 > > > Diff: https://reviews.apache.org/r/70889/diff/3/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70889: Wrapped isolators in `IsolatorTracker`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70889/#review216647 --- src/slave/main.cpp Lines 495 (patched) <https://reviews.apache.org/r/70889/#comment303849> Kill this newline. - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70889/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Gilbert Song and Qian Zhang. > > > Bugs: MESOS-9841 > https://issues.apache.org/jira/browse/MESOS-9841 > > > Repository: mesos > > > Description > --- > > This patch wraps every isolator in instance of `IsolatorTracker` class. > If an isolator gets stuck in some operation, `pendingFutures` will > return the isolator name, method name along with its arguments such as > `containerId`, `pid`, etc. > > > Diffs > - > > src/slave/containerizer/containerizer.hpp > d33c65cd3fae9bb4d347681cb00dbdb25ecf57b8 > src/slave/containerizer/containerizer.cpp > 5ce0d9c4bfa707495ea644c90fa6e35eb0fe25cd > src/slave/containerizer/mesos/containerizer.hpp > 558e412b28a84ae0ca1bf05b596540072bb216d6 > src/slave/containerizer/mesos/containerizer.cpp > c9a369bcdbfc1676912430c3e84fa567bbd7f766 > src/slave/main.cpp ef5ea02f169427d9913b807664bf3073d72cd9b6 > src/tests/cluster.hpp c04ee14beafe350568a295c8258566aa3704028a > src/tests/cluster.cpp b43f806eab096b7d4e86e2b5d4b81d6baecb0ee5 > > > Diff: https://reviews.apache.org/r/70889/diff/3/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70887: Added `PendingFutureTracker` class for tracking pending futures.
> On July 16, 2019, 3:03 p.m., Gilbert Song wrote: > > src/common/future_tracker.hpp > > Lines 103 (patched) > > <https://reviews.apache.org/r/70887/diff/4/?file=2154856#file2154856line103> > > > > is there a variable name better than `name`? so that easier for dev to > > understand this parameter. > > > > function or method? Maybe `operation`? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70887/#review216640 --- On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70887/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Benjamin Mahler, Gilbert Song, James Peach, Meng > Zhu, and Qian Zhang. > > > Bugs: MESOS-9837 > https://issues.apache.org/jira/browse/MESOS-9837 > > > Repository: mesos > > > Description > --- > > This patch introduces a mechanism for tracking pending futures. > This feature allows detection of hanging operations, which get > stuck on a blocking operation or asynchronously. However, this > feature does not provide any mechanism for tracking pending > promises, because `Promise` objects might not be accessible in > various cases. Thereby, we introduce a new class that can be > used to track pending futures, so it might facilitate debugging > of stuck issues. > > > Diffs > - > > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/common/future_tracker.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70887/diff/4/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70887: Added `PendingFutureTracker` class for tracking pending futures.
> On July 15, 2019, 4:35 p.m., Qian Zhang wrote: > > src/common/future_track.hpp > > Lines 90 (patched) > > <https://reviews.apache.org/r/70887/diff/3/?file=2154659#file2154659line90> > > > > I'd suggest to use `Owned` instead of raw pointer. > > Andrei Budnik wrote: > I'd prefer to leave `Try` as it's more consistent with similar factory > methods `static Try<...> Class::create(...)`. I mean replacing `PendingFutureTracker*` with `Owned`. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70887/#review216603 --- On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70887/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Benjamin Mahler, Gilbert Song, James Peach, Meng > Zhu, and Qian Zhang. > > > Bugs: MESOS-9837 > https://issues.apache.org/jira/browse/MESOS-9837 > > > Repository: mesos > > > Description > --- > > This patch introduces a mechanism for tracking pending futures. > This feature allows detection of hanging operations, which get > stuck on a blocking operation or asynchronously. However, this > feature does not provide any mechanism for tracking pending > promises, because `Promise` objects might not be accessible in > various cases. Thereby, we introduce a new class that can be > used to track pending futures, so it might facilitate debugging > of stuck issues. > > > Diffs > - > > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/common/future_tracker.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70887/diff/4/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70887: Added `PendingFutureTracker` class for tracking pending futures.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70887/#review216603 --- src/Makefile.am Lines 1070 (patched) <https://reviews.apache.org/r/70887/#comment303808> Should we name it `future_tracker.hpp` to be consistent with the class in it (`PendingFutureTracker`)? src/common/future_track.hpp Lines 17 (patched) <https://reviews.apache.org/r/70887/#comment303812> s/__FUTURE_TRACK_HPP__/__FUTURE_TRACKER_HPP__/ src/common/future_track.hpp Lines 34 (patched) <https://reviews.apache.org/r/70887/#comment303811> I do not think we need this since this file has been moved to `src/common/`. src/common/future_track.hpp Lines 55-56 (patched) <https://reviews.apache.org/r/70887/#comment303809> Suggest to merge these two lines into a single line. src/common/future_track.hpp Lines 90 (patched) <https://reviews.apache.org/r/70887/#comment303810> I'd suggest to use `Owned` instead of raw pointer. - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70887/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Benjamin Mahler, Gilbert Song, James Peach, Meng > Zhu, and Qian Zhang. > > > Bugs: MESOS-9837 > https://issues.apache.org/jira/browse/MESOS-9837 > > > Repository: mesos > > > Description > --- > > This patch introduces a mechanism for tracking pending futures. > This feature allows detection of hanging operations, which get > stuck on a blocking operation or asynchronously. However, this > feature does not provide any mechanism for tracking pending > promises, because `Promise` objects might not be accessible in > various cases. Thereby, we introduce a new class that can be > used to track pending futures, so it might facilitate debugging > of stuck issues. > > > Diffs > - > > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/common/future_track.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70887/diff/3/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Review Request 71072: Renamed agent flag `--default_shm_size`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71072/ --- Review request for mesos and Gilbert Song. Bugs: MESOS-9833 https://issues.apache.org/jira/browse/MESOS-9833 Repository: mesos Description --- Renamed agent flag `--default_shm_size`. Diffs - docs/configuration/agent.md 2046bf25838c4bba9409b0d5e7b29c35882356d2 docs/isolators/namespaces-ipc.md 43ed8423a7d3877f02408947b78989b79dcd4907 include/mesos/mesos.proto e0a23913ed64ff6e82c07db227e9d6e0f71a8b55 include/mesos/v1/mesos.proto af29a140dc7f0a869c8e3b606ec4fb9a22ba1f7b src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 9a984765a317e540e8c7c630dc46cf911f2a33a7 src/slave/flags.hpp a10bf698dc447b1411c06083c2ba7585d7a78389 src/slave/flags.cpp b4e3eb99221a09404dbbf813da33607867a78691 src/tests/containerizer/isolator_tests.cpp a493a309464f8c7b3cb6f0fc45a2762d12071c67 Diff: https://reviews.apache.org/r/71072/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 70859: Updated the test `NamespacesIsolatorTest.ROOT_IPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70859/ --- (Updated July 13, 2019, 4:27 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Cleanup the files created under `/dev/shm/`. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- This test is updated to verify the backward compatibility is kept after we implement the configurable IPC namespaces and /dev/shm. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp bb2cda47ad1fdd7ad16c419eb04f2e0b9293d2b6 Diff: https://reviews.apache.org/r/70859/diff/2/ Changes: https://reviews.apache.org/r/70859/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70860: Added the test `ROOT_IPCNamespaceWithIPCIsolatorDisabled`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70860/ --- (Updated July 13, 2019, 4:26 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Cleanup the files created under `/dev/shm`. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `ROOT_IPCNamespaceWithIPCIsolatorDisabled`. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp bb2cda47ad1fdd7ad16c419eb04f2e0b9293d2b6 Diff: https://reviews.apache.org/r/70860/diff/2/ Changes: https://reviews.apache.org/r/70860/diff/1-2/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 70857: Added the test `ROOT_DisallowShareAgentIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70857/ --- (Updated July 13, 2019, 4:22 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Minor change. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `ROOT_DisallowShareAgentIPCNamespace`. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp bb2cda47ad1fdd7ad16c419eb04f2e0b9293d2b6 Diff: https://reviews.apache.org/r/70857/diff/2/ Changes: https://reviews.apache.org/r/70857/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70852: Added the test `NamespacesIsolatorTest.ROOT_ShareAgentIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70852/ --- (Updated July 13, 2019, 4:21 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Cleanup the filed created under `/dev/shm`. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_ShareAgentIPCNamespace`. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp bb2cda47ad1fdd7ad16c419eb04f2e0b9293d2b6 Diff: https://reviews.apache.org/r/70852/diff/2/ Changes: https://reviews.apache.org/r/70852/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70849: Added the test `NamespacesIsolatorTest.ROOT_PrivateIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70849/ --- (Updated July 13, 2019, 4:19 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Minor change. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_PrivateIPCNamespace`. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp bb2cda47ad1fdd7ad16c419eb04f2e0b9293d2b6 Diff: https://reviews.apache.org/r/70849/diff/2/ Changes: https://reviews.apache.org/r/70849/diff/1-2/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 70845: Added the test `NamespacesIsolatorTest.ROOT_ShareIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70845/ --- (Updated July 13, 2019, 4:16 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Enabled `docker/runtime` isolator by default. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_ShareIPCNamespace`. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp bb2cda47ad1fdd7ad16c419eb04f2e0b9293d2b6 Diff: https://reviews.apache.org/r/70845/diff/3/ Changes: https://reviews.apache.org/r/70845/diff/2-3/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 71004: Added a test `ROOT_DOCKER_AllocationRoleEnvironmentVariable`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71004/ --- Review request for mesos and Gilbert Song. Bugs: MESOS-9874 https://issues.apache.org/jira/browse/MESOS-9874 Repository: mesos Description --- Added a test `ROOT_DOCKER_AllocationRoleEnvironmentVariable`. Diffs - src/tests/containerizer/docker_containerizer_tests.cpp a6217581e20168c5114f733323e927a83ac59fd0 Diff: https://reviews.apache.org/r/71004/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 71003: Added a test `DefaultExecutorTest.AllocationRoleEnvironmentVariable`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71003/ --- Review request for mesos and Gilbert Song. Bugs: MESOS-9874 https://issues.apache.org/jira/browse/MESOS-9874 Repository: mesos Description --- Added a test `DefaultExecutorTest.AllocationRoleEnvironmentVariable`. Diffs - src/tests/default_executor_tests.cpp 1c3b48848cf387fe9fb109abfaffa5be04e2a3e1 Diff: https://reviews.apache.org/r/71003/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 71002: Added a test `CommandExecutorTest.AllocationRoleEnvironmentVariable`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71002/ --- Review request for mesos and Gilbert Song. Bugs: MESOS-9874 https://issues.apache.org/jira/browse/MESOS-9874 Repository: mesos Description --- Added a test `CommandExecutorTest.AllocationRoleEnvironmentVariable`. Diffs - src/tests/command_executor_tests.cpp 02ae250a599043f30ef5879ce528815e0ded5d3d Diff: https://reviews.apache.org/r/71002/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 70989: Set the `MESOS_ALLOCATION_ROLE` environment variable for task.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70989/ --- Review request for mesos and Gilbert Song. Bugs: MESOS-9874 https://issues.apache.org/jira/browse/MESOS-9874 Repository: mesos Description --- Set the `MESOS_ALLOCATION_ROLE` environment variable for task. Diffs - src/docker/docker.cpp 9ebb5f27464b898895e03971fa462349fb68e1f0 src/launcher/default_executor.cpp 5837cfa4deba557cae43112092ff24b97137951f src/launcher/executor.cpp 38d82614ed82e8a6644334f0401cecdee6a025bf Diff: https://reviews.apache.org/r/70989/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70892: Added `/containerizer/debug` endpoint.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70892/#review216264
---
src/slave/http.hpp
Lines 84 (patched)
<https://reviews.apache.org/r/70892/#comment303393>
s/containerizer_debug/containerizerDebug/
src/slave/http.hpp
Lines 130 (patched)
<https://reviews.apache.org/r/70892/#comment303395>
s/_containerizer_debug/_containerizerDebug/
src/slave/http.cpp
Lines 2350-2352 (patched)
<https://reviews.apache.org/r/70892/#comment303394>
Can we merge them into a single paragraph?
src/slave/http.cpp
Lines 2428-2429 (patched)
<https://reviews.apache.org/r/70892/#comment303397>
Why do we put an array into an array (`futures`)? I think we should put
`JSON::Object` into the `futures` array, and in the `JSON::Object` there should
be two fields: name and args.
src/slave/http.cpp
Lines 2431 (patched)
<https://reviews.apache.org/r/70892/#comment303396>
If this endpoint will only return pending operations, do we really need the
`pending` here? I think we can just return a `JSON::ARRAY` instead of
`JSON::OBJECT` and each element in the array is a pending operations.
Or we may consider to add other info (other than pending operations) into
this endpoint?
src/slave/slave.cpp
Lines 837 (patched)
<https://reviews.apache.org/r/70892/#comment303399>
Do we plan to return pending operations for other components rather than
just containerizer in future? If yes, then I think we'd better give it a
generic name (like `/debug`) and return component name (currently it is just
`containerizer`) along with operation name and args in the response.
- Qian Zhang
On June 19, 2019, 10:49 p.m., Andrei Budnik wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70892/
> ---
>
> (Updated June 19, 2019, 10:49 p.m.)
>
>
> Review request for mesos, Gilbert Song, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9829
> https://issues.apache.org/jira/browse/MESOS-9829
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch introduces an agent's `/containerizer/debug` endpoint,
> which exposes the debug info related to Mesos containerizer.
> Currently, this endpoint returns a list of pending futures related to
> isolators or containerizer launcher. This endpoint is experimental,
> and the format of its output may change over time.
>
>
> Diffs
> -
>
> src/slave/http.hpp b8c83f1db95c2175bb94f6cd76cc2c58aa118c4e
> src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
>
>
> Diff: https://reviews.apache.org/r/70892/diff/1/
>
>
> Testing
> ---
>
> Manual testing.
> 1. Add `::sleep(5)` to the beginning of the
> `LinuxSeccompIsolatorProcess::prepare` method.
> 2. Rebuild Mesos and then spin up a local Mesos cluster.
> 3. Run simple task, e.g.:
> ```
> ./src/mesos-execute --master="`hostname`:5050" --name="test"
> --containerizer=mesos --command="sleep 1"
> ```
> 4. In the parallel terminal session:
> ```
> curl `hostname`:5051/containerizer/debug
> # will return:
> {"pending":[["linux/seccomp::prepare",{"containerId":""}]]}
> ```
> 5. After 5 seconds this endpoint returns an empty list of pending futures:
> `{"pending":[]}`
>
> Unit tests: TBD in the following patches.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 70891: Wrapped launcher in `LauncherTracker`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70891/#review216262 --- src/slave/containerizer/mesos/containerizer.cpp Lines 347-349 (patched) <https://reviews.apache.org/r/70891/#comment303392> Can we merge these into a single line? - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70891/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Gilbert Song and Qian Zhang. > > > Bugs: MESOS-9841 > https://issues.apache.org/jira/browse/MESOS-9841 > > > Repository: mesos > > > Description > --- > > This patch wraps a container launcher in instance of `LauncherTracker` > class. If the launcher gets stuck in some operation, `pendingFutures` > will return the method name along with its arguments such as > `containerId`, `pid`, etc. > > > Diffs > - > > src/slave/containerizer/mesos/containerizer.cpp > 043244841a73fa3f5f7119bc38f6d3a04be8990b > > > Diff: https://reviews.apache.org/r/70891/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70890: Added `LauncherTracker` for tracking calls of launcher methods.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70890/#review216261 --- src/slave/containerizer/mesos/launcher_tracker.cpp Lines 58 (patched) <https://reviews.apache.org/r/70890/#comment303391> This method should be tracked as well. - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70890/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Gilbert Song and Qian Zhang. > > > Bugs: MESOS-9840 > https://issues.apache.org/jira/browse/MESOS-9840 > > > Repository: mesos > > > Description > --- > > This patch adds a new `LauncherTracker` class that intercepts > `Launcher` calls and uses `track` function for detecting very > slow or stuck operations on the real launcher. > > > Diffs > - > > src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/slave/containerizer/mesos/launcher_tracker.hpp PRE-CREATION > src/slave/containerizer/mesos/launcher_tracker.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70890/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70889: Wrapped isolators in `IsolatorTracker`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70889/#review216260 --- src/slave/containerizer/mesos/containerizer.cpp Lines 586-589 (patched) <https://reviews.apache.org/r/70889/#comment303390> Why do we need a dedicated for loop to do this? Can we just do it when each isolator is created (line 549 and line 563)? - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70889/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Gilbert Song and Qian Zhang. > > > Bugs: MESOS-9841 > https://issues.apache.org/jira/browse/MESOS-9841 > > > Repository: mesos > > > Description > --- > > This patch wraps every isolator in instance of `IsolatorTracker` class. > If an isolator gets stuck in some operation, `pendingFutures` will > return the isolator name, method name along with its arguments such as > `containerId`, `pid`, etc. > > > Diffs > - > > src/slave/containerizer/mesos/containerizer.cpp > 043244841a73fa3f5f7119bc38f6d3a04be8990b > > > Diff: https://reviews.apache.org/r/70889/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70888: Added `IsolatorTracker` for tracking calls of isolator methods.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70888/#review216257 --- src/slave/containerizer/mesos/isolator_tracker.cpp Lines 95-96 (patched) <https://reviews.apache.org/r/70888/#comment303386> Do we really need to track `watch`? The future returned by `watch` is supposed to be pending as long as the container is running, right? I think what we care about is the future which is supposed to be ready soon but pending for a long time somehow. src/slave/containerizer/mesos/isolator_tracker.cpp Lines 111 (patched) <https://reviews.apache.org/r/70888/#comment303387> Better to add `stringify(resources)`. - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70888/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Gilbert Song and Qian Zhang. > > > Bugs: MESOS-9839 > https://issues.apache.org/jira/browse/MESOS-9839 > > > Repository: mesos > > > Description > --- > > This patch adds a new `IsolatorTracker` class that intercepts > `Isolator` calls and uses `track` function for detecting very > slow or stuck operations on the real isolator. > > > Diffs > - > > src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/slave/constants.hpp cc81e5111e8c3082c940b918230f660a1121a1ac > src/slave/containerizer/mesos/isolator_tracker.hpp PRE-CREATION > src/slave/containerizer/mesos/isolator_tracker.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70888/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70887: Added `track`, `pendingFutures` functions for tracking pending futures. (WIP)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70887/#review216253 --- src/Makefile.am Lines 1223 (patched) <https://reviews.apache.org/r/70887/#comment303381> It seems the implementation of `future_track.hpp` is not Mesos containerizer specific, do we want to move it a common place? src/slave/containerizer/mesos/future_track.hpp Lines 85 (patched) <https://reviews.apache.org/r/70887/#comment303380> Can we use `std::vector` instead? - Qian Zhang On June 19, 2019, 10:49 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70887/ > --- > > (Updated June 19, 2019, 10:49 p.m.) > > > Review request for mesos, Benjamin Mahler, Gilbert Song, James Peach, Meng > Zhu, and Qian Zhang. > > > Bugs: MESOS-9837 > https://issues.apache.org/jira/browse/MESOS-9837 > > > Repository: mesos > > > Description > --- > > This patch introduces a mechanism for tracking of pending futures. > This feature can be used to detect hanging operations, which might > get stuck on a blocking operation or asynchronously. Both `track` > and `pendingFutures` functions depend on `PendingFutureTracker` > class, which is instantiated the first time they are accessed. > > > Diffs > - > > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/slave/containerizer/mesos/future_track.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70887/diff/2/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70798: Improved `namespaces/ipc` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70798/ --- (Updated June 29, 2019, 10:10 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Added more comments in the code. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Improved `namespaces/ipc` isolator for configurable IPC support. Diffs (updated) - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 src/slave/containerizer/mesos/paths.hpp a5e0920bd4d753f6ba6f3b092c92cb405e9edf35 src/slave/containerizer/mesos/paths.cpp 4281abc522c942c87fcfd811af26f95cbd6f734f src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70798/diff/5/ Changes: https://reviews.apache.org/r/70798/diff/4-5/ Testing --- Thanks, Qian Zhang
Re: Review Request 70798: Improved `namespaces/ipc` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70798/ --- (Updated June 21, 2019, 9:08 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Addressed review comments. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Improved `namespaces/ipc` isolator for configurable IPC support. Diffs (updated) - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 src/slave/containerizer/mesos/paths.hpp a5e0920bd4d753f6ba6f3b092c92cb405e9edf35 src/slave/containerizer/mesos/paths.cpp 4281abc522c942c87fcfd811af26f95cbd6f734f src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70798/diff/4/ Changes: https://reviews.apache.org/r/70798/diff/3-4/ Testing --- Thanks, Qian Zhang
Re: Review Request 70820: Updated `filesystem/linux` isolator for configurable IPC support.
> On June 21, 2019, 8:27 a.m., Gilbert Song wrote: > > could we consider to do the decouple now? > > Gilbert Song wrote: > probably still need a deprecation cycle :( Yes, and I have added a TODO in mesos.proto for the deprecation cycle. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70820/#review216030 --- On June 12, 2019, 10:32 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70820/ > --- > > (Updated June 12, 2019, 10:32 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9826 > https://issues.apache.org/jira/browse/MESOS-9826 > > > Repository: mesos > > > Description > --- > > If `namespaces/ipc` isolator is not enabled, for backward > compatibility /dev/shm will still be handled in `filesystem/linux` > isolator as before. Otherwise, both /dev/shm and IPC namespace > will be handled by `namespaces/ipc` isolator. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 3cfb6e97a565420c8be2a0e31b481b39cd09d9da > > > Diff: https://reviews.apache.org/r/70820/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 70844: Implemented `cleanup` method of the `namespaces/ipc` isolator.
> On June 21, 2019, 8:26 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Lines 400 (patched) > > <https://reviews.apache.org/r/70844/diff/1/?file=2149226#file2149226line400> > > > > if the shmPath not exists, should we log a warning? No, because shmPath not exists is a normal case for the containers which share its parent's shm. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70844/#review216029 --- On June 12, 2019, 10:36 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70844/ > --- > > (Updated June 12, 2019, 10:36 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9788 > https://issues.apache.org/jira/browse/MESOS-9788 > > > Repository: mesos > > > Description > --- > > Implemented `cleanup` method of the `namespaces/ipc` isolator. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp > 32c888309ca536d944e4d73641aed214805ccce2 > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 > > > Diff: https://reviews.apache.org/r/70844/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 70798: Improved `namespaces/ipc` isolator for configurable IPC support.
> On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Lines 52-54 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148577#file2148577line52> > > > > consider to return Result for no parent case? What do you mean for "no parent case"? This method will only be called for the nested container case, i.e., the container must have parent. > On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Lines 56 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148577#file2148577line56> > > > > a bit confusing. the parentId could be assigned as a containerId > > without a parent? That is for the top-level container which does not have parent. However, I agree it is confusing, let me improve it. > On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Lines 90-93 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148577#file2148577line90> > > > > what if for SHARE_PARENT case but the container does not have a parent? We will return `AGENT_SHM_PATH` because it is the case that a top-level container whose `ipc_mode` is `SHARE_PARENT` (i.e., it will share agent's /dev/shm). > On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Lines 137-143 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148577#file2148577line137> > > > > is this a new dependency? Yes. > On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Lines 212 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148577#file2148577line215> > > > > are we going to remove the `createContainerMount()` in linux filesystem > > isolator? Do you mean the `createContainerMount()` in linux filesystem isolator for /dev/shm? in https://reviews.apache.org/r/70820 I have updated linux filesystem to do `createContainerMount()` for /dev/shm only when `namespaces/ipc` isolator is not enabled. > On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > > Line 94 (original), 382 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148577#file2148577line385> > > > > if we introduce cleanup(), does it mean we could avoid the dependency > > on linux filesystem isolator? No, we still need linux filesystem to ensure non-debug container has its own mount namespace and debug container enters its parent's mount namespace. We have the similar dependency in `namespaces/pid` isolator. > On June 21, 2019, 8:24 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/paths.hpp > > Lines 85 (patched) > > <https://reviews.apache.org/r/70798/diff/3/?file=2148578#file2148578line85> > > > > how about `CONTAINER_SHM_DIRECTORY` or just `CONTAINER_SHM`? Agree! Let's go with `CONTAINER_SHM_DIRECTORY`. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70798/#review216026 --- On June 11, 2019, 10:33 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70798/ > --- > > (Updated June 11, 2019, 10:33 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9788 > https://issues.apache.org/jira/browse/MESOS-9788 > > > Repository: mesos > > > Description > --- > > Improved `namespaces/ipc` isolator for configurable IPC support. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp > 32c888309ca536d944e4d73641aed214805ccce2 > src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp > 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 > src/slave/containerizer/mesos/paths.hpp > a5e0920bd4d753f6ba6f3b092c92cb405e9edf35 > src/slave/containerizer/mesos/paths.cpp > 4281abc522c942c87fcfd811af26f95cbd6f734f > src/tests/containerizer/isolator_tests.cpp > 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 > > > Diff: https://reviews.apache.org/r/70798/diff/3/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 70775: Added `ipc_mode` and `shm_size` fields into `LinuxInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70775/ --- (Updated June 20, 2019, 2:04 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Addressed review comments. Bugs: MESOS-9827 https://issues.apache.org/jira/browse/MESOS-9827 Repository: mesos Description --- Added `ipc_mode` and `shm_size` fields into `LinuxInfo`. Diffs (updated) - include/mesos/mesos.proto 2b4f350815935220c2d2b0dd0e52346bc74c91d9 include/mesos/v1/mesos.proto bafc27499f810791700c4a30dcb1da33b6f31d2e Diff: https://reviews.apache.org/r/70775/diff/2/ Changes: https://reviews.apache.org/r/70775/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70775: Added `ipc_mode` and `shm_size` fields into `LinuxInfo`.
> On June 20, 2019, 8:44 a.m., Gilbert Song wrote: > > include/mesos/mesos.proto > > Lines 3269-3270 (patched) > > <https://reviews.apache.org/r/70775/diff/1/?file=2147578#file2147578line3269> > > > > delete: with a possibility to share them with its child containers > > > > Is a nested container allowed to have its own ipc namespace and > > /dev/shm? I would suggest to keep `"with a possibility to share them with its child containers"`, actually Docker has something similar, see `--ipc=shareable` in https://docs.docker.com/engine/reference/run/#ipc-settings---ipc > Is a nested container allowed to have its own ipc namespace and /dev/shm? Yes. > On June 20, 2019, 8:44 a.m., Gilbert Song wrote: > > include/mesos/mesos.proto > > Lines 3279 (patched) > > <https://reviews.apache.org/r/70775/diff/1/?file=2147578#file2147578line3279> > > > > s/do it/ share from the agent's/g The word `share from the agent's` makes the whole sentence a bit redundant, so I would suggest to keep the word `do it`. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70775/#review215974 --- On June 7, 2019, 7:23 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70775/ > --- > > (Updated June 7, 2019, 7:23 a.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9827 > https://issues.apache.org/jira/browse/MESOS-9827 > > > Repository: mesos > > > Description > --- > > Added `ipc_mode` and `shm_size` fields into `LinuxInfo`. > > > Diffs > - > > include/mesos/mesos.proto dc6a87f14864cb4d46b9856f251c2946a475162d > include/mesos/v1/mesos.proto e8086e0f70cac73876a8ae31db3365b5059b5c44 > > > Diff: https://reviews.apache.org/r/70775/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 70773: Added `--disallow_sharing_agent_ipc_namespace` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70773/ --- (Updated June 19, 2019, 11:02 a.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9825 https://issues.apache.org/jira/browse/MESOS-9825 Repository: mesos Description --- Added `--disallow_sharing_agent_ipc_namespace` agent flag. Diffs - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70773/diff/3/ Testing --- Thanks, Qian Zhang
Re: Review Request 70774: Added `--default_shm_size` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70774/ --- (Updated June 19, 2019, 2:26 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Updated the description of the `--default_shm_size` agent flag. Summary (updated) - Added `--default_shm_size` agent flag. Bugs: MESOS-9833 https://issues.apache.org/jira/browse/MESOS-9833 Repository: mesos Description (updated) --- Added `--default_shm_size` agent flag. Diffs (updated) - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70774/diff/3/ Changes: https://reviews.apache.org/r/70774/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 70773: Added `--disallow_sharing_agent_ipc_namespace` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70773/ --- (Updated June 19, 2019, 2:21 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Summary (updated) - Added `--disallow_sharing_agent_ipc_namespace` agent flag. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description (updated) --- Added `--disallow_sharing_agent_ipc_namespace` agent flag. Diffs (updated) - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70773/diff/3/ Changes: https://reviews.apache.org/r/70773/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 70863: Assign cgroup processes after configuring the subsystem.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70863/#review215932 --- Ship it! Ship It! - Qian Zhang On June 17, 2019, 10:47 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70863/ > --- > > (Updated June 17, 2019, 10:47 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-9805 > https://issues.apache.org/jira/browse/MESOS-9805 > > > Repository: mesos > > > Description > --- > > Currently, the PID targeted by the cgroups isolator is moved into > the cgroup before the subsystem runs to apply any type-specific > cgroup configuration. We should reverse the order of this so that > the PID is only moved once the cgroup is fully configured by the > subsystem. > > A specific case that can happen is where a PID is assigned to a net_cls > cgroup before that cgroup has its class ID set. This intermediate > process state can be observed by system monitoring process, causing > confusion that is hard to debug. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp > 4a1871b3b06b54a02dfe09289f7fb304a3f7f24c > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp > e7819d732172bdbd215106e3b781588c1f78b2ec > > > Diff: https://reviews.apache.org/r/70863/diff/1/ > > > Testing > --- > > sudo make check (Fedora 30) > > > Thanks, > > James Peach > >
Review Request 70870: Updated `namespaces-ipc.md` for configurable IPC namespace and /dev/shm.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70870/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9828 https://issues.apache.org/jira/browse/MESOS-9828 Repository: mesos Description --- Updated `namespaces-ipc.md` for configurable IPC namespace and /dev/shm. Diffs - docs/isolators/namespaces-ipc.md e550e752ef0ca4bc363fd3c5c190c44f5dd4187a Diff: https://reviews.apache.org/r/70870/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70860: Added the test `ROOT_IPCNamespaceWithIPCIsolatorDisabled`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70860/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `ROOT_IPCNamespaceWithIPCIsolatorDisabled`. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70860/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 70859: Updated the test `NamespacesIsolatorTest.ROOT_IPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70859/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- This test is updated to verify the backward compatibility is kept after we implement the configurable IPC namespaces and /dev/shm. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70859/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70852: Added the test `NamespacesIsolatorTest.ROOT_ShareAgentIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70852/ --- (Updated June 14, 2019, 11:01 a.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_ShareAgentIPCNamespace`. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70852/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70857: Added the test `ROOT_DisallowShareAgentIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70857/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `ROOT_DisallowShareAgentIPCNamespace`. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70857/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70852: Added the test `NamespacesIsolatorTest.ROOT_ShareAgentIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70852/ --- Review request for mesos. Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_ShareAgentIPCNamespace`. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70852/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70849: Added the test `NamespacesIsolatorTest.ROOT_PrivateIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70849/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_PrivateIPCNamespace`. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70849/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 70845: Added the test `NamespacesIsolatorTest.ROOT_ShareIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70845/ --- (Updated June 13, 2019, 2:57 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_ShareIPCNamespace`. Diffs (updated) - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70845/diff/2/ Changes: https://reviews.apache.org/r/70845/diff/1-2/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 70845: Added the test `NamespacesIsolatorTest.ROOT_ShareIPCNamespace`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70845/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added the test `NamespacesIsolatorTest.ROOT_ShareIPCNamespace`. Diffs - src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70845/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 70844: Implemented `cleanup` method of the `namespaces/ipc` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70844/ --- (Updated June 12, 2019, 10:36 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Implemented `cleanup` method of the `namespaces/ipc` isolator. Diffs - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 Diff: https://reviews.apache.org/r/70844/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70844: Implemented `cleanup` method of the `namespaces/ipc` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70844/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9826 https://issues.apache.org/jira/browse/MESOS-9826 Repository: mesos Description --- Implemented `cleanup` method of the `namespaces/ipc` isolator. Diffs - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 Diff: https://reviews.apache.org/r/70844/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70820: Updated `filesystem/linux` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70820/ --- (Updated June 12, 2019, 10:32 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9826 https://issues.apache.org/jira/browse/MESOS-9826 Repository: mesos Description --- If `namespaces/ipc` isolator is not enabled, for backward compatibility /dev/shm will still be handled in `filesystem/linux` isolator as before. Otherwise, both /dev/shm and IPC namespace will be handled by `namespaces/ipc` isolator. Diffs - src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 3cfb6e97a565420c8be2a0e31b481b39cd09d9da Diff: https://reviews.apache.org/r/70820/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70798: Improved `namespaces/ipc` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70798/ --- (Updated June 11, 2019, 10:33 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Added comments in the code. Summary (updated) - Improved `namespaces/ipc` isolator for configurable IPC support. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description (updated) --- Improved `namespaces/ipc` isolator for configurable IPC support. Diffs (updated) - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 src/slave/containerizer/mesos/paths.hpp a5e0920bd4d753f6ba6f3b092c92cb405e9edf35 src/slave/containerizer/mesos/paths.cpp 4281abc522c942c87fcfd811af26f95cbd6f734f src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70798/diff/3/ Changes: https://reviews.apache.org/r/70798/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 70827: Improved container-specific cgroups test by checking `cpu.shares`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70827/ --- (Updated June 11, 2019, 10:48 a.m.) Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Bugs: MESOS-9769 https://issues.apache.org/jira/browse/MESOS-9769 Repository: mesos Description --- This is to ensure the symbolic links (see below as an example) we create for the container exist. ln -s /sys/fs/cgroup/cpu,cpuacct /sys/fs/cgroup/cpu Diffs - src/tests/containerizer/cgroups_isolator_tests.cpp 957f72d78f9ab0bf2775687915099c0109dac6e1 Diff: https://reviews.apache.org/r/70827/diff/1/ Testing (updated) --- sudo make check The tests updated in this patch would fail without the previous patch. Thanks, Qian Zhang
Review Request 70827: Improved container-specific cgroups test by checking `cpu.shares`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70827/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Bugs: MESOS-9769 https://issues.apache.org/jira/browse/MESOS-9769 Repository: mesos Description --- This is to ensure the symbolic links (see below as an example) we create for the container exist. ln -s /sys/fs/cgroup/cpu,cpuacct /sys/fs/cgroup/cpu Diffs - src/tests/containerizer/cgroups_isolator_tests.cpp 957f72d78f9ab0bf2775687915099c0109dac6e1 Diff: https://reviews.apache.org/r/70827/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70826: Supported file operations for command tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70826/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Bugs: MESOS-9769 https://issues.apache.org/jira/browse/MESOS-9769 Repository: mesos Description --- Supported file operations for command tasks. Diffs - src/launcher/executor.cpp fa4bcaad9ac36bf380484dadb14d0b0a86a30aae Diff: https://reviews.apache.org/r/70826/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70798: WIP: Improved `namespaces/ipc` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70798/ --- (Updated June 10, 2019, 12:57 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Fixed an error in unit test. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- WIP: Improved `namespaces/ipc` isolator for configurable IPC support. Diffs (updated) - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 src/slave/containerizer/mesos/paths.hpp a5e0920bd4d753f6ba6f3b092c92cb405e9edf35 src/slave/containerizer/mesos/paths.cpp 4281abc522c942c87fcfd811af26f95cbd6f734f src/tests/containerizer/isolator_tests.cpp 9c14f3acbc19631b2f5cac4dc7cd9caba8527712 Diff: https://reviews.apache.org/r/70798/diff/2/ Changes: https://reviews.apache.org/r/70798/diff/1-2/ Testing --- Thanks, Qian Zhang
Review Request 70820: Updated `filesystem/linux` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70820/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9826 https://issues.apache.org/jira/browse/MESOS-9826 Repository: mesos Description --- If `namespaces/ipc` isolator is not enabled, for backward compatibility /dev/shm will still be handled in `filesystem/linux` isolator as before. Otherwise, both /dev/shm and IPC namespace will be handled by `namespaces/ipc` isolator. Diffs - src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 3cfb6e97a565420c8be2a0e31b481b39cd09d9da Diff: https://reviews.apache.org/r/70820/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70798: WIP: Improved `namespaces/ipc` isolator for configurable IPC support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70798/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- WIP: Improved `namespaces/ipc` isolator for configurable IPC support. Diffs - src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 32c888309ca536d944e4d73641aed214805ccce2 src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 src/slave/containerizer/mesos/paths.hpp a5e0920bd4d753f6ba6f3b092c92cb405e9edf35 src/slave/containerizer/mesos/paths.cpp 4281abc522c942c87fcfd811af26f95cbd6f734f Diff: https://reviews.apache.org/r/70798/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70775: Added `ipc_mode` and `shm_size` fields into `LinuxInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70775/ --- (Updated June 7, 2019, 7:23 a.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9827 https://issues.apache.org/jira/browse/MESOS-9827 Repository: mesos Description --- Added `ipc_mode` and `shm_size` fields into `LinuxInfo`. Diffs - include/mesos/mesos.proto dc6a87f14864cb4d46b9856f251c2946a475162d include/mesos/v1/mesos.proto e8086e0f70cac73876a8ae31db3365b5059b5c44 Diff: https://reviews.apache.org/r/70775/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70774: Added `default_shm_size` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70774/ --- (Updated June 7, 2019, 7:19 a.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9833 https://issues.apache.org/jira/browse/MESOS-9833 Repository: mesos Description --- Added `default_shm_size` agent flag. Diffs - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70774/diff/2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70774: Added `default_shm_size` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70774/ --- (Updated June 3, 2019, 2:38 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Rebased. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added `default_shm_size` agent flag. Diffs (updated) - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70774/diff/2/ Changes: https://reviews.apache.org/r/70774/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70773: Added `disallow_sharing_agent_ipc_namespace` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70773/ --- (Updated June 3, 2019, 2:38 p.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Changes --- Moved `disallow_sharing_agent_ipc_namespace` and `disallow_sharing_agent_pid_namespace` to the right place. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added `disallow_sharing_agent_ipc_namespace` agent flag. Diffs (updated) - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70773/diff/2/ Changes: https://reviews.apache.org/r/70773/diff/1-2/ Testing --- Thanks, Qian Zhang
Review Request 70774: Added `default_shm_size` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70774/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added `default_shm_size` agent flag. Diffs - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70774/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70775: Added `ipc_mode` and `shm_size` fields into `LinuxInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70775/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added `ipc_mode` and `shm_size` fields into `LinuxInfo`. Diffs - include/mesos/mesos.proto dc6a87f14864cb4d46b9856f251c2946a475162d include/mesos/v1/mesos.proto e8086e0f70cac73876a8ae31db3365b5059b5c44 Diff: https://reviews.apache.org/r/70775/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70773: Added `disallow_sharing_agent_ipc_namespace` agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70773/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9788 https://issues.apache.org/jira/browse/MESOS-9788 Repository: mesos Description --- Added `disallow_sharing_agent_ipc_namespace` agent flag. Diffs - docs/configuration/agent.md 91a5e931c35fb353c87bee35004cb73e8546c8d3 src/slave/flags.hpp 9684a09071be930f25a7d10821ee65b5a965fa83 src/slave/flags.cpp e23061ad929c8e3c24806f52c95f82766e3d1911 Diff: https://reviews.apache.org/r/70773/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70581: Add flag to decouple docker runtime.
> On May 15, 2019, 2:16 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/containerizer.cpp > > Lines 230-235 (patched) > > <https://reviews.apache.org/r/70581/diff/4/?file=2145030#file2145030line230> > > > > s/ignore_docker_runtime_isolator/docker_ignore_runtime/ > > > > And erasing `docker/runtime` from `isolations` seems a bit weird to me, > > should we instead add a flag (like `--ignore-docker-image-manifest`) and > > let `docker/runtime` isolator handle it (i.e., if it is true the isolator > > will just do nothing)? Or maybe this should be a configuration per > > container? Like add a field in `ContainerInfo`. > > James Peach wrote: > > s/ignore_docker_runtime_isolator/docker_ignore_runtime/ > > Agreed. > > > And erasing docker/runtime from isolations seems a bit weird to me, > should we instead add a flag (like --ignore-docker-image-manifest) and > > let docker/runtime isolator handle it (i.e., if it is true the isolator > will just do nothing)? Or maybe this should be a configuration per > > container? Like add a field in ContainerInfo. > > We did go back and forth on this a bit. The documentation is pretty clear > in a number of places that `docker/runtime` is required and it is the > isolator that deals with the Docker runtime config. So in the end I thought > that it was clearer to link the flag name to the isolator name quite > explicitly. Once you do that, then the implementation follows naturally. > Whether we drop the isolator or whether the isolator remains but takes no > action is an implementation detail that is invisible to an operator. > > I would have prefered that we just allow the `docker/runtime` isolator > the be absent, but we felt constrained by backwards compatibility a bit here. > > It did not occur to me to make this a per-image flag. In the future, this > could still be added. We could change the `docker/runtime` isolator to check > the `ContainerInfo` flag as you suggest. However, our reason for needing this > is that we have to retain compatibility with an existing PaaS implementation, > so I think that the need for this as a container configuration is limited. I think what we usually do in Mesos code is an agent flag for agent-wide configuration and a protobuf message field for per container configuration. I agree that we can implement the per container configuration later, but for the agent flag I would suggest to let the `docker/runtime` isolatior handle it rather than using the flag to drop the isolator because in future when we add the per containner configuration, the implemenation would be nature and easy to read since all the codes are in a single place (the isolator) rather than in multiple places (like the containerizer and store). - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70581/#review215279 --- On May 15, 2019, 2:51 a.m., Jacob Janco wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70581/ > --- > > (Updated May 15, 2019, 2:51 a.m.) > > > Review request for mesos, Gilbert Song and James Peach. > > > Bugs: MESOS-9760 > https://issues.apache.org/jira/browse/MESOS-9760 > > > Repository: mesos > > > Description > --- > > Docker runtime isolator propagates manifest configuration metadata. > This is not always desirable, e.g. a customer may want to ignore the > defined WORKDIR/ENV defined in the manifest. This patch decouples > isolator work from the selection of Docker as an image provider. > > https://issues.apache.org/jira/browse/MESOS-9760 > > > Diffs > - > > docs/configuration/agent.md 04d7114e92c189afd04a8889b3ab853ade9deec6 > docs/upgrades.md ece8462b9950626166c832d3b554ad51f68f42e1 > src/slave/containerizer/mesos/containerizer.cpp > c4a6827c5574586475152b1dc31572d6e1821a82 > src/slave/containerizer/mesos/provisioner/store.cpp > 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a > src/slave/flags.hpp 09921cb6172202b5c1d2f8d03f9ccaeb3d0e8c94 > src/slave/flags.cpp 49a350f9e13409493fa9612c53d9d58b62122371 > src/tests/containerizer/provisioner_docker_tests.cpp > 1c5d72091d570001ad543e342ab0b9128ffc01ae > > > Diff: https://reviews.apache.org/r/70581/diff/4/ > > > Testing > --- > > > Thanks, > > Jacob Janco > >
Re: Review Request 70581: Add flag to decouple docker runtime.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70581/#review215279 --- src/slave/containerizer/mesos/containerizer.cpp Lines 230-235 (patched) <https://reviews.apache.org/r/70581/#comment301933> s/ignore_docker_runtime_isolator/docker_ignore_runtime/ And erasing `docker/runtime` from `isolations` seems a bit weird to me, should we instead add a flag (like `--ignore-docker-image-manifest`) and let `docker/runtime` isolator handle it (i.e., if it is true the isolator will just do nothing)? Or maybe this should be a configuration per container? Like add a field in `ContainerInfo`. - Qian Zhang On May 15, 2019, 2:51 a.m., Jacob Janco wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70581/ > --- > > (Updated May 15, 2019, 2:51 a.m.) > > > Review request for mesos, Gilbert Song and James Peach. > > > Bugs: MESOS-9760 > https://issues.apache.org/jira/browse/MESOS-9760 > > > Repository: mesos > > > Description > --- > > Docker runtime isolator propagates manifest configuration metadata. > This is not always desirable, e.g. a customer may want to ignore the > defined WORKDIR/ENV defined in the manifest. This patch decouples > isolator work from the selection of Docker as an image provider. > > https://issues.apache.org/jira/browse/MESOS-9760 > > > Diffs > - > > docs/configuration/agent.md 04d7114e92c189afd04a8889b3ab853ade9deec6 > docs/upgrades.md ece8462b9950626166c832d3b554ad51f68f42e1 > src/slave/containerizer/mesos/containerizer.cpp > c4a6827c5574586475152b1dc31572d6e1821a82 > src/slave/containerizer/mesos/provisioner/store.cpp > 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a > src/slave/flags.hpp 09921cb6172202b5c1d2f8d03f9ccaeb3d0e8c94 > src/slave/flags.cpp 49a350f9e13409493fa9612c53d9d58b62122371 > src/tests/containerizer/provisioner_docker_tests.cpp > 1c5d72091d570001ad543e342ab0b9128ffc01ae > > > Diff: https://reviews.apache.org/r/70581/diff/4/ > > > Testing > --- > > > Thanks, > > Jacob Janco > >
Re: Review Request 70514: Made nested contaienr can access its sandbox via `MESOS_SANDBOX`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70514/ --- (Updated April 29, 2019, 4:18 p.m.) Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Changes --- Addressed review comments. Bugs: MESOS-9536 https://issues.apache.org/jira/browse/MESOS-9536 Repository: mesos Description --- Previously in MESOS-8332 we narrowed task sandbox permissions from 0755 to 0750 which will cause nested container may not has permission to access its sandbox via the environment variable `MESOS_SANDBOX`. Now in this patch, for nested container which does not have its own rootfs, we bind mount its sandbox to the directory specified via the agent flag `--sandbox_directory` and set `MESOS_SANDBOX` to `--sandbox_directory` as well, in this way such nested container will have the permission to access its sandbox via `MESOS_SANDBOX`. Diffs (updated) - src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7 Diff: https://reviews.apache.org/r/70514/diff/3/ Changes: https://reviews.apache.org/r/70514/diff/2-3/ Testing --- Thanks, Qian Zhang
Review Request 70561: Removed the duplicate pid check in Docker containerizer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70561/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9695 https://issues.apache.org/jira/browse/MESOS-9695 Repository: mesos Description --- Removed the duplicate pid check in Docker containerizer. Diffs - src/slave/containerizer/docker.cpp 7f1d47113045f16b02a92a13ed42d004ecab8f09 Diff: https://reviews.apache.org/r/70561/diff/1/ Testing --- sudo make check. Thanks, Qian Zhang
Re: Review Request 70514: Made nested contaienr can access its sandbox via `MESOS_SANDBOX`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70514/ --- (Updated April 26, 2019, 3:10 p.m.) Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Changes --- Addressed review comments. Bugs: MESOS-9536 https://issues.apache.org/jira/browse/MESOS-9536 Repository: mesos Description (updated) --- Previously in MESOS-8332 we narrowed task sandbox permissions from 0755 to 0750 which will cause nested container may not has permission to access its sandbox via the environment variable `MESOS_SANDBOX`. Now in this patch, for nested container which does not have its own rootfs, we bind mount its sandbox to the directory specified via the agent flag `--sandbox_directory` and set `MESOS_SANDBOX` to `--sandbox_directory` as well, in this way such nested container will have the permission to access its sandbox via `MESOS_SANDBOX`. Diffs (updated) - src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7 Diff: https://reviews.apache.org/r/70514/diff/2/ Changes: https://reviews.apache.org/r/70514/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 70514: Made nested contaienr can access its sandbox via `MESOS_SANDBOX`.
> On April 24, 2019, 7:26 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > > Lines 508 (patched) > > <https://reviews.apache.org/r/70514/diff/1/?file=2140522#file2140522line508> > > > > Seems like this mount point will be on the host fs forever Yes, but we will only do the bind mount in container's mount namespace. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70514/#review214832 --- On April 22, 2019, 9:25 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70514/ > --- > > (Updated April 22, 2019, 9:25 p.m.) > > > Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. > > > Bugs: MESOS-9536 > https://issues.apache.org/jira/browse/MESOS-9536 > > > Repository: mesos > > > Description > --- > > Previously in MESOS-8332 we narrowed task sandbox permissions from 0755 > to 0750 which will cause nested container may not has permission to > access its sandbox via the environment variable `MESOS_SANDBOX`. Now in > this patch, for nested container which has no its own rootfs, we bind > mount its sandbox to the directory specified via the agent flag > `--sandbox_directory` and set `MESOS_SANDBOX` to `--sandbox_directory` > as well, in this way such nested container will have the permission > to access its sandbox via `MESOS_SANDBOX`. > > > Diffs > - > > src/slave/containerizer/mesos/containerizer.cpp > 043244841a73fa3f5f7119bc38f6d3a04be8990b > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7 > > > Diff: https://reviews.apache.org/r/70514/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 70514: Made nested contaienr can access its sandbox via `MESOS_SANDBOX`.
> On April 24, 2019, 7:26 a.m., Gilbert Song wrote: > > src/slave/containerizer/mesos/containerizer.cpp > > Lines 1844 (patched) > > <https://reviews.apache.org/r/70514/diff/1/?file=2140521#file2140521line1844> > > > > s/has no/does not have/g > > > > could we also mention that currently the container new mount namespace > > is not configurable, it bases on whether or not the filesystem/linux > > isolator is turned on. > could we also mention that currently the container new mount namespace is not > configurable, it bases on whether or not the filesystem/linux isolator is > turned on. I think this might not be the right place to mention that, and actually it is not only base on `filesystem/linux`, we `launchInfo.add_clone_namespaces(CLONE_NEWNS);` in a couple of isolators, like: cgroups, Docker volume, CNI, etc. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70514/#review214832 --- On April 22, 2019, 9:25 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70514/ > --- > > (Updated April 22, 2019, 9:25 p.m.) > > > Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. > > > Bugs: MESOS-9536 > https://issues.apache.org/jira/browse/MESOS-9536 > > > Repository: mesos > > > Description > --- > > Previously in MESOS-8332 we narrowed task sandbox permissions from 0755 > to 0750 which will cause nested container may not has permission to > access its sandbox via the environment variable `MESOS_SANDBOX`. Now in > this patch, for nested container which has no its own rootfs, we bind > mount its sandbox to the directory specified via the agent flag > `--sandbox_directory` and set `MESOS_SANDBOX` to `--sandbox_directory` > as well, in this way such nested container will have the permission > to access its sandbox via `MESOS_SANDBOX`. > > > Diffs > - > > src/slave/containerizer/mesos/containerizer.cpp > 043244841a73fa3f5f7119bc38f6d3a04be8990b > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7 > > > Diff: https://reviews.apache.org/r/70514/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Review Request 70515: Added a test to verify non-root nested container can access its sandbox.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70515/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Bugs: MESOS-9536 https://issues.apache.org/jira/browse/MESOS-9536 Repository: mesos Description --- Added a test to verify non-root nested container can access its sandbox. Diffs - src/tests/containerizer/nested_mesos_containerizer_tests.cpp bbf83fa24966a7c9f585b9912fa77bf3460db26f Diff: https://reviews.apache.org/r/70515/diff/1/ Testing --- sudo make check This test will fail without the previous patch (https://reviews.apache.org/r/70514/ ). Thanks, Qian Zhang
Review Request 70514: Made nested contaienr can access its sandbox via `MESOS_SANDBOX`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70514/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Bugs: MESOS-9536 https://issues.apache.org/jira/browse/MESOS-9536 Repository: mesos Description --- Previously in MESOS-8332 we narrowed task sandbox permissions from 0755 to 0750 which will cause nested container may not has permission to access its sandbox via the environment variable `MESOS_SANDBOX`. Now in this patch, for nested container which has no its own rootfs, we bind mount its sandbox to the directory specified via the agent flag `--sandbox_directory` and set `MESOS_SANDBOX` to `--sandbox_directory` as well, in this way such nested container will have the permission to access its sandbox via `MESOS_SANDBOX`. Diffs - src/slave/containerizer/mesos/containerizer.cpp 043244841a73fa3f5f7119bc38f6d3a04be8990b src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 725754f26855ea54ccf8cbcb288ee3b29e8ed4e7 Diff: https://reviews.apache.org/r/70514/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70484: Supported docker manifest v2s2 config with image GC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70484/#review214694 --- Ship it! Ship It! - Qian Zhang On April 16, 2019, 2:08 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70484/ > --- > > (Updated April 16, 2019, 2:08 p.m.) > > > Review request for mesos, Andrei Budnik, Jie Yu, Qian Zhang, and Zhitao Li. > > > Bugs: MESOS-9704 > https://issues.apache.org/jira/browse/MESOS-9704 > > > Repository: mesos > > > Description > --- > > After the docker manifest v2 schema2 support, the manifest config > is saved as a layer in image store. However, the config file > cannot survive if the image GC is triggered. > > This patch supports checkpointing the config path in provisioner, > so that the provisioner has the information of configs so that > they will not be deleted during GC. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp > 53de6253257b37173ccc8b7f90f8aa422ed0430a > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 36b2c7de87bcdfd3e2af6477499572b235ea1933 > src/slave/containerizer/mesos/provisioner/provisioner.cpp > ac402fbead81e4b356cb35cea08a00049002e870 > > > Diff: https://reviews.apache.org/r/70484/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70483: Added docker manifest v2s2 config to 'ImageInfo'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70483/#review214683 --- Fix it, then Ship it! src/slave/containerizer/mesos/provisioner/store.hpp Line 49 (original), 49 (patched) <https://reviews.apache.org/r/70483/#comment300888> This comment seems not accurate, this field should be either Docker v1 image manifest or Docker v2 s2 image configuration. - Qian Zhang On April 16, 2019, 2:07 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70483/ > --- > > (Updated April 16, 2019, 2:07 p.m.) > > > Review request for mesos, Andrei Budnik, Jie Yu, Qian Zhang, and Zhitao Li. > > > Bugs: MESOS-9704 > https://issues.apache.org/jira/browse/MESOS-9704 > > > Repository: mesos > > > Description > --- > > Added docker manifest v2s2 config to 'ImageInfo'. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/store.hpp > a4ae00e1d95f738f1489afbb0fbde9e1d9dafff6 > > > Diff: https://reviews.apache.org/r/70483/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70482: Added docker manifest v2s2 config to protobuf 'ContainerLayers'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70482/#review214691 --- Ship it! Ship It! - Qian Zhang On April 16, 2019, 2:07 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70482/ > --- > > (Updated April 16, 2019, 2:07 p.m.) > > > Review request for mesos, Andrei Budnik, Jie Yu, Qian Zhang, and Zhitao Li. > > > Bugs: MESOS-9704 > https://issues.apache.org/jira/browse/MESOS-9704 > > > Repository: mesos > > > Description > --- > > Added docker manifest v2s2 config to protobuf 'ContainerLayers'. > > > Diffs > - > > include/mesos/slave/containerizer.proto > 8de897ed70e59a4d3244e61c4fb97aecc5ba77b9 > > > Diff: https://reviews.apache.org/r/70482/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70446: Added a test `CurlFetcherPluginTest.CURL_ValidUriWithOutputFileName`.
> On April 11, 2019, 6:46 a.m., Gilbert Song wrote: > > are you going to add this option to the docker plugin? I think we can do it later when it is required. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70446/#review214554 --- On April 10, 2019, 5:32 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70446/ > --- > > (Updated April 10, 2019, 5:32 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9715 > https://issues.apache.org/jira/browse/MESOS-9715 > > > Repository: mesos > > > Description > --- > > Added a test `CurlFetcherPluginTest.CURL_ValidUriWithOutputFileName`. > > > Diffs > - > > src/tests/uri_fetcher_tests.cpp bb224d8b991d13aa7ea7185dd78a99def0c18c7f > > > Diff: https://reviews.apache.org/r/70446/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Qian Zhang > >
Re: Review Request 70398: [WIP] Fixed the URI fetcher image fetch test failure on windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70398/#review214564 --- Ship it! Ship It! - Qian Zhang On April 6, 2019, 5:42 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70398/ > --- > > (Updated April 6, 2019, 5:42 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Fixed the URI fetcher image fetch test failure on windows. > > > Diffs > - > > src/tests/uri_fetcher_tests.cpp f8bacc0bb9e30917f0792d25f172534a0ec27a2c > > > Diff: https://reviews.apache.org/r/70398/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Review Request 70446: Added a test `CurlFetcherPluginTest.CURL_ValidUriWithOutputFileName`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70446/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9715 https://issues.apache.org/jira/browse/MESOS-9715 Repository: mesos Description --- Added a test `CurlFetcherPluginTest.CURL_ValidUriWithOutputFileName`. Diffs - src/tests/uri_fetcher_tests.cpp bb224d8b991d13aa7ea7185dd78a99def0c18c7f Diff: https://reviews.apache.org/r/70446/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 70445: Allowed caller to specify output file name for curl fetcher plugin.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70445/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9715 https://issues.apache.org/jira/browse/MESOS-9715 Repository: mesos Description --- Allowed caller to specify output file name for curl fetcher plugin. Diffs - src/uri/fetchers/curl.cpp 51ac244ac7359d2dab6f8885dc6ba9c7f5a9433c Diff: https://reviews.apache.org/r/70445/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70444: Added an parameter `outputFileName` to the fetcher plugin interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70444/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9715 https://issues.apache.org/jira/browse/MESOS-9715 Repository: mesos Description --- Added an parameter `outputFileName` to the fetcher plugin interface. Diffs - include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b src/uri/fetcher.cpp 3147e41f2d61712d50d8378c750a72790eeb27bb src/uri/fetchers/copy.hpp 90542724a9c0ff26e00bb7c797c8c47c0e48d12e src/uri/fetchers/copy.cpp 4f482ccc7792f8716ec5d54c4cda7357ec3cf6df src/uri/fetchers/curl.hpp 19ade6a684eac47418d3a1a640b4d0cef6c03714 src/uri/fetchers/curl.cpp 51ac244ac7359d2dab6f8885dc6ba9c7f5a9433c src/uri/fetchers/docker.hpp 2abe73586312f53ee510e15eb8ae34959f67 src/uri/fetchers/docker.cpp 631db2d6ecd6e696a322b6353dedda88c2aff7db src/uri/fetchers/hadoop.hpp 2d8d9be46dd37bb84504da9a6e241abfce5a7bdb src/uri/fetchers/hadoop.cpp 1c861e88a88e066436774cbd2c0575ffd7de4dd0 Diff: https://reviews.apache.org/r/70444/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 70443: Added an optional parameter `outputFileName` to the fetcher interface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70443/ --- Review request for mesos, Andrei Budnik and Gilbert Song. Bugs: MESOS-9715 https://issues.apache.org/jira/browse/MESOS-9715 Repository: mesos Description --- Added an optional parameter `outputFileName` to the fetcher interface. Diffs - include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b src/tests/uri_fetcher_tests.cpp bb224d8b991d13aa7ea7185dd78a99def0c18c7f src/uri/fetcher.cpp 3147e41f2d61712d50d8378c750a72790eeb27bb Diff: https://reviews.apache.org/r/70443/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214368 --- Ship it! Ship It! - Qian Zhang On April 4, 2019, 4 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 4, 2019, 4 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/7/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70291: Added gcr registry test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70291/#review214366 --- Ship it! Ship It! - Qian Zhang On April 3, 2019, 3:18 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70291/ > --- > > (Updated April 3, 2019, 3:18 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Added gcr registry test. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > a2a7f11eeb533d7790890988061612e11a798067 > > > Diff: https://reviews.apache.org/r/70291/diff/3/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70290: Fixed docker fetcher plugin unit test for v2s2 change.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70290/#review214365 --- Ship it! Ship It! - Qian Zhang On April 3, 2019, 3:18 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70290/ > --- > > (Updated April 3, 2019, 3:18 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Fixed docker fetcher plugin unit test for v2s2 change. > > > Diffs > - > > src/tests/uri_fetcher_tests.cpp f8bacc0bb9e30917f0792d25f172534a0ec27a2c > > > Diff: https://reviews.apache.org/r/70290/diff/3/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70289: Added a TODO for additional URLs support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70289/#review214364 --- Ship it! Ship It! - Qian Zhang On April 2, 2019, 4:11 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70289/ > --- > > (Updated April 2, 2019, 4:11 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added a TODO for additional URLs support. > > > Diffs > - > > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70289/diff/2/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70365: Added protobuf for docker v2 schema2 config path in 'Image'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70365/#review214363 --- Commit message need to be updated, it is config digest now rather than config path. - Qian Zhang On April 3, 2019, 3:17 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70365/ > --- > > (Updated April 3, 2019, 3:17 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added protobuf for docker v2 schema2 config path in 'Image'. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/message.proto > a55ac9010d5e5b5691ec34b6c342e57811aee4ce > > > Diff: https://reviews.apache.org/r/70365/diff/3/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214334 --- src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 565 (patched) <https://reviews.apache.org/r/70288/#comment300534> This line breaks our image caching functionality. Here the `reference` is **normalized** in the beginning of `RegistryPullerProcess::_pull`, so for some public images (like alpine) in Docker Hub, it will have the `library/` prefix in front of the actual image name and it will be checkpointed in our image cache (i.e., what is save in the cache is `library/alpine` rather than `alpine`). Next time when we try to launch another container with the same image, we will look up `alpine` in the cache and we will not find anything since what is saved in cache is `library/alpine`. This will cause our cache is always missed. - Qian Zhang On April 3, 2019, 4:01 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 4:01 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/6/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214329 --- src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 329-331 (patched) <https://reviews.apache.org/r/70288/#comment300532> We should only do this if `image->has_config_digest()` is true. - Qian Zhang On April 3, 2019, 4:01 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 4:01 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/6/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214324 --- src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 458-462 (patched) <https://reviews.apache.org/r/70288/#comment300529> Do we really need this check? I think even without this check, the `os::rename` in the code below will also give us reasonable error message if `configSource` does not exist. - Qian Zhang On April 3, 2019, 4:01 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 4:01 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/6/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
> On April 2, 2019, 9:18 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > > Lines 548 (patched) > > <https://reviews.apache.org/r/70288/diff/3/?file=2136767#file2136767line548> > > > > Can we use `layerIds` in this for loop instead of `digests`? If so, > > then we do not need the `if (digest == manifest.config().digest())` check > > below (since `layerIds` does not contain config digest) and we actually do > > not need this `digests` parameter for this `pull` method. > > Gilbert Song wrote: > I thought about that, but still prefer `digests`. because: > > `digests` ~ contents that really got downloaded to the staging dir. > `layderIds` ~ from the manifest, some of them are not in the staging dir. > > if we add check to skip those layers not in staging dir, then the rm() > error msg would become meaningless. Agree! - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214264 --- On April 3, 2019, 4:01 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 4:01 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/6/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
> On April 2, 2019, 9:18 p.m., Qian Zhang wrote: > > src/uri/fetchers/docker.cpp > > Line 842 (original), 742 (patched) > > <https://reviews.apache.org/r/70288/diff/3/?file=2136769#file2136769line843> > > > > Is it possible that there is no `Content-Type`? Should we treat no > > `Content-Type` as v2 s1 by default > > Gilbert Song wrote: > I don't think so. > > Also, our old behavior regards that as a failure. I do not think our old behavior regards that as a failure: https://github.com/apache/mesos/blob/1.7.2/src/uri/fetchers/docker.cpp#L682:L704 , if there is not `Content-Type`, we just parse it as v2 s1 manifest by default. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214264 --- On April 3, 2019, 4:01 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 4:01 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/6/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70365: Added protobuf for docker v2 schema2 config path in 'Image'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70365/#review214321 --- Ship it! Ship It! - Qian Zhang On April 3, 2019, 3:17 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70365/ > --- > > (Updated April 3, 2019, 3:17 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added protobuf for docker v2 schema2 config path in 'Image'. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/message.proto > a55ac9010d5e5b5691ec34b6c342e57811aee4ce > > > Diff: https://reviews.apache.org/r/70365/diff/3/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70366: Added a unit test for Mesos containerizer image force pulling.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70366/#review214303 --- Fix it, then Ship it! src/tests/containerizer/provisioner_docker_tests.cpp Lines 830-832 (patched) <https://reviews.apache.org/r/70366/#comment300515> The indent seems not correct. - Qian Zhang On April 2, 2019, 4:12 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70366/ > --- > > (Updated April 2, 2019, 4:12 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added a unit test for Mesos containerizer image force pulling. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > a2a7f11eeb533d7790890988061612e11a798067 > > > Diff: https://reviews.apache.org/r/70366/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214310 --- src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 447-458 (patched) <https://reviews.apache.org/r/70288/#comment300518> We need to check if the config exists in the store before moving it, otherwise the unit test https://reviews.apache.org/r/70366/ will fail: ``` E0402 20:01:52.671329 1856 slave.cpp:6464] Container 'a81b645b-e58a-47ff-b790-70cae2de900a' for executor 'test' of framework 0e53f4ec-517f-4507-b1a5-d66677ee2f01-0001 failed to start: Failed to find docker manifest config at '/opt/qzhang/mesos/store/docker/staging/sGvKLO/sha256:5cb3aa00f89934411ffba5c063a9bc98ace875d8f92e77d0029543d9f2ef4ad0' ``` - Qian Zhang On April 3, 2019, 9:08 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 9:08 a.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/4/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214300 --- src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 404-405 (original), 411-412 (patched) <https://reviews.apache.org/r/70288/#comment300514> I think this comment should be moved up to line 406. - Qian Zhang On April 3, 2019, 9:08 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 3, 2019, 9:08 a.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/4/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
> On March 25, 2019, 3:56 p.m., Qian Zhang wrote: > > src/uri/fetchers/docker.cpp > > Line 1028 (original), 956 (patched) > > <https://reviews.apache.org/r/70288/diff/1/?file=2133764#file2133764line1087> > > > > I think this will make `urlFetchBlob` not working as what we expect. > > The reason is in your implementation we will fetch manifest only once > > (either v2 s1 or v2 s2, so there is only one manifest in staging dir), but > > originally we will fetch manifest twice (v2 s1 for all platforms and v2 s2 > > only for Windows). So now in `urlFetchBlob` we will read and parse the same > > manifest again (because there is only one manifest) which I think is not > > what we want. > > Gilbert Song wrote: > I don't understand. On windows, we will read and parse v2s2 manifest. For > windows images, we should always fetch v2s2. Previous method is not a > graceful solution. > > Anyway, our following patch will address all issues. > I don't understand. On windows, we will read and parse v2s2 manifest. For > windows images, we should always fetch v2s2. Previous method is not a > graceful solution. Agree. But now how we fetch a blob for Windows is, we fetch it based on layer digest first, if it failed, then we fetch it based on layer's foreign urls. I do suggest we fetch blobs based on layer's media type as I mentioned here: https://reviews.apache.org/r/70289/#comment300090 , in that way, we will fetch a blob only once (rather than try one way first and if it fails try another way), and another benefit is we will not have any Windows specific code for fetching blobs. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review213964 --- On April 2, 2019, 4:11 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated April 2, 2019, 4:11 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214264 --- src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 100 (patched) <https://reviews.apache.org/r/70288/#comment300472> s/blobSums/digests/ to make it consistent with the implementation of this method. src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 548 (patched) <https://reviews.apache.org/r/70288/#comment300479> Can we use `layerIds` in this for loop instead of `digests`? If so, then we do not need the `if (digest == manifest.config().digest())` check below (since `layerIds` does not contain config digest) and we actually do not need this `digests` parameter for this `pull` method. src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 559-560 (patched) <https://reviews.apache.org/r/70288/#comment300475> We can merge these two line into a single line. src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 642-643 (patched) <https://reviews.apache.org/r/70288/#comment300478> I would suggest: ``` VLOG(1) << "Fetching layer '" << digest << "' for image '" << reference << "'"; ``` src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 329 (patched) <https://reviews.apache.org/r/70288/#comment300480> If we change the field name from `config_path` to `config_id` as I mentioned in https://reviews.apache.org/r/70365 , then I think here we need to change `!os::exists(image->config_path())` to `!os::exists(paths::getImageLayerPath(storeDir, image->config_path())`. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 458 (patched) <https://reviews.apache.org/r/70288/#comment300482> Again if we change the field name from `config_path` to `config_id`, then here we do not need this `_image` variable, we can just return `image`. src/uri/fetchers/docker.cpp Lines 685-686 (original), 677-678 (patched) <https://reviews.apache.org/r/70288/#comment300483> I do not understand `"because some registries (e.g., quay.io) only support one schema type"`, actually `quay.io` will always return `v1+prettyjws` no matter what media type is specified in the header. I think here we need to mention that the reason we need `v2+json` in the header is if registries drop the s1 support in future, UCR can still pull images with s2 manifest. src/uri/fetchers/docker.cpp Line 842 (original), 742 (patched) <https://reviews.apache.org/r/70288/#comment300484> Is it possible that there is no `Content-Type`? Should we treat no `Content-Type` as v2 s1 by default src/uri/fetchers/docker.cpp Lines 747 (patched) <https://reviews.apache.org/r/70288/#comment300485> I think it is five (including both s1 and s2) rather than three. src/uri/fetchers/docker.cpp Lines 832-835 (patched) <https://reviews.apache.org/r/70288/#comment300489> I think we also need to add config's digest into `digests` since it is also part of fetching blobs. - Qian Zhang On April 2, 2019, 4:11 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > ------- > > (Updated April 2, 2019, 4:11 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70365: Added protobuf for docker v2 schema2 config path in 'Image'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70365/#review214273 --- src/slave/containerizer/mesos/provisioner/docker/message.proto Lines 38 (patched) <https://reviews.apache.org/r/70365/#comment300481> I see in https://reviews.apache.org/r/70288 (the method `RegistryPullerProcess::pull`), what we set into this field is actually the config digest rather than a path, so maybe we should name it `config_id` which is also consistent with the above `layer_ids` field. - Qian Zhang On April 2, 2019, 4:10 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70365/ > --- > > (Updated April 2, 2019, 4:10 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added protobuf for docker v2 schema2 config path in 'Image'. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/message.proto > a55ac9010d5e5b5691ec34b6c342e57811aee4ce > > > Diff: https://reviews.apache.org/r/70365/diff/2/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70354: Refactored the UCR docker store to construct 'Image' proto at pullers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70354/#review214252 --- Fix it, then Ship it! src/slave/containerizer/mesos/provisioner/docker/image_tar_puller.cpp Lines 298 (patched) <https://reviews.apache.org/r/70354/#comment300463> s/dockerImage/image/ src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Line 429 (original), 429 (patched) <https://reviews.apache.org/r/70354/#comment300466> Ditto. src/tests/containerizer/provisioner_docker_tests.cpp Line 328 (original), 329 (patched) <https://reviews.apache.org/r/70354/#comment300469> Comment (`return list`) need to be updated? - Qian Zhang On April 2, 2019, 9:21 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70354/ > --- > > (Updated April 2, 2019, 9:21 a.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-9694 > https://issues.apache.org/jira/browse/MESOS-9694 > > > Repository: mesos > > > Description > --- > > This refactoring is needed for supporting docker manifest v2s2 because > the puller has to let the docker store knows the v1 config, and this > is also needed for garbage collecting the v1 config in the image layer > store. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/image_tar_puller.hpp > b5ec924b6c54fa507965248b6cb06513b0c5c256 > src/slave/containerizer/mesos/provisioner/docker/image_tar_puller.cpp > 3a33388f19b8c6a949661ed9f576cb4ca694302b > src/slave/containerizer/mesos/provisioner/docker/message.proto > a55ac9010d5e5b5691ec34b6c342e57811aee4ce > src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp > cfafd442af3b164050257ff945226db2596555f1 > src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp > 847eb9a74c353fc0561ed29f12b9868a8010940d > src/slave/containerizer/mesos/provisioner/docker/puller.hpp > a79f2ad107aeb3ebfda22e80a85f83ac9685fa85 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp > 926fab19b6de7e9b3625ee5aa483817c804457c1 > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 60507aa1b7951666ed758d1b3800eddd67ba7be6 > src/tests/containerizer/provisioner_docker_tests.cpp > a2a7f11eeb533d7790890988061612e11a798067 > > > Diff: https://reviews.apache.org/r/70354/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review214032 --- I think we need unit test to verify the behavior of v2 s2 image with force pulling enabled and disabled. src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 545 (patched) <https://reviews.apache.org/r/70288/#comment300199> We should not do this if `manifest.config()` is already in the storeDir, otherwise launching a container with an image which is already pulled and with force pulling enabled will fail: ``` Failed to launch container: Failed to move the layer manifest from '/opt/qzhang/mesos/store/docker/staging/kOP3Sb/sha256:d8233ab899d419c58cf3634c0df54ff5d8acc28f8173f09c21df4a07229e1205' to '/opt/qzhang/mesos/store/docker/staging/kOP3Sb/sha256:697743189b6d255069caf6c455be10c7f8cae8076c6f94d224ae15cd41420e87/json': No such file or directory ``` src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp Lines 633 (patched) <https://reviews.apache.org/r/70288/#comment300198> As I mentioned in the comment below, we should treat fetching `manifest->config()` as part of fetching blobs rather than part of fetching manifest, that means here we need to add `manifest->config().digest()` into `digests` if it is not already in the storeDir. src/uri/fetchers/docker.cpp Line 931 (original), 819-822 (patched) <https://reviews.apache.org/r/70288/#comment300197> So here we always download `manifest->config()` regardless `uri.scheme()`. But in the case that an image has been pulled to launch a container and then we use that image to launch another container, we will download `manifest->config()` again even it is already in storeDir, that's a duplicated download to me. Basically I think we should treat fetching `manifest->config()` as part of fetching blobs rather than part of fetching manifest. - Qian Zhang On March 24, 2019, 12:33 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated March 24, 2019, 12:33 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/tests/containerizer/provisioner_docker_tests.cpp > a2a7f11eeb533d7790890988061612e11a798067 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70288: Supported docker manifest v2 schema2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70288/#review213973 --- src/uri/fetchers/docker.cpp Lines 828 (patched) <https://reviews.apache.org/r/70288/#comment300108> Can we add a TODO somewhere for verifying the digest after the blob is fetched? - Qian Zhang On March 24, 2019, 12:33 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70288/ > --- > > (Updated March 24, 2019, 12:33 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Supported docker manifest v2 schema2. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 > src/tests/containerizer/provisioner_docker_tests.cpp > a2a7f11eeb533d7790890988061612e11a798067 > src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc > > > Diff: https://reviews.apache.org/r/70288/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70291: Added gcr registry test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70291/#review213970 --- src/tests/containerizer/provisioner_docker_tests.cpp Lines 634-638 (original), 634-639 (patched) <https://reviews.apache.org/r/70291/#comment300098> I think we should use `alpine` (e.g., gcr.io/google-containers/alpine-with-bash) since the prefix here is `ImageAlpine`. - Qian Zhang On March 24, 2019, 12:34 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70291/ > --- > > (Updated March 24, 2019, 12:34 p.m.) > > > Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6934 > https://issues.apache.org/jira/browse/MESOS-6934 > > > Repository: mesos > > > Description > --- > > Added gcr registry test. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > a2a7f11eeb533d7790890988061612e11a798067 > > > Diff: https://reviews.apache.org/r/70291/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
