You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Eric Biederman <eb...@xmission.com> on 2013/07/29 23:57:56 UTC
Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
Repository: mesos
Description
-------
balloon_framework_test.sh: Tweak so the test runs properly
Change the location of the mesos hierarchy and the mesos root directory
to match other tests.
Stop exporting variables that begin with MESOS_ into the mesos environment.
mesos-master and mesos-slave will not start if their are unrecognized variables
that begin with MESOS_ in the environment.
Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
will be able to find all of their pieces and execute proper without
needing to be installed.
Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
fully cleaned it up. This is necessary to prevent a problem where the hierarchy
can never be mounted again.
Update the command line options to mesos-master to pass the hierarchy and
the mesos root directory in that hierarchy.
Clearly report when the balloon hierarchy returns with an unexpected return
code. Indicating the test failed because of a problem in the baloon framework
or within mesos.
With this set of changes the test is running properly. And when not tripping
over kernel bugs the test appears to pass.
Diffs
-----
src/tests/balloon_framework_test.sh 4309f40
Diff: https://reviews.apache.org/r/13034/diff/
Testing
-------
su -c 'make check' and watched the kernel crash!
Thanks,
Eric Biederman
Re: Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/#review25393
-----------------------------------------------------------
Ship it!
Thanks Eric! Can you mark this as submitted? I've committed this along with a small patch to use mktemp for the slave work directory.
- Ben Mahler
On July 29, 2013, 9:57 p.m., Eric Biederman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13034/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> balloon_framework_test.sh: Tweak so the test runs properly
>
> Change the location of the mesos hierarchy and the mesos root directory
> to match other tests.
>
> Stop exporting variables that begin with MESOS_ into the mesos environment.
> mesos-master and mesos-slave will not start if their are unrecognized variables
> that begin with MESOS_ in the environment.
>
> Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
> will be able to find all of their pieces and execute proper without
> needing to be installed.
>
> Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
> fully cleaned it up. This is necessary to prevent a problem where the hierarchy
> can never be mounted again.
>
> Update the command line options to mesos-master to pass the hierarchy and
> the mesos root directory in that hierarchy.
>
> Clearly report when the balloon hierarchy returns with an unexpected return
> code. Indicating the test failed because of a problem in the baloon framework
> or within mesos.
>
> With this set of changes the test is running properly. And when not tripping
> over kernel bugs the test appears to pass.
>
>
> Diffs
> -----
>
> src/tests/balloon_framework_test.sh 4309f40
>
> Diff: https://reviews.apache.org/r/13034/diff/
>
>
> Testing
> -------
>
> su -c 'make check' and watched the kernel crash!
>
>
> Thanks,
>
> Eric Biederman
>
>
Re: Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
Posted by Vinod Kone <vi...@gmail.com>.
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > src/tests/balloon_framework_test.sh, lines 57-60
> > <https://reviews.apache.org/r/13034/diff/1/?file=330349#file330349line57>
> >
> > Can you add a comment for why are doing this. You can pluck this from description in this RB. Also note that, this behavior of mesos failing on unrecognized flags might be changing in near future.
by unrecognized flags, i meant unrecognized MESOS_* variables in the environment.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/#review24265
-----------------------------------------------------------
On July 29, 2013, 9:57 p.m., Eric Biederman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13034/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> balloon_framework_test.sh: Tweak so the test runs properly
>
> Change the location of the mesos hierarchy and the mesos root directory
> to match other tests.
>
> Stop exporting variables that begin with MESOS_ into the mesos environment.
> mesos-master and mesos-slave will not start if their are unrecognized variables
> that begin with MESOS_ in the environment.
>
> Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
> will be able to find all of their pieces and execute proper without
> needing to be installed.
>
> Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
> fully cleaned it up. This is necessary to prevent a problem where the hierarchy
> can never be mounted again.
>
> Update the command line options to mesos-master to pass the hierarchy and
> the mesos root directory in that hierarchy.
>
> Clearly report when the balloon hierarchy returns with an unexpected return
> code. Indicating the test failed because of a problem in the baloon framework
> or within mesos.
>
> With this set of changes the test is running properly. And when not tripping
> over kernel bugs the test appears to pass.
>
>
> Diffs
> -----
>
> src/tests/balloon_framework_test.sh 4309f40
>
> Diff: https://reviews.apache.org/r/13034/diff/
>
>
> Testing
> -------
>
> su -c 'make check' and watched the kernel crash!
>
>
> Thanks,
>
> Eric Biederman
>
>
Re: Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
Posted by Vinod Kone <vi...@gmail.com>.
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > src/tests/balloon_framework_test.sh, lines 54-56
> > <https://reviews.apache.org/r/13034/diff/1/?file=330349#file330349line54>
> >
> > Any reason why you extracted these into variables? Also, if extracting is necessary, we prefer to declare them close to where they are used.
>
> Eric Biederman wrote:
> It is necessary to create variables and to do it here. MESOS_BUILD_DIR and MESOS_SOURCE_DIR must be unset, and this is the last place we can use them before they can be unset.
>
gotcha.
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > src/tests/balloon_framework_test.sh, lines 57-60
> > <https://reviews.apache.org/r/13034/diff/1/?file=330349#file330349line57>
> >
> > Can you add a comment for why are doing this. You can pluck this from description in this RB. Also note that, this behavior of mesos failing on unrecognized flags might be changing in near future.
>
> Vinod Kone wrote:
> by unrecognized flags, i meant unrecognized MESOS_* variables in the environment.
>
> Eric Biederman wrote:
> This will continue to work if mesos stops failing on unrecognized MESOS_* environment variables so as a future change that is irrelevant here.
>
> Adding an extra comment seems reasonable as a follow on change. I don't see it worth holding up a necessary bug fix, or invalidating the testing I have already done.
>
Hey Eric. Just to be clear I'm not suggesting to drop this change/fix. We need this to fix the test. I was suggesting you add a comment on why you were doing unsets. This would help other people understand the context when they look at the code at a later point in time.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/#review24265
-----------------------------------------------------------
On July 29, 2013, 9:57 p.m., Eric Biederman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13034/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> balloon_framework_test.sh: Tweak so the test runs properly
>
> Change the location of the mesos hierarchy and the mesos root directory
> to match other tests.
>
> Stop exporting variables that begin with MESOS_ into the mesos environment.
> mesos-master and mesos-slave will not start if their are unrecognized variables
> that begin with MESOS_ in the environment.
>
> Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
> will be able to find all of their pieces and execute proper without
> needing to be installed.
>
> Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
> fully cleaned it up. This is necessary to prevent a problem where the hierarchy
> can never be mounted again.
>
> Update the command line options to mesos-master to pass the hierarchy and
> the mesos root directory in that hierarchy.
>
> Clearly report when the balloon hierarchy returns with an unexpected return
> code. Indicating the test failed because of a problem in the baloon framework
> or within mesos.
>
> With this set of changes the test is running properly. And when not tripping
> over kernel bugs the test appears to pass.
>
>
> Diffs
> -----
>
> src/tests/balloon_framework_test.sh 4309f40
>
> Diff: https://reviews.apache.org/r/13034/diff/
>
>
> Testing
> -------
>
> su -c 'make check' and watched the kernel crash!
>
>
> Thanks,
>
> Eric Biederman
>
>
Re: Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
Posted by Eric Biederman <eb...@xmission.com>.
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > Thanks for cleaning this up!
> >
> > Does it also work when running with "./src/tests/balloon_framework_test.sh" inside the build directory?
Yes. I have been running it as: MESOS_SOURCE_DIR=$PWD MESOS_BUILD_DIR=$PWD src/tests/balloon_framework_test.sh
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > src/tests/balloon_framework_test.sh, lines 54-56
> > <https://reviews.apache.org/r/13034/diff/1/?file=330349#file330349line54>
> >
> > Any reason why you extracted these into variables? Also, if extracting is necessary, we prefer to declare them close to where they are used.
It is necessary to create variables and to do it here. MESOS_BUILD_DIR and MESOS_SOURCE_DIR must be unset, and this is the last place we can use them before they can be unset.
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > src/tests/balloon_framework_test.sh, lines 57-60
> > <https://reviews.apache.org/r/13034/diff/1/?file=330349#file330349line57>
> >
> > Can you add a comment for why are doing this. You can pluck this from description in this RB. Also note that, this behavior of mesos failing on unrecognized flags might be changing in near future.
>
> Vinod Kone wrote:
> by unrecognized flags, i meant unrecognized MESOS_* variables in the environment.
This will continue to work if mesos stops failing on unrecognized MESOS_* environment variables so as a future change that is irrelevant here.
Adding an extra comment seems reasonable as a follow on change. I don't see it worth holding up a necessary bug fix, or invalidating the testing I have already done.
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/#review24265
-----------------------------------------------------------
On July 29, 2013, 9:57 p.m., Eric Biederman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13034/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> balloon_framework_test.sh: Tweak so the test runs properly
>
> Change the location of the mesos hierarchy and the mesos root directory
> to match other tests.
>
> Stop exporting variables that begin with MESOS_ into the mesos environment.
> mesos-master and mesos-slave will not start if their are unrecognized variables
> that begin with MESOS_ in the environment.
>
> Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
> will be able to find all of their pieces and execute proper without
> needing to be installed.
>
> Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
> fully cleaned it up. This is necessary to prevent a problem where the hierarchy
> can never be mounted again.
>
> Update the command line options to mesos-master to pass the hierarchy and
> the mesos root directory in that hierarchy.
>
> Clearly report when the balloon hierarchy returns with an unexpected return
> code. Indicating the test failed because of a problem in the baloon framework
> or within mesos.
>
> With this set of changes the test is running properly. And when not tripping
> over kernel bugs the test appears to pass.
>
>
> Diffs
> -----
>
> src/tests/balloon_framework_test.sh 4309f40
>
> Diff: https://reviews.apache.org/r/13034/diff/
>
>
> Testing
> -------
>
> su -c 'make check' and watched the kernel crash!
>
>
> Thanks,
>
> Eric Biederman
>
>
Re: Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
Posted by Eric Biederman <eb...@xmission.com>.
> On July 30, 2013, 7:22 p.m., Vinod Kone wrote:
> > src/tests/balloon_framework_test.sh, lines 57-60
> > <https://reviews.apache.org/r/13034/diff/1/?file=330349#file330349line57>
> >
> > Can you add a comment for why are doing this. You can pluck this from description in this RB. Also note that, this behavior of mesos failing on unrecognized flags might be changing in near future.
>
> Vinod Kone wrote:
> by unrecognized flags, i meant unrecognized MESOS_* variables in the environment.
>
> Eric Biederman wrote:
> This will continue to work if mesos stops failing on unrecognized MESOS_* environment variables so as a future change that is irrelevant here.
>
> Adding an extra comment seems reasonable as a follow on change. I don't see it worth holding up a necessary bug fix, or invalidating the testing I have already done.
>
>
> Vinod Kone wrote:
> Hey Eric. Just to be clear I'm not suggesting to drop this change/fix. We need this to fix the test. I was suggesting you add a comment on why you were doing unsets. This would help other people understand the context when they look at the code at a later point in time.
I will be happy to add a comment just not in this patch/reviewboard. As respinning the patch when there are no bugs and invalidating all of the testing I have done just to add a comment that makes the code marginally more readable is silly.
I have seen way to many pieces of code destroyed by people making suggested changes and not bothering to rerun all of their tests because they know the changes won't affect anything.
I will send a follow on change to add the missing comment. Does that sound good?
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/#review24265
-----------------------------------------------------------
On July 29, 2013, 9:57 p.m., Eric Biederman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13034/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> balloon_framework_test.sh: Tweak so the test runs properly
>
> Change the location of the mesos hierarchy and the mesos root directory
> to match other tests.
>
> Stop exporting variables that begin with MESOS_ into the mesos environment.
> mesos-master and mesos-slave will not start if their are unrecognized variables
> that begin with MESOS_ in the environment.
>
> Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
> will be able to find all of their pieces and execute proper without
> needing to be installed.
>
> Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
> fully cleaned it up. This is necessary to prevent a problem where the hierarchy
> can never be mounted again.
>
> Update the command line options to mesos-master to pass the hierarchy and
> the mesos root directory in that hierarchy.
>
> Clearly report when the balloon hierarchy returns with an unexpected return
> code. Indicating the test failed because of a problem in the baloon framework
> or within mesos.
>
> With this set of changes the test is running properly. And when not tripping
> over kernel bugs the test appears to pass.
>
>
> Diffs
> -----
>
> src/tests/balloon_framework_test.sh 4309f40
>
> Diff: https://reviews.apache.org/r/13034/diff/
>
>
> Testing
> -------
>
> su -c 'make check' and watched the kernel crash!
>
>
> Thanks,
>
> Eric Biederman
>
>
Re: Review Request 13034: balloon_framework_test.sh: Tweak so the test runs
properly
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13034/#review24265
-----------------------------------------------------------
Thanks for cleaning this up!
Does it also work when running with "./src/tests/balloon_framework_test.sh" inside the build directory?
src/tests/balloon_framework_test.sh
<https://reviews.apache.org/r/13034/#comment48087>
Any reason why you extracted these into variables? Also, if extracting is necessary, we prefer to declare them close to where they are used.
src/tests/balloon_framework_test.sh
<https://reviews.apache.org/r/13034/#comment48088>
Can you add a comment for why are doing this. You can pluck this from description in this RB. Also note that, this behavior of mesos failing on unrecognized flags might be changing in near future.
- Vinod Kone
On July 29, 2013, 9:57 p.m., Eric Biederman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13034/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> balloon_framework_test.sh: Tweak so the test runs properly
>
> Change the location of the mesos hierarchy and the mesos root directory
> to match other tests.
>
> Stop exporting variables that begin with MESOS_ into the mesos environment.
> mesos-master and mesos-slave will not start if their are unrecognized variables
> that begin with MESOS_ in the environment.
>
> Set the LD_LIBRARY_PATH so that the balloon binaries built with libtool
> will be able to find all of their pieces and execute proper without
> needing to be installed.
>
> Cleanup the cleanup code so we don't unmount a cgroup hierarchy unless we have
> fully cleaned it up. This is necessary to prevent a problem where the hierarchy
> can never be mounted again.
>
> Update the command line options to mesos-master to pass the hierarchy and
> the mesos root directory in that hierarchy.
>
> Clearly report when the balloon hierarchy returns with an unexpected return
> code. Indicating the test failed because of a problem in the baloon framework
> or within mesos.
>
> With this set of changes the test is running properly. And when not tripping
> over kernel bugs the test appears to pass.
>
>
> Diffs
> -----
>
> src/tests/balloon_framework_test.sh 4309f40
>
> Diff: https://reviews.apache.org/r/13034/diff/
>
>
> Testing
> -------
>
> su -c 'make check' and watched the kernel crash!
>
>
> Thanks,
>
> Eric Biederman
>
>