You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/05/02 15:58:50 UTC
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.
-----------------------------------------------------------
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 70533: Implemented the UPDATE_FRAMEWORK call in
the V1 API.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2877-2880 (original), 2884-2887 (patched)
> > <https://reviews.apache.org/r/70533/diff/5/?file=2142312#file2142312line2884>
> >
> > Hm.. why doesn't the broadcast function send it to subscribers too? why the need to have this separately done outside the function?
> >
> > Probably want to rename it if we're able to do both in the function: `broadcastFramwerkUpdate(...)`
There was one case in which only FRAMEWORK_UPDATED was sent - most likely, a bug.
Moved this refactoring into a preceding patch: https://reviews.apache.org/r/70665/
> On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3279-3280 (patched)
> > <https://reviews.apache.org/r/70533/diff/5/?file=2142312#file2142312line3279>
> >
> > This.. seems odd. Do we need to rename this function now that it's more a master-stateful validation of the framework info?
> >
> > Reading this code, I'm left thinking that the validation of the old vs new FrameworkInfo is accidentally omitted, but I'm guessing your previous patches actually put that within the `validateFrameworkSubscription(...)` function and that's how it's getting validated here?
> >
> > Seems like we need to do some renaming here, it seems to be more a stateful validation of the framework info now, not subscription.
I've renaming the method into `Master::validateFramework()` (and would appreciate a better naming suggestion).
Also, after looking at all this (including the race between two SUBSCRIBE calls), I decided to keep the validation against the existing FrameworkInfo separate from the all the other validation.
See preceding patches https://reviews.apache.org/r/70666 and https://reviews.apache.org/r/70668
> On May 12, 2019, 5:46 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3287-3291 (patched)
> > <https://reviews.apache.org/r/70533/diff/5/?file=2142312#file2142312line3287>
> >
> > Seems unfortunate that this is getting caught here instead of within validation of the Call. It would be nice if this function can just CHECK it.
Moved this into call validation. Now this method does not need the `frameworkId` at all.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review215205
-----------------------------------------------------------
On May 17, 2019, 3:18 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> -----------------------------------------------------------
>
> (Updated May 17, 2019, 3:18 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch is based on the previous implementation attempt:
> https://reviews.apache.org/r/66229/
>
>
> Diffs
> -----
>
> src/master/http.cpp c2c7b9b65bd66679420f62d37b01d90e3692c71d
> src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42
> src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d
> src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8
>
>
> Diff: https://reviews.apache.org/r/70533/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review215205
-----------------------------------------------------------
src/master/master.hpp
Lines 665-668 (patched)
<https://reviews.apache.org/r/70533/#comment301786>
No need to re-iterate all these details here, we're unlikely to keep it in sync with what the code does.
src/master/master.hpp
Lines 674 (patched)
<https://reviews.apache.org/r/70533/#comment301789>
s/make/Make/
src/master/master.hpp
Lines 676 (patched)
<https://reviews.apache.org/r/70533/#comment301788>
s/is/it/
src/master/master.hpp
Lines 677-678 (patched)
<https://reviews.apache.org/r/70533/#comment301787>
The early newline makes it look like this last sentence is potentially separate from the TODO when reading this, how about just continuing from the previous line?
```
// currently it is not, see MESOS-9746. At that point, we can
// potentially remove the `broadcastFrameworkUpdate()` function.
```
src/master/master.hpp
Lines 685 (patched)
<https://reviews.apache.org/r/70533/#comment301780>
The arguments are generally named in these declarations, we should stay consistent with that
src/master/master.hpp
Lines 1073-1079 (patched)
<https://reviews.apache.org/r/70533/#comment301779>
The arguments are generally named in the other declarations, we should stay consistent with that
src/master/master.cpp
Lines 2877-2880 (original), 2884-2887 (patched)
<https://reviews.apache.org/r/70533/#comment301778>
Hm.. why doesn't the broadcast function send it to subscribers too? why the need to have this separately done outside the function?
Probably want to rename it if we're able to do both in the function: `broadcastFramwerkUpdate(...)`
src/master/master.cpp
Lines 2886-2894 (original), 2901-2909 (patched)
<https://reviews.apache.org/r/70533/#comment301777>
Seems like we can construct the message outside the loop? Feel free to follow up with a patch later for that or have a patch in front of this patch.
src/master/master.cpp
Lines 3279-3280 (patched)
<https://reviews.apache.org/r/70533/#comment301785>
This.. seems odd. Do we need to rename this function now that it's more a master-stateful validation of the framework info?
Reading this code, I'm left thinking that the validation of the old vs new FrameworkInfo is accidentally omitted, but I'm guessing your previous patches actually put that within the `validateFrameworkSubscription(...)` function and that's how it's getting validated here?
Seems like we need to do some renaming here, it seems to be more a stateful validation of the framework info now, not subscription.
src/master/master.cpp
Lines 3287-3291 (patched)
<https://reviews.apache.org/r/70533/#comment301783>
Seems unfortunate that this is getting caught here instead of within validation of the Call. It would be nice if this function can just CHECK it.
src/master/master.cpp
Lines 3309 (patched)
<https://reviews.apache.org/r/70533/#comment301790>
Start all TODOs with a capital letter, you should find that's the norm if you grep the codebase
src/master/master.cpp
Lines 3327 (patched)
<https://reviews.apache.org/r/70533/#comment301784>
Can we be more clear here?
Are we referring to concurrent SUBSCRIBE or UPDATE_FRAMEWORK calls?
It seems to me we're relying on authorization preserving the ordering
src/master/master.cpp
Lines 3338 (patched)
<https://reviews.apache.org/r/70533/#comment301781>
no double newline here
src/master/master.cpp
Lines 3345 (patched)
<https://reviews.apache.org/r/70533/#comment301782>
newline
- Benjamin Mahler
On May 7, 2019, 1:09 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> -----------------------------------------------------------
>
> (Updated May 7, 2019, 1:09 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch is based on the previous implementation attempt:
> https://reviews.apache.org/r/66229/
>
>
> Diffs
> -----
>
> 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/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in
the V1 API.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On May 22, 2019, 7:52 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3172-3173 (patched)
> > <https://reviews.apache.org/r/70533/diff/8/?file=2146402#file2146402line3172>
> >
> > This is undefined behavior (and may crash, this has caught me before), because `call` is moved in the same expression as the `call.framework_info()` is touched, and C++ doesn't not guarantee the order of evaluation.
> >
> > We have to split it out:
> >
> > ```
> > Future<bool> authorized = authorizeFramework(call.framework_info());
> >
> > return authorized
> > .then(defer(self(), &Self::_updateFramework, std::move(call), lambda::_1));
> > ```
Yeah, and a crash could be a relatively good outcome of UB ;) Many thanks for catching this!
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review215466
-----------------------------------------------------------
On May 22, 2019, 1:49 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 1:49 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch is based on the previous implementation attempt:
> https://reviews.apache.org/r/66229/
>
>
> Diffs
> -----
>
> src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d
> src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412
> src/master/master.cpp 7c9d9c3d566e29d3f8a5781ee6cdf11973a755e6
> src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27
>
>
> Diff: https://reviews.apache.org/r/70533/diff/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in
the V1 API.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/#review215466
-----------------------------------------------------------
Fix it, then Ship it!
Nice and clean patch!
There's a bug below, but I'll get this fixed and committed shortly.
src/master/master.cpp
Lines 3172-3173 (patched)
<https://reviews.apache.org/r/70533/#comment302184>
This is undefined behavior (and may crash, this has caught me before), because `call` is moved in the same expression as the `call.framework_info()` is touched, and C++ doesn't not guarantee the order of evaluation.
We have to split it out:
```
Future<bool> authorized = authorizeFramework(call.framework_info());
return authorized
.then(defer(self(), &Self::_updateFramework, std::move(call), lambda::_1));
```
- Benjamin Mahler
On May 22, 2019, 1:49 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 1:49 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch is based on the previous implementation attempt:
> https://reviews.apache.org/r/66229/
>
>
> Diffs
> -----
>
> src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d
> src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412
> src/master/master.cpp 7c9d9c3d566e29d3f8a5781ee6cdf11973a755e6
> src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27
>
>
> Diff: https://reviews.apache.org/r/70533/diff/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in
the V1 API.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/
-----------------------------------------------------------
(Updated May 22, 2019, 1:49 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Fixed indentation.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch is based on the previous implementation attempt:
https://reviews.apache.org/r/66229/
Diffs (updated)
-----
src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d
src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412
src/master/master.cpp 7c9d9c3d566e29d3f8a5781ee6cdf11973a755e6
src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27
Diff: https://reviews.apache.org/r/70533/diff/8/
Changes: https://reviews.apache.org/r/70533/diff/7-8/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in
the V1 API.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/
-----------------------------------------------------------
(Updated May 21, 2019, 1:54 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch is based on the previous implementation attempt:
https://reviews.apache.org/r/66229/
Diffs (updated)
-----
src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d
src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e
src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de
src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27
Diff: https://reviews.apache.org/r/70533/diff/7/
Changes: https://reviews.apache.org/r/70533/diff/6-7/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in
the V1 API.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/
-----------------------------------------------------------
(Updated May 17, 2019, 3:18 p.m.)
Review request for mesos and Benjamin Mahler.
Summary (updated)
-----------------
Implemented the UPDATE_FRAMEWORK call in the V1 API.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch is based on the previous implementation attempt:
https://reviews.apache.org/r/66229/
Diffs (updated)
-----
src/master/http.cpp c2c7b9b65bd66679420f62d37b01d90e3692c71d
src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42
src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d
src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8
Diff: https://reviews.apache.org/r/70533/diff/6/
Changes: https://reviews.apache.org/r/70533/diff/5-6/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70533/
-----------------------------------------------------------
(Updated May 7, 2019, 1:09 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description (updated)
-------
This patch is based on the previous implementation attempt:
https://reviews.apache.org/r/66229/
Diffs
-----
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/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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