> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 184-185
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line184>
> >
> > FYI, our style is to keep '+' on previous line and indent as though it
> > was a continued argument. Can you please do a quick pass and clean this up
> > everywhere? Thanks Till!
>
> Till Toenshoff wrote:
> return Failure("Cannot start already running container '" +
> containerId.value() + "'");
>
> It is, as a continued argument gets indented matching the argument start,
> correct?
>
Ok, got that wrong for some reason. The correct way should be:
return Failure("Cannot start already running container '" +
containerId.value() + "'");
Continued arguments always get 4 chars indention.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 48-50
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line48>
> >
> > Just FYI, if we ever decide to modify the interface of
> > Containerizer::usage,wait,destroy then we'll effectively break external
> > containerizer programs. If we actually had a
> > containerizer::Usage,Wait,Destroy protobuf then we could likely add
> > optional fields to them as a way of extending the interface in the future.
>
> Till Toenshoff wrote:
> That is a very good point I did not see earlier - adding these to
> containerizer.proto now.
https://reviews.apache.org/r/20668/
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/tests/external_containerizer_test.cpp, lines 215-217
> > <https://reviews.apache.org/r/17567/diff/20/?file=562063#file562063line215>
> >
> > The way that you've done this totally works, but I wanted to suggest an
> > alternative that we use other places because it's fairly extensible and
> > very much in the 'mocking' mindset.
> >
> > Rather than extend ExternalContainerizer with a
> > TestExternalContainerizer make it a MockExternalContainerizer and mock the
> > 'launch' method:
> >
> > class MockExternalContainerizer : public ExternalContainerizer
> > {
> > public:
> > MockExternalContainerizer()
> > {
> > // Set up defaults for mocked methods.
> > // NOTE: See TestContainerizer::setup for why we use
> > // 'EXPECT_CALL' and 'WillRepeatedly' here instead of
> >
> > // 'ON_CALL' and 'WillByDefault'.
> > EXPECT_CALL(*this, launch(_, _, _, _, _, _, _))
> > .WillRepeatedly(Invoke(this, &ExternalContainerizer::launch));
> > }
> >
> > MOCK_METHOD7(
> > launch,
> > process::Future<Nothing>(
> > const ContainerID&,
> > const ExecutorInfo&,
> > const std::string&,
> > const Option<std::string>&,
> > const SlaveID&,
> > const process::PID<slave::Slave>&,
> > bool checkpoint));
> > };
> >
> > Now you can set up an expectation to get the launch in the test body!
> > That is, up by 'driver.launchTasks' you can do (or some variation):
> >
> > // Expect the launch but don't do anything.
> > Future<Nothing> launch;
> > EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
> > .WillOnce(DoAll(FutureSatisfy(&launch),
> > Invoke(&containerizer,
> > &ExternalContainerizer::launch)));
> >
> >
> > And later you can wait in the test until you know the launch has
> > occurred:
> >
> > AWAIT_READY(launch);
> >
> > And in your case since you wanted the container ID you could explicitly
> > pull that out in the expectation:
> >
> > Future<ContainerID> containerId;
> > EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
> > .WillOnce(DoAll(FutureArg<0>(&containerId),
> > Invoke(&containerizer,
> > &ExternalContainerizer::launch)));
> >
> > ...
> >
> > AWAIT_READY(containerId);
> >
> > Kind of cool right!? See https://reviews.apache.org/r/20179 for how
> > this is technique is being used other places.
While this indeed is really cool, it also seems to be rather fragile when it
comes to mocking overloaded functions (Launch in its two new variants). It
might be worth investigating the details here but for now, I ended up
introducing a proxy function within the Mocked class to get the invocations
working without causing infinite recursion - not beautiful but working fine.
Thanks Niklas for coming up with this workaround!
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/tests/external_containerizer_test.cpp, lines 226-230
> > <https://reviews.apache.org/r/17567/diff/20/?file=562063#file562063line226>
> >
> > Just so I'm clear, we're returning dummy data right!? Maybe we should
> > add a TODO here that captures that and also add a TODO in the Python code
> > that captures that we should eventually use mesos-usage! (After I get it
> > committed, will do that pronto and it will be another utility we might want
> > beyond just mesos-executor as mentioned before).
Aye
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review40828
-----------------------------------------------------------
On April 24, 2014, 8:14 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 8:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> Bugs: MESOS-816
> https://issues.apache.org/jira/browse/MESOS-816
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This patch adds the so-called external containerizer. This
> containerizer delegates all containerizer calls directly to
> an external containerizer program (which can be specified on
> start-up).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < Usage > ResourceStatistics
> wait < Wait > Termination
> destroy < Destroy
>
> When protocol buffers need to be provided, the Mesos side of
> the external containerizer implementation will serialize the
> protos on stdin and vice-versa for reading protos on stdout as
> drafted in the above scheme.
>
>
> Diffs
> -----
>
> configure.ac c964452
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp 722f3fe
> src/slave/flags.hpp d5c54c0
> src/tests/external_containerizer_test.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17567/diff/
>
>
> Testing
> -------
>
> make check and functional testing.
>
>
> Thanks,
>
> Till Toenshoff
>
>
