Merge lp://staging/~barry/ubuntu-system-image/lp1444347 into lp://staging/ubuntu-system-image/server

Proposed by Barry Warsaw
Status: Merged
Merged at revision: 271
Proposed branch: lp://staging/~barry/ubuntu-system-image/lp1444347
Merge into: lp://staging/ubuntu-system-image/server
Diff against target: 433 lines (+254/-63)
2 files modified
lib/systemimage/diff.py (+89/-51)
lib/systemimage/tests/test_diff.py (+165/-12)
To merge this branch: bzr merge lp://staging/~barry/ubuntu-system-image/lp1444347
Reviewer Review Type Date Requested Status
Steve Langasek (community) Needs Fixing
Caio Begotti (community) Approve
Stéphane Graber Pending
Registry Administrators Pending
Review via email: mp+260597@code.staging.launchpad.net

Description of the change

Addresses LP: #1444347 by ensuring that any hardlinks which at first appear not to change between the source and target, but which their underlying link target *does* change, gets properly modified in the diff. This handles the following cases (among others):

source:
a (regular file)
b -> a (via hardlink)

target
a (regular file, modified)
b -> a (via hardlink)

If the diff doesn't not also include b then after application of the diff, b will point to a's *old* inode, but not the "new" a. This branch ensures that in this case, b also gets modified by the diff so that it will point to a's new inode.

Coincidentally, this also fixes a related previously broken case:

source:
a (regular file)
b -> a (via hardlink)

target:
b (regular file)
a -> b (via hardlink)

In this case, both a and b need to be modified in the diff.

This branch includes a few other drive-by improvements.

* Rather than use hard to read nested indexes to get at the bits of information, we use a namedtuple for the internal data structures, which allow us to use easy to read attribute access to ensure we're getting at the bits and pieces we intend to get to.

* Along those lines, this actually fixes some broken indexing in the big "switched hardlinks" conditional. E.g. devminor wasn't being checked previously, nor was gid. Those are now being checked.

* Lots of comments :)

* A few minor code style improvements.

One more important note as you're reviewing this branch. There is a change in an existing test, but I believe it is legitimate. In the original test, the hardlink c/h -> d did not get modified, but this is exactly the case we're trying to fix above. In the test, d does change but the hardlink to d (i.e. c/h) wasn't being changed. That means that after the delta, c/h would point to the old d inode, and we want it to point to the new d.

To post a comment you must log in.
Revision history for this message
Caio Begotti (caio1982) wrote :

Looks alright given the bug info, even though I can't reproduce or test it in a device myself :-)

review: Approve
Revision history for this message
Steve Langasek (vorlon) wrote :

Applying the changes to the test suite without the changes to the code, I see only the following set of test failures:

test_generate_tarball (systemimage.tests.test_diff.DiffTests) ... FAIL
test_print_changes (systemimage.tests.test_diff.DiffTests) ... FAIL
test_link_count_2_order_ab (systemimage.tests.test_diff.TestHardLinkTargetIsModified) ... FAIL

Crucially, the two unpack tests did *not* fail:

test_unpack_ab (systemimage.tests.test_diff.TestHardLinkTargetIsModified) ... ok
test_unpack_ab_swap_roles (systemimage.tests.test_diff.TestHardLinkTargetIsModified) ... ok

This indicates that the unpack tests have not succeeded in modeling the failure that was seen in the wild (see further comments inline).

Without that, I don't think we can be confident in the correctness of this fix.

I've posted a follow-up comment on the bug with some analysis of the original failed delta. I believe the processing of the 'removed' file is key - in the original scenario, system/usr/bin/python3.4m is removed and then re-unpacked, and system/usr/bin/python3.4 is unchanged as a result (because the removal breaks the relationship between the two hardlinks).

The unpack tests need to accurately replicate the behavior of the upgrader.

review: Needs Fixing
274. By Barry Warsaw

A more accurate test of the unpack case.

Revision history for this message
Barry Warsaw (barry) wrote :

Good catch, thanks! I've merged your branch into mine, so this should be ready to go. I'm running the full tox suite locally, but I expect it to pass.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: