Re: Review Request 69787: Added a proxy mode to the test CSI plugin.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69787 was successfully built and tested.

Reviews applied: `['69787']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2799/mesos-review-69787

- Mesos Reviewbot Windows


On Jan. 18, 2019, 5:46 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69787/
> ---
> 
> (Updated Jan. 18, 2019, 5:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9517
> https://issues.apache.org/jira/browse/MESOS-9517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `--proxy` flag is set, the test CSI plugin would forward all gRPC
> requests to the specified gRPC server URI, and return the response to
> the caller. This can be used to forward all CSI calls to a mock CSI
> plugin object in the test process.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp af183037b280bab65578a4c447196a9ccf261e32 
> 
> 
> Diff: https://reviews.apache.org/r/69787/diff/1/
> 
> 
> Testing
> ---
> 
> Manually tweaked the plugin to forward requests to itself and all tests pass.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 69787: Added a proxy mode to the test CSI plugin.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

If the `--proxy` flag is set, the test CSI plugin would forward all gRPC
requests to the specified gRPC server URI, and return the response to
the caller. This can be used to forward all CSI calls to a mock CSI
plugin object in the test process.


Diffs
-

  src/examples/test_csi_plugin.cpp af183037b280bab65578a4c447196a9ccf261e32 


Diff: https://reviews.apache.org/r/69787/diff/1/


Testing
---

Manually tweaked the plugin to forward requests to itself and all tests pass.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69158: Added an integration test for resource provider removal.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp
Lines 10721 (patched)


This is not necessary.



src/tests/slave_tests.cpp
Lines 10792-10794 (patched)


Nit: indent by extra 2 spaces. I'd suggest that we don't break the line 
after `if (`, or break it after the first &&:
```
if (resource.has_provider_id() &&
resource.has_disk() &&
...) {
  rawDisk = resources;
  break;
}
```
So the condition and body are distinguishable.



src/tests/slave_tests.cpp
Lines 10802 (patched)


The term "inject" make me think that this operation is injected through an 
internal mechanism. How about just say "Create a pending operation."?



src/tests/slave_tests.cpp
Lines 10863 (patched)


an operation



src/tests/slave_tests.cpp
Lines 10880 (patched)


there is no resource managed


- Chun-Hung Hsiao


On Jan. 17, 2019, 11:17 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Jan. 17, 2019, 11:17 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an integration test for resource provider removal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/7/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69786: Added agent capability for operation feedback on default resources.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69786 was successfully built and tested.

Reviews applied: `['69786']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2798/mesos-review-69786

- Mesos Reviewbot Windows


On Jan. 18, 2019, 2:30 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69786/
> ---
> 
> (Updated Jan. 18, 2019, 2:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Gastón Kleiman.
> 
> 
> Bugs: MESOS-9525
> https://issues.apache.org/jira/browse/MESOS-9525
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent capability for operation feedback on default resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 2ef6ba3aef67cf34227569948fd3ddc8dfd5879d 
>   include/mesos/v1/mesos.proto 1a701da65f653fe4191f92ff1fb1436809b50acb 
>   src/common/protobuf_utils.hpp f5294710cbdbcca34a6468ad418938f2fe4e3d15 
>   src/common/protobuf_utils.cpp a0159fed8d325808c5e8519da06173441deb 
>   src/slave/constants.cpp 103384c1d75792357623f55fca03f0d798305339 
>   src/slave/flags.cpp 6bac8e1409f04d639204c45eda8a90c098e3dbd0 
>   src/tests/master_tests.cpp 1f8da63aa313c4679c0d7a3934802af1474e8f28 
>   src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 
> 
> 
> Diff: https://reviews.apache.org/r/69786/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Chun-Hung Hsiao]

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




src/tests/storage_local_resource_provider_tests.cpp
Lines 3330 (patched)


Is this really required?


- Chun-Hung Hsiao


On Jan. 17, 2019, 11:15 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69781/
> ---
> 
> (Updated Jan. 17, 2019, 11:15 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we addressed one source of flakiness of this test in
> `2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
> flakiness (agents send more than the expected number of
> `UpdateSlaveMessage`s since they failed a timeout in registration with
> the master).
> 
> This patch ensures that the agent registers successfully before
> continuing with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 164e93a3749d4d668e12de31641aecece384 
> 
> 
> Diff: https://reviews.apache.org/r/69781/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68147: Added agent support to remove local resource providers.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Jan. 17, 2019, 11:17 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68147/
> ---
> 
> (Updated Jan. 17, 2019, 11:17 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for triggering permanent removal of local
> resource providers. We also add authorization and tests as part of this
> patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
>   src/common/protobuf_utils.cpp a0159fed8d325808c5e8519da06173441deb 
>   src/master/master.cpp 7c8d3ce3271fd1d6b8e6e85e996f96572a7d48a6 
>   src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
>   src/slave/http.cpp e0dc09103e9186aa3f7328d2052fc6747f5be9bb 
>   src/slave/slave.hpp 2eadf5fce9a314f1ec0ac5d51820c6381f5f1468 
>   src/slave/slave.cpp 10cbc190acc7eea5734efa0066541168545a66b6 
>   src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
>   src/tests/api_tests.cpp 76ec56b530ed259b8e2cb7794074b6b72cec86b3 
> 
> 
> Diff: https://reviews.apache.org/r/68147/diff/15/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Chun-Hung Hsiao]

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



Ah... thanks for fixing this. Actually when I reviewed r/69687 I did have a 
concern about expecting two `UpdateSlaveMessage`s without a paused clock but 
then forgot about the concern and gave a ship-it :(


src/tests/storage_local_resource_provider_tests.cpp
Line 3319 (original), 3319 (patched)


How about moving this to the very beginning and pause the clock for the 
whole test?



src/tests/storage_local_resource_provider_tests.cpp
Lines 3340 (patched)


Could it be possible that the second `UpdateSlaveMessage` has been received 
before this `FUTURE_PROTOBUF` is set up?


- Chun-Hung Hsiao


On Jan. 17, 2019, 11:15 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69781/
> ---
> 
> (Updated Jan. 17, 2019, 11:15 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we addressed one source of flakiness of this test in
> `2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
> flakiness (agents send more than the expected number of
> `UpdateSlaveMessage`s since they failed a timeout in registration with
> the master).
> 
> This patch ensures that the agent registers successfully before
> continuing with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 164e93a3749d4d668e12de31641aecece384 
> 
> 
> Diff: https://reviews.apache.org/r/69781/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69782: Moved logging of resource provider HTTP actions before authorization.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Jan. 17, 2019, 11:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69782/
> ---
> 
> (Updated Jan. 17, 2019, 11:16 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved logging of resource provider HTTP actions before authorization.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp e0dc09103e9186aa3f7328d2052fc6747f5be9bb 
> 
> 
> Diff: https://reviews.apache.org/r/69782/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 69786: Added agent capability for operation feedback on default resources.

[Greg Mann]

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

Review request for mesos, Benno Evers and Gastón Kleiman.


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


Repository: mesos


Description
---

Added agent capability for operation feedback on default resources.


Diffs
-

  include/mesos/mesos.proto 2ef6ba3aef67cf34227569948fd3ddc8dfd5879d 
  include/mesos/v1/mesos.proto 1a701da65f653fe4191f92ff1fb1436809b50acb 
  src/common/protobuf_utils.hpp f5294710cbdbcca34a6468ad418938f2fe4e3d15 
  src/common/protobuf_utils.cpp a0159fed8d325808c5e8519da06173441deb 
  src/slave/constants.cpp 103384c1d75792357623f55fca03f0d798305339 
  src/slave/flags.cpp 6bac8e1409f04d639204c45eda8a90c098e3dbd0 
  src/tests/master_tests.cpp 1f8da63aa313c4679c0d7a3934802af1474e8f28 
  src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 


Diff: https://reviews.apache.org/r/69786/diff/1/


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 69723: Enabled operation feedback on agent default resources.

[Greg Mann]

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



This patch LGTM, modulo the note below regarding the extra capability check, 
once we have that in place. Thanks Benno!!


src/master/master.cpp
Lines 4367-4374 (original), 4359-4366 (patched)


As we discussed elsewhere, we need to perform a similar check for the case 
where `(!slave->capabilities.agentOperationFeedback && 
!operation.has_resource_provider_id())` once we have the new capability in 
place.



src/tests/api_tests.cpp
Lines 5260 (patched)


s/that the we/that schedulers/


- Greg Mann


On Jan. 16, 2019, 3:54 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69723/
> ---
> 
> (Updated Jan. 16, 2019, 3:54 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-9472
> https://issues.apache.org/jira/browse/MESOS-9472
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up to now, the master would validate that any operation
> requesting operation feedback would only act on resources
> belonging to a resource provider.
> 
> This patch removes this restriction and enables frameworks
> to request operation feedback on an agent's default
> resources.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 2339207149a85578ea47cf66f28392182f9075f2 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/slave/slave.cpp 10cbc190acc7eea5734efa0066541168545a66b6 
>   src/tests/api_tests.cpp 79a2d2966ff6533629aeeefaaaf19e849b76e0e6 
>   src/tests/master_tests.cpp 1f8da63aa313c4679c0d7a3934802af1474e8f28 
>   src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 
> 
> 
> Diff: https://reviews.apache.org/r/69723/diff/2/
> 
> 
> Testing
> ---
> 
> ./src/mesos-tests
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

[GitHub] dlazarus commented on issue #324: MESOS-9499 extended URI syntax to support any Zookeeper authentication schemes

[GitBox]

dlazarus commented on issue #324: MESOS-9499 extended URI syntax to support any 
Zookeeper authentication schemes
URL: https://github.com/apache/mesos/pull/324#issuecomment-455238388
 
 
   OK. I propose I am going to add a unit test for the new parsing logic and 
we'll merge this pull request.
   
   If you want to develop a better approach to configure authentication data 
(which is definitely a good idea) it is better to do in a separate ticket. 
Because it is a separate issue.
   
   Auth data in URL will stay anyway at least for backward compatibility.
   
   Right now, they only way to use Mesos securely is to isolate the whole 
cluster in a private network. Because digest authentication is not good enough. 
On my work, without this patch, we cannot use Mesos in production. So it is a 
critical issue.
   
   
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 69776: Fixed flakiness by adding per agent config dir for mesos test.

[Andrei Budnik]

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


Ship it!




Ship It!

- Andrei Budnik


On Jan. 16, 2019, 10:08 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69776/
> ---
> 
> (Updated Jan. 16, 2019, 10:08 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Greg Mann, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In mesos tests, there are some cases that multiple agents are
> running simultanuously. From commit 07bccc63, we start to have
> agents share the same config dir in the test sandbox. Unit
> tests that use the same credicial file dir may fail if there
> are agents read and write to the same credential file
> simultanuously. We should set unique dir per agent.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.cpp 0d51687f2fc05804d2668837f0cbec4bc2b2e35a 
> 
> 
> Diff: https://reviews.apache.org/r/69776/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 69158: Added an integration test for resource provider removal.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69158 was successfully built and tested.

Reviews applied: `['69782', '68147', '69158']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2797/mesos-review-69158

- Mesos Reviewbot Windows


On Jan. 17, 2019, 3:17 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Jan. 17, 2019, 3:17 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an integration test for resource provider removal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/7/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69781 was successfully built and tested.

Reviews applied: `['69781']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2796/mesos-review-69781

- Mesos Reviewbot Windows


On Jan. 17, 2019, 7:15 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69781/
> ---
> 
> (Updated Jan. 17, 2019, 7:15 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9130
> https://issues.apache.org/jira/browse/MESOS-9130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we addressed one source of flakiness of this test in
> `2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
> flakiness (agents send more than the expected number of
> `UpdateSlaveMessage`s since they failed a timeout in registration with
> the master).
> 
> This patch ensures that the agent registers successfully before
> continuing with the test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 164e93a3749d4d668e12de31641aecece384 
> 
> 
> Diff: https://reviews.apache.org/r/69781/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69735: Fixed maintenance causes machines not in schedule rescinding offers.

[fei long]

> On Jan. 14, 2019, 8:57 a.m., Gilbert Song wrote:
> > Is `MasterAPITest.OperationUpdatesUponAgentGone/1` failure releted to the 
> > change?

I run "./bin/mesos-tests.sh --verbose 
--gtest_filter="*OperationUpdatesUponAgentGone*" --gtest_break_on_failure 
--gtest_repeat=100" locally and it passed. I believe it's caused by something 
else.


- fei


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


On Jan. 14, 2019, 3:34 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69735/
> ---
> 
> (Updated Jan. 14, 2019, 3:34 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9394
> https://issues.apache.org/jira/browse/MESOS-9394
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed maintenance causes machines not in schedule rescinding offers.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
> 
> 
> Diff: https://reviews.apache.org/r/69735/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fei long
> 
>

Re: Review Request 69735: Fixed maintenance causes machines not in schedule rescinding offers.

[fei long]

> On Jan. 14, 2019, 6:52 p.m., Joseph Wu wrote:
> > Here's a test I wrote for this issue: https://reviews.apache.org/r/65366/
> > The patch is a bit old, but could be re-used if necessary.

Since your patch has not been merged, I copied your code and passed the test by 
running "./bin/mesos-tests.sh --verbose --gtest_filter="*RescindOffers*" 
--gtest_break_on_failure --gtest_repeat=100".


- fei


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


On Jan. 14, 2019, 3:34 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69735/
> ---
> 
> (Updated Jan. 14, 2019, 3:34 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9394
> https://issues.apache.org/jira/browse/MESOS-9394
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed maintenance causes machines not in schedule rescinding offers.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
> 
> 
> Diff: https://reviews.apache.org/r/69735/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> fei long
> 
>

Re: Review Request 69158: Added an integration test for resource provider removal.

[Benjamin Bannier]

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

(Updated Jan. 17, 2019, 12:17 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


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


Repository: mesos


Description
---

Added an integration test for resource provider removal.


Diffs (updated)
-

  src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 


Diff: https://reviews.apache.org/r/69158/diff/7/

Changes: https://reviews.apache.org/r/69158/diff/6-7/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 68147: Added agent support to remove local resource providers.

[Benjamin Bannier]

> On Jan. 17, 2019, 5:42 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 3355 (patched)
> > 
> >
> > Most handlers print this line *before* creating the approvers. Let's 
> > move this before L3345.
> > 
> > (Not yours, but the logging of `addResourceProviderConfig`, 
> > `updateResourceProviderConfig` and `removeResourceProviderConfig` should be 
> > adjusted instead. We can address the consistency issue with a followup 
> > patch.)

Fixed and created https://reviews.apache.org/r/69782/.


> On Jan. 17, 2019, 5:42 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8020 (patched)
> > 
> >
> > How about adding the following check?
> > ```
> > CHECK(!protobuf::isTerminalState(operation->latest_status().state()));
> > ```

Good point, we actually only want to work on non-terminal operations here.

Adding a hard `CHECK` here couples us strongly to what operations the agent 
stores, something I'd like to avoid -- if only because it increases the amount 
of invariants one needs to keep in mind. How about just skipping non-terminal 
operations here instead? I updated the patch for that.

Dropping for now, please reopen if you feel this needs further discussion.


- Benjamin


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


On Jan. 17, 2019, 12:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68147/
> ---
> 
> (Updated Jan. 17, 2019, 12:17 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for triggering permanent removal of local
> resource providers. We also add authorization and tests as part of this
> patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
>   src/common/protobuf_utils.cpp a0159fed8d325808c5e8519da06173441deb 
>   src/master/master.cpp 7c8d3ce3271fd1d6b8e6e85e996f96572a7d48a6 
>   src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
>   src/slave/http.cpp e0dc09103e9186aa3f7328d2052fc6747f5be9bb 
>   src/slave/slave.hpp 2eadf5fce9a314f1ec0ac5d51820c6381f5f1468 
>   src/slave/slave.cpp 10cbc190acc7eea5734efa0066541168545a66b6 
>   src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
>   src/tests/api_tests.cpp 76ec56b530ed259b8e2cb7794074b6b72cec86b3 
> 
> 
> Diff: https://reviews.apache.org/r/68147/diff/15/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69158: Added an integration test for resource provider removal.

[Benjamin Bannier]

> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10755 (patched)
> > 
> >
> > We can use a const reference here: `const v1::ResourceProviderID&`

Moved down to first use.


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10834 (patched)
> > 
> >
> > If you think it is a good idea to create this alias, how about moving 
> > it to the beginning so the above two occurrences of `ContentType::PROTOBUF` 
> > can all be replaced?

Hmm. The other uses of `ContentType`s have no relation to the one here and do 
not need to be identical. We need all entities using `contentType` to use the 
same value though. Dropping.


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10895-10902 (patched)
> > 
> >
> > Is the order guaranteed? If yes, it seems to me that it is sufficient 
> > to just verify that the scheduler receives `OPERATION_GONE_BY_OPERATOR` 
> > right? The order validation here makes the test a bit hard to follow, so I 
> > suggest just remove the check here, as well as the two futures.
> > 
> > If the order is not guaranteed, then it seems we have a problem to 
> > resolve.

Yes, with the way the agent handler for `ResourceProviderMessage::Type::REMOVE` 
is written we currently always send the status update before the agent resource 
update. Removed this extra test. Like you write, should we ever break this 
invariant this test would already fail.

I added a comment to `src/slave.cpp` in https://reviews.apache.org/r/68147 to 
make this more explicit.


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10924-10926 (patched)
> > 
> >
> > To make this check more reliable, we should add a `Clock::settle()` at 
> > the end of the test.
> > 
> > Even so, it cannot 100% verify the property to test since HTTP messages 
> > could be in transmission when the test stops.

Good point about settling the clock.

While I agree that a test for "X never happens" can never be 100% certain (one 
would need to wait forever to test "never"), it sets up a falsifiable model for 
the test which adds value (a single failure would be enough to prove that the 
test assumption are incorrect; see K. Popper et al. :D ).


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10946 (patched)
> > 
> >
> > Let's check that the operation status has an agent ID but no resource 
> > provider ID.

Very good point. Adding this check uncovered that we didn't set the agent ID in 
the status update like noted by you here, 
https://reviews.apache.org/r/68147/#comment297704.


- Benjamin


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


On Jan. 17, 2019, 12:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> ---
> 
> (Updated Jan. 17, 2019, 12:17 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an integration test for resource provider removal.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/7/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68147: Added agent support to remove local resource providers.

[Benjamin Bannier]

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

(Updated Jan. 17, 2019, 12:17 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, Greg Mann, and Jan 
Schlicht.


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


Repository: mesos


Description
---

This patch adds support for triggering permanent removal of local
resource providers. We also add authorization and tests as part of this
patch.


Diffs (updated)
-

  include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
  include/mesos/v1/agent/agent.proto 5d1ab6fd7f6f2159367c66ed35ee90aea07f 
  src/common/protobuf_utils.cpp a0159fed8d325808c5e8519da06173441deb 
  src/master/master.cpp 7c8d3ce3271fd1d6b8e6e85e996f96572a7d48a6 
  src/slave/http.hpp 5b113fa2f7e3421d4219e0ece113010937b204c1 
  src/slave/http.cpp e0dc09103e9186aa3f7328d2052fc6747f5be9bb 
  src/slave/slave.hpp 2eadf5fce9a314f1ec0ac5d51820c6381f5f1468 
  src/slave/slave.cpp 10cbc190acc7eea5734efa0066541168545a66b6 
  src/slave/validation.cpp df5e1373dbe497bc859455dcaf4e064e923bd72e 
  src/tests/api_tests.cpp 76ec56b530ed259b8e2cb7794074b6b72cec86b3 


Diff: https://reviews.apache.org/r/68147/diff/15/

Changes: https://reviews.apache.org/r/68147/diff/14-15/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69782: Moved logging of resource provider HTTP actions before authorization.

[Benjamin Bannier]

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

Review request for mesos.


Repository: mesos


Description
---

Moved logging of resource provider HTTP actions before authorization.


Diffs
-

  src/slave/http.cpp e0dc09103e9186aa3f7328d2052fc6747f5be9bb 


Diff: https://reviews.apache.org/r/69782/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 69781: Fixed flakiness of resource provider ContainerTerminationMetric test.

[Benjamin Bannier]

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

Review request for mesos and Chun-Hung Hsiao.


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


Repository: mesos


Description
---

While we addressed one source of flakiness of this test in
`2117f671c450d7c74edc53078e8c0ed6035020aa` we introduced a new source of
flakiness (agents send more than the expected number of
`UpdateSlaveMessage`s since they failed a timeout in registration with
the master).

This patch ensures that the agent registers successfully before
continuing with the test.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
164e93a3749d4d668e12de31641aecece384 


Diff: https://reviews.apache.org/r/69781/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier