image backend checklist - dianaclarke/openstack-notes GitHub Wiki
-
Generic Questions
-
Add back pre-gizzly stuff DONE
- Added back for only the "from image" case (not "from func" case too)
- Matt's scratch code from before: http://paste.openstack.org/show/506660/
- The old code from master: http://paste.openstack.org/show/506691/
- tempest tested? [TODO]
-
Should preallocate?
- Only the
create_from_image
case or both? BOTH
- Only the
-
Should resize?
- I think only the
create_from_image
case? YES - perhaps it should always be:
if size > cached_image.virtual_size:
? YES
- I think only the
-
Verify base size?
- I think only the
create_from_image
case? YES
- I think only the
-
What about the original
os.access(self.path, os.W_OK)
. Add back? NO -
Locking? [TODO]
-
Audit things Matt added that I removed [TODO]
-
Don't use the cache for a few backends? DONE
- removed cache calls for the
create_from_func
case for LVM & Flat
- removed cache calls for the
-
What about
max_size
in the original? DONE- Matt removed
max_size
: https://review.openstack.org/#/c/326947/
- Matt removed
-
Where is the
fileutils.ensure_tree(base_dir)
check inImage.cache
going to go, post refactor?- The image cache handles this now.
-
Do we need the
if size
inif size and size > image_info.virtual_size:
?- Yes, unless you are resizing size with be
None
.
- Yes, unless you are resizing size with be
-
Consider adding back check Matt added: [TODO]
"Qcow2 disk has backing file which is not in the image cache"
- And:
if cached_image.path == backing_path:
-
Safe vs unsafe calls DONE
- Don't need the unsafe call anymore, added it to the delete patch.
-
Revisit preallocate testing,
should_preallocate
, negative fallocate tests? Done -
Answer these questions DONE
-
What about this old clone code? [TODO]
if image_meta.get('disk_format') not in ['raw', 'iso']: reason = _('Image is not raw format') raise exception.ImageUnacceptable(image_id=image_id_or_uri, reason=reason)
-
and this old clone code? [TODO]
reason = _('No image locations are accessible') raise exception.ImageUnacceptable(image_id=image_id_or_uri, reason=reason)
-
-
base
- What about this short circuit case in
Image.cache()
? Answer: assume the caller will only call these new methods when they actually want to create something. That is, feel free to ditch the abort logic.
# If self.path doesn't exist or base doesn't exit... if not self.exists() or not os.path.exists(base): self.create_image(fetch_func_sync, base, size, *args, **kwargs)
- What about this short circuit case in
-
flat
-
Example paths:
NoBacking(instance, disk_name='disk') base: instances/_base/sha1-image-uuid path: instances/instance-uuid/disk
-
Shouldn't use the image cache for create_from_func DONE
-
Should
ensure_tree(self.path)
? NO- Before: no
- After: no, this really only makes sense for ploop
-
Should
remove_path_on_error(self.path)
? YES- Before: sometimes
- After: yes, we should cleanup on error
- Tested: [TODO]
-
Should
os.path.exists(self.path)
short circuit? Confirm correct: NO- Before: yes
- After: no, we are intentionally not short circuiting on
self.path
exists anymore
-
Verify base size? YES
create_from_image
case only
-
Should resize? YES
- Before:
if size:
, plusif size and size > self.get_disk_size
fromImage.cache()
- After:
if size and size > cached_image.virtual_size:
(create_from_image
case only)
- Before:
-
Should preallocate? YES
- preallocate: maybe
- _can_fallocate: maybe (inherited)
- Tested: yes, but not full branch coverage
-
Matt's was quite different. Why? DONE
- Especially the
libvirt_utils.create_image()
call
- Especially the
-
-
qcow2
-
Example paths:
Qcow2(instance, disk_name='disk') base: instances/_base/sha1-image-uuid path: instances/instance-uuid/disk
-
Should
ensure_tree(self.path)
? NO- Before: no
- After: no, this really only makes sense for ploop
-
Should
remove_path_on_error(self.path)
? YES- Before: yes
- After: yes
- Tested: [TODO]
-
Should
os.path.exists(self.path)
short circuit? NO- Before: no
- After: no, we are intentionally not short circuiting on
self.path
exists anymore
-
Verify base size? YES
create_from_image
case only
-
Should resize? YES
- Before:
if size:
, plusif size and size > self.get_disk_size
fromImage.cache()
- After:
if size and size > cached_image.virtual_size:
(create_from_image
case only)
- Before:
-
Should preallocate? YES
- preallocate: maybe
- _can_fallocate: maybe (inherited)
-
-
lvm
-
Example paths:
Lvm(instance, disk_name='disk') base: instances/_base/sha1-image-uuid path: /dev/some-volume-group/instance-uuid_disk
-
Shouldn't use the image cache for create_from_func DONE
-
Should
ensure_tree(self.path)
? NO- Before: no
- After: no, this really only makes sense for ploop
-
Should
self.remove_volume_on_error(self.path)
? YES- Before: yes
- After: yes
- Tested: ish, on key manager KeyError [TODO]
-
Should
os.path.exists(self.path)
short circuit? NO- Before: no
- After: no, we are intentionally not short circuiting on
self.path
exists anymore
-
Verify base size? YES
create_from_image
case only
-
Should resize? YES
- Before:
if resize:
, plusif size and size > self.get_disk_size
fromImage.cache()
- After:
if size and size > cached_image.virtual_size:
(create_from_image
case only)
- Before:
-
Should preallocate? NO
- preallocate: maybe <--- dead? [TODO]
- _can_fallocate: NO
-
-
rbd
-
Example paths:
Rbd(instance, disk_name='disk') base: instances/_base/sha1-image-uuid path: rbd:FakePool/instance-uuid_disk:id=FakeUser:conf=FakeConf rbd_name: instance-uuid_disk
- Note that the clone case has a nested
lock()
call, revisit that N/A- no longer has the lock
- Note that the clone case has a nested
-
Should
ensure_tree
? NO- I don't think so since it didn't before and they are not regular file paths
-
Should
remove_path_on_error
? YES- It didn't before, but perhaps it should using
remove_volume_on_error
. The clone case callsremove_volume_on_error
. Should thecreate_from_image
andcreate_from_func
cases do that too?- Matt said we should always clean up on error, come back to this.
- Tested: [TODO]
- It didn't before, but perhaps it should using
-
Should
os.path.exists(self.rbd_name)
short circuit? NO- Before: yes
- After: no, we are intentionally not short circuiting on
self.rbd_name
exists anymore
-
Verify base size? YES
create_from_image
case only
-
Should resize? YES
- Before:
if size and size > self.get_disk_size
fromRbd.create_image()
, plusif size and size > self.get_disk_size
fromImage.cache()
- After:
if size and size > cached_image.virtual_size:
(create_from_image
case only) - What about the clone case? It does not resize, should it? [TODO]
- Matt says yes, it should resize in the clone case. Come back to this later.
- Before:
-
Should preallocate? NO
- preallocate: NO
- _can_fallocate: NO
-
-
ploop
-
Example paths:
Ploop(instance, disk_name='disk') base: instances/_base/sha1-image-uuid path: instances/instance-uuid/disk target: instances/instance-uuid/disk/root.hds
-
Should
ensure_tree(self.path)
? YES- Before: yes
- After: yes, b/c the backend nests one level deeper than the others
- Tested: [TODO]
-
Should
remove_path_on_error(self.path)
? YES- Why the special remove function? Answer: b/c this backend nests one level deeper than the others
- Before: yes
- After: yes
- Tested: [TODO]
-
Should
os.path.exists(self.path)
short circuit? NO- Before: yes
- After: no, we are intentionally not short circuiting on
self.path
exists anymore
-
Verify base size? YES
create_from_image
case only
-
Should resize? YES
- Before:
if size:
fromPloop.create_image()
, plusif size and size > self.get_disk_size
fromImage.cache()
- After:
if size and size > cached_image.virtual_size:
(create_from_image
case only)
- Before:
-
Should preallocate? NO
- preallocate: NO
- _can_fallocate: maybe (inherited)
- But! There are preallocate unit tests for ploop... why? [TODO]
-