Re: Review Request 70530: Refactored Framework updates for the UPDATE_FRAMEWORK call; fixed race.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70583, 70531, 70530]

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 May 10, 2019, 2:50 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70530/
> ---
> 
> (Updated May 10, 2019, 2:50 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main concern of this patch is the behaviour of the framework
> re-subscription and of the upcoming UPDATE_FRAMEWORK call in cases when
> the framework attempts to change the "immutable" fields of
> FrameworkInfo, namely: `user`, `checkpoint`, `principal` and `id`.
> 
> The changes introduced by this patch:
> - The method `Framework::update()` is simplified: the logic of ignoring
>   the immutable fields is removed, this method fails the program if the
>   new values differ from the old ones. This is needed to simplify
>   keeping UPDATE_FRAMEWORK consistent with re-subscription.
> - The method `Master::validateFrameworkSubscription()` now returns
>   validation errors on attempts to change `user` or `checkpoint` fields.
>   This is needed to enable validating them in the UPDATE_FRAMEWORK call.
> - A method `Master::sanitizeFrameworkSubscription()` is introduced to
>   preserve the legacy behaviour of re-subscription (which ignores the
>   updates of `user` and `checkpoint`).
> - A second call of `validateFrameworkSubscription()` is performed after
>   the authorization. This is a crude fix of the already existing race
>   between two re-subscriptions against an empty master (see MESOS-9763).
>   It is necessary because otherwise the change in `Framework::update()`
>   would exacerbate this race (namely, it would be possible to crash the
>   master).
> 
> 
> Diffs
> -
> 
>   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/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70530: Refactored Framework updates for the UPDATE_FRAMEWORK call; fixed race.

[Benjamin Mahler]

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



Thanks for the description! Overall this is looking pretty good, some 
suggestions:

* In general we try to aim for small independent patches, as it makes reviewing 
a lot easier. Can you pull out the validateUpdate call into it's own patch in 
front of this one? Also, a patch after it to unit test it? That will help us 
land the work incrementally and overall more quickly. I think we would stil use 
this even with the next suggestion:
* The sanitize function seems rather unfortunate: (1) It depends on the master 
state in a non-trivial way (i.e. it's not provided the new vs old, it has to go 
and find the old one itself) and (2) it doesn't seem intuitive for a reader to 
know what sanitization is in this context (I usually see "sanitization" used to 
refer to cleaning out sensitive data, one example being ip addresses and job 
names from logs). I think a reader would more find it more intuitive if 
updating returns errors, and then those errors are ignored for SUBSCRIBE and 
not ignored for UPDATE_FRAMEWORK. This would probably mean that validation 
would be done within the update call, and the error surfaced up to the caller.

A minor follow up:

* Framework::update rather inefficiently handles the roles, and there may be a 
large number of roles and a somewhat frequent updating of the framework info in 
the future. Feel free to write a patch at the end of this chain that avoids all 
the unnecessary copying and set contruction, as well as moves from the passed 
in FrameworkInfo if possible.


src/master/master.cpp
Line 2578 (original), 2621 (patched)


newline bewteen the if and the assignment

space between ) and {



src/master/master.cpp
Line 2578 (original), 2621 (patched)


newline bewteen the if and the assignment



src/master/master.cpp
Line 2851 (original), 2908 (patched)


just one newline here



src/master/master.cpp
Line 2976 (original), 3033 (patched)


newline between the brace and the comment



src/master/validation.cpp
Lines 574 (patched)


the braces here don't seem warranted



src/master/validation.cpp
Lines 577 (patched)


inconsistent indentation?



src/master/validation.cpp
Lines 586-591 (patched)


How about we just return this in the error message for all cases? e.g.:

Caller: "Invalid framework update for : " + ...
This function: "Updating 'FrameworkInfo.user' is unsupported; attempted to 
update from 'root' to 'nobody'"

That way the caller can just log the error and we don't need to have 
special warning logging inside this function?


- Benjamin Mahler


On May 10, 2019, 2:50 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70530/
> ---
> 
> (Updated May 10, 2019, 2:50 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main concern of this patch is the behaviour of the framework
> re-subscription and of the upcoming UPDATE_FRAMEWORK call in cases when
> the framework attempts to change the "immutable" fields of
> FrameworkInfo, namely: `user`, `checkpoint`, `principal` and `id`.
> 
> The changes introduced by this patch:
> - The method `Framework::update()` is simplified: the logic of ignoring
>   the immutable fields is removed, this method fails the program if the
>   new values differ from the old ones. This is needed to simplify
>   keeping UPDATE_FRAMEWORK consistent with re-subscription.
> - The method `Master::validateFrameworkSubscription()` now returns
>   validation errors on attempts to change `user` or `checkpoint` fields.
>   This is needed to enable validating them in the UPDATE_FRAMEWORK call.
> - A method `Master::sanitizeFrameworkSubscription()` is introduced to
>   preserve the legacy behaviour of re-subscription (which ignores the
>   updates of `user` and `checkpoint`).
> - A second call of `validateFrameworkSubscription()` is performed after
>   the authorization. This is a crude fix of the already existing race
>   between two re-subscriptions against an empty master (see MESOS-9763).
>   It is necessary because otherwise the change in `Framework::update()`
>   would 

Re: Review Request 70530: Refactored Framework updates for the UPDATE_FRAMEWORK call; fixed race.

[Andrei Sekretenko]

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

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


Review request for mesos and Benjamin Mahler.


Changes
---

Fixed formatting of the description.


Summary (updated)
-

Refactored Framework updates for the UPDATE_FRAMEWORK call; fixed race.


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 the framework
re-subscription and of the upcoming UPDATE_FRAMEWORK call in cases when
the framework attempts to change the "immutable" fields of
FrameworkInfo, namely: `user`, `checkpoint`, `principal` and `id`.

The changes introduced by this patch:
- The method `Framework::update()` is simplified: the logic of ignoring
  the immutable fields is removed, this method fails the program if the
  new values differ from the old ones. This is needed to simplify
  keeping UPDATE_FRAMEWORK consistent with re-subscription.
- The method `Master::validateFrameworkSubscription()` now returns
  validation errors on attempts to change `user` or `checkpoint` fields.
  This is needed to enable validating them in the UPDATE_FRAMEWORK call.
- A method `Master::sanitizeFrameworkSubscription()` is introduced to
  preserve the legacy behaviour of re-subscription (which ignores the
  updates of `user` and `checkpoint`).
- A second call of `validateFrameworkSubscription()` is performed after
  the authorization. This is a crude fix of the already existing race
  between two re-subscriptions against an empty master (see MESOS-9763).
  It is necessary because otherwise the change in `Framework::update()`
  would exacerbate this race (namely, it would be possible to crash the
  master).


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/4/

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


Testing
---


Thanks,

Andrei Sekretenko