Merge lp:~smoser/maas/lp-1313550 into lp:~maas-committers/maas/trunk

[Scott Moser]

use xattr flags when available

Currently when maas collects the tarball from a image, it does not collect
xattrs. This means its a lossy conversion, which can cause problems
in runtime such as those seen in bug 1313550.

The change here adds the flags
  --xattrs '--xattrs-include=*'
if and only if the system 'tar' supports those. Note, tar on 12.04
does *not* support xattrs.

[Scott Moser (smoser)]

Note, I know tests are broken by this. Probably need to mock tar_xattr_opts and then test both possible returns.

[Mike Pontillo (mpontillo)]

We won't be able to land it until the tests pass. Let me pull your branch and take a look.

[Mike Pontillo (mpontillo)]

Here's what will need to be fixed before we can land this:

$ make lint
src/provisioningserver/import_images/uec2roottar.py:155:19: F821 undefined name 'curtin'
make: *** [lint-py] Error 123

Test failures:
http://paste.ubuntu.com/11720348/

Obviously the lint problem will need to be fixed, but see the note in the diff below regarding the test failures.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

You may be able to quickly fix the tests if you just figure out how you want to remove the 'str' parameter from the isinstance() call.

[Scott Moser (smoser)]

  You can drop the 'isinstance' entirely, but then the caller just has to pass in a list.
  All that did was say "if the caller gave me a string make it a list" as the list is what is necessary for check_output.

I dont see how that would affect tests.

[Mike Pontillo (mpontillo)]

The isinstance() itself isn't wrong; it's just that in MAAS we set "str = None" at the top, so you can't use "str" in the isinstance(). That's why the tests failed. Had you changed it to test for "not a list" or "unicode" instead, it probably would have been fine.

[Gavin Panella (allenap)]

Or use `bytes` in Python 2.

[Scott Moser (smoser)]

oh. ok. well, its dropped as unnecesary now.

[Mike Pontillo (mpontillo)]

Different error this time:

http://paste.ubuntu.com/11727540/

review: Needs Fixing

[Raphaël Badin (rvb)]

Thanks Scott. I've added the tests to your code in https://code.launchpad.net/~rvb/maas/lp-1313550/+merge/262314.

=== modified file 'src/provisioningserver/import_images/uec2roottar.py'
--- src/provisioningserver/import_images/uec2roottar.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/import_images/uec2roottar.py	2015-06-16 13:47:22 +0000
@@ -144,25 +144,41 @@
         unmount(mountpoint)
 
 
+def tar_xattr_opts(cmd=None):
+    # if tar cmd supports xattrs, return the required flags to extract them.
+    if cmd is None:
+        cmd = ['tar']
+
+    out = check_output(cmd + ['--help'], capture=True)
+
+    if "xattr" in out:
+        return ['--xattrs', '--xattrs-include=*']
+    return []
+
+
 def extract_image(image, output):
     """Loop-mount `image`, and tar its contents into `output`."""
+
+    tar = ['tar']
+    xattr_opts = tar_xattr_opts(tar)
     with tempdir() as mountpoint:
+        cmd = tar + xattr_opts + [
+            # Work from mountpoint as the current directory.
+            '-C', mountpoint,
+            # Options:
+            #    -c: Create tarfile.
+            #    -p: Preserve permissions.
+            #    -S: Handle sparse files efficiently (images have those).
+            #    -z: Compress using gzip.
+            #    -f: Work on given tar file.
+            '-cpSzf', output,
+            '--numeric-owner',
+            # Tar up the "current directory": the mountpoint.
+            '.',
+            ]
+
         with loop_mount(image, mountpoint):
-            check_call([
-                'tar',
-                # Work from mountpoint as the current directory.
-                '-C', mountpoint,
-                # Options:
-                #    -c: Create tarfile.
-                #    -p: Preserve permissions.
-                #    -S: Handle sparse files efficiently (images have those).
-                #    -z: Compress using gzip.
-                #    -f: Work on given tar file.
-                '-cpSzf', output,
-                '--numeric-owner',
-                # Tar up the "current directory": the mountpoint.
-                '.',
-                ])
+            check_call(cmd)
 
 
 def set_ownership(path, user=None):