Code review comment for lp://qastaging/~vishvananda/nova/cow

Revision history for this message
Soren Hansen (soren) wrote :

>>> + sudo modprobe nbd
>> This belongs in Nova proper. If qemu-nbd doesn't do this on its own, we
>> should do it from Nova.
> I feel like punting on this for now, since I'm not sure where this should be
> done.

Ok. Right before the first call to qemu-nbd, I would guess, but we can
fix that post-FF.

>> Have you given any thought to how the move from partitions to full disks
>> will affect guests? I mean.. The whole partitioning madness wasn't added
>> to being with because someone was bored :)
> It may require changes to some images, but amazon already has a second drive
> instead of partitions for some of their instance types, so there is precedent

There is precedent, yes. In the Ubuntu images we (at least used to)
handle this by having different fstab's depending on whether it was i386
images or amd64 (which is the deciding factor for this on EC2). Bah,
let's try it, deal with the fall out later.

>>> +
>>> + If cow is True, it will make a CoW image instead of a copy.
>>> + """
>>> + if not os.path.exists(target):
>>> + base_dir = os.path.join(FLAGS.instances_path, '_base')
>>> + if not os.path.exists(base_dir):
>>> + os.mkdir(base_dir)
>>> + os.chmod(base_dir, 0777)
> > I really don't like this chmod call. Who needs access? Let's work ou
> > twho to make that happen properly.
> yes the create image is doing the same thing to make sure libvirt/qemu can
> access it. I don't want to risk breaking permissions so I made a bug for
> this:
> https://bugs.launchpad.net/nova/+bug/702651

Cool.

> >> + if inst['instance_type'] == 'm1.tiny' or prefix == 'rescue-':
> >> + size = None
> >> + root_fname += "_sm"
> > What does this suffix denote?
> The sm is for images that aren't resized to 10G
> so if you launch ami-tty as m1.tiny, the 25M image will be cached as ami-
> tty_sm and if you launch it as m1.small the 10G resized image will be cached
> as ami-tty. I needed to differentiate so that we can still launch without
> forcing a resize, which is truly necessary for testing.

Makes sense.

Alright, lgtm.

review: Approve

« Back to merge proposal