You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2019/04/02 00:50:44 UTC
Re: Review Request 70335: Sent FrameworkInfo to agents when applying
operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70335/#review214247
-----------------------------------------------------------
src/slave/slave.hpp
Lines 889 (patched)
<https://reviews.apache.org/r/70335/#comment300461>
Why do we need this hashmap? I see that the code inserts/removes entries, but nothing ever gets anything from it.
src/slave/slave.cpp
Lines 4420-4421 (patched)
<https://reviews.apache.org/r/70335/#comment300451>
`message.framework_info()` might not be set if the operation is triggered by an operator and not a framework, so these variables should probably be options.
src/slave/slave.cpp
Lines 4433 (patched)
<https://reviews.apache.org/r/70335/#comment300452>
Nit: I'd name this `unscheduleGC`.
src/slave/slave.cpp
Lines 4439-4443 (patched)
<https://reviews.apache.org/r/70335/#comment300458>
Should we also unschedule the garbage collection of the framework work directory?
It gets scheduled for GC here: https://github.com/apache/mesos/blob/1acb38c27306326a53f866b5386b5e28a6dc9314/src/slave/slave.cpp#L6834-L6838
src/slave/slave.cpp
Lines 4452-4456 (patched)
<https://reviews.apache.org/r/70335/#comment300456>
Nit: fits in one line.
src/slave/slave.cpp
Lines 4459 (patched)
<https://reviews.apache.org/r/70335/#comment300457>
I noticed that `Slave::run()` only checkpoints the framework if `frameworkInfo.checkpoint()` returns `true`.
Should we maybe not checkpoint operations/frameworkInfos of frameworks that set `checkpoint` to `false`?
src/slave/slave.cpp
Lines 4479 (patched)
<https://reviews.apache.org/r/70335/#comment300459>
Nit: I'd say: "Failed to unschedule framework directories scheduled for gc"
I would also include the path that couldn't be gc'd.
- Gastón Kleiman
On March 28, 2019, 4:27 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70335/
> -----------------------------------------------------------
>
> (Updated March 28, 2019, 4:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benno Evers, Chun-Hung Hsiao, Gastón Kleiman, Joseph Wu, and Meng Zhu.
>
>
> Bugs: MESOS-8582
> https://issues.apache.org/jira/browse/MESOS-8582
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the master to send a framework's full
> `FrameworkInfo` to the agent in the `ApplyOperationMessage`.
> The agent is updated to checkpoint frameworks when applying
> operations, and to unschedule GC on the meta directory when
> a new framework is created.
>
> The test `TerminalOrphanOperationAfterMasterFailover` is
> removed since this patch eliminates the case of orphan
> operations relevant to that test.
>
>
> Diffs
> -----
>
> src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026
> src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0
> src/slave/slave.hpp 2bffdc4b1e282d3c6dae2dcf23584a8a4550bf94
> src/slave/slave.cpp 5373cee5d30c2403497939eeba2ee5405117237e
> src/tests/persistent_volume_tests.cpp 7e929a5a3a92e16a5dec10206f37caebc20d66a8
> src/tests/reservation_tests.cpp cd84cd24d3587fafc01ae1861f22c47262f2d7e9
> src/tests/storage_local_resource_provider_tests.cpp 55c9389453754227de31e0a76d32ba663cc8ca7c
>
>
> Diff: https://reviews.apache.org/r/70335/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 70335: Sent FrameworkInfo to agents when applying
operations.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 2, 2019, 12:50 a.m., Gastón Kleiman wrote:
> > src/slave/slave.hpp
> > Lines 889 (patched)
> > <https://reviews.apache.org/r/70335/diff/1/?file=2136016#file2136016line889>
> >
> > Why do we need this hashmap? I see that the code inserts/removes entries, but nothing ever gets anything from it.
Yea we can nuke this.
> On April 2, 2019, 12:50 a.m., Gastón Kleiman wrote:
> > src/slave/slave.cpp
> > Lines 4439-4443 (patched)
> > <https://reviews.apache.org/r/70335/diff/1/?file=2136017#file2136017line4439>
> >
> > Should we also unschedule the garbage collection of the framework work directory?
> >
> > It gets scheduled for GC here: https://github.com/apache/mesos/blob/1acb38c27306326a53f866b5386b5e28a6dc9314/src/slave/slave.cpp#L6834-L6838
My thought was that we don't need to unschedule the work dir because, in the case of non-launch operations, we won't be creating any executor/task directories in the framework's work dir. However, the following sequence seems possible:
1) All of a framework's tasks on an agent go terminal, so the framework is removed and its meta/work dirs are scheduled for GC
2) The framework submits a non-launch operation on this agent, and we create a new framework in this patch but do NOT unschedule GC on the framework's work dir
3) The framework launches a task on this agent, we create a new executor dir in it's work dir, and then later on the framework's work dir is incorrectly GC'd because we never unscheduled it in #2
So, I think you're right. We should unschedule GC on the framework's work dir.
> On April 2, 2019, 12:50 a.m., Gastón Kleiman wrote:
> > src/slave/slave.cpp
> > Lines 4459 (patched)
> > <https://reviews.apache.org/r/70335/diff/1/?file=2136017#file2136017line4459>
> >
> > I noticed that `Slave::run()` only checkpoints the framework if `frameworkInfo.checkpoint()` returns `true`.
> >
> > Should we maybe not checkpoint operations/frameworkInfos of frameworks that set `checkpoint` to `false`?
I think the case of operations is different. Since non-launch operations are not coupled to the framework lifecycle the way tasks are, we want to retain the FrameworkInfo even if the framework does not have checkpointing enabled. For example, a framework could submit a CREATE_DISK operation which takes a long time, then immediately tear itself down. If checkpointing is not enabled, we would still want to have the FrameworkInfo checkpointed so that if the master fails over and the agent reregisters, we can create the Framework struct in the master and track the operation.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70335/#review214247
-----------------------------------------------------------
On March 28, 2019, 11:27 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70335/
> -----------------------------------------------------------
>
> (Updated March 28, 2019, 11:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benno Evers, Chun-Hung Hsiao, Gastón Kleiman, Joseph Wu, and Meng Zhu.
>
>
> Bugs: MESOS-8582
> https://issues.apache.org/jira/browse/MESOS-8582
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the master to send a framework's full
> `FrameworkInfo` to the agent in the `ApplyOperationMessage`.
> The agent is updated to checkpoint frameworks when applying
> operations, and to unschedule GC on the meta directory when
> a new framework is created.
>
> The test `TerminalOrphanOperationAfterMasterFailover` is
> removed since this patch eliminates the case of orphan
> operations relevant to that test.
>
>
> Diffs
> -----
>
> src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026
> src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0
> src/slave/slave.hpp 2bffdc4b1e282d3c6dae2dcf23584a8a4550bf94
> src/slave/slave.cpp 5373cee5d30c2403497939eeba2ee5405117237e
> src/tests/persistent_volume_tests.cpp 7e929a5a3a92e16a5dec10206f37caebc20d66a8
> src/tests/reservation_tests.cpp cd84cd24d3587fafc01ae1861f22c47262f2d7e9
> src/tests/storage_local_resource_provider_tests.cpp 55c9389453754227de31e0a76d32ba663cc8ca7c
>
>
> Diff: https://reviews.apache.org/r/70335/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>