Re: Review Request 71197: Updated the `disk/du` disk isolator tests with rootfs cases.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71197/#review217099
---



Bad patch!

Reviews applied: [71192, 71193, 71194, 71195, 71196, 71197]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71197"]

Error:
..
16 master.cpp:3951] Authorizing principal 'test-principal' to create volumes 
'[{"disk":{"persistence":{"id":"id1","principal":"test-principal"},"volume":{"container_path":"path1","mode":"RW"}},"name":"disk","reservations":[{"role":"role1","type":"STATIC"}],"scalar":{"value":64.0},"type":"SCALAR"}]'
I0806 02:52:53.281088 18922 master.cpp:12576] Sending operation '' (uuid: 
fa068103-3d1b-4d71-9e44-1c4dba232bf0) to agent 
c4e72597-cf7f-492a-8ce8-058650ddecc3-S0 at slave(415)@172.17.0.2:46205 
(98b2a936be6f)
I0806 02:52:53.282253 18922 slave.cpp:4430] Updated checkpointed operations 
from [  ] to [ fa068103-3d1b-4d71-9e44-1c4dba232bf0 (CREATE, latest state: 
OPERATION_PENDING) ]
I0806 02:52:53.282691 18922 slave.cpp:8870] Updating the state of operation 
with no ID (uuid: fa068103-3d1b-4d71-9e44-1c4dba232bf0) for an operation API 
call (latest state: OPERATION_FINISHED, status update state: OPERATION_FINISHED)
W0806 02:52:53.282994 18899 process.cpp:2877] Attempted to spawn already 
running process version@172.17.0.2:46205
I0806 02:52:53.284008 18899 sched.cpp:239] Version: 1.9.0
I0806 02:52:53.284965 18907 sched.cpp:343] New master detected at 
master@172.17.0.2:46205
I0806 02:52:53.285145 18907 sched.cpp:408] Authenticating with master 
master@172.17.0.2:46205
I0806 02:52:53.285179 18907 sched.cpp:415] Using default CRAM-MD5 authenticatee
I0806 02:52:53.285676 18916 authenticatee.cpp:121] Creating new client SASL 
connection
I0806 02:52:53.286075 18912 master.cpp:10578] Authenticating 
scheduler-946593eb-1581-411d-8602-db7717a132a5@172.17.0.2:46205
I0806 02:52:53.286245 18904 authenticator.cpp:414] Starting authentication 
session for crammd5-authenticatee(863)@172.17.0.2:46205
I0806 02:52:53.286636 18909 authenticator.cpp:98] Creating new server SASL 
connection
I0806 02:52:53.286897 18919 authenticatee.cpp:213] Received SASL authentication 
mechanisms: CRAM-MD5
I0806 02:52:53.286931 18919 authenticatee.cpp:239] Attempting to authenticate 
with mechanism 'CRAM-MD5'
I0806 02:52:53.287062 18918 authenticator.cpp:204] Received SASL authentication 
start
I0806 02:52:53.287140 18918 authenticator.cpp:326] Authentication requires more 
steps
I0806 02:52:53.287251 18918 authenticatee.cpp:259] Received SASL authentication 
step
I0806 02:52:53.287394 18918 authenticator.cpp:232] Received SASL authentication 
step
I0806 02:52:53.287426 18918 auxprop.cpp:109] Request to lookup properties for 
user: 'test-principal' realm: '98b2a936be6f' server FQDN: '98b2a936be6f' 
SASL_AUXPROP_VERIFY_AGAINST_HASH: false SASL_AUXPROP_OVERRIDE: false 
SASL_AUXPROP_AUTHZID: false 
I0806 02:52:53.287441 18918 auxprop.cpp:181] Looking up auxiliary property 
'*userPassword'
I0806 02:52:53.287474 18918 auxprop.cpp:181] Looking up auxiliary property 
'*cmusaslsecretCRAM-MD5'
I0806 02:52:53.287498 18918 auxprop.cpp:109] Request to lookup properties for 
user: 'test-principal' realm: '98b2a936be6f' server FQDN: '98b2a936be6f' 
SASL_AUXPROP_VERIFY_AGAINST_HASH: false SASL_AUXPROP_OVERRIDE: false 
SASL_AUXPROP_AUTHZID: true 
I0806 02:52:53.287510 18918 auxprop.cpp:131] Skipping auxiliary property 
'*userPassword' since SASL_AUXPROP_AUTHZID == true
I0806 02:52:53.287519 18918 auxprop.cpp:131] Skipping auxiliary property 
'*cmusaslsecretCRAM-MD5' since SASL_AUXPROP_AUTHZID == true
I0806 02:52:53.287534 18918 authenticator.cpp:318] Authentication success
I0806 02:52:53.287663 18911 authenticatee.cpp:299] Authentication success
I0806 02:52:53.287714 18921 master.cpp:10610] Successfully authenticated 
principal 'test-principal' at 
scheduler-946593eb-1581-411d-8602-db7717a132a5@172.17.0.2:46205
I0806 02:52:53.287765 18906 authenticator.cpp:432] Authentication session 
cleanup for crammd5-authenticatee(863)@172.17.0.2:46205
I0806 02:52:53.288120 18900 sched.cpp:520] Successfully authenticated with 
master master@172.17.0.2:46205
I0806 02:52:53.288168 18900 sched.cpp:835] Sending SUBSCRIBE call to 
master@172.17.0.2:46205
I0806 02:52:53.288414 18900 sched.cpp:870] Will retry registration in 
1.154788978secs if necessary
I0806 02:52:53.288729 18920 master.cpp:2908] Received SUBSCRIBE call for 
framework 'default' at 
scheduler-946593eb-1581-411d-8602-db7717a132a5@172.17.0.2:46205
I0806 02:52:53.288723 18922 slave.cpp:4422] Updated checkpointed resources from 
{} to disk(reservations: [(STATIC,role1)])[id1:path1]:64
I0806 02:52:53.288805 18920 

Re: Review Request 71235: Added a test `ROOT_VETH_VerifyNestedContainerIPAfterReboot`.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71235/#review217095
---


Ship it!




Ship It!

- Gilbert Song


On Aug. 3, 2019, 12:24 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71235/
> ---
> 
> (Updated Aug. 3, 2019, 12:24 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-9868
> https://issues.apache.org/jira/browse/MESOS-9868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ROOT_VETH_VerifyNestedContainerIPAfterReboot`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 0fc664b968485ca96444e186e58190eadc660830 
> 
> 
> Diff: https://reviews.apache.org/r/71235/diff/1/
> 
> 
> Testing
> ---
> 
> Ran this test repeatedly and all the iterations succeeded.
> 
> This test would fail without the previous patch 
> https://reviews.apache.org/r/71172/ .
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71172: Looked up parent's IP for the non-checkpointed nested containers.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71172/#review217094
---


Ship it!




Ship It!

- Gilbert Song


On July 29, 2019, 1:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71172/
> ---
> 
> (Updated July 29, 2019, 1:40 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-9868
> https://issues.apache.org/jira/browse/MESOS-9868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Looked up parent's IP for the non-checkpointed nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f2989cf7a2161154bb7d9bf2112bee8dd3cc5cf5 
> 
> 
> Diff: https://reviews.apache.org/r/71172/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71219: Updated maintenance docs to include agent draining.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71219/#review217092
---


Ship it!




LGTM!

- Joseph Wu


On July 31, 2019, 8:16 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71219/
> ---
> 
> (Updated July 31, 2019, 8:16 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-9845
> https://issues.apache.org/jira/browse/MESOS-9845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated maintenance docs to include agent draining.
> 
> 
> Diffs
> -
> 
>   docs/maintenance.md bec69e0640c19c610787c5e25bf53cad197ff8d1 
>   docs/operator-http-api.md ddf29e9e29dcf9d0801118382402be5945d8117d 
> 
> 
> Diff: https://reviews.apache.org/r/71219/diff/1/
> 
> 
> Testing
> ---
> 
> Viewed using the website Docker container.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71193: Supported multiple quota paths in the `disk/xfs` isolator.

[James Peach]

> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 570 (original), 566 (patched)
> > 
> >
> > You are not changing the logic here but could you remind me why this 
> > error doesn't fail the update?
> 
> James Peach wrote:
> I think that we are just trying to be robust against host problems. This 
> would only fail if there was a serious disk error, so we just try to maintain 
> our internal consistency.
> 
> Jiang Yan Xu wrote:
> If it's btween continuing without being able to enforce quota and failing 
> task + inititating clean up, I'd say the latter is better protecting 
> consistency?

If we change the semantics of this, it should be in a separate review with some 
analysis. Let's not mix our concerns.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
---


On Aug. 5, 2019, 9:17 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> ---
> 
> (Updated Aug. 5, 2019, 9:17 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
> 
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 94d44e7f39dc01037461015b499a1fc3169b24e8 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 646330c65b24aa28801ec99d7909db08a3e05c79 
> 
> 
> Diff: https://reviews.apache.org/r/71193/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71151/#review217087
---



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 5, 2019, 8:15 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 5, 2019, 8:15 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71196: Updated the `disk/du` isolator to support rootfs checks.

[James Peach]

> On Aug. 5, 2019, 3:30 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp
> > Lines 271 (patched)
> > 
> >
> > missing `&`:
> > ```
> > const Resources& quota = quotas[info->sandbox];
> > ```

This is deliberate, since we are mutating the hashmap. The spec is for 
references to elements to not be invalidated, but I think that's a little 
non-obvious.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71196/#review217077
---


On Aug. 2, 2019, 1:40 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71196/
> ---
> 
> (Updated Aug. 2, 2019, 1:40 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the `disk/du` isolator to support isolating rootfs ephemeral
> volumes. We need to keep track of the ephemeral paths and start a `du`
> check for each one of them, but otherwise treat them in the same way
> as the sandbox path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> f513dfc55a6e680ca451586015f98f91e0b6abc5 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> 4869c72f77ea15978496f66997c63b07276d85f0 
> 
> 
> Diff: https://reviews.apache.org/r/71196/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71151/
---

(Updated Aug. 5, 2019, 10:15 p.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Rebase


Bugs: MESOS-9254
https://issues.apache.org/jira/browse/MESOS-9254


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


Diff: https://reviews.apache.org/r/71151/diff/3/

Changes: https://reviews.apache.org/r/71151/diff/2-3/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71150: Factored out storage provider method to update resources.

[Benjamin Bannier]

> On Aug. 2, 2019, 9:24 a.m., Chun-Hung Hsiao wrote:
> > I'm not sure if this is

Could you rephrase that?


> On Aug. 2, 2019, 9:24 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 730 (patched)
> > 
> >
> > Remove this debug log.

Moved the removal in https://reviews.apache.org/r/71151/ here.


> On Aug. 2, 2019, 9:24 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 735 (patched)
> > 
> >
> > Remove this debug log.

Moved the removal in https://reviews.apache.org/r/71151/ here.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71150/#review216947
---


On July 23, 2019, 10:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71150/
> ---
> 
> (Updated July 23, 2019, 10:18 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Factored out storage provider method to update resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
> 
> 
> Diff: https://reviews.apache.org/r/71150/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71193: Supported multiple quota paths in the `disk/xfs` isolator.

[Jiang Yan Xu]

> On Aug. 2, 2019, 3:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 570 (original), 566 (patched)
> > 
> >
> > You are not changing the logic here but could you remind me why this 
> > error doesn't fail the update?
> 
> James Peach wrote:
> I think that we are just trying to be robust against host problems. This 
> would only fail if there was a serious disk error, so we just try to maintain 
> our internal consistency.

If it's btween continuing without being able to enforce quota and failing task 
+ inititating clean up, I'd say the latter is better protecting consistency?


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
---


On Aug. 5, 2019, 2:17 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> ---
> 
> (Updated Aug. 5, 2019, 2:17 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
> 
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 94d44e7f39dc01037461015b499a1fc3169b24e8 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 646330c65b24aa28801ec99d7909db08a3e05c79 
> 
> 
> Diff: https://reviews.apache.org/r/71193/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71195: Update `disk/xfs` tests for rootfs quotas.

[Andrei Budnik]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71195/#review217081
---


Fix it, then Ship it!





src/tests/containerizer/xfs_quota_tests.cpp
Lines 1839 (patched)


```
ASSERT_SOME(dirs);
ASSERT_FALSE(dirs->empty())
```


- Andrei Budnik


On Авг. 1, 2019, 1:11 д.п., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71195/
> ---
> 
> (Updated Авг. 1, 2019, 1:11 д.п.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Parameterize the `disk/xfs` isolator tests on whether we
> are enforcing quotas on the sandbox or on the container
> rootfs. Separating out the rootfs and sandbox cases wth
> parameterization helps to ensure that quota is correctly
> applied in both cases and tests don't accidentally pass.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.cpp 206cab69f329e714ffb65681be36ba190bf272fe 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 96cd40d2fa05664571b04ffe08482c584d2935ba 
>   src/tests/mesos.hpp cd50673f7e4f05539137abda73490d934a9f6dc4 
>   src/tests/mesos.cpp df363abbfa5717f1748511d7fe155493c35fd17b 
> 
> 
> Diff: https://reviews.apache.org/r/71195/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71213: Updated operator HTTP API documentation regarding `UPDATE_QUOTA`.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71213/#review217079
---


Fix it, then Ship it!





docs/operator-http-api.md
Line 2457 (original), 2457 (patched)


to be clear it's one or more, how about: "the quota for the specified 
role(s)"



docs/operator-http-api.md
Line 2470 (original), 2473 (patched)


can you keep the space after the : character for all these lines?



docs/operator-http-api.md
Lines 2476-2480 (original), 2478-2493 (patched)


Seems more readable like the following?

```
  "guarantees": {
"cpus": { "value": 1.0 },
"mem":  { "value": 1024.0}
  },
  "limits": {
"cpus": { "value": 2.0 },
"mem":  { "value": 2048.0 }
  }
```

(note I added back the spaces after the : characters)


- Benjamin Mahler


On Aug. 1, 2019, 1:02 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71213/
> ---
> 
> (Updated Aug. 1, 2019, 1:02 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The deprecated calls `SET_QUOTA` and `REMOVE_QUOTA` are
> now hidden.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md ddf29e9e29dcf9d0801118382402be5945d8117d 
> 
> 
> Diff: https://reviews.apache.org/r/71213/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71230: Moved the sorter code under the `master/allocator/mesos/`.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71230/#review217078
---


Ship it!




Ship It!

- Benjamin Mahler


On Aug. 2, 2019, 12:30 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71230/
> ---
> 
> (Updated Aug. 2, 2019, 12:30 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The sorter interface is designed and used by the built-in
> hierarchical allocator. It is not intended to be used
> by other allocators. This patch moves it under the built-in
> mesos allocator folder for structural clarity.
> 
> 
> Diffs
> -
> 
>   docs/allocation-module.md 81f9d0c294eeb23f475c787da9f53f9c219bf6c3 
>   src/CMakeLists.txt e0200eb5e685cdac543defc8cda40b7452340c80 
>   src/Makefile.am d27d4b697cef034b44f89c198158a794ee52de4d 
>   src/local/local.cpp 68dc9fb0331e070ee10aee62e78381d76a52b7b0 
>   src/master/allocator/mesos/hierarchical.hpp 
> d2658bc8997202b17bac0f765bd4f522ebee350e 
>   src/master/allocator/sorter/drf/metrics.hpp  
>   src/master/allocator/sorter/drf/metrics.cpp 
> ece151c79b1ba7728b7ce338e74d2c682a1ef4f2 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 84c33ca49c5288d221f82eeb968d640461867da7 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 9367469132e426f0b4b66a80ad300c157fba6bf2 
>   src/master/allocator/sorter/random/sorter.hpp 
> ca687d4590387d5d788919f81fe32b43baba6b3c 
>   src/master/allocator/sorter/random/sorter.cpp 
> 9899cfd570607a60dbd7980d340a8e7d9d3e6df5 
>   src/master/allocator/sorter/random/utils.hpp  
>   src/master/allocator/sorter/sorter.hpp  
>   src/tests/sorter_tests.cpp 5cd2a64cb8c06f0d27c04bc0f40d347e6e61095e 
> 
> 
> Diff: https://reviews.apache.org/r/71230/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71196: Updated the `disk/du` isolator to support rootfs checks.

[Andrei Budnik]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71196/#review217077
---


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/posix/disk.cpp
Lines 271 (patched)


missing `&`:
```
const Resources& quota = quotas[info->sandbox];
```


- Andrei Budnik


On Авг. 2, 2019, 1:40 д.п., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71196/
> ---
> 
> (Updated Авг. 2, 2019, 1:40 д.п.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the `disk/du` isolator to support isolating rootfs ephemeral
> volumes. We need to keep track of the ephemeral paths and start a `du`
> check for each one of them, but otherwise treat them in the same way
> as the sandbox path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> f513dfc55a6e680ca451586015f98f91e0b6abc5 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> 4869c72f77ea15978496f66997c63b07276d85f0 
> 
> 
> Diff: https://reviews.apache.org/r/71196/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71197: Updated the `disk/du` disk isolator tests with rootfs cases.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71197/#review217074
---



Patch looks great!

Reviews applied: [71192, 71193, 71194, 71195, 71196, 71197]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 2, 2019, 1:40 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71197/
> ---
> 
> (Updated Aug. 2, 2019, 1:40 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the disk enforcement type parameter to the `disk/du` disk
> isolator tests to verify rootfs ephemeral storage quota handling.
> 
> 
> Diffs
> -
> 
>   src/tests/disk_quota_tests.cpp cbb1ccff19ed9032298b40164e263ab3a0b0744d 
> 
> 
> Diff: https://reviews.apache.org/r/71197/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71192: Propagate ephemeral volume information from rootfs.

[James Peach]

> On Aug. 2, 2019, 9:02 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/provisioner/backend.hpp
> > Lines 55 (patched)
> > 
> >
> > The backend abstraction probably shouldn't care about quota so probably 
> > phrasing it as "additional paths the backend has created" or something?

I'd prefer to leave this since it's more explicit and the reader has less work 
to find out why this is here.


> On Aug. 2, 2019, 9:02 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp
> > Lines 243 (patched)
> > 
> >
> > Nit: with initializers I don't think we wrap elements with spaces.

Fixed.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71192/#review217057
---


On Aug. 1, 2019, 1:03 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71192/
> ---
> 
> (Updated Aug. 1, 2019, 1:03 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate the overlayfs upperdir path to the containerization
> layer in a general form as a set of ephemeral volume paths.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> a60c96302a6cec90ecd0a0885b844fff8d37db71 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a01edc8793a2eaa655f1729a01a01f1f61fbf7cb 
>   src/slave/containerizer/mesos/provisioner/backend.hpp 
> 7257d3a962ecdf87fe9d52facbd6a2619311a018 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> 2c2518775d2bcb3b1857775723828c4ac08111ca 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> 2eba55228e7596c170dddea2a9930ba6cf73404f 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> c4a1d5f35710f6eab13938333ed4cb97d9ff2f6b 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 4cc4c520aa6872c05f474bc7837a353b43983e0b 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 5dc9a2fd38cdfafda02129b0d144eee3230a1ff2 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> 10516caab82d910944814b80e0a1d9aeba19007c 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 362e02172d2fd8e6e241fb6f5689f569ba74a0d1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> f2040cf36c601a13281a78ff844ebd41000a2d65 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 7f84aa499b21140eb29ef7f81e2608743b12eb75 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> a081fb059f9e0788c2153d1e04eda37fe514a1f7 
> 
> 
> Diff: https://reviews.apache.org/r/71192/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71193: Supported multiple quota paths in the `disk/xfs` isolator.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/
---

(Updated Aug. 5, 2019, 9:17 a.m.)


Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.


Changes
---

Address review feedback.


Bugs: MESOS-9900
https://issues.apache.org/jira/browse/MESOS-9900


Repository: mesos


Description
---

The `disk/xfs` isolator assumed that there would only be a single
directory path for each project quota. When we apply project quotas
to the overlayfs upperdir, that won't be true any more, since the
upperdir will come under the same quota as the task sandbox.

Update the quota reclamation tracking to keep a set of disk paths that
the quota has been applied to, and only reclaim the project ID once all
those paths have been removed.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
94d44e7f39dc01037461015b499a1fc3169b24e8 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
646330c65b24aa28801ec99d7909db08a3e05c79 


Diff: https://reviews.apache.org/r/71193/diff/4/

Changes: https://reviews.apache.org/r/71193/diff/3-4/


Testing
---

sudo make check (Frdora 30)


Thanks,

James Peach

Re: Review Request 71193: Supported multiple quota paths in the `disk/xfs` isolator.

[James Peach]

> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 570 (original), 566 (patched)
> > 
> >
> > You are not changing the logic here but could you remind me why this 
> > error doesn't fail the update?

I think that we are just trying to be robust against host problems. This would 
only fail if there was a serious disk error, so we just try to maintain our 
internal consistency.


> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 776 (patched)
> > 
> >
> > For `hashmap` there's already a `contains` defined.

I did this to avoid doing multiple hash lookups, but I agree that using 
`contains` improves the readability.


> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 778-779 (patched)
> > 
> >
> > You could've used a `else if` followed by an `else`  to avoid another 
> > level of nesting.

Aftew switching to `contains`, I think that this makes the most sense as is.


> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 807 (original)
> > 
> >
> > We could move the logging about the `dir` up above `erase` if we still 
> > want to log it (maybe for `VLOG(1)`)?

Yeh, I think adding a `VLOG` makes sense here.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
---


On July 30, 2019, 7:52 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> ---
> 
> (Updated July 30, 2019, 7:52 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
> 
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 94d44e7f39dc01037461015b499a1fc3169b24e8 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 646330c65b24aa28801ec99d7909db08a3e05c79 
> 
> 
> Diff: https://reviews.apache.org/r/71193/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>