Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On June 24, 2017, 10:30 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 24, 2017, 10:30 a.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Adam B]

> On June 23, 2017, 7:21 p.m., Adam B wrote:
> > src/slave/slave.cpp
> > Lines 1405-1407 (original), 1405-1408 (patched)
> > 
> >
> > If this agent has refinements, and we send post format to an old 
> > master, will the old master safely reject the registration, crash and burn, 
> > or something in between?
> 
> Neil Conway wrote:
> The master will basically consider the resources to be unreserved; 
> because the master and agent will have inconsistent views of the resource 
> state at the agent, this will cause problems.
> 
> Since you need a new master to create reservation refinements in the 
> first place, you can only arrive in this situation by:
> 
> Upgrading master
> Upgrading agent
> Creating res refinement
> Downgrading master
> 
> Which arguably falls under the "don't downgrade if you are using new 
> featues" bucket. But yes, this is certainly unfortunate. Hard to prevent 
> without introducing something similar to master capabilities, which we 
> definitely need (MESOS-5675). I'll drop this issue for now, since AFAIK 
> there's not much we can do to improve this in the short term.

Sounds good. Thanks for the explanation. At least we don't crash..


> On June 23, 2017, 7:21 p.m., Adam B wrote:
> > src/slave/slave.cpp
> > Line 1408 (original), 1412-1414 (patched)
> > 
> >
> > We could at least log an INFO/WARN if we aren't able to downgrade, and 
> > still send it anyway.
> 
> Neil Conway wrote:
> Hmm, not sure a warning/log is warranted here. In the common case 
> (refined reservation, >= 1.4 master, >= 1.4 agent), we will fail to downgrade 
> the resources, but that is fine and expected. Should we really be cluttering 
> the logs with this information?

Good point. Don't want to over-log in the common case. Would be nice if we had 
master capabilities so we only had to log failure-to-downgrade when we're 
(re)registering with an old master.


- Adam


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


On June 24, 2017, 10:30 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 24, 2017, 10:30 a.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Neil Conway]

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

(Updated June 24, 2017, 5:30 p.m.)


Review request for mesos, Adam B and Michael Park.


Changes
---

Clarify comment.


Repository: mesos


Description
---

Documented resource format in agent <-> master protocol.


Diffs (updated)
-

  src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
  src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 


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

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Neil Conway]

> On June 24, 2017, 2:21 a.m., Adam B wrote:
> > src/slave/slave.cpp
> > Lines 1405-1407 (original), 1405-1408 (patched)
> > 
> >
> > If this agent has refinements, and we send post format to an old 
> > master, will the old master safely reject the registration, crash and burn, 
> > or something in between?

The master will basically consider the resources to be unreserved; because the 
master and agent will have inconsistent views of the resource state at the 
agent, this will cause problems.

Since you need a new master to create reservation refinements in the first 
place, you can only arrive in this situation by:

Upgrading master
Upgrading agent
Creating res refinement
Downgrading master

Which arguably falls under the "don't downgrade if you are using new featues" 
bucket. But yes, this is certainly unfortunate. Hard to prevent without 
introducing something similar to master capabilities, which we definitely need 
(MESOS-5675). I'll drop this issue for now, since AFAIK there's not much we can 
do to improve this in the short term.


> On June 24, 2017, 2:21 a.m., Adam B wrote:
> > src/slave/slave.cpp
> > Line 1408 (original), 1412-1414 (patched)
> > 
> >
> > We could at least log an INFO/WARN if we aren't able to downgrade, and 
> > still send it anyway.

Hmm, not sure a warning/log is warranted here. In the common case (refined 
reservation, >= 1.4 master, >= 1.4 agent), we will fail to downgrade the 
resources, but that is fine and expected. Should we really be cluttering the 
logs with this information?


- Neil


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


On June 24, 2017, 1:48 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 24, 2017, 1:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Neil Conway]

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




src/slave/slave.cpp
Lines 1405-1407 (original), 1405-1408 (patched)


The master will basically consider the resources to be unreserved; because 
the master and agent will have inconsistent views of the resource state at the 
agent, this will cause problems.

Since you need a new master to create reservation refinements in the first 
place, you can only arrive in this situation by:

1. Upgrading master
2. Upgrading agent
3. Creating res refinement
4. Downgrading master

Which arguably falls under the "don't downgrade if you are using new 
featues" bucket. But yes, this is certainly unfortunate. Hard to prevent 
without introducing something similar to master capabilities, which we 
definitely need (MESOS-5675). I'll drop this issue for now, since AFAIK there's 
not much we can do to improve this in the short term.



src/slave/slave.cpp
Line 1408 (original), 1412-1414 (patched)


Hmm, not sure a warning/log is warranted here. In the common case (refined 
reservation, >= 1.4 master, >= 1.4 agent), we will fail to downgrade the 
resources, but that is fine and expected. Should we really be cluttering the 
logs with this information?


- Neil Conway


On June 24, 2017, 1:48 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 24, 2017, 1:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Adam B]

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



Just a couple of clarifying questions before we commit these comments.


src/messages/messages.proto
Line 436 (original), 439-442 (patched)


Since this is a `repeated Resource` list, is it possible that some 
resources in the list are pre-format and some are post? Or will they all be 
post if any resource has a refinement?



src/slave/slave.cpp
Lines 1405-1407 (original), 1405-1408 (patched)


If this agent has refinements, and we send post format to an old master, 
will the old master safely reject the registration, crash and burn, or 
something in between?



src/slave/slave.cpp
Line 1408 (original), 1412-1414 (patched)


We could at least log an INFO/WARN if we aren't able to downgrade, and 
still send it anyway.


- Adam B


On June 23, 2017, 6:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 23, 2017, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Neil Conway]

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

(Updated June 24, 2017, 12:31 a.m.)


Review request for mesos and Michael Park.


Changes
---

Tweak comment.


Repository: mesos


Description
---

Documented resource format in agent <-> master protocol.


Diffs (updated)
-

  src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
  src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 


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

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


Testing
---


Thanks,

Neil Conway

Review Request 60405: Documented resource format in agent <-> master protocol.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Documented resource format in agent <-> master protocol.


Diffs
-

  src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 


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


Testing
---


Thanks,

Neil Conway