Merge ~jibel/curtin/+git/add-layer-images-support:master into curtin:master

Proposed by Jean-Baptiste Lallement on 2018-11-28
Status: Merged
Approved by: Ryan Harper on 2019-02-15
Approved revision: a333c135a5d9192791697cec586562a4bb8be6df
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~jibel/curtin/+git/add-layer-images-support:master
Merge into: curtin:master
Diff against target: 737 lines (+544/-44)
8 files modified
curtin/commands/extract.py (+119/-1)
curtin/commands/install.py (+0/-4)
curtin/url_helper.py (+36/-24)
curtin/util.py (+5/-1)
doc/topics/config.rst (+76/-2)
tests/unittests/helpers.py (+20/-0)
tests/unittests/test_commands_extract.py (+227/-3)
tests/unittests/test_url_helper.py (+61/-9)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2019-02-15
Ryan Harper Approve on 2019-02-15
Chad Smith Approve on 2019-02-07
Scott Moser 2018-11-28 Needs Information on 2018-12-18
Review via email: mp+359761@code.launchpad.net

Commit message

Support for multi-layers images fsimage-layered:// URI

Curtin can now download and mount a layered filesystem image for use
as a source image. Local file or url are supported. Filesystem can be
any filesystem type mountable by the running kernel.

A "fsimage-layered" install source is a string representing one or
more mountable images from a single local or remote directory. The
string is dot-separated where each value between the dots represents a
particular image and the location of the name within the string
encodes the order in which it is to be mounted. The resulting list of
images are downloaded (if needed) then mounted and overlayed into a
single directory which is used as the source for installation.

See documentation for further details.

To post a comment you must log in.
Scott Moser (smoser) wrote :

Maybe it would be more sane to add 'fsimage-stack://' or
'layered-fsimage://' rather than extending fsimage://.

Then we dont have to make the 'fsimage:' path any more complicated.

Jean-Baptiste Lallement (jibel) wrote :

Adding a new source type was our initial thought too. But it finally made more sense to extend the existing fsimage.
Ultimately the API is the same and a path to a single filesystem image is passed as an installation source. The user doesn't have to have the knowledge about the type of build (flat image or layered). Moreover the code to handle a flat image or a layered image in Curtin is very similar, the main difference being the computation of the path then merging with an overlay. It reduces the redundancy of the code and its overall complexity is factorized into this single fsimage type.

Scott Moser (smoser) wrote :

@Jibel,

Thanks for fixing the lint errors.

You suggest that "The user doesn't have to have the knowledge about the
type of build (flat image or layered)." but reality is that the user
probably *does* know that, or is easily updated to know that. While
"figuring it out" in curtin is heuristic.

The thing I don't like about it is that currently, fsimage can be a url.
  fsimage://http://some.thing.here/path/to/image

We'd have to build in a lot of heuristics to make that work for
   fsimage://http://some.thing.here/path/to/image/multipart/

But it doesn't take much other than a 'image-index.json' or something
if we make the user tell it that it is a layered image.
Ie, we can make both of these work:
  layered-fsimage:///path/to/some/dir
and
  layered-fsimage://http://example.com/path/

the url would just require that it either be given a path to an
image-index.json or it would add image-index.json. Then image-index.json
would just contain
something like this:
 {
   "images": [
     "01-main1.fs",
     "02-main2.fs",
     "02-main2/sublayer1.fs",
     "02-main2/sublayer2.fs",
     "02-main2/sublayer2/sublayera.fs",
     "02-main2/sublayer2/sublayerb.fs",
     "03-main3.fs"
   ],
   "version": 1,
 }

Didier Roche (didrocks) wrote :

Hey Scott,

you should have a look at the tests, the sublayering works actually a bit different than you think :)

You can only point to the final layers (the topmost), then, the structure on disk built with livecd-rootfs will find its way to the root layer (all layers stack on top of each other and can have one single children).
This is because there is only a single path to the root layers, and they can not be composed that way. If we force the user to explicitely set them manually, it means we'll have another validation round to do in curtin. For instance, you can't compose "02-main2/sublayer2/sublayera.fs", "02-main2/sublayer2/sublayerb.fs".

If the image is invalid, the current proposed code will raise an exception.

I don't think we have anything against a syntax "foo:///pathtosomesquashfs". However, we spotted that the current curtin syntax, if we are correct, is "fsimage: /path/to/some/squashfs", which contradicts the current published docs. Is that an issue on the code?
Having then one single codepath with our additional tests enable skipping some code duplication, as both only points to one single squashfs file.

Does it make more sense?
(Note: jibel is away for a week and half)

Scott Moser (smoser) wrote :

@Didier,

(Note I did type incorrectly previously with 'fsimage://http://'
the shortcut fsimage http support is like fsimage:http://).

I agree I don't really understand how _get_image_paths works.
But that isn't really my issue.

The issue is that the support added for layered images
relies upon doing a directory listing of its input
(via glob.glob1 call in _get_image_paths).

That means that fsimage:http://example.com/your/path/to/some/layered.fs
will not work because we can't do a listing of the contents on
http://example.com/your/path/to/some .

Rather than extending fsimage: to work for layered filesystem images
that are local and fail in an inconsistent way for remote layeredd filesystem
images, I'd prefer either:
 a.) to add support for 'fsimage-layered:' that supports only local
     filesystems and errors if provided with a fsimage-layered:http://..
or
 b.) to add some mechanism for "list"ing a remote layered image. That
     is what I was suggesting with the index.

Didier Roche (didrocks) wrote :

@Scott:
Ack, the remote use-case is indeed a valid argument. I'll rediscuss that with Jean-Baptiste once he's back mid next-week and we'll fix it (I guess with a separate 'fsimage-layered:', but I'll wait on jibel) and give another round of testing as those layered systems only makes sense for a local filesystem (otherwise, you would just point to one, pre-compiled, full squashfs IMHO).

Jean-Baptiste Lallement (jibel) wrote :

@Scott, following your advice, this update adds a separate fsimage-layered type to manage multi-layers images.
The tests and documentation have been updated accordingly.

Jean-Baptiste Lallement (jibel) wrote :

This branch has been rebased on master.

Scott Moser (smoser) wrote :

commit message in this MP needs updating.

Jean-Baptiste Lallement (jibel) wrote :

Updated.

Scott Moser (smoser) wrote :

Thanks for the updates. This does look pretty good.

A couple of minor things inline below.
One thing that needs fixing is that now 'fsimage' will behave like fsimage-layered.

Thank you for your patience and for re-working some things.

And welcome back from holiday.

Jean-Baptiste Lallement (jibel) wrote :

Thanks for the review, all your points have been addressed

Scott Moser (smoser) wrote :

https://code.launchpad.net/~smoser/curtin/+git/curtin/+merge/360950
I put some changes there that i think make sense. As of
 c65cd19723140ca9a4cebc50a60fa6fb8be7ff1e
you should be able to just pull.

2 other comments:
a.) it sure would be nice to have *some* test of extract_root_layered_fsimage.
b.) ultimately we're going to need some vmtest here. Thats our only way to do integration test and actually ensure that this doesnt work.

I dont' want to push on 'b' right now, but we do need it for sure.

Jean-Baptiste Lallement (jibel) wrote :

I merged you proposed changes and fixed the documentation.

Scott Moser (smoser) wrote :

One inline comment that you missed in previous review.

review: Needs Information
Scott Moser (smoser) wrote :

http://paste.ubuntu.com/p/ycp2Hwy3zX/

^ that adds a very light test to the happy path of extract.

Ryan Harper (raharper) wrote :

I'm OK with the branch. I think the critical element to capture is:

1) what is the expected input (what's the structure of the input, directories of files)
2) what is the expected output (an overlayroot with a set of images mounted)

I want to make sure that curtin logs what it has found and mounted as this is our
contract with the caller of curtin. If the result of the copy of the overlay'ed dir does not succeed or does not contain the expected output then we'll want the logs to provide details as to what composed the overlay mountpoint.

Lastly, just for my understanding; is there a specific reason why curtin needs to do the mounting and creation of the overlay? Is this layered fs structure and overlay something that's in an existing product?

Jean-Baptiste Lallement (jibel) wrote :

Thanks for the review

About 1 and 2, this is documented in, where do you want to capture these elements? Or is there something to clarify in the documentation?

Ok, to add more logs but we already log the list of image detected and if an error occurs, the image that caused this error. What else would you like to log?
We could add more debug output in _get_image_path but since it's recursive it would be excessively verbose.

About your last point, the goal is to provide to curtin a single leaf filesystem in its yaml config file and it mounts the entire hierarchy of filesystems without any knowledge extra in the caller of curtin. It's very similar to the existing fsimage but for multilayer filesystems. This is implemented for the desktop images and is a pre-requisite for the new desktop installer which will rely on curtin.

For instance for the desktop the filesystems will be minimal, standard, and languages packs for minimal and standard, depending on the choice of the user (minimal vs normal and the language) We'll point curtin to the right squashfs containing the language pack and it will install the corresponding squashfses for the entire hierarchy. This has been tested successfully with this patch.

Jean-Baptiste Lallement (jibel) wrote :

To illustrate this feature and for the sack of completeness, here is an example:

Curtin configuration file:

=====
showtrace: true
storage:
    version: 1
    config:
      - id: vda
        type: disk
        ptable: msdos
        path: /dev/vda
        wipe: superblock
        grub_device: true
      - id: vda1
        type: partition
        number: 1
        size: 10GB
        device: vda
        flag: boot
      - id: vda1_root
        type: format
        fstype: ext4
        volume: vda1
        label: 'desktop-rootfs'
      - id: vda1_mount
        type: mount
        path: /
        device: vda1_root
sources:
  - fsimage-layered://cdrom/casper/02-install/desktop-fr.squashfs
=====

Install a desktop from this configuration with this command:

$ sudo curtin -vv -c config.yaml install

Installation logs: http://paste.ubuntu.com/p/bFbrXbP7CQ/

The Desktop image used for this test is to big for my bandwidth to upload it somewhere but we can do a hangout if you want a live demo.

Ryan Harper (raharper) wrote :

The above is very helpful. I've spent a bit of time with the branch, a couple comments inline. I think we can remove the source sanitize in install.py (it's done in extract). And then I started on expanding the documentation.

It was very interesting to see how the implementation works; and I want to document the expectations between "main-layers" and other images. There are some interesting side-effects that I think we must document. In particular, only main images seem to have peers at the same level, for example showing nested main layers; that is we cannot put a set of main layers in a sublayer. Pointing at any of the sub-images in the tree below only gets the sub-image, no peers nor parents, is that expected?

├── 10-first
│   ├── 10-main-nest1.img
│   ├── 20-main-nest2
│   │   └── 10-main-nest2-nest1.img
│   ├── 20-main-nest2.img
│   └── 30-main-test3.img
├── 10-first.img
├── 20-second
│   ├── 20-second-nest1
│   │   ├── 30-second-nest1-nest2
│   │   │   └── 40-second-nest1-nest2-nest3.img
│   │   └── 30-second-nest1-nest2.img
│   └── 20-second-nest1.img
└── 20-second.img

Michael Hudson-Doyle (mwhudson) wrote :

So the basic idea of this looks fine to me and something that the live server could use (and would make the live server ISO somewhat simpler to assemble).

There is an aspect I struggle I a bit with though (and jibel knows this applies to the livecd-rootfs side too). This work introduces a new concept (well, the concept is there for the live server work but that's all done in a very by hand way there) which is the concept of a tree of filesystem overlays together with the need to be able to assemble the complete overlay just from the name of one of them. And then basically casper will set the root filesystem to be one of these choices and the installer will pass a different choice to curtin based on the users actions. The way this work does is one possible way of representing this -- and has the feature that no changes to casper are required for it to work -- but is it the best possible one? I find it a bit klunky, although I don't really have any amazingly better ideas. I would like us all to have thought about it explicitly though.

Jean-Baptiste Lallement (jibel) wrote :
Download full text (3.2 KiB)

>
> It was very interesting to see how the implementation works; and I want to
> document the expectations between "main-layers" and other images. There are
> some interesting side-effects that I think we must document. In particular,
> only main images seem to have peers at the same level, for example showing
> nested main layers; that is we cannot put a set of main layers in a sublayer.
> Pointing at any of the sub-images in the tree below only gets the sub-image,
> no peers nor parents, is that expected?
>

It's a very good point and not expected. We've fixed the behaviour to load a set of main layers in a sublayer. The only constraint is that the names must start with 2 digits. If they don't start with 2 digits it'll try again with fs in the parent directory until it reaches the uppermost level.

Here is the change that we will push when all the other comments are addressed

diff --git a/curtin/commands/extract.py b/curtin/commands/extract.py
index 1ab8f767..5e14f24d 100644
--- a/curtin/commands/extract.py
+++ b/curtin/commands/extract.py
@@ -148,7 +148,7 @@ def _get_image_paths(path):
     images = [path]

     m = re.match(r"([0-9]{2})-.*", imgname)
- # Main layers
+ # Directory with peer fs images
     if m:
         i = int(m.group(1)) - 1
         match = "{:02d}-*{}".format(i, fileext)
@@ -160,11 +160,11 @@ def _get_image_paths(path):
         if candidate:
             images.extend(
                 _get_image_paths(os.path.join(dirname, candidate[-1])))
- return images
- else:
- # Climbing up from uppermost squashfes sublayers
- images.extend(_get_image_paths(dirname + fileext))
- return images
+ return images
+
+ # Climbing up from uppermost squashfes sublayers
+ images.extend(_get_image_paths(dirname + fileext))
+ return images

 def copy_to_target(source, target):
diff --git a/tests/unittests/test_commands_extract.py b/tests/unittests/test_commands_extract.py
index b85f3a6c..b90ad8e2 100644
--- a/tests/unittests/test_commands_extract.py
+++ b/tests/unittests/test_commands_extract.py
@@ -123,6 +123,26 @@ class TestGetImagesPaths(CiTestCase):
             _get_image_paths(
                 os.path.join(tdir, "02-main2/sublayer2/sublayera.fs")))

+ def test_leading_digits_mounts_peer_images(self):
+ """_get_image_paths mounts peer images in a folder when starting with leading digits"""
+ tdir = self._create_temporary_fs_struct([
+ "main1.fs",
+ "main2.fs",
+ "main2/01-sublayer1.fs",
+ "main2/02-sublayer2.fs",
+ "main2/02-sublayer2/sublayera.fs",
+ "main2/02-sublayer2/sublayerb.fs",
+ "main3.fs",
+ ])
+ expected_base = ["main2/02-sublayer2/sublayerb.fs",
+ "main2/02-sublayer2.fs",
+ "main2/01-sublayer1.fs",
+ "main2.fs"]
+ self.assertEqual(
+ [os.path.join(tdir, p) for p in expected_base],
+ _get_image_paths(
+ os.path.join(tdir, "main2/02-sublayer2/sublayerb.fs")))
+
     def test_missing_file(self):
         """_get_image_paths returns an empty...

Read more...

Jean-Baptiste Lallement (jibel) wrote :

See comments inline

Jean-Baptiste Lallement (jibel) wrote :

This is one way to implement it. The reasoning behind pointing to a leaf filesystem then mount the filesystems included in the branch together with an overlay is that, as currently used by desktop and server live, there is a strong dependency chain between layers. Most of them are modifying common files, like apt database, seed.yaml snap… Thus, the order is important and they cannot be mounted arbitrarily. Reflecting this in the directory structure is a way to make it more obvious for the user and less error-prone.
We would have to check anyway in curtin that layers are validly stacked on top of each other, hence the idea to only point to a leaf filesystem.
One case not covered by this implementation is if you want layers that can be arbitrarily stacked on top of any other layer, with no common files (like data assets, debug symbols, external apps in /opt). However, does it cover any practical or existing use cases we have nowadays?
This could still be achieved with this system by using different extensions or directories for a distinct set of images and let curtin mount that list of leaf layers (after dependency resolution) on top of each other.
This would require to extend fsimage-layered to support a list of leaf images and modify casper mount several branches of images.

Didier Roche (didrocks) wrote :

Thanks both for your feedbacks! We just pushed the code changes that should address the inline
comments and previous remarks + the doc changes to apply to the behavior changes with additional examples.

We rebased on origin/master and tested a VM install with those changes.

Ryan Harper (raharper) wrote :

This is looking quite nice. Thanks for updating the branch.

Michael Hudson-Doyle (mwhudson) wrote :

> This is one way to implement it. The reasoning behind pointing to a leaf
> filesystem then mount the filesystems included in the branch together with an
> overlay is that, as currently used by desktop and server live, there is a
> strong dependency chain between layers. Most of them are modifying common
> files, like apt database, seed.yaml snap… Thus, the order is important and
> they cannot be mounted arbitrarily. Reflecting this in the directory structure
> is a way to make it more obvious for the user and less error-prone.

That's not really the problem I have, it's more that I find the way the order is reflected in the directory structure to be a bit baroque. I've tried to explain further on the livecd-rootfs MP.

Jean-Baptiste Lallement (jibel) wrote :

Here is the updated MP to switch from the directory structure to a dot separated naming convention.

We ran the unit tests and verified several installation scenarii in a VM:
* Positive tests
  - Local single layer
  - Local multiple layers (up to 3)
  - Remote single layer
  - Remote multiple layers (up to 3)
* Negative tests
  - Local missing intermediary layer
  - Local empty file layer (0 byte)
  - Remote missing intermediary layer
  - Remote full local file system (cannot save to tmp directory)

Ryan Harper (raharper) wrote :

This looks quite straight forward. Thanks.

I would like to see the parts of the main extract_root_* broken up into
seperate functions so we can add unittests for each of those portions.

Specifically:

I'd like to unittest the downloading of images (do we call download on the items provided by the path stack), what happens if one of the downloads fail, what happens when download works but file is empty.

I'd also like a unittest over the main extract_root_*;

review: Needs Fixing
Didier Roche (didrocks) :
Jean-Baptiste Lallement (jibel) wrote :

The function extract_root_layered_fsimage has been broken up into two parts to separate the download logic from the mount.

Several unittests have been added to cover positive and negative case for local and remote fs.

Chad Smith (chad.smith) wrote :

Thanks for this branch and the discussion. I've got a couple of suggestions and thoughts inline. Feel free to take what you will from this as I don't think they are really critical, but nice to have.

Chad Smith (chad.smith) :
Didier Roche (didrocks) :
Jean-Baptiste Lallement (jibel) wrote :

@Chad, thanks for review.

We've updated the branch with most of your proposals:
- The download code if the layers of a stack image have been moved to its function.
- We've added retries and retry_delay argument to the url_helper.download and set it to 3 by default in existing download functions.
- There is now a test to cover download errors.
- unittests have been added to cover the new download helper functionality and verified the coverage.

Note that tests of the extract module have not been updated, as covering the public commands extract API already covers this submodule, and there isn’t much value in adding separated unittests.

Chad Smith (chad.smith) wrote :

Thanks for this rework, this looks good to me, minor nits inline.
+1

review: Approve
Jean-Baptiste Lallement (jibel) wrote :

Branch updated with a 3s delay.

Ryan Harper (raharper) wrote :

Thanks for splitting the logic and the extra unittests.

One minor change in the unittests suggested in-line comments
to remove duplicate setup code.

Jean-Baptiste Lallement (jibel) wrote :

We originally tried to follow the coding style and conventions of the existing code even if it didn't seem to follow the best practices (no setup and copy'n paste)

However, the common test initialization code has been mode to setUp()

We cannot move add_patch to setUp() because 1) not all the tests must be patched (eg test_download_file_url) and 2) in test_download_file_url_retry_then_success we create a closure over the original object to restore it on second try.

Moreover, the MaaS tests use decorators and it's consistent with the download test.

Note that the MaaS tests should be factorized in the same manner by moving common code to either a function or in a setUp method.

Jean-Baptiste Lallement (jibel) wrote :

I think we've never been closer to this MP being accepted :) Could someone please do a review of the last update and tell if there is still anything to fix?

Ryan Harper (raharper) wrote :
Ryan Harper (raharper) wrote :

146 was interrupted, restarted 147 completed, all but 4 tests passed, it looks transient, so I've started just those tests again:

tests/vmtests/test_bcache_bug1718699.py:XenialTestBcacheBug1718699 tests/vmtests/test_multipath.py:XenialGATestMultipathBasic tests/vmtests/test_network:CosmicTestNetworkBasic tests/vmtests/test_network_alias.py:CosmicTestNetworkAlias

We should be able to land today.

Ryan Harper (raharper) :
review: Approve

Commit message lints:
 - Line #2 has 128 too many characters. Line starts with: "Adds type fsimage-layered"... - Line #4 has 89 too many characters. Line starts with: "If the final image is"...

review: Needs Fixing
review: Approve (continuous-integration)

Commit message lints:
 - Line #1 has 4 too many characters. Line starts with: " "...

review: Needs Fixing
Ryan Harper (raharper) wrote :

*sigh*

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/commands/extract.py b/curtin/commands/extract.py
2index ec7a791..8567659 100644
3--- a/curtin/commands/extract.py
4+++ b/curtin/commands/extract.py
5@@ -1,6 +1,7 @@
6 # This file is part of curtin. See LICENSE file for copyright and license info.
7
8 import os
9+import shutil
10 import sys
11 import tempfile
12
13@@ -64,7 +65,7 @@ def extract_root_fsimage_url(url, target):
14 wfp = tempfile.NamedTemporaryFile(suffix=".img", delete=False)
15 wfp.close()
16 try:
17- url_helper.download(url, wfp.name)
18+ url_helper.download(url, wfp.name, retries=3)
19 return _extract_root_fsimage(wfp.name, target)
20 finally:
21 os.unlink(wfp.name)
22@@ -85,6 +86,121 @@ def _extract_root_fsimage(path, target):
23 os.rmdir(mp)
24
25
26+def extract_root_layered_fsimage_url(uri, target):
27+ ''' Build images list to consider from a layered structure
28+
29+ uri: URI of the layer file
30+ target: Target file system to provision
31+
32+ return: None
33+ '''
34+ path = _path_from_file_url(uri)
35+
36+ image_stack = _get_image_stack(path)
37+ LOG.debug("Considering fsimages: '%s'", ",".join(image_stack))
38+
39+ tmp_dir = None
40+ try:
41+ # Download every remote images if remote url
42+ if url_helper.urlparse(path).scheme != "":
43+ tmp_dir = tempfile.mkdtemp()
44+ image_stack = _download_layered_images(image_stack, tmp_dir)
45+
46+ # Check that all images exists on disk and are not empty
47+ for img in image_stack:
48+ if not os.path.isfile(img) or os.path.getsize(img) <= 0:
49+ raise ValueError("Failed to use fsimage: '%s' doesn't exist " +
50+ "or is invalid", img)
51+
52+ return _extract_root_layered_fsimage(image_stack, target)
53+ finally:
54+ if tmp_dir and os.path.exists(tmp_dir):
55+ shutil.rmtree(tmp_dir)
56+
57+
58+def _download_layered_images(image_stack, tmp_dir):
59+ local_image_stack = []
60+ try:
61+ for img_url in image_stack:
62+ dest_path = os.path.join(tmp_dir,
63+ os.path.basename(img_url))
64+ url_helper.download(img_url, dest_path, retries=3)
65+ local_image_stack.append(dest_path)
66+ except url_helper.UrlError as e:
67+ LOG.error("Failed to download '%s'" % img_url)
68+ raise e
69+ return local_image_stack
70+
71+
72+def _extract_root_layered_fsimage(image_stack, target):
73+ mp_base = tempfile.mkdtemp()
74+ mps = []
75+ try:
76+ # Create a mount point for each image file and mount the image
77+ try:
78+ for img in image_stack:
79+ mp = os.path.join(mp_base, os.path.basename(img) + ".dir")
80+ os.mkdir(mp)
81+ util.subp(['mount', '-o', 'loop,ro', img, mp], capture=True)
82+ mps.insert(0, mp)
83+ except util.ProcessExecutionError as e:
84+ LOG.error("Failed to mount '%s' for extraction: %s", img, e)
85+ raise e
86+
87+ # Prepare
88+ if len(mps) == 1:
89+ root_dir = mps[0]
90+ else:
91+ # Multiple image files, merge them with an overlay and do the copy
92+ root_dir = os.path.join(mp_base, "root.dir")
93+ os.mkdir(root_dir)
94+ try:
95+ util.subp(['mount', '-t', 'overlay', 'overlay', '-o',
96+ 'lowerdir=' + ':'.join(mps), root_dir],
97+ capture=True)
98+ mps.append(root_dir)
99+ except util.ProcessExecutionError as e:
100+ LOG.error("overlay mount to %s failed: %s", root_dir, e)
101+ raise e
102+
103+ copy_to_target(root_dir, target)
104+ finally:
105+ umount_err_mps = []
106+ for mp in reversed(mps):
107+ try:
108+ util.subp(['umount', mp], capture=True)
109+ except util.ProcessExecutionError as e:
110+ LOG.error("can't unmount %s: %e", mp, e)
111+ umount_err_mps.append(mp)
112+ if umount_err_mps:
113+ raise util.ProcessExecutionError(
114+ "Failed to umount: %s", ", ".join(umount_err_mps))
115+ shutil.rmtree(mp_base)
116+
117+
118+def _get_image_stack(uri):
119+ '''Find a list of dependent images for given layered fsimage path
120+
121+ uri: URI of the layer file
122+ return: tuple of path to dependent images
123+ '''
124+
125+ image_stack = []
126+ root_dir = os.path.dirname(uri)
127+ img_name = os.path.basename(uri)
128+ _, img_ext = os.path.splitext(img_name)
129+
130+ img_parts = img_name.split('.')
131+ # Last item is the extension
132+ for i in img_parts[:-1]:
133+ image_stack.append(
134+ os.path.join(
135+ root_dir,
136+ '.'.join(img_parts[0:img_parts.index(i)+1]) + img_ext))
137+
138+ return image_stack
139+
140+
141 def copy_to_target(source, target):
142 if source.startswith("cp://"):
143 source = source[5:]
144@@ -133,6 +249,8 @@ def extract(args):
145 copy_to_target(source['uri'], target)
146 elif source['type'] == "fsimage":
147 extract_root_fsimage_url(source['uri'], target=target)
148+ elif source['type'] == "fsimage-layered":
149+ extract_root_layered_fsimage_url(source['uri'], target=target)
150 else:
151 extract_root_tgz_url(source['uri'], target=target)
152
153diff --git a/curtin/commands/install.py b/curtin/commands/install.py
154index 244683c..ad17508 100644
155--- a/curtin/commands/install.py
156+++ b/curtin/commands/install.py
157@@ -408,10 +408,6 @@ def cmd_install(args):
158 if not len(cfg.get('sources', [])):
159 raise util.BadUsage("no sources provided to install")
160
161- for i in cfg['sources']:
162- # we default to tgz for old style sources config
163- cfg['sources'][i] = util.sanitize_source(cfg['sources'][i])
164-
165 migrate_proxy_settings(cfg)
166 for k in ('http_proxy', 'https_proxy', 'no_proxy'):
167 if k in cfg['proxy']:
168diff --git a/curtin/url_helper.py b/curtin/url_helper.py
169index 767510d..ee4640b 100644
170--- a/curtin/url_helper.py
171+++ b/curtin/url_helper.py
172@@ -82,39 +82,51 @@ class UrlReader(object):
173 self.close()
174
175
176-def download(url, path, reporthook=None, data=None):
177+def download(url, path, reporthook=None, data=None, retries=0, retry_delay=3):
178 """Download url to path.
179
180 reporthook is compatible with py3 urllib.request.urlretrieve.
181 urlretrieve does not exist in py2."""
182
183 buflen = 8192
184- wfp = open(path, "wb")
185+ attempts = 0
186
187- try:
188- buf = None
189- blocknum = 0
190- fsize = 0
191- start = time.time()
192- with UrlReader(url) as rfp:
193- if reporthook:
194- reporthook(blocknum, buflen, rfp.size)
195-
196- while True:
197- buf = rfp.read(buflen)
198- if not buf:
199- break
200- blocknum += 1
201+ while True:
202+ wfp = open(path, "wb")
203+ try:
204+ buf = None
205+ blocknum = 0
206+ fsize = 0
207+ start = time.time()
208+ with UrlReader(url) as rfp:
209 if reporthook:
210 reporthook(blocknum, buflen, rfp.size)
211- wfp.write(buf)
212- fsize += len(buf)
213- timedelta = time.time() - start
214- LOG.debug("Downloaded %d bytes from %s to %s in %.2fs (%.2fMbps)",
215- fsize, url, path, timedelta, fsize / timedelta / 1024 / 1024)
216- return path, rfp.info
217- finally:
218- wfp.close()
219+
220+ while True:
221+ buf = rfp.read(buflen)
222+ if not buf:
223+ break
224+ blocknum += 1
225+ if reporthook:
226+ reporthook(blocknum, buflen, rfp.size)
227+ wfp.write(buf)
228+ fsize += len(buf)
229+ timedelta = time.time() - start
230+ LOG.debug("Downloaded %d bytes from %s to %s in %.2fs (%.2fMbps)",
231+ fsize, url, path, timedelta,
232+ fsize / timedelta / 1024 / 1024)
233+ return path, rfp.info
234+ except UrlError as e:
235+ # retry on internal server errors up to "retries #"
236+ if (e.code < 500 or attempts >= retries):
237+ raise e
238+ LOG.debug("Current download failed with error: %s. Retrying in"
239+ " %d seconds.",
240+ e.code, retry_delay)
241+ attempts += 1
242+ time.sleep(retry_delay)
243+ finally:
244+ wfp.close()
245
246
247 def get_maas_version(endpoint):
248diff --git a/curtin/util.py b/curtin/util.py
249index 35e004f..34d09a8 100644
250--- a/curtin/util.py
251+++ b/curtin/util.py
252@@ -45,6 +45,10 @@ except NameError:
253 from . import paths
254 from .log import LOG, log_call
255
256+binary_type = bytes
257+if sys.version_info[0] < 3:
258+ binary_type = str
259+
260 _INSTALLED_HELPERS_PATH = 'usr/lib/curtin/helpers'
261 _INSTALLED_MAIN = 'usr/bin/curtin'
262
263@@ -891,7 +895,7 @@ def sanitize_source(source):
264 # already sanitized?
265 return source
266 supported = ['tgz', 'dd-tgz', 'dd-tbz', 'dd-txz', 'dd-tar', 'dd-bz2',
267- 'dd-gz', 'dd-xz', 'dd-raw', 'fsimage']
268+ 'dd-gz', 'dd-xz', 'dd-raw', 'fsimage', 'fsimage-layered']
269 deftype = 'tgz'
270 for i in supported:
271 prefix = i + ":"
272diff --git a/doc/topics/config.rst b/doc/topics/config.rst
273index 218bc17..bad8fc2 100644
274--- a/doc/topics/config.rst
275+++ b/doc/topics/config.rst
276@@ -506,9 +506,83 @@ configures the method used to copy the image to the target system.
277 - **cp://**: Use ``rsync`` command to copy source directory to target.
278 - **file://**: Use ``tar`` command to extract source to target.
279 - **http[s]://**: Use ``wget | tar`` commands to extract source to target.
280-- **fsimage://**: mount filesystem image and copy contents to target.
281- Local file or url are supported. Filesystem can be any filesystem type
282+- **fsimage://** mount filesystem image and copy contents to target.
283+ Local file or url are supported. Filesystem can be any filesystem type
284 mountable by the running kernel.
285+- **fsimage-layered://** mount layered filesystem image and copy contents to target.
286+ A ``fsimage-layered`` install source is a string representing one or more mountable
287+ images from a single local or remote directory. The string is dot-separated where
288+ each value between the dots represents a particular image and the location of the
289+ name within the string encodes the order in which it is to be mounted. The resulting
290+ list of images are downloaded (if needed) then mounted and overlayed into a single
291+ directory which is used as the source for installation.
292+
293+**Image Name Pattern**
294+
295+ [[<parent_layer>.]...]<layer name>.<file extension pattern>
296+
297+Example::
298+
299+ 10-base.img
300+ minimal.img
301+ minimal.standard.live.squashfs
302+ http://example.io/standard.squashfs
303+
304+**Layer Dependencies**
305+
306+Layers are parts of the name seperated by dots. Any layer in the name will
307+be included as a dependency. The file extension pattern is used to find
308+related layers.
309+
310+Examples:
311+
312+ Base use case::
313+
314+ /images
315+ ├── main.squashfs
316+ ├── main.upper.squashfs
317+ └── main.upper.debug.squashfs
318+
319+ source='fsimage-layered://images/main.squashfs' -> images='/images/main.squashfs'
320+ source='fsimage-layered://images/main.upper.squashfs' -> images='/images/main.upper.squashfs, /images/main.squashfs'
321+ source='fsimage-layered://images/main.upper.debug.squashfs' -> images='/images/main.upper.debug.squashfs, /images/main.upper.squashfs, /images/main.squashfs'
322+
323+ Multiple extensions::
324+
325+ /images
326+ ├── main.squashfs
327+ ├── main.img
328+ ├── main.upper.squashfs
329+ ├── main.upper.img
330+ └── main.upper.debug.squashfs
331+
332+ source='fsimage-layered://images/main.upper.squashfs' -> images='/images/main.upper.squashfs, /images/main.squashfs'
333+ source='fsimage-layered://images/main.upper.img' -> images='/images/main.upper.img, /images/main.img'
334+
335+ Missing intermediary layer::
336+
337+ /images
338+ ├── main.squashfs
339+ └── main.upper.debug.squashfs
340+
341+If there is a missing image in the path to a leaf, an error will be raised
342+
343+ source='fsimage-layered://images/main.squashfs' -> images='/images/main.squashfs'
344+ source='fsimage-layered://images/main.upper.debug.squashfs' -> Raised Error'
345+
346+ Remote Layers::
347+
348+ http://example.io/base.extended.debug.squashfs
349+
350+
351+The URI passed to ``fsimage-layered`` may be on a remote system. Curtin
352+will parse the URI and then download each layer from the remote system.
353+This results in Curtin downloading the following URLs::
354+
355+- http://example.io/base.squashfs
356+- http://example.io/base.extended.squashfs
357+- http://example.io/base.extended.debug.squashfs
358+
359
360 **Example Cloud-image**::
361
362diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
363index 1268880..0963483 100644
364--- a/tests/unittests/helpers.py
365+++ b/tests/unittests/helpers.py
366@@ -11,6 +11,8 @@ import string
367 import tempfile
368 from unittest import TestCase
369
370+from curtin import util
371+
372
373 def builtin_module_name():
374 options = ('builtins', '__builtin__')
375@@ -87,4 +89,22 @@ def dir2dict(startdir, prefix=None):
376 flist[key] = fp.read()
377 return flist
378
379+
380+def populate_dir(path, files):
381+ if not os.path.exists(path):
382+ os.makedirs(path)
383+ ret = []
384+ for (name, content) in files.items():
385+ p = os.path.sep.join([path, name])
386+ util.ensure_dir(os.path.dirname(p))
387+ with open(p, "wb") as fp:
388+ if isinstance(content, util.binary_type):
389+ fp.write(content)
390+ else:
391+ fp.write(content.encode('utf-8'))
392+ fp.close()
393+ ret.append(p)
394+
395+ return ret
396+
397 # vi: ts=4 expandtab syntax=python
398diff --git a/tests/unittests/test_commands_extract.py b/tests/unittests/test_commands_extract.py
399index cc117bb..c318699 100644
400--- a/tests/unittests/test_commands_extract.py
401+++ b/tests/unittests/test_commands_extract.py
402@@ -4,12 +4,15 @@ import os
403 from .helpers import CiTestCase
404
405 from curtin import util
406-from curtin.commands.extract import extract_root_fsimage_url
407+from curtin.commands.extract import (extract_root_fsimage_url,
408+ extract_root_layered_fsimage_url,
409+ _get_image_stack)
410+from curtin.url_helper import UrlError
411
412
413 class TestExtractRootFsImageUrl(CiTestCase):
414 """Test extract_root_fsimage_url."""
415- def _fake_download(self, url, path):
416+ def _fake_download(self, url, path, retries=0):
417 self.downloads.append(os.path.abspath(path))
418 with open(path, "w") as fp:
419 fp.write("fake content from " + url + "\n")
420@@ -43,7 +46,7 @@ class TestExtractRootFsImageUrl(CiTestCase):
421 target = self.tmp_path("target_d", tmpd)
422 fpath = self.tmp_path("my.img", tmpd)
423 util.write_file(fpath, fpath + " data\n")
424- extract_root_fsimage_url("file://" + fpath, target)
425+ extract_root_fsimage_url("file:///" + fpath, target)
426 self.assertEqual(1, self.m__extract_root_fsimage.call_count)
427 self.assertEqual(0, self.m_download.call_count)
428
429@@ -70,4 +73,225 @@ class TestExtractRootFsImageUrl(CiTestCase):
430 self.assertEqual(0, self.m_download.call_count)
431
432
433+class TestExtractRootLayeredFsImageUrl(CiTestCase):
434+ """Test extract_root_layared_fsimage_url."""
435+ def _fake_download(self, url, path, retries=0):
436+ self.downloads.append(os.path.abspath(path))
437+ with open(path, "w") as fp:
438+ fp.write("fake content from " + url + "\n")
439+
440+ def setUp(self):
441+ super(TestExtractRootLayeredFsImageUrl, self).setUp()
442+ self.downloads = []
443+ self.add_patch("curtin.commands.extract.url_helper.download",
444+ "m_download", side_effect=self._fake_download)
445+ self.add_patch("curtin.commands.extract._extract_root_layered_fsimage",
446+ "m__extract_root_layered_fsimage")
447+
448+ def test_relative_local_file_single(self):
449+ """extract_root_layered_fsimage_url supports relative file:// uris."""
450+ tmpd = self.tmp_dir()
451+ target = self.tmp_path("target_d", tmpd)
452+ startdir = os.getcwd()
453+ fname = "my.img"
454+ try:
455+ os.chdir(tmpd)
456+ util.write_file(fname, fname + " data\n")
457+ extract_root_layered_fsimage_url("file://" + fname, target)
458+ finally:
459+ os.chdir(startdir)
460+ self.assertEqual(1, self.m__extract_root_layered_fsimage.call_count)
461+ self.assertEqual(0, self.m_download.call_count)
462+
463+ def test_absolute_local_file_single(self):
464+ """extract_root_layered_fsimage_url supports absolute file:/// uris."""
465+ tmpd = self.tmp_dir()
466+ target = self.tmp_path("target_d", tmpd)
467+ fpath = self.tmp_path("my.img", tmpd)
468+ util.write_file(fpath, fpath + " data\n")
469+ extract_root_layered_fsimage_url("file:///" + fpath, target)
470+ self.assertEqual(1, self.m__extract_root_layered_fsimage.call_count)
471+ self.assertEqual(0, self.m_download.call_count)
472+
473+ def test_local_file_path_single(self):
474+ """extract_root_layered_fsimage_url supports normal file path without
475+ file:"""
476+ tmpd = self.tmp_dir()
477+ target = self.tmp_path("target_d", tmpd)
478+ fpath = self.tmp_path("my.img", tmpd)
479+ util.write_file(fpath, fpath + " data\n")
480+ extract_root_layered_fsimage_url(os.path.abspath(fpath), target)
481+ self.assertEqual(1, self.m__extract_root_layered_fsimage.call_count)
482+ self.assertEqual(0, self.m_download.call_count)
483+
484+ def test_local_file_path_multiple(self):
485+ """extract_root_layered_fsimage_url supports normal hierarchy file
486+ path"""
487+ tmpd = self.tmp_dir()
488+ target = self.tmp_path("target_d", tmpd)
489+ arg = os.path.abspath(self.tmp_path("minimal.standard.debug.squashfs",
490+ tmpd))
491+ for f in ["minimal.squashfs",
492+ "minimal.standard.squashfs",
493+ "minimal.standard.debug.squashfs"]:
494+ fpath = self.tmp_path(f, tmpd)
495+ util.write_file(fpath, fpath + " data\n")
496+ extract_root_layered_fsimage_url(arg, target)
497+ self.assertEqual(1, self.m__extract_root_layered_fsimage.call_count)
498+ self.assertEqual(0, self.m_download.call_count)
499+
500+ def test_local_file_path_multiple_one_missing(self):
501+ """extract_root_layered_fsimage_url supports normal hierarchy file
502+ path but intermediate layer missing"""
503+ tmpd = self.tmp_dir()
504+ target = self.tmp_path("target_d", tmpd)
505+ arg = os.path.abspath(self.tmp_path("minimal.standard.debug.squashfs",
506+ tmpd))
507+ for f in ["minimal.squashfs",
508+ "minimal.standard.debug.squashfs"]:
509+ fpath = self.tmp_path(f, tmpd)
510+ util.write_file(fpath, fpath + " data\n")
511+ self.assertRaises(ValueError, extract_root_layered_fsimage_url, arg,
512+ target)
513+ self.assertEqual(0, self.m__extract_root_layered_fsimage.call_count)
514+ self.assertEqual(0, self.m_download.call_count)
515+
516+ def test_local_file_path_multiple_one_empty(self):
517+ """extract_root_layered_fsimage_url supports normal hierarchy file
518+ path but intermediate layer empty"""
519+ tmpd = self.tmp_dir()
520+ target = self.tmp_path("target_d", tmpd)
521+ arg = os.path.abspath(self.tmp_path("minimal.standard.debug.squashfs",
522+ tmpd))
523+ for f in ["minimal.squashfs",
524+ "minimal.standard.squashfs"
525+ "minimal.standard.debug.squashfs"]:
526+ fpath = self.tmp_path(f, tmpd)
527+ if f == "minimal.standard.squashfs":
528+ util.write_file(fpath, "")
529+ else:
530+ util.write_file(fpath, fpath + " data\n")
531+ self.assertRaises(ValueError, extract_root_layered_fsimage_url, arg,
532+ target)
533+ self.assertEqual(0, self.m__extract_root_layered_fsimage.call_count)
534+ self.assertEqual(0, self.m_download.call_count)
535+
536+ def test_remote_file_single(self):
537+ """extract_root_layered_fsimage_url supports http:// urls."""
538+ tmpd = self.tmp_dir()
539+ target = self.tmp_path("target_d", tmpd)
540+ myurl = "http://example.io/minimal.squashfs"
541+ extract_root_layered_fsimage_url(myurl, target)
542+ self.assertEqual(1, self.m__extract_root_layered_fsimage.call_count)
543+ self.assertEqual(1, self.m_download.call_count)
544+ self.assertEqual("http://example.io/minimal.squashfs",
545+ self.m_download.call_args_list[0][0][0])
546+ # ensure the file got cleaned up.
547+ self.assertEqual([], [f for f in self.downloads if os.path.exists(f)])
548+
549+ def test_remote_file_multiple(self):
550+ """extract_root_layered_fsimage_url supports normal hierarchy from
551+ http:// urls."""
552+ tmpd = self.tmp_dir()
553+ target = self.tmp_path("target_d", tmpd)
554+ myurl = "http://example.io/minimal.standard.debug.squashfs"
555+ extract_root_layered_fsimage_url(myurl, target)
556+ self.assertEqual(1, self.m__extract_root_layered_fsimage.call_count)
557+ self.assertEqual(3, self.m_download.call_count)
558+ for i, image_url in enumerate(["minimal.squashfs",
559+ "minimal.standard.squashfs",
560+ "minimal.standard.debug.squashfs"]):
561+ self.assertEqual("http://example.io/" + image_url,
562+ self.m_download.call_args_list[i][0][0])
563+ # ensure the file got cleaned up.
564+ self.assertEqual([], [f for f in self.downloads if os.path.exists(f)])
565+
566+ def test_remote_file_multiple_one_missing(self):
567+ """extract_root_layered_fsimage_url supports normal hierarchy from
568+ http:// urls with one layer missing."""
569+
570+ def fail_download_minimal_standard(url, path, retries=0):
571+ if url == "http://example.io/minimal.standard.squashfs":
572+ raise UrlError(url, 404, "Couldn't download",
573+ None, None)
574+ return self._fake_download(url, path, retries)
575+ self.m_download.side_effect = fail_download_minimal_standard
576+
577+ tmpd = self.tmp_dir()
578+ target = self.tmp_path("target_d", tmpd)
579+ myurl = "http://example.io/minimal.standard.debug.squashfs"
580+ self.assertRaises(UrlError, extract_root_layered_fsimage_url,
581+ myurl, target)
582+ self.assertEqual(0, self.m__extract_root_layered_fsimage.call_count)
583+ self.assertEqual(2, self.m_download.call_count)
584+ for i, image_url in enumerate(["minimal.squashfs",
585+ "minimal.standard.squashfs"]):
586+ self.assertEqual("http://example.io/" + image_url,
587+ self.m_download.call_args_list[i][0][0])
588+ # ensure the file got cleaned up.
589+ self.assertEqual([], [f for f in self.downloads if os.path.exists(f)])
590+
591+ def test_remote_file_multiple_one_empty(self):
592+ """extract_root_layered_fsimage_url supports normal hierarchy from
593+ http:// urls with one layer empty."""
594+
595+ def empty_download_minimal_standard(url, path, retries=0):
596+ if url == "http://example.io/minimal.standard.squashfs":
597+ self.downloads.append(os.path.abspath(path))
598+ with open(path, "w") as fp:
599+ fp.write("")
600+ return
601+ return self._fake_download(url, path, retries)
602+ self.m_download.side_effect = empty_download_minimal_standard
603+
604+ tmpd = self.tmp_dir()
605+ target = self.tmp_path("target_d", tmpd)
606+ myurl = "http://example.io/minimal.standard.debug.squashfs"
607+ self.assertRaises(ValueError, extract_root_layered_fsimage_url,
608+ myurl, target)
609+ self.assertEqual(0, self.m__extract_root_layered_fsimage.call_count)
610+ self.assertEqual(3, self.m_download.call_count)
611+ for i, image_url in enumerate(["minimal.squashfs",
612+ "minimal.standard.squashfs",
613+ "minimal.standard.debug.squashfs"]):
614+ self.assertEqual("http://example.io/" + image_url,
615+ self.m_download.call_args_list[i][0][0])
616+ # ensure the file got cleaned up.
617+ self.assertEqual([], [f for f in self.downloads if os.path.exists(f)])
618+
619+
620+class TestGetImageStack(CiTestCase):
621+ """Test _get_image_stack."""
622+
623+ def test_get_image_stack(self):
624+ """_get_image_paths returns a tuple of depending fsimages
625+ with same extension"""
626+ self.assertEqual(
627+ ['/path/to/aa.fs',
628+ '/path/to/aa.bbb.fs',
629+ '/path/to/aa.bbb.cccc.fs'],
630+ _get_image_stack("/path/to/aa.bbb.cccc.fs"))
631+
632+ def test_get_image_stack_none(self):
633+ """_get_image_paths returns an empty tuple with no entry"""
634+ self.assertEqual(
635+ [],
636+ _get_image_stack(""))
637+
638+ def test_get_image_stack_no_dependency(self):
639+ """_get_image_paths returns a tuple a single element when fsimage
640+ has no dependency"""
641+ self.assertEqual(
642+ ['/path/to/aa.fs'],
643+ _get_image_stack("/path/to/aa.fs"))
644+
645+ def test_get_image_stack_with_urls(self):
646+ """_get_image_paths returns a tuple of depending fsimages
647+ with same extension and same urls"""
648+ self.assertEqual(
649+ ['https://path.com/to/aa.fs',
650+ 'https://path.com/to/aa.bbb.fs',
651+ 'https://path.com/to/aa.bbb.cccc.fs'],
652+ _get_image_stack("https://path.com/to/aa.bbb.cccc.fs"))
653+
654 # vi: ts=4 expandtab syntax=python
655diff --git a/tests/unittests/test_url_helper.py b/tests/unittests/test_url_helper.py
656index 9cc9a5d..1b550f6 100644
657--- a/tests/unittests/test_url_helper.py
658+++ b/tests/unittests/test_url_helper.py
659@@ -10,19 +10,71 @@ from .helpers import CiTestCase
660
661
662 class TestDownload(CiTestCase):
663+ def setUp(self):
664+ super(TestDownload, self).setUp()
665+ self.tmpd = self.tmp_dir()
666+ self.src_file = self.tmp_path("my-source", self.tmpd)
667+ with open(self.src_file, "wb") as fp:
668+ # Write the min amount of bytes
669+ fp.write(b':-)\n' * int(8200/4))
670+ self.target_file = self.tmp_path("my-target", self.tmpd)
671+
672 def test_download_file_url(self):
673 """Download a file to another file."""
674- tmpd = self.tmp_dir()
675- src_file = self.tmp_path("my-source", tmpd)
676- target_file = self.tmp_path("my-target", tmpd)
677+ url_helper.download("file://" + self.src_file, self.target_file)
678+ self.assertTrue(filecmp.cmp(self.src_file, self.target_file),
679+ "Downloaded file differed from source file.")
680+
681+ @mock.patch('curtin.url_helper.UrlReader')
682+ def test_download_file_url_retry(self, urlreader_mock):
683+ """Retry downloading a file with server error (http 5xx)."""
684+ urlreader_mock.side_effect = url_helper.UrlError(None, code=500)
685+
686+ self.assertRaises(url_helper.UrlError, url_helper.download,
687+ "file://" + self.src_file, self.target_file,
688+ retries=3, retry_delay=0)
689+ self.assertEquals(4, urlreader_mock.call_count,
690+ "Didn't call UrlReader 4 times (retries=3)")
691+
692+ @mock.patch('curtin.url_helper.UrlReader')
693+ def test_download_file_url_no_retry(self, urlreader_mock):
694+ """No retry by default on downloading a file with server error
695+ (http 5xx)."""
696+ urlreader_mock.side_effect = url_helper.UrlError(None, code=500)
697+
698+ self.assertRaises(url_helper.UrlError, url_helper.download,
699+ "file://" + self.src_file, self.target_file,
700+ retry_delay=0)
701+ self.assertEquals(1, urlreader_mock.call_count,
702+ "Didn't call UrlReader once (retries=0)")
703+
704+ @mock.patch('curtin.url_helper.UrlReader')
705+ def test_download_file_url_no_retry_on_client_error(self, urlreader_mock):
706+ """No retry by default on downloading a file with 4xx http error."""
707+ urlreader_mock.side_effect = url_helper.UrlError(None, code=404)
708+
709+ self.assertRaises(url_helper.UrlError, url_helper.download,
710+ "file://" + self.src_file, self.target_file,
711+ retries=3, retry_delay=0)
712+ self.assertEquals(1, urlreader_mock.call_count,
713+ "Didn't call UrlReader once (400 class error)")
714
715- # Make sure we have > 8192 bytes in the file (buflen of UrlReader)
716- with open(src_file, "wb") as fp:
717- for line in range(0, 1024):
718- fp.write(b'Who are the people in your neighborhood.\n')
719+ def test_download_file_url_retry_then_success(self):
720+ """Retry downloading a file with server error and then succeed."""
721+ url_reader = url_helper.UrlReader
722
723- url_helper.download("file://" + src_file, target_file)
724- self.assertTrue(filecmp.cmp(src_file, target_file),
725+ with mock.patch('curtin.url_helper.UrlReader') as urlreader_mock:
726+ # return first an error, then, real object
727+ def urlreader_download(url):
728+ urlreader_mock.side_effect = url_reader
729+ raise url_helper.UrlError(None, code=500)
730+ urlreader_mock.side_effect = urlreader_download
731+ url_helper.download("file://" + self.src_file, self.target_file,
732+ retries=3, retry_delay=0)
733+ self.assertEquals(2, urlreader_mock.call_count,
734+ "Didn't call UrlReader twice (first failing,"
735+ "then success)")
736+ self.assertTrue(filecmp.cmp(self.src_file, self.target_file),
737 "Downloaded file differed from source file.")
738
739

Subscribers

People subscribed via source and target branches