You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/12/12 09:58:34 UTC
Review Request: Slave Restart (Part Quatre): Checkpointed slave, framework,
executor and task
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs
-----
src/exec/exec.cpp 11cb64dd3f6d3992072582f2b6d6e6d9220eb48d
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp ede080069d814f410662f759806d1d5b260e8569
src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Will write new tests as part of Part 5, when I write the analogous read/replay functions of the checkpoint functions.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 12, 2012, 6:51 p.m., Benjamin Hindman wrote:
> > src/tests/protobuf_io_tests.cpp, line 57
> > <https://reviews.apache.org/r/8535/diff/1/?file=237229#file237229line57>
> >
> > Did the compiler want/need these?
yes. because we have two different namespaces mesos::internal::protobuf (common/protobuf_utils.hpp) and protobuf (stout/protobuf.hpp)
> On Dec. 12, 2012, 6:51 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 500
> > <https://reviews.apache.org/r/8535/diff/1/?file=237228#file237228line500>
> >
> > I wonder if the flag should really be called 'checkpoint' and the 'recover' flag should be whether or not to recover from an existing checkpoint?
As discussed offline, we are going to have two flags: 'checkpoint' (boolean) and 'recover' (string). I will add the 'recover' flag in part 5.
> On Dec. 12, 2012, 6:51 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, lines 499-500
> > <https://reviews.apache.org/r/8535/diff/1/?file=237228#file237228line499>
> >
> > Hmm, I'm not sure how I feel about these semantics. I think I'd prefer to see the frameworks tasks keep going lost or failed with a message "Couldn't launch task because framework expects checkpointing but slave doesn't support it ..." rather than the slave being killed. Thoughts?
Agreed. Fixed.
> On Dec. 12, 2012, 6:51 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 600
> > <https://reviews.apache.org/r/8535/diff/1/?file=237228#file237228line600>
> >
> > Could we deal with some of these failures by just killing the framework/task?
As discussed offline, failing fast when there are file i/o errors keeps the code clean and simple.
> On Dec. 12, 2012, 6:51 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 1312
> > <https://reviews.apache.org/r/8535/diff/1/?file=237228#file237228line1312>
> >
> > Not following this ...
removed this in favor of just checking for error. I added a TODO in one of the earlier reviews to fix os::write/read and protobuf::write/read to not return bools.
> On Dec. 12, 2012, 6:51 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, lines 1330-1331
> > <https://reviews.apache.org/r/8535/diff/1/?file=237228#file237228line1330>
> >
> > Why a CHECK above and just a LOG here?
good catch. reverted everything to CHECK
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14360
-----------------------------------------------------------
On Dec. 12, 2012, 8:58 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 8:58 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 11cb64dd3f6d3992072582f2b6d6e6d9220eb48d
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp ede080069d814f410662f759806d1d5b260e8569
> src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
> src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
> Will write new tests as part of Part 5, when I write the analogous read/replay functions of the checkpoint functions.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14360
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30554>
Hmm, I'm not sure how I feel about these semantics. I think I'd prefer to see the frameworks tasks keep going lost or failed with a message "Couldn't launch task because framework expects checkpointing but slave doesn't support it ..." rather than the slave being killed. Thoughts?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30556>
I wonder if the flag should really be called 'checkpoint' and the 'recover' flag should be whether or not to recover from an existing checkpoint?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30553>
s/.//
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30558>
Could we deal with some of these failures by just killing the framework/task?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30560>
Ditto question above.
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30563>
CHECK_SOME?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30562>
Not following this ...
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30564>
Why a CHECK above and just a LOG here?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30566>
Ditto questions above.
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30568>
Same comments as above.
src/tests/protobuf_io_tests.cpp
<https://reviews.apache.org/r/8535/#comment30569>
Did the compiler want/need these?
- Benjamin Hindman
On Dec. 12, 2012, 8:58 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 8:58 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 11cb64dd3f6d3992072582f2b6d6e6d9220eb48d
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp ede080069d814f410662f759806d1d5b260e8569
> src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
> src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
> Will write new tests as part of Part 5, when I write the analogous read/replay functions of the checkpoint functions.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review15789
-----------------------------------------------------------
Ship it!
Although I would prefer to see the EXIT replaced with TASK_LOST.
src/slave/lxc_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment33995>
Seems better to go to LOST IMO, since this could happen potentially much later than when the slave starts, right?
Couldn't you just do the same as you're doing in runTask below, when the slave doesn't support checkpointing?
- Ben Mahler
On Jan. 19, 2013, 1:09 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2013, 1:09 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review16901
-----------------------------------------------------------
Ship it!
src/slave/flags.hpp
<https://reviews.apache.org/r/8535/#comment35876>
s/re-connect/reconnect/
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment35873>
I'd prefer you did frameworkInfo.checkpoint() everywhere because it's more clear that checkpoint "blocks" are framework specific and not slave specific.
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment35872>
Move this comment.
src/slave/state.cpp
<https://reviews.apache.org/r/8535/#comment35874>
Single quotes here and elsewhere?
src/tests/slave_state_tests.cpp
<https://reviews.apache.org/r/8535/#comment35875>
Did you remove this from paths.hpp too?
- Benjamin Hindman
On Feb. 19, 2013, 8:02 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 8:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
> src/tests/protobuf_io_tests.cpp 5c9bf4664d413a34859608dd727deacf3ce4b402
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
> On March 1, 2013, 12:15 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 505
> > <https://reviews.apache.org/r/8535/diff/8/?file=261376#file261376line505>
> >
> > Can you make this comment a little clearer? My suggestion:
> >
> > Before
> > // Checkpoint the forked pid, if necessary.
> > // We do this here instead of in the parent process, because if we do it
> > // in the slave, the slave might die immediately after it forks but before
> > // it writes the the pid to disk. This will result in an orphaned executor
> > // process that the slave has no idea about when doing recovery.
> >
> > s/the the/the
> >
> > After:
> > // Checkpoint the forked pid, if necessary.
> > // The checkpointing must be done in the forked process, because
> > // the slave process can die immediately after the isolation module forks but before it would have
> > // a chance to write the pid to disk. That would result in an orphaned
> > // executor process unknown to the slave when doing recovery.
> >
done. thanks.
> On March 1, 2013, 12:15 a.m., Ben Mahler wrote:
> > src/slave/lxc_isolation_module.cpp, line 124
> > <https://reviews.apache.org/r/8535/diff/8/?file=261380#file261380line124>
> >
> > Ditto on my earlier review, I'd rather this send TASK_FAILED or TASK_LOST with this message. Or at least, please add a TODO that this is not implemented because we're deprecating the lxc isolation module?
> >
> > Seems pretty bad that a framework can cause slaves to exit at will!
Thansk for the push. Decided to make this a CHECK instead.
Also added logic in slave/main.cpp to stop users from starting a slave w/ lxc and checkpointing.
I think failing fast is the right semantics.
> On March 1, 2013, 12:15 a.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 137
> > <https://reviews.apache.org/r/8535/diff/8/?file=261384#file261384line137>
> >
> > Why did you move these in this review, as opposed to the review where you introduced them?
moved them back.
> On March 1, 2013, 12:15 a.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 196
> > <https://reviews.apache.org/r/8535/diff/8/?file=261384#file261384line196>
> >
> > s/This callback is called/This continuation runs
I don't like "this continuation runs".
Instead, I went with s/callback//
> On March 1, 2013, 12:15 a.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 915
> > <https://reviews.apache.org/r/8535/diff/8/?file=261385#file261385line915>
> >
> > getLibprocessPIDPath sounds ambiguous, maybe this would be better named as getExecutorDriverPIDPath?
We chose "libprocess" in the pid name, to differentiate with the os pid ('forkedpid').
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review17210
-----------------------------------------------------------
On Feb. 23, 2013, 8:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
> src/tests/protobuf_io_tests.cpp 5bd1e676761ac35d30b9491fb397830c8a008795
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review17210
-----------------------------------------------------------
Ship it!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment36502>
Can you make this comment a little clearer? My suggestion:
Before
// Checkpoint the forked pid, if necessary.
// We do this here instead of in the parent process, because if we do it
// in the slave, the slave might die immediately after it forks but before
// it writes the the pid to disk. This will result in an orphaned executor
// process that the slave has no idea about when doing recovery.
s/the the/the
After:
// Checkpoint the forked pid, if necessary.
// The checkpointing must be done in the forked process, because
// the slave process can die immediately after the isolation module forks but before it would have
// a chance to write the pid to disk. That would result in an orphaned
// executor process unknown to the slave when doing recovery.
src/slave/flags.hpp
<https://reviews.apache.org/r/8535/#comment36503>
Since operators will be reading this string, let's make it very clear for them:
Before:
"Whether to checkpoint relevant info about the slave and frameworks\n"
"to disk. This will enable a restarted slave to recover\n"
"status updates and reconnect with old executors, if so desired."
After:
"Enable checkpointing of slave and framework information to disk.\n"
"This enables a restarted slave to recover status updates and reconnect\n"
"with old executors, if --recover is specified."
What is if so desired? If --recover is specified?
src/slave/lxc_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment36505>
Ditto on my earlier review, I'd rather this send TASK_FAILED or TASK_LOST with this message. Or at least, please add a TODO that this is not implemented because we're deprecating the lxc isolation module?
Seems pretty bad that a framework can cause slaves to exit at will!
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment36506>
ditto above comment
src/slave/slave.hpp
<https://reviews.apache.org/r/8535/#comment36511>
Why did you move these in this review, as opposed to the review where you introduced them?
src/slave/slave.hpp
<https://reviews.apache.org/r/8535/#comment36508>
s/This callback is called/This continuation runs
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment36512>
s/but the slave doesn't support checkpointing!/but checkpointing is not enabled on the slave!/
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment36513>
ditto "not enabled" vs "supported"
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment36509>
ditto from my review of part 3, with respect to just having statusUpdate
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment36510>
getLibprocessPIDPath sounds ambiguous, maybe this would be better named as getExecutorDriverPIDPath?
- Ben Mahler
On Feb. 23, 2013, 8:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
> src/tests/protobuf_io_tests.cpp 5bd1e676761ac35d30b9491fb397830c8a008795
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated March 13, 2013, 6:12 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased off trunk for posterity. no need for review.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/slave/flags.hpp b2441c773b13365a14b3daad4f81ae1ec0733439
src/slave/isolation_module.hpp d7cc02b8ea0cde591ee61059efd79fbacaa74769
src/slave/lxc_isolation_module.hpp 6be99038fb547e43e74ef5469f2339bbe82017cf
src/slave/lxc_isolation_module.cpp 1ed3c87f884089e032bba2ed89be109abe6c1c13
src/slave/main.cpp fd338662e3d63e6e683347179b80e07e178b6c98
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/process_based_isolation_module.hpp 6a4e6aef9ab4b13cce9341f6f35c6b24e6f8fde7
src/slave/process_based_isolation_module.cpp ff98d105af675dfc66070feaa43b42c1aa438fd8
src/slave/slave.hpp 11f17aef6d5deefe83b2d4706e4b8b24adaac5f4
src/slave/slave.cpp 889e9fe7671c9b158486fd7b85fef139c4ab8c9b
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
src/tests/protobuf_io_tests.cpp 5bd1e676761ac35d30b9491fb397830c8a008795
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated March 2, 2013, 12:14 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
src/slave/flags.hpp 9cd436cae5772b906b3566eb1f344904d5894214
src/slave/isolation_module.hpp d7cc02b8ea0cde591ee61059efd79fbacaa74769
src/slave/lxc_isolation_module.hpp 6be99038fb547e43e74ef5469f2339bbe82017cf
src/slave/lxc_isolation_module.cpp 1ed3c87f884089e032bba2ed89be109abe6c1c13
src/slave/main.cpp fd338662e3d63e6e683347179b80e07e178b6c98
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/process_based_isolation_module.hpp 6a4e6aef9ab4b13cce9341f6f35c6b24e6f8fde7
src/slave/process_based_isolation_module.cpp ff98d105af675dfc66070feaa43b42c1aa438fd8
src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
src/slave/slave.cpp c13d268c5e0e107902f64e30304a18128927a571
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
src/tests/protobuf_io_tests.cpp 5bd1e676761ac35d30b9491fb397830c8a008795
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp b648b631d8c11c26e6adfa3f5b16012044557e25
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Feb. 23, 2013, 8:12 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
src/tests/protobuf_io_tests.cpp 5bd1e676761ac35d30b9491fb397830c8a008795
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Feb. 19, 2013, 8:02 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Instead of storing slave info under meta/slaves, its now stored under meta/slaves/slaveid. This makes it consistent with all other states.
Current slave is capture by having a symbolic link "meta/slaves/latest", similar to what we do for find the latest run.
Other minor re-ordering in checkpoints.
Rebased off trunk.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b
src/tests/protobuf_io_tests.cpp 5c9bf4664d413a34859608dd727deacf3ce4b402
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Jan. 29, 2013, 11:04 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added comments to helpers. doesn't address BenM's comments, yet
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Jan. 19, 2013, 1:09 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments and rebased off trunk.
there are corresponding changes in downstream reviews, but i will sneak them when i address their comments.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 16, 2013, 7:43 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 573
> > <https://reviews.apache.org/r/8535/diff/2-3/?file=237646#file237646line573>
> >
> > Instead of a helper, you could use an option and compute it only when checkpoint is true above. A helper seems a bit clunky, no?
>
> Vinod Kone wrote:
> I don't get it. Option for what? Do you mean make 'path' and 't' as options and pre-compute at the top when if (executor!=NULL)? Doesn't work for when we have to create the executor at the bottom.
Yeah, doesn't work at the bottom but I imagined it would the options would just be contained _inside_ the if (executor != NULL). I guess this doesn't save much since it doesn't eliminate the last computation at the bottom.
It's unfortunate that this function was already fairly complex =/, it's now 175 lines!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14476
-----------------------------------------------------------
On Jan. 19, 2013, 1:09 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2013, 1:09 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 16, 2013, 7:43 p.m., Ben Mahler wrote:
> > src/slave/lxc_isolation_module.cpp, line 122
> > <https://reviews.apache.org/r/8535/diff/2-3/?file=237642#file237642line122>
> >
> > Hm.. failing at runtime seems pretty bad, even though we're deprecated lxc, have you considered logging an error and proceeding?
> >
> > Or an EXIT?
I think failing fast is better here. Using EXIT though.
> On Jan. 16, 2013, 7:43 p.m., Ben Mahler wrote:
> > src/slave/process_based_isolation_module.cpp, line 198
> > <https://reviews.apache.org/r/8535/diff/2-3/?file=237644#file237644line198>
> >
> > Why did you kill all the logging for checkpointing?
> >
> > Seems useful for debugging any issues with this.
Had to because we don't want to using any GLOG functions (due to lock inheritance issue we found in launcher.cpp).
I will print it out to cerr here though.
> On Jan. 16, 2013, 7:43 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 573
> > <https://reviews.apache.org/r/8535/diff/2-3/?file=237646#file237646line573>
> >
> > Instead of a helper, you could use an option and compute it only when checkpoint is true above. A helper seems a bit clunky, no?
I don't get it. Option for what? Do you mean make 'path' and 't' as options and pre-compute at the top when if (executor!=NULL)? Doesn't work for when we have to create the executor at the bottom.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14476
-----------------------------------------------------------
On Dec. 19, 2012, 1:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 1:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14476
-----------------------------------------------------------
I forgot to publish these comments, they may be stale. I'll do another review as well.
src/slave/lxc_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30846>
Hm.. failing at runtime seems pretty bad, even though we're deprecated lxc, have you considered logging an error and proceeding?
Or an EXIT?
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30847>
Why did you kill all the logging for checkpointing?
Seems useful for debugging any issues with this.
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30848>
Instead of a helper, you could use an option and compute it only when checkpoint is true above. A helper seems a bit clunky, no?
- Ben Mahler
On Dec. 19, 2012, 1:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 1:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 16, 2013, 8:07 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 429
> > <https://reviews.apache.org/r/8535/diff/4/?file=240998#file240998line429>
> >
> > What is path?
> >
> > I think this should be renamed.. to pidCheckpointPath?
> >
> > For that matter, isn't the bool redundant?
> > checkpoint == pidCheckpointPath.isSome(), right?
> >
> > ditto on this comment in each isolation module
I used 'path' everywhere, to indicate where one has to checkpoint the data. I think its un-ambigous?
Me and BeH had discussions on whether we want to use 'checkpoint' or just infer based on path as you suggested. We decided on being explicit to make the api clearer. Also, fwiw, in further downstream reviews the 'checkpoint' parameter has been removed and we directly get it from frameworkInfo.
> On Jan. 16, 2013, 8:07 p.m., Ben Mahler wrote:
> > src/tests/slave_state_tests.cpp, line 183
> > <https://reviews.apache.org/r/8535/diff/4/?file=241010#file241010line183>
> >
> > You also want to check that this is the result is true, right?
> On Jan. 16, 2013, 8:07 p.m., Ben Mahler wrote:
> > src/messages/messages.proto, line 263
> > <https://reviews.apache.org/r/8535/diff/4/?file=240996#file240996line263>
> >
> > I can't give a shipit if this is required, you'll have to either:
> > 1. Convince me it's ok to leave this required.
> > 2. Document an upgrade plan that allows for this to be required.
> >
> > You might do 1 by telling me that this message is only contained within the slave, and never hits the master?
> > Will the slave and executors be rolled to have this new proto at the same time?
Good catch. Decided to actually get rid of checkpointing this pid. So, we no longer need this field in the protobuf.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review15402
-----------------------------------------------------------
On Dec. 19, 2012, 1:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 1:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review15402
-----------------------------------------------------------
src/messages/messages.proto
<https://reviews.apache.org/r/8535/#comment33166>
I can't give a shipit if this is required, you'll have to either:
1. Convince me it's ok to leave this required.
2. Document an upgrade plan that allows for this to be required.
You might do 1 by telling me that this message is only contained within the slave, and never hits the master?
Will the slave and executors be rolled to have this new proto at the same time?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment33168>
What is path?
I think this should be renamed.. to pidCheckpointPath?
For that matter, isn't the bool redundant?
checkpoint == pidCheckpointPath.isSome(), right?
ditto on this comment in each isolation module
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment33169>
"Could not" or "Unable to" or "Failed to"
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment33170>
Kill this TODO? Seems outdated..
src/tests/slave_state_tests.cpp
<https://reviews.apache.org/r/8535/#comment33171>
You also want to check that this is the result is true, right?
- Ben Mahler
On Dec. 19, 2012, 1:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 1:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
> src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Dec. 19, 2012, 1:32 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
minor fixes.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Dec. 13, 2012, 11:06 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Refactored checkpoint*() functions into a single checkpoint() function and moved it to state:: namespace.
Updated checkpoint tests.
BenM's comments.
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445
src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e
src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing (updated)
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 13, 2012, 7:33 p.m., Ben Mahler wrote:
> > src/messages/messages.proto, line 263
> > <https://reviews.apache.org/r/8535/diff/2/?file=237636#file237636line263>
> >
> > Again, I'm scared about adding required fields.
lets get back to this when we have all the slave recovery in place.
> On Dec. 13, 2012, 7:33 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 433
> > <https://reviews.apache.org/r/8535/diff/2/?file=237638#file237638line433>
> >
> > I know it's the same but given what you're checking I think it's clearer to leave the logic to read literally as your message reads:
> >
> > !(checkpoint && path.isNone())
> >
> > or
> >
> > if (checkpoint) {
> > CHECK_SOME(path) << "Blah";
> > }
sure. i like the first one better.
> On Dec. 13, 2012, 7:33 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 528
> > <https://reviews.apache.org/r/8535/diff/2/?file=237638#file237638line528>
> >
> > Use CHECK_SOME (you don't need to store the write result if you don't like).
> > Change the message to use the 'failed' format.
> >
> > CHECK_SOME(os::write(path.get(), stringify(getpid())))
> > << "Failed to checkpoint forked pid " << add pid to message?
done. btw, checkpointing is now moved into slave/state.cpp
> On Dec. 13, 2012, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 213
> > <https://reviews.apache.org/r/8535/diff/2/?file=237645#file237645line213>
> >
> > With checkpointing enabled, will the slave id stay the same across recovered rolling of the slave?
yes!
> On Dec. 13, 2012, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 515
> > <https://reviews.apache.org/r/8535/diff/2/?file=237646#file237646line515>
> >
> > Do we end these messages with periods?
no we don't
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14456
-----------------------------------------------------------
On Dec. 12, 2012, 11:16 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 11:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
> Will write new tests as part of Part 5, when I write the analogous read/replay functions of the checkpoint functions.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/#review14456
-----------------------------------------------------------
src/exec/exec.cpp
<https://reviews.apache.org/r/8535/#comment30767>
Can you add these for getpid?
#include <unistd.h>
#include <sys/types.h>
src/messages/messages.proto
<https://reviews.apache.org/r/8535/#comment30768>
Again, I'm scared about adding required fields.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30769>
I know it's the same but given what you're checking I think it's clearer to leave the logic to read literally as your message reads:
!(checkpoint && path.isNone())
or
if (checkpoint) {
CHECK_SOME(path) << "Blah";
}
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30771>
Use CHECK_SOME (you don't need to store the write result if you don't like).
Change the message to use the 'failed' format.
CHECK_SOME(os::write(path.get(), stringify(getpid())))
<< "Failed to checkpoint forked pid " << add pid to message?
src/slave/flags.hpp
<https://reviews.apache.org/r/8535/#comment30772>
s/Enable checkpointing/checkpoint
src/slave/flags.hpp
<https://reviews.apache.org/r/8535/#comment30776>
Can you be more explicit about the fact that checkpoint enables slave recovery? Or is there another flag for that?
Can you mention that we checkpoint to disk? (there's a cost of disk io)
src/slave/lxc_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30777>
ditto on the check
src/slave/lxc_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30778>
ditto on my other comments with check_some and the check message
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30779>
ditto on the check
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/8535/#comment30780>
ditto on the check_some and the check message
src/slave/slave.hpp
<https://reviews.apache.org/r/8535/#comment30781>
With checkpointing enabled, will the slave id stay the same across recovered rolling of the slave?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30782>
Do we end these messages with periods?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30786>
Can you add a relevant message?
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30783>
might as well not store result, if not storing the mkdir result either:
CHECK_SOME(::protobuf::write(path, id)) << "Error checkpointing slave id " << id;
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30787>
ditto for adding message
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30784>
ditto for not storing result if you don't like
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30788>
ditto for adding message
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30789>
ditto for not storing if you don't like
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30790>
ditto for not storing
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30791>
ditto for adding message
src/slave/slave.cpp
<https://reviews.apache.org/r/8535/#comment30793>
ditto for not storing
- Ben Mahler
On Dec. 12, 2012, 11:16 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8535/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 11:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
> src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
> src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
> src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
> src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
> src/tests/status_update_manager_tests.cpp PRE-CREATION
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8535/diff/
>
>
> Testing
> -------
>
> make check
>
> Will write new tests as part of Part 5, when I write the analogous read/replay functions of the checkpoint functions.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Quatre): Checkpointed slave,
framework, executor and task
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8535/
-----------------------------------------------------------
(Updated Dec. 12, 2012, 11:16 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
re-based off trunk.
BenH's comments
Description
-------
Checkpoints slave id, framework pid, executor pids (libprocess, execed, forked) and task info.
Diffs (updated)
-----
src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 0f2975d1adf874dba4d0a539eb5c99233cef6e6b
src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd
src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3
src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4
src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9
src/tests/status_update_manager_tests.cpp PRE-CREATION
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8535/diff/
Testing
-------
make check
Will write new tests as part of Part 5, when I write the analogous read/replay functions of the checkpoint functions.
Thanks,
Vinod Kone