> On June 14, 2019, 2:34 a.m., Benjamin Bannier wrote: > > src/messages/messages.proto > > Lines 986 (patched) > > <https://reviews.apache.org/r/70822/diff/3/?file=2148591#file2148591line986> > > > > What does it mean if the master sends a `DRAINED` state to the agent? > > Is that something we need to reject in validation? > > > > Does it maybe make sense to instead break out `DrainInfo.state` into > > its own message to use in state reporting? > > Greg Mann wrote: > Yes we can have a CHECK on the agent to make sure the master doesn't send > DRAINED in a DrainSlaveMessage. That will happen in another patch. > > What's the benefit of moving `DrainInfo.State` outside of `DrainInfo`? > > Benjamin Bannier wrote: > The goal would be to use dedicated messages for triggering draining > (master->agent) and to report draining (agent->master). That might not only > be conceptually simplier, but would likely also lead to simpler, less > redundant code. Is there any benefit in using the same message? > > Greg Mann wrote: > What do you mean by "report draining"? The design doesn't involve any > explicit communication of draining progress between agent and master, the > master just receives task status updates. > > Benjamin Bannier wrote: > Sorry, there's no process of communicating `DrainInfo` back to master, > but we do report it to users with https://reviews.apache.org/r/70835/. > > Greg Mann wrote: > Are you proposing having a separate message for inclusion in the agent > API outputs? > > Vinod Kone wrote: > IIUC, Benjamin is questioning why we are storing both the spec (max grace > period, mark_gone) and the status (state) in the same proto (do we do this > elsewhere in the code base?). If they were separate protos, say DrainRequest > and DrainStatus, master would send DrainRequest to the agent, and show > DrainStatus (and possibly DrainRequest) in the operator API response. It > looks a bit weird that we would send DrainInfo (as it is currently written > to the agent and do a CHECK on valid state. The fact that an agent got a > Drain message is enough for the agent to know that it needs to drain, it > doesn't need to look into the `DrainInfo::State`? > > Vinod Kone wrote: > Let me repharse my first sentence. I think storing both spec and status > in the same proto is probably fine if it makes thing easy to expose in API > endpoints and use it internally in the master. But I still think it's weird > to send the status in a message to the agent which is not expected to use it. > So, if there's a way we can avoid it that would be great.
>From the master's perspective, having the `State` in the DrainInfo complicates >what the master needs to do in the registry. To report the correct state to >any endpoints, the master will need to assume all agents are in a `DRAINING` >state, until the agent connects, reconciles, and says it has nothing running. >This means the state within the registry will always be `DRAINING` (and there >is no reason to transition that state). So moving the `State` field into a separate message would be ideal. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70822/#review215896 ----------------------------------------------------------- On June 18, 2019, 4:17 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70822/ > ----------------------------------------------------------- > > (Updated June 18, 2019, 4:17 p.m.) > > > Review request for mesos, Benjamin Bannier, Benno Evers, Benjamin Mahler, > Joseph Wu, and Vinod Kone. > > > Bugs: MESOS-9753 > https://issues.apache.org/jira/browse/MESOS-9753 > > > Repository: mesos > > > Description > ------- > > This patch makes protobuf message updates which will be used > by both the master and the agent to facilitate automatic > draining of agents. > > > Diffs > ----- > > include/mesos/mesos.proto 2b4f350815935220c2d2b0dd0e52346bc74c91d9 > include/mesos/type_utils.hpp 57b1893160dbe874aa9fec00a3d1b640b9c54906 > include/mesos/v1/mesos.proto bafc27499f810791700c4a30dcb1da33b6f31d2e > src/common/type_utils.cpp ef1b3ea15cde1c7a8e0735fb9d7566dd1fd2cfdb > src/internal/devolve.hpp fefe86e450fa5083b9ff50e92f4594ffb30a54c8 > src/internal/devolve.cpp 1d300b49d5cc3de4b8ed409902eb881c7afc07ea > src/internal/evolve.hpp 1044d9df75b6fc1f60d3704be9cb5751e6d4321d > src/internal/evolve.cpp 19c155967bf090fb2ec39211805ff1385787ab59 > src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0 > > > Diff: https://reviews.apache.org/r/70822/diff/5/ > > > Testing > ------- > > `make` > > > Thanks, > > Greg Mann > >
