Re: Review Request 42222: Added a comment on allocator recovery.

[Alexander Rukletsov]

> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > Thanks Alex! I ended up going over the structure of the recover code and 
> > left some higher level comments. There also appears to be a bug that will 
> > crash the master that I marked as an issue :)
> > 
> > Have we convinced ourselves that we want to act on partial state when quota 
> > is *not* set? It seems to complicate things here, and acting on partial 
> > state w/o quota has some issues as well?
> 
> Alexander Rukletsov wrote:
> Thanks a bunch for a thorough review, Ben! I have opened a separate 
> ticket to track the bug and other comments you had left: 
> https://issues.apache.org/jira/browse/MESOS-4417.

> Have we convinced ourselves that we want to act on partial state when quota 
> is not set?

I think it's fine, because the allocator does not provide any guarantees 
without quota, neither it enforces the sharing scheme computed by the sorter. 
There can be other scenarios other than recovery that may lead to biased 
cluster sharing. Do you have any specific use cases in mind why shall we 
*always* recover?


> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 149
> > 
> >
> > It now seems odd to start the allocation loop within initialize rather 
> > than within recover, we'll do the following:
> > 
> > 
> > intialize -> unpauses, starts allocation loop but the allocation loop 
> > should do nothing
> > recover -> pauses allocations, waits for condition to be met, resumes 
> > allocation.
> > 
> > 
> > It seems a simpler way to think about this would be to just start 
> > allocating after the recovery condition is met. This would consist of (1) 
> > started = true* and (2) the initial delay to 'batch' that starts the delay 
> > loop.
> > 
> > * Although keeping the pause concept may be useful if we want to 
> > support pausing/resuming allocation from endpoints.

When we were thinking about how to implement it in the most consistent way, we 
have figured out, that the widely used pattern is:
`initialize()` -> [optional] `recover()`. Both `initialize()` and `recover()` 
are called from the client code. Hence we decided it would be great to ensure 
things work if `recover()` has not been called. We can also always call 
`recover()` even though there is no quota to recover.

Anyway I favour to keep the pause concept regardless which workflow we opt for.


> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 168
> > 
> >
> > Should this comment be moved down?
> > 
> > I'm also not quite sure what it's expressing, is it a re-hash of the 
> > lifecycle documentation?
> > 
> > ```
> >* Because it is hard to define recovery for a running allocator, this
> >* method should be called after `initialize()`, but before actual
> >* allocation starts (i.e. `addSlave()` is called).
> > ```
> > 
> > For example:
> > 
> > ```
> > CHECK(initialized);
> > 
> > // Recovery should be called after initialize and before other calls.
> > CHECK_EQ(0u, slaves.size());
> > CHECK_EQ(0, quotaRoleSorter->count());
> > ```
> > 
> > Could we extend our existing 'initialized' state boolean into an enum 
> > to express UNINITIALIZED, INITIALIZED, RECOVERED? As far as I can tell, the 
> > existing methods with `CHECK(initialized)` are also interested in checking 
> > that recover was called?

I think the question here is whether we want recovery be optional or not. If it 
is optional, tracking state transitions may become tedious.


> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 171-172
> > 
> >
> > Not for this review, but looks like we should update `Sorter::count` to 
> > return a size_t.
> > 
> > The following CHECK implies that `_expectedAgentCount` should just be a 
> > size_t?
> > 
> > ```
> > CHECK(_expectedAgentCount >= 0);
> > ```
> > 
> > With a size_t we don't have to do this kind of check :)

This is a general question: shall we use `size_t` for non-negative integers or 
follow google style guide (and protobufs!) and use int. I'm not sure we have an 
agreement on this one.


> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 468
> > 
> >
> > Hm.. did you need the static cast here?

I believe so, one is `size_t` and the other one is `int`.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:

Re: Review Request 42222: Added a comment on allocator recovery.

[Alexander Rukletsov]

> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > Thanks Alex! I ended up going over the structure of the recover code and 
> > left some higher level comments. There also appears to be a bug that will 
> > crash the master that I marked as an issue :)
> > 
> > Have we convinced ourselves that we want to act on partial state when quota 
> > is *not* set? It seems to complicate things here, and acting on partial 
> > state w/o quota has some issues as well?

Thanks a bunch for a thorough review, Ben! I have opened a separate ticket to 
track the bug and other comments you had left: 
https://issues.apache.org/jira/browse/MESOS-4417.


> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 205-206
> > 
> >
> > It looks like if we trip the `resume` call in `addSlave`, this delayed 
> > resume will crash the master due to the `CHECK(paused)` that currently 
> > resides in `resume`.
> > 
> > Something I'm missing?
> 
> Joris Van Remoortere wrote:
> Thanks for your review Ben!
> I'm committing just the comment change for this patch.
> I'll leave this issue open as we want to address it in a separate patch.

Filed: https://issues.apache.org/jira/browse/MESOS-4417


- Alexander


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


On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated Jan. 12, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42222: Added a comment on allocator recovery.

[Joris Van Remoortere]

> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 205-206
> > 
> >
> > It looks like if we trip the `resume` call in `addSlave`, this delayed 
> > resume will crash the master due to the `CHECK(paused)` that currently 
> > resides in `resume`.
> > 
> > Something I'm missing?

Thanks for your review Ben!
I'm committing just the comment change for this patch.
I'll leave this issue open as we want to address it in a separate patch.


- Joris


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


On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated Jan. 12, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42222: Added a comment on allocator recovery.

[Joris Van Remoortere]

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

Ship it!


I've rephrased and moved the comment as per BenM's suggestion.
Let's tackle the other comments in separate reviews. I think it is valuable to 
add this comment now regardless.

- Joris Van Remoortere


On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated Jan. 12, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42222: Added a comment on allocator recovery.

[Ben Mahler]

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


Thanks Alex! I ended up going over the structure of the recover code and left 
some higher level comments. There also appears to be a bug that will crash the 
master that I marked as an issue :)

Have we convinced ourselves that we want to act on partial state when quota is 
*not* set? It seems to complicate things here, and acting on partial state w/o 
quota has some issues as well?


src/master/allocator/mesos/hierarchical.cpp (line 149)


It now seems odd to start the allocation loop within initialize rather than 
within recover, we'll do the following:

intialize -> unpauses, starts allocation loop but the allocation loop 
should do nothing
recover -> pauses allocations, waits for condition to be met, resumes 
allocation.

It seems a simpler way to think about this would be to just start 
allocating after the recovery condition is met. This would consist of (1) 
started = true* and (2) the initial delay to 'batch' that starts the delay loop.

* Although keeping the pause concept may be useful if we want to support 
pausing/resuming allocation from endpoints.



src/master/allocator/mesos/hierarchical.cpp (lines 153 - 163)


It seems a little odd to document at the function level when this interface 
function is already documented:

```
  /**
   * Informs the allocator of the recovered state from the master.
   *
   * Because it is hard to define recovery for a running allocator, this
   * method should be called after `initialize()`, but before actual
   * allocation starts (i.e. `addSlave()` is called).
   *
   * TODO(alexr): Consider extending the signature with expected
   * frameworks count once it is available upon the master failover.
   *
   * @param quotas A (possibly empty) collection of quotas, keyed by
   * their role, known to the master.
   */
  virtual void recover(
  const int expectedAgentCount,
  const hashmap& quotas) = 0;
```

How about moving it in to the body after the checks, with some slight 
rephrasing:

```
void HierarchicalAllocatorProcess::recover(
const int _expectedAgentCount,
const hashmap& quotas)
{
  // Recovery should start before actual allocation starts.
  CHECK(initialized);
  CHECK_EQ(0u, slaves.size());
  CHECK_EQ(0, quotaRoleSorter->count());
  CHECK(_expectedAgentCount >= 0);

  // If there is no quota, recovery is a no-op. Otherwise, we need
  // to delay allocations while agents are re-registering because 
  // otherwise we perform allocations on a partial view of resources!
  // We would consequently perform unnecessary allocations to satisfy
  // quota constraints, which can over-allocate non-revocable resources
  // to roles using quota. Then, frameworks in roles without quota can
  // be unnecessarily deprived of resources. We may also be unable to
  // satisfy all of the quota constraints. Repeated master failovers
  // exacerbate the issue.
  
  if (quotas.empty()) {
VLOG(1) << "Skipping recovery of hierarchical allocator: "
<< "nothing to recover";

return;
  }
  
  ...
  
}
```

It's great that we went through the exercise of writing this comment 
because it seems we should consider whether we want to avoid acting on partial 
state even when no quota is set.



src/master/allocator/mesos/hierarchical.cpp (line 165)


I wonder if we can use a better word than "expected" here; something that 
conveys that this represents the count of agents that were registered prior to 
failover / persisted in the registry. For example, the following would be 
really clear:

```
void HierarchicalAllocatorProcess::recover(
const registry::Slaves& recoveredSlaves,
const hashmap& quotas)
```



src/master/allocator/mesos/hierarchical.cpp (line 168)


Should this comment be moved down?

I'm also not quite sure what it's expressing, is it a re-hash of the 
lifecycle documentation?

```
   * Because it is hard to define recovery for a running allocator, this
   * method should be called after `initialize()`, but before actual
   * allocation starts (i.e. `addSlave()` is called).
```

For example:

```
CHECK(initialized);

// Recovery should be called after initialize and before other calls.
CHECK_EQ(0u, 

Re: Review Request 42222: Added a comment on allocator recovery.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42221, 4]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated Jan. 12, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42222: Added a comment on allocator recovery.

[Klaus Ma]

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

Ship it!


Ship It!

- Klaus Ma


On Jan. 13, 2016, 7:06 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated Jan. 13, 2016, 7:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 42222: Added a comment on allocator recovery.

[Alexander Rukletsov]

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

Review request for mesos, Ben Mahler and Joris Van Remoortere.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 42222: Added a comment on allocator recovery.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 一月 12, 2016, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4/
> ---
> 
> (Updated 一月 12, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f 
> 
> Diff: https://reviews.apache.org/r/4/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>