Merge lp:~doanac/linaro-image-tools/slash-proc-spam-version2 into lp:linaro-image-tools/11.11

[Andy Doan]

This is version 2 of the patch I discussed with Loic to fix the error messages.

[Guilherme Salgado (salgado)]

Thanks for working on this, Andy. I like your solution but there's one
thing which doesn't seem right to me; see below.

On Tue, 2011-01-25 at 18:22 +0000, Andy Doan wrote:
[...]
> === modified file 'linaro_media_create/hwpack.py'
> --- linaro_media_create/hwpack.py 2011-01-17 17:51:13 +0000
> +++ linaro_media_create/hwpack.py 2011-01-25 18:22:39 +0000
> def install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes):
> @@ -63,12 +63,14 @@
> Also register a function in local_atexit to unmount that /proc filesystem.
> """
> chroot_proc = os.path.join(chroot_dir, 'proc')
> +
> + def umount_chroot_proc():
> + cmd_runner.run(['umount', '-v', chroot_proc], as_root=True).wait()
> + local_atexit.append(umount_chroot_proc)
> +

I don't think we want to register this on local_atexit before mounting
the proc fs because if for some reason we fail to mount it, this will be
called and will also fail, maybe preventing other cleanup functions from
being called.

> proc = cmd_runner.run(
> ['mount', 'proc', chroot_proc, '-t', 'proc'], as_root=True)
> proc.wait()
> - def umount_chroot_proc():
> - cmd_runner.run(['umount', '-v', chroot_proc], as_root=True).wait()
> - local_atexit.append(umount_chroot_proc)
>
>
> def copy_file(filepath, directory):
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

[Loïc Minier (lool)]

But if your interrupt during mount, then it might not get unmounted?

How about we ignore errors from umount? Or test whether it's mounted before unmounting

[Andy Doan (doanac)]

On 01/25/2011 03:15 PM, Guilherme Salgado wrote:
> I don't think we want to register this on local_atexit before mounting
> the proc fs because if for some reason we fail to mount it, this will be
> called and will also fail, maybe preventing other cleanup functions from
> being called.

We sort of have a timing thing here. Loic actually suggested I make this
change.

If you leave the umount registration *after* the call to mount then you
have a small window where a user could hit ctrl-c between the mount and
the call to local_atext.append

I'm not a python expert, but would it be a smart approach to put the
atexit calls in a try/finally block? ie:
 def run_local_atexit_funcs():
     # Run the funcs in LIFO order, just like atexit does.
     while len(local_atexit) > 0:
- local_atexit.pop()()
+ try:
+ local_atexit.pop()()
+ finally:

Then we can safely call the umount even if it has failed?

[Guilherme Salgado (salgado)]

On Tue, 2011-01-25 at 23:31 +0000, Andy Doan wrote:
> On 01/25/2011 03:15 PM, Guilherme Salgado wrote:
> > I don't think we want to register this on local_atexit before mounting
> > the proc fs because if for some reason we fail to mount it, this will be
> > called and will also fail, maybe preventing other cleanup functions from
> > being called.
>
> We sort of have a timing thing here. Loic actually suggested I make this
> change.
>
> If you leave the umount registration *after* the call to mount then you
> have a small window where a user could hit ctrl-c between the mount and
> the call to local_atext.append
>
> I'm not a python expert, but would it be a smart approach to put the
> atexit calls in a try/finally block? ie:
> def run_local_atexit_funcs():
> # Run the funcs in LIFO order, just like atexit does.
> while len(local_atexit) > 0:
> - local_atexit.pop()()
> + try:
> + local_atexit.pop()()
> + finally:
>
> Then we can safely call the umount even if it has failed?

I like that. Another option would be to use subprocess.Popen() in the
cleanup functions as that doesn't raise an exception when the subprocess
ends with a non-zero return code, but I think using a try/finally is
better at ensuring all cleanup functions are executed.

--
Guilherme Salgado <https://launchpad.net/~salgado>

[James Westby (james-w)]

On Tue, 25 Jan 2011 23:31:04 -0000, Andy Doan <email address hidden> wrote:
> I'm not a python expert, but would it be a smart approach to put the
> atexit calls in a try/finally block? ie:
> def run_local_atexit_funcs():
> # Run the funcs in LIFO order, just like atexit does.
> while len(local_atexit) > 0:
> - local_atexit.pop()()
> + try:
> + local_atexit.pop()()
> + finally:
>
> Then we can safely call the umount even if it has failed?

The thing to be careful of here is reporting errors. If one fails and
then another is run it can fail with a knock-on error. We want both to
be reported, not just the last, as it can save hours of head scratching.

Thanks,

James

[Andy Doan (doanac)]

On 01/26/2011 07:13 AM, James Westby wrote:
> The thing to be careful of here is reporting errors. If one fails and
> then another is run it can fail with a knock-on error. We want both to
> be reported, not just the last, as it can save hours of head scratching.

Good point. I'm not sure if this is poor form in Python, but in Java
I've seen this type of approach (not elegant, but subtle bugs don't get
ignored by accident):

#for main loop logic:
try:
    mount_chroot_proc(chroot_dir)
    for hwpack_file in hwpack_files:
        install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes)
except Exception as e:
    print e
finally:
    run_local_atexit_funcs()

#cleanup logic:
def run_local_atexit_funcs():
    # Run the funcs in LIFO order, just like atexit does.
    while len(local_atexit) > 0:
        try:
            local_atexit.pop()()
        except Exception as e:
            print e

Another alternative: I know you guys are about to make some changes to
l-m-c so maybe we should drop this for now?

[James Westby (james-w)]

On Wed, 26 Jan 2011 18:58:41 -0000, Andy Doan <email address hidden> wrote:
> Good point. I'm not sure if this is poor form in Python, but in Java
> I've seen this type of approach (not elegant, but subtle bugs don't get
> ignored by accident):

Pretty much what I'd go for.

> Another alternative: I know you guys are about to make some changes to
> l-m-c so maybe we should drop this for now?

I don't know if we have anything planned in this area.

Thanks,

James

[Guilherme Salgado (salgado)]

On Wed, 2011-01-26 at 18:58 +0000, Andy Doan wrote:
> On 01/26/2011 07:13 AM, James Westby wrote:
> > The thing to be careful of here is reporting errors. If one fails and
> > then another is run it can fail with a knock-on error. We want both to
> > be reported, not just the last, as it can save hours of head scratching

[Guilherme Salgado (salgado)]

(Re-sending because Launchpad somehow failed to parsed my last message
and only included the first quoted paragraph in the comment.)

On Wed, 2011-01-26 at 18:58 +0000, Andy Doan wrote:
> On 01/26/2011 07:13 AM, James Westby wrote:
> > The thing to be careful of here is reporting errors. If one fails and
> > then another is run it can fail with a knock-on error. We want both to
> > be reported, not just the last, as it can save hours of head scratching

[Guilherme Salgado (salgado)]

(giving up on the email interface and now pasting my email here)

On Wed, 2011-01-26 at 18:58 +0000, Andy Doan wrote:
> On 01/26/2011 07:13 AM, James Westby wrote:
> > The thing to be careful of here is reporting errors. If one fails and
> > then another is run it can fail with a knock-on error. We want both to
> > be reported, not just the last, as it can save hours of head scratching.

This won't happen because on a try/finally the exception is re-raised
after the finally block is executed. I don't know what I had in mind
when I made my comment that the try/finally in run_local_atexit_funcs()
would make sure all cleanup functions executed.

>
> Good point. I'm not sure if this is poor form in Python, but in Java
> I've seen this type of approach (not elegant, but subtle bugs don't get
> ignored by accident):
>
> #for main loop logic:
> try:
> mount_chroot_proc(chroot_dir)
> for hwpack_file in hwpack_files:
> install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes)
> except Exception as e:
> print e
> finally:
> run_local_atexit_funcs()

We don't want this as it would swallow any exceptions raised by
install_hwpack() and let the script proceed as if the hwpack was
installed. I think the try/finally is the right thing here.

>
> #cleanup logic:
> def run_local_atexit_funcs():
> # Run the funcs in LIFO order, just like atexit does.
> while len(local_atexit) > 0:
> try:
> local_atexit.pop()()
> except Exception as e:
> print e

I think here we should do the same that the atexit module does:

    while len(local_atexit) > 0:
        func = local_atexit.pop()
        try:
            func()
        except SystemExit:
            exc_info = sys.exc_info()
        except:
            import traceback
            print >> sys.stderr, "Error in local_atexit:"
            traceback.print_exc()
            exc_info = sys.exc_info()

    if exc_info is not None:
        raise exc_info[0], exc_info[1], exc_info[2]

>
> Another alternative: I know you guys are about to make some changes to
> l-m-c so maybe we should drop this for now?

We're always making changes in lmc, but this is a nice fix for an
annoying bug so I don't see why we'd drop it.

[lp:~doanac/linaro-image-tools/slash-proc-spam-version2]

261. By Andy Doan on 2011-01-26

add exception handling to hwpack cleanup logic

This will help ensure all exit functions are attempted even
if one of them raises an exception

[Andy Doan (doanac)]

Okay. I've added another commit to this branch that I think satisfies concerns from both Guilherme and James.

[Guilherme Salgado (salgado)]

This looks great!

It'd be nice to have at least one test, just to show that run_local_atexit_funcs() is called even if install_hwpack() raises an exception. To do that you could easily use MockSomethingFixture to mock both functions, having the former just change an instance variable so you know it was called and the latter just raise an exception. After that you'd just call install_hwpacks(), but you'll also need to mock sys.stdout and our custom Popen (the existing test_install_hwpacks method there should work as an example)

review: Approve

[Andy Doan (doanac)]

> It'd be nice to have at least one test, just to show that
> run_local_atexit_funcs() is called even if install_hwpack() raises an
> exception. To do that you could easily use MockSomethingFixture to mock both
> functions, having the former just change an instance variable so you know it
> was called and the latter just raise an exception. After that you'd just call
> install_hwpacks(), but you'll also need to mock sys.stdout and our custom
> Popen (the existing test_install_hwpacks method there should work as an
> example)

I've put together a new patch to test this. I'm new to Python, but have done introspection in Java. Depending on how many beers you've had, this "setattr" feature of Python is either the most awesome thing ever or the most disgusting thing ever :)

salgado - I think this is what you were wanting me to add. Let me know what you think, and I can add this to the branch since it hasn't been merged yet.

=== modified file 'linaro_media_create/tests/fixtures.py'
--- linaro_media_create/tests/fixtures.py 2011-01-27 17:54:57 +0000
+++ linaro_media_create/tests/fixtures.py 2011-01-28 05:59:40 +0000
@@ -3,9 +3,7 @@
 import subprocess
 import tempfile

-from linaro_media_create import partitions
-from linaro_media_create import cmd_runner
-
+from linaro_media_create import (cmd_runner, hwpack, partitions)

 class CreateTempDirFixture(object):

@@ -122,10 +120,12 @@
     containing the positional arguments given to it to self.calls.
     """
     calls = None
+ count = 0
     return_value = None
     def __call__(self, *args):
         if self.calls is None:
             self.calls = []
+ self.count += 1
         self.calls.append(args)
         return self.return_value

@@ -137,3 +137,16 @@
         mock.return_value = ('', '')
         super(MockRunSfdiskCommandsFixture, self).__init__(
             partitions, 'run_sfdisk_commands', mock)
+
+class MockHWPackAtExitFixture(MockSomethingFixture):
+
+ def __init__(self):
+ mock = MockCallableWithPositionalArgs()
+ mock.return_value = ('', '', '')
+ super(MockHWPackAtExitFixture, self).__init__(
+ hwpack, 'run_local_atexit_funcs', mock)
+
+ def tearDown(self):
+ super(MockHWPackAtExitFixture, self).tearDown()
+ hwpack.local_atexit = []
+

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py 2011-01-27 18:43:55 +0000
+++ linaro_media_create/tests/test_media_create.py 2011-01-28 06:02:21 +0000
@@ -72,6 +72,7 @@
     MockCmdRunnerPopenFixture,
     MockSomethingFixture,
     MockRunSfdiskCommandsFixture,
+ MockHWPackAtExitFixture,
     )

@@ -1015,3 +1016,19 @@
              ['sudo', 'mv', '-f', '/tmp/dir/hosts', 'chroot/etc'],
              ['sudo', 'mv', '-f', '/tmp/dir/resolv.conf', 'chroot/etc']],
             fixture.mock.calls)
+
+ def test_hwpack_atexit(self):
+ def install_hwpack(p1, p2, p3):
+ raise Exception('hwpack mock exception')
+
+ self.useFixture(MockSomethingFixture(
+ sys, 'stdout', open('/dev/null', 'w')))
+ self.useFixture(MockCmdRunnerPopenFixture())
+ self.useFixture(MockSomethi...

[Guilherme Salgado (salgado)]

On Fri, 2011-01-28 at 06:13 +0000, Andy Doan wrote:
> > It'd be nice to have at least one test, just to show that
> > run_local_atexit_funcs() is called even if install_hwpack() raises an
> > exception. To do that you could easily use MockSomethingFixture to mock both
> > functions, having the former just change an instance variable so you know it
> > was called and the latter just raise an exception. After that you'd just call
> > install_hwpacks(), but you'll also need to mock sys.stdout and our custom
> > Popen (the existing test_install_hwpacks method there should work as an
> > example)
>
> I've put together a new patch to test this. I'm new to Python, but
> have done introspection in Java. Depending on how many beers you've
> had, this "setattr" feature of Python is either the most awesome thing
> ever or the most disgusting thing ever :)

Thanks for the patch, Andy. I guess I've gotten used to setattr() but I
can imagine how it feels the first time you see it.

>
> salgado - I think this is what you were wanting me to add. Let me know
> what you think, and I can add this to the branch since it hasn't been
> merged yet.

Yep, this is what I was thinking; I've just one suggestion below.

>
> === modified file 'linaro_media_create/tests/fixtures.py'
> --- linaro_media_create/tests/fixtures.py 2011-01-27 17:54:57 +0000
> +++ linaro_media_create/tests/fixtures.py 2011-01-28 05:59:40 +0000
> @@ -3,9 +3,7 @@
> import subprocess
> import tempfile
>
> -from linaro_media_create import partitions
> -from linaro_media_create import cmd_runner
> -
> +from linaro_media_create import (cmd_runner, hwpack, partitions)
>
> class CreateTempDirFixture(object):
>
> @@ -122,10 +120,12 @@
> containing the positional arguments given to it to self.calls.
> """
> calls = None
> + count = 0
> return_value = None
> def __call__(self, *args):
> if self.calls is None:
> self.calls = []
> + self.count += 1
> self.calls.append(args)
> return self.return_value
>
> @@ -137,3 +137,16 @@
> mock.return_value = ('', '')
> super(MockRunSfdiskCommandsFixture, self).__init__(
> partitions, 'run_sfdisk_commands', mock)
> +
> +class MockHWPackAtExitFixture(MockSomethingFixture):
> +
> + def __init__(self):
> + mock = MockCallableWithPositionalArgs()
> + mock.return_value = ('', '', '')
> + super(MockHWPackAtExitFixture, self).__init__(
> + hwpack, 'run_local_atexit_funcs', mock)
> +
> + def tearDown(self):
> + super(MockHWPackAtExitFixture, self).tearDown()
> + hwpack.local_atexit = []
> +
>
> === modified file 'linaro_media_create/tests/test_media_create.py'
> --- linaro_media_create/tests/test_media_create.py 2011-01-27 18:43:55 +0000
> +++ linaro_media_create/tests/test_media_create.py 2011-01-28 06:02:21 +0000
> @@ -72,6 +72,7 @@
> MockCmdRunnerPopenFixture,
> MockSomethingFixture,
> MockRunSfdiskCommandsFixture,
> + MockHWPackAtExitFixture,
> )
>
>
> @@ -1015,3 +1016,19 @@
> ['sudo', 'mv', '-f', '/tmp/dir/hosts', 'chroot/etc'],
> ['sudo', 'mv', '-f'...

[lp:~doanac/linaro-image-tools/slash-proc-spam-version2]

262. By Andy Doan on 2011-02-01

add test case for atexit fix

[Andy Doan (doanac)]

I believe the latest commit should include the testing that salgado last suggested.

[Guilherme Salgado (salgado)]

This looks fine but the new run_local_atexit_funcs() code uses the sys module which is not available in hwpack.py, which means not all code paths of run_local_atexit_funcs() are tested. I'll see if I can add a test for the missing path but otherwise I'll just add the missing import and merge.

[Guilherme Salgado (salgado)]

Andy, I'm landing your branch with http://paste.ubuntu.com/561047/ on top of it to fix the issue I mentioned earlier. Once again, thanks a lot for taking care of this.

=== modified file 'linaro_media_create/hwpack.py'
--- linaro_media_create/hwpack.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/hwpack.py	2011-02-01 00:42:20 +0000
@@ -49,12 +49,12 @@
     copy_file(linaro_hwpack_install_path,
               os.path.join(chroot_dir, 'usr', 'bin'))
 
-    mount_chroot_proc(chroot_dir)
-
-    for hwpack_file in hwpack_files:
-        install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes)
-
-    run_local_atexit_funcs()
+    try:
+        mount_chroot_proc(chroot_dir)
+        for hwpack_file in hwpack_files:
+            install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes)
+    finally:
+        run_local_atexit_funcs()
 
 
 def install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes):
@@ -82,12 +82,14 @@
     Also register a function in local_atexit to unmount that /proc filesystem.
     """
     chroot_proc = os.path.join(chroot_dir, 'proc')
+
+    def umount_chroot_proc():
+        cmd_runner.run(['umount', '-v', chroot_proc], as_root=True).wait()
+    local_atexit.append(umount_chroot_proc)
+
     proc = cmd_runner.run(
         ['mount', 'proc', chroot_proc, '-t', 'proc'], as_root=True)
     proc.wait()
-    def umount_chroot_proc():
-        cmd_runner.run(['umount', '-v', chroot_proc], as_root=True).wait()
-    local_atexit.append(umount_chroot_proc)
 
 
 def copy_file(filepath, directory):
@@ -130,5 +132,19 @@
 
 def run_local_atexit_funcs():
     # Run the funcs in LIFO order, just like atexit does.
+    exc_info = None
     while len(local_atexit) > 0:
-        local_atexit.pop()()
+        func = local_atexit.pop()
+        try:
+            func()
+        except SystemExit:
+            exc_info = sys.exc_info()
+        except:
+            import traceback
+            print >> sys.stderr, "Error in local_atexit:"
+            traceback.print_exc()
+            exc_info = sys.exc_info()
+
+    if exc_info is not None:
+        raise exc_info[0], exc_info[1], exc_info[2]
+

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-02-01 00:42:20 +0000
@@ -1065,3 +1065,38 @@
              ['sudo', 'mv', '-f', '/tmp/dir/hosts', 'chroot/etc'],
              ['sudo', 'mv', '-f', '/tmp/dir/resolv.conf', 'chroot/etc']],
             fixture.mock.calls)
+
+    def test_hwpack_atexit(self):
+        self.run_local_atexit_functions_called = False
+
+        # ensure this list gets cleared, or other test functions may
+        # fail because the list isn't what they expected
+        def clear_atexits():
+            linaro_media_create.hwpack.local_atexit = []
+        self.addCleanup(clear_atexits)
+
+        def mock_run_local_atexit_functions():
+            self.run_local_atexit_functions_called = True
+
+        def mock_install_hwpack(p1, p2, p3):
+            raise Exception('hwpack mock exception')
+
+        self.useFixture(MockSomethingFixture(
+            sys, 'stdout', open('/dev/null', 'w')))
+        self.useFixture(MockCmdRunnerPopenFixture())
+        self.useFixture(MockSomethingFixture(
+            linaro_media_create.hwpack, 'install_hwpack', mock_install_hwpack))
+        self.useFixture(MockSomethingFixture(
+            linaro_media_create.hwpack, 'run_local_atexit_funcs',
+            mock_run_local_atexit_functions))
+
+
+        force_yes = True
+        exception_caught = False
+        try:
+            install_hwpacks('chroot', '/tmp/dir', force_yes, 'hwp.tgz', 'hwp2.tgz')
+        except:
+            exception_caught = True
+        self.assertTrue(self.run_local_atexit_functions_called)
+        self.assertTrue(exception_caught)
+