Re: Review Request 70441: Made CSI volume managers no longer maintain a list of services.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70468, 70431, 70441]

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 April 9, 2019, 9:45 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70441/
> ---
> 
> (Updated April 9, 2019, 9:45 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the service manager is passed in, there is no need for the volume
> manager to maintain a separated list of services provided by the service
> manager. This patch cleans up this part and make the service manager
> exposes the list of services it contains. It also add more validation
> in the service manager to make the code more cohesive.
> 
> 
> Diffs
> -
> 
>   src/csi/service_manager.hpp 5dd6bc7e044670c7416e2fc452ef46a2a50a6cf5 
>   src/csi/service_manager.cpp 0a3663cfd0dae2672a11eebcb6ffa3f8fad68ae0 
>   src/csi/v0_volume_manager.hpp 6c15f290389dfa150edf073c6033f0580e2e32b3 
>   src/csi/v0_volume_manager.cpp 02de17ee33193a078617210959a944501132fd5c 
>   src/csi/v0_volume_manager_process.hpp 
> c3cd6caa0e53bdb19213e1383f458ce7aab9485d 
>   src/csi/v1_volume_manager.hpp f8e609501253de37b9a9fa3ae0c760329e6ecfd4 
>   src/csi/v1_volume_manager.cpp bd334f1340be7fc3ef3faba56dfff8ffa69c2ea0 
>   src/csi/v1_volume_manager_process.hpp 
> 1c80399be434781401acda753ed5d69e1dc12a32 
>   src/csi/volume_manager.hpp cc20f461868b6b578b7e55ca0df66fbe26d8cd49 
>   src/csi/volume_manager.cpp cbe45cb439defbdd22bc57648b455e69679bdb4f 
>   src/resource_provider/storage/provider.cpp 
> 2dc5c26b11f3fff179b4357b0dcc149e93d4e12f 
> 
> 
> Diff: https://reviews.apache.org/r/70441/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70133: Removed unnecessary accept filters in SLRP tests.

[Chun-Hung Hsiao]

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

(Updated May 2, 2019, 11:40 p.m.)


Review request for mesos, Benjamin Bannier and Meng Zhu.


Changes
---

Removed the accept filters in a newly-added test.


Repository: mesos


Description
---

Removed unnecessary accept filters in SLRP tests.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
8bf4d2331b59770a7b7f3768499047bec2d67677 


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

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


Testing
---

`sudo make check`

Especially, tested that each of the three modified tests finishes in 5 seconds.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69955: Added SLRP unit tests for destroying unpublished persistent volumes.

[Chun-Hung Hsiao]

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

(Updated May 2, 2019, 10:46 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

This patch adds 3 unit tests: `DestroyUnpublishedPersistentVolume`,
`DestroyUnpublishedPersistentVolumeWithRecovery`, and
`DestroyUnpublishedPersistentVolumeWithReboot` to test that the SLRP is
resilient to misbehaved CSI plugins that fail to publish volumes.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
8bf4d2331b59770a7b7f3768499047bec2d67677 


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

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


Testing
---

make check

All tests passed 400 iterations (200 per parameter value) under stress.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69954: Added a SLRP unit test for persistent block volume creation.

[Chun-Hung Hsiao]

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

(Updated May 2, 2019, 10:44 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Addressed Benjamin's comment.


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


Repository: mesos


Description
---

Test `CreateDestroyPersistentBlockVolume` verifies that SLRP would fail
a `CREATE` operation on a BLOCK disk resource, and a followup `DESTROY`
will be dropped (instead of failing the SLRP).


Diffs (updated)
-

  src/tests/mock_csi_plugin.cpp cbcb59f720642c2801f5b86c438793e24437b414 
  src/tests/storage_local_resource_provider_tests.cpp 
8bf4d2331b59770a7b7f3768499047bec2d67677 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69954: Added a SLRP unit test for persistent block volume creation.

[Chun-Hung Hsiao]

> On April 16, 2019, 1:06 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3128 (patched)
> > 
> >
> > Are we checking that the resources get returned to the offer pool? In 
> > that case let's confirm that `offers` looks like what we would expect.
> > 
> > Alternatively we might not want to test this here and could just remove 
> > this and add a matching ignore when setting up the mock expectation above. 
> > That seems to make more sense to me.
> 
> Chun-Hung Hsiao wrote:
> It's more about making it deterministic, and also as part of the 
> end-to-end scenario. The contents of the offer has already been checked by 
> the matcher.
> 
> I could instead ignore the offer by doing a `Times(AtMost(1))`. Do you 
> think this would make the test simpler?

As discussed offline, the offer has been checked with the matcher set in the 
expectation.
Instead of using `AWAIT_READY`, we can use `AWAIT_EXPECT_READY` with logging 
here.


- Chun-Hung


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


On April 12, 2019, 3:15 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69954/
> ---
> 
> (Updated April 12, 2019, 3:15 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9565
> https://issues.apache.org/jira/browse/MESOS-9565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test `CreateDestroyPersistentBlockVolume` verifies that SLRP would fail
> a `CREATE` operation on a BLOCK disk resource, and a followup `DESTROY`
> will be dropped (instead of failing the SLRP).
> 
> 
> Diffs
> -
> 
>   src/tests/mock_csi_plugin.cpp cbcb59f720642c2801f5b86c438793e24437b414 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 8bf4d2331b59770a7b7f3768499047bec2d67677 
> 
> 
> Diff: https://reviews.apache.org/r/69954/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70431: Made the `RetryRpcWithExponentialBackoff` SLRP test work with CSI v1.

[Chun-Hung Hsiao]

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

(Updated May 2, 2019, 10:42 p.m.)


Review request for mesos, Benjamin Bannier and Jan Schlicht.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

This patch enables the unit test to test against CSI v1 through the
following changes:

  * The forwarding mode of the test CSI plugin now respects the
`--api_version` option. When specified, only requests of the proper
CSI version will be forwarded.

  * The expectations of `CreateVolume` and `DeleteVolume` calls in the
unit tests are parameterized against the CSI version string.

  * The mock CSI plugin now provides a default implementation for the
`GetCapacity` call so the unit test can be simplified.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp b54d666a878576f5845027fe6f704e10195079e1 
  src/tests/mock_csi_plugin.cpp cbcb59f720642c2801f5b86c438793e24437b414 
  src/tests/storage_local_resource_provider_tests.cpp 
8bf4d2331b59770a7b7f3768499047bec2d67677 


Diff: https://reviews.apache.org/r/70431/diff/2/

Changes: https://reviews.apache.org/r/70431/diff/1-2/


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70581: Add flag ignoring docker manifest config metadata.

[James Peach]

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



The new flag should also be documented in the 
[upgrade](https://github.com/apache/mesos/blob/master/docs/upgrades.md) doc. 
You'll need to add a new 1.9.x section.

- James Peach


On May 2, 2019, 6:06 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70581/
> ---
> 
> (Updated May 2, 2019, 6:06 p.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.
> 
> https://issues.apache.org/jira/browse/MESOS-9760
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md 04d7114e92c189afd04a8889b3ab853ade9deec6 
>   src/slave/containerizer/mesos/provisioner/store.cpp 
> 11fce0eb47e9e6dfce6289afe04a1d58a0c4461a 
>   src/slave/flags.hpp 09921cb6172202b5c1d2f8d03f9ccaeb3d0e8c94 
>   src/slave/flags.cpp 49a350f9e13409493fa9612c53d9d58b62122371 
> 
> 
> Diff: https://reviews.apache.org/r/70581/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Review Request 70587: Fixed unguarded calls to `Option::get()` in the master.

[Greg Mann]

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

Review request for mesos, Benno Evers, Gastón Kleiman, Joseph Wu, and Till 
Toenshoff.


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


Repository: mesos


Description
---

This patch avoids making unguarded calls to `Option::get()`
in `Master::updateOperationStatus()`. During agent reregistration, it's
possible that a `ReregisterSlaveMessage` from the agent can race with a
`SlaveReregisteredMessage` from the master, leading to multiple rounds
of master/agent operation reconciliation. The duplicate operation status
updates which occur as a result would crash the master before this fix.


Diffs
-

  src/master/master.cpp 7dcdc9ab62a46638a027eb9a54c1dff173785927 
  src/tests/agent_operation_feedback_tests.cpp 
e427441b3ef702acf0fba52adf7ba027ea6bc508 


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


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*DroppedOperationDuplicateStatusUpdate*" 
--gtest_repeat=-1 --gtest_break_on_failure`


Thanks,

Greg Mann

Review Request 70585: Test for a race between resubscriptions with different principals - WIP.

[Andrei Sekretenko]

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

Review request for mesos.


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


Repository: mesos


Description
---

Test for a race between resubscriptions with different principals.

This is a work in progress: currently I'm lucky enough that this test fails 
EVERY time, but it is necessary to add something to reproduce this race 
reliably.


Diffs
-

  src/tests/http_fault_tolerance_tests.cpp 
a9e8d0c8962d9c978e0488e16317117d55d214fa 


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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70581: Add flag ignoring docker manifest config metadata.

[Jacob Janco]

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

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.

https://issues.apache.org/jira/browse/MESOS-9760


Diffs
-

  docs/configuration/agent.md 04d7114e92c189afd04a8889b3ab853ade9deec6 
  src/slave/containerizer/mesos/provisioner/store.cpp 
11fce0eb47e9e6dfce6289afe04a1d58a0c4461a 
  src/slave/flags.hpp 09921cb6172202b5c1d2f8d03f9ccaeb3d0e8c94 
  src/slave/flags.cpp 49a350f9e13409493fa9612c53d9d58b62122371 


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


Testing
---


Thanks,

Jacob Janco

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 4:06 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added tests for updates sent by the master to slaves and API subscribers.


Summary (updated)
-

Added tests for UPDATE_FRAMEWORK.


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


Repository: mesos


Description
---

This patch adds tests for the UPDATE_FRAMEWORK call.

The reason behind implementing the MasterAPISubscriber class with mock methods 
is the need to wait for interesting API events while skipping uninteresting 
ones. 
This is needed to properly test the FRAMEWORK_UPDATED event sent by the master 
(for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip 
everything else).

I'm in doubts about the proper location for the MasterAPISubscriber. On one 
hand, it is used only by these tests and the implementation is by no means 
complete; on the other hand, this might be useful in other tests of the master 
API behaviour. 
Probably, turning the MasterAPISubscriber into a general-purpose mock (and 
moving it into another file at that point) should be a separate task.


Diffs (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/update_framework_tests.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 4:04 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added tests for updates sent by the master to slaves and API subscribers.


Summary (updated)
-

Added tests for the UPDATE_FRAMEWORK call.


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


Repository: mesos


Description (updated)
---

This patch adds tests for the UPDATE_FRAMEWORK call.

The reason behind implementing the MasterAPISubscriber class with mock methods 
is the need to wait for interesting API events while skipping uninteresting 
ones. 
This is needed to properly test the FRAMEWORK_UPDATED event sent by the master 
(for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip 
everything else).

I'm in doubts about the proper location for the MasterAPISubscriber. On one 
hand, it is used only by these tests and the implementation is by no means 
complete; on the other hand, this might be useful in other tests of the master 
API behaviour. 
Probably, turning the MasterAPISubscriber into a general-purpose mock (and 
moving it into another file at that point) should be a separate task.


Diffs (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/2/

Changes: https://reviews.apache.org/r/70534/diff/1-2/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 4:03 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added sending updates to slaves and master API subscribers.

Added re-validation of the call after authorization to avoid possible races 
with concurrent changes of the master's state. (Unfortunately, I cannot simply 
remove the first validation, because the authorizer needs a valid 
FrameworkInfo).


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


Repository: mesos


Description
---

Implemented the UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


Diff: https://reviews.apache.org/r/70533/diff/5/

Changes: https://reviews.apache.org/r/70533/diff/4-5/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70133: Removed unnecessary accept filters in SLRP tests.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On April 9, 2019, 9:21 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70133/
> ---
> 
> (Updated April 9, 2019, 9:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary accept filters in SLRP tests.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bd35150200a23ba57408f5d64a975f8a2f062018 
> 
> 
> Diff: https://reviews.apache.org/r/70133/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Especially, tested that each of the three modified tests finishes in 5 
> seconds.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70521: Renamed variables in `Master::_accept` to improve readability.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On May 1, 2019, 8:36 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70521/
> ---
> 
> (Updated May 1, 2019, 8:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed variables in `Master::_accept` to improve readability.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ad54ae217863a08f4e6d743b39c176b171353084 
> 
> 
> Diff: https://reviews.apache.org/r/70521/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70558: Added sending UpdateFrameworkMessage and FRAMEWORK_UPDATED - WIP.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 26, 2019, 3:32 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70558/
> ---
> 
> (Updated April 26, 2019, 3:32 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added sending UpdateFrameworkMessage and FRAMEWORK_UPDATED - WIP.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
>   src/master/master.cpp 7dcdc9ab62a46638a027eb9a54c1dff173785927 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70558/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 3:58 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added sending updates to slaves and master API subscribers.

Added re-validation of the call after authorization to avoid possible races 
with concurrent changes of the master's state. (Unfortunately, I cannot simply 
remove the first validation, because the authorizer needs a valid 
FrameworkInfo).


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


Repository: mesos


Description
---

Implemented the UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
35a04a3d0183697f93dadb3cbfead3ee0c2fea08 
  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70532: Added an UPDATE_FRAMEWORK scheduler::Call.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 3:58 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Fixed the comment describing UpdateFramework.


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


Repository: mesos


Description (updated)
---

Added an UPDATE_FRAMEWORK scheduler::Call.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
b6d79b1c433dce77a6ee4278b3ddf6b69868c1d8 
  include/mesos/v1/scheduler/scheduler.proto 
bddd5c449fd4b294d15430746b708731aff18f2a 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70531: Made using validateFrameworkSubscription() in UPDATE_FRAMEWORK possible.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 3:58 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

To keep the UPDATE_FRAMEWORK call consistent with re-subscription we need to be 
able to use the code in Master::validateFrameworkSubscription() to validate not 
only the Call:Subscribe but also the Call::UpdateFramework submessage. 

Thus I'm changing this method to take the FrameworkInfo and the list of 
suppressed roles as arguments (instead of Call::subscribe).


Diffs (updated)
-

  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70530: Refactored FrameworkInfo updates for the UPDATE_FRAMEWORK call + race fix.

[Andrei Sekretenko]

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

(Updated May 2, 2019, 3:58 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Refactored FrameworkInfo updates for the UPDATE_FRAMEWORK call + race fix.


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


Repository: mesos


Description (updated)
---

The main concern of this patch is the behaviour of re-subscription and the new 
UPDATE_FRAMEWORK call when the framework attempts to change the "immutable" 
fields of  FrameworkInfo, namely `user`, `checkpoint`, `principal` and `id`.

The purposes of this patch are:
- to make the `Framework::update()` method simpler by removing the logic of 
ignoring the immutable fields
- to make `Master::validateFrameworkSubscription()` return validation errors on 
attempt to change `user` and `checkpoint` fields (needed for the new 
UPDATE_FRAMEWORK call)
- at the same time, to preserve the legacy behaviour of silently ignoring 
`user` and `checkpoint` on re-subscription
- to fix the already existing race between two re-subscriptions against an 
empty master

Changes made by this patch:
- `Framework::update()` now fails the program when the new values for the 
immutable fields differ from the old ones
- a function `validation::framework::validateUpdate()` is introduced for 
validating immutable fields against their old values
- a method `Master::sanitizeFrameworkSubscription` is introduced to preserve 
the legacy behaviour of re-subscription
- `validateFrameworkSubscription()` is called on subscription once again after 
authorization.

NOTE: currently there is a race between two different concurrent SUBSCRIBE 
calls, which might arise due to network partiotioning of a (buggy) framework.
The origin of the race is the non-atomic nature of the resubscription: Validate 
- Authorize (by another actor)- UpdateFramework(deferred).

The obvious case is an attempt to re-subscribe with two different principals 
against an empty (for example, failed over) master. 
If the scheduler A succeeds in setting `principal` between authorization and 
update by the scheduler B, the scheduler B will have its `principal` silently 
ignored (instead of getting an error).
Changing the `Framework::update()` from silently ignoring immutable fields to 
CHECKing them exacerbates the race: now it is a way to crash the master. 

To remove this race I'm simply adding the second validation run after 
authorization. 
A proper fix would probably require splitting the validation into two parts: 
checks required for the authorizer(s) to work correctly and checks required for 
everything else.


Diffs (updated)
-

  src/master/framework.cpp 05f5514c589b2dba08afe77281e5fbc4e29f232b 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
  src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Review Request 70583: Move suppressedRoles into Master::_subscribe() as RepeatedPtrField.

[Andrei Sekretenko]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

For the changes in the SUBCRIBE validation in 
https://reviews.apache.org/r/70530 I need the suppressed roles to be available 
in the `Master::_subscribe()` methods as RepeatedFieldPtr.


Diffs
-

  src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
  src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
  src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 


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


Testing
---


Thanks,

Andrei Sekretenko