Merge lp://qastaging/~caio1982/ubuntu-system-image/remote-sis-generator-skips-metadata into lp://qastaging/ubuntu-system-image/server

Proposed by Caio Begotti
Status: Needs review
Proposed branch: lp://qastaging/~caio1982/ubuntu-system-image/remote-sis-generator-skips-metadata
Merge into: lp://qastaging/ubuntu-system-image/server
Diff against target: 157 lines (+44/-14)
1 file modified
lib/systemimage/generators.py (+44/-14)
To merge this branch: bzr merge lp://qastaging/~caio1982/ubuntu-system-image/remote-sis-generator-skips-metadata
Reviewer Review Type Date Requested Status
Barry Warsaw (community) Needs Fixing
Review via email: mp+260210@code.qastaging.launchpad.net

Description of the change

Feel free to replace this code with whatever you guys think is best but please do fix this bug :-)

I think the commit message is pretty descriptive so let me know if you need more info. We found a situation with Capomastro's instance of image server where only the first device in a channel list would have its Ubuntu rootfs's versioning right, so this address that problem as... well... we definitely need the rootfs versions to be right. People were chasing us for that last week :-(

Before: https://pastebin.canonical.com/132002/

After: https://pastebin.canonical.com/132001/

To post a comment you must log in.
273. By Caio Begotti

add missing debug logging to the existing (!!!) fixes for the problem we originally saw... so apparently only the remote s-i-s generator was behind this :-)

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

Thanks for taking on this bug. I've added some comments, but none that you really need to address in this branch.

What I *do* think needs fixing before this can land is... tests! Can you please add a test for the bug? As Steve pointed out in my own branch, the best way to ensure this fixes the problem is to apply the test-adding diff, see the test fail, then add the fix and see it succeed.

The change looks okay on the face of it, but please add a test.

review: Needs Fixing
Revision history for this message
Caio Begotti (caio1982) wrote :

Thanks, Barry! Just filed https://bugs.launchpad.net/ubuntu-system-image/+bug/1461263 for the encoding bit. It might take me a while to write tests for this MR though as I honestly don't know the code much and I see all the other generators are missing tests for metadata (at least for version_detail) and they should have some.

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

On Jun 02, 2015, at 09:05 PM, Caio Begotti wrote:

>Thanks, Barry! Just filed
>https://bugs.launchpad.net/ubuntu-system-image/+bug/1461263 for the encoding
>bit.

Thanks!

>It might take me a while to write tests for this MR though as I honestly
>don't know the code much and I see all the other generators are missing tests
>for metadata (at least for version_detail) and they should have some.

Yep. You can't get to 100% in one fell swoop, but every new test is an
investment in the future quality of the project.

Unmerged revisions

273. By Caio Begotti

add missing debug logging to the existing (!!!) fixes for the problem we originally saw... so apparently only the remote s-i-s generator was behind this :-)

272. By Caio Begotti

make the remote system-image generator respect the metadata of existing images

this is a really big problem when you have multiple devices in the same channel and both pull the same ubuntu rootfs, exactly like we have right now for arale... e.g. 'arale' and 'generic' devices both pulling a stable ubuntu rootfs from system-image.ubuntu.com

device 'generic' first pulls it just fine and loads its json with the metadata, including ubuntu='' versioning, but then when the 'arale' device imports its stuff, the ubuntu rootfs has been imported already so the path matches and the generator returns it without loading its json file

no ubuntu='' version string for any other devices in the channel :-(

fun fact: a 'generic' device is always processed first, even if it was created later because of a sorting done in the device list (i haven't really checked it so i assum it happens), so with the current code ONLY the 'generic' devices get their ubuntu='' version right for all those rootfs tarballs

271. By Caio Begotti

remove unnecessary (and too verbose) logging, as over time the server will be so crowded with images this might be totally useless and poluting

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: