Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-02-22
16:51:11 bauzas I'm way off reviewing specs, and I need to
16:51:16 dansmith efried: okay
16:51:18 bauzas so I'll stop looking at bugs
16:51:27 dansmith I expect it's fixed in queens because of migration allocations
16:51:32 mriedem bauzas: that's likely a bug that claudiub has a fix up for
16:51:32 efried I didn't know whether we punted fixing that in pike because too complicated, and just fixed it in Queens with cdent's POST allocations business
16:51:34 dansmith but we probably need a fix for pike still
16:51:43 mriedem dansmith: bauzas: not necessarily
16:51:46 mriedem depends on where the failure happens
16:51:52 efried yeah. But I also didn't know whether resize ~= migration in this case
16:52:02 mriedem resize = cold migrate
16:52:13 efried but still subject to duplicate allocations mriedem?
16:52:25 efried i would spect so
16:52:27 mriedem bauzas: dansmith: this https://review.openstack.org/#/c/543971/
16:52:37 dansmith mriedem: um, what? we only need a fix depending on where it happens?
16:52:50 mriedem efried: all move operations require an allocation on the source and dest hosts,
16:53:06 efried makes sense.
16:53:07 mriedem pre-queens, we just doubled those rather than split them
16:53:11 bauzas mriedem: excellent, tz
16:53:17 bauzas will close the bug as duplicate
16:53:21 mriedem dansmith: if prep_resize fails, we cleanup the allocations
16:53:31 mriedem if resize_instance fails, we don't, which is https://review.openstack.org/#/c/543971/
16:53:42 mriedem there are 10 places you can blow off your foot with resize
16:53:57 dansmith sure, sounded like you were disagreeing with me that we need a fix
16:54:24 mriedem i think we're talking about different things
16:54:33 mriedem or different failure scenarios
16:54:36 bauzas right
16:54:56 bauzas anyway, I'm done with triaging bugs for that week
16:55:02 mriedem bauzas: i don't know if it's a duplicate, depends on where it fails
16:55:18 mriedem where does resize fail for missing ssh host keys?
16:55:25 bauzas mriedem: anyhow, the bug was reported because the user forgot to provide SSH keys
16:55:35 bauzas while we ask for that
16:55:45 mriedem ok let's assume it's the same problem then :)
16:55:52 mriedem if it's not, they can re-open
16:55:57 bauzas zactly
17:02:35 openstackgerrit Merged openstack/nova stable/queens: Add functional test for deleting BFV server with old attach flow https://review.openstack.org/546803
17:02:51 openstackgerrit Merged openstack/nova stable/queens: Ensure attachment_id always exists for block device mapping https://review.openstack.org/546804
17:03:57 mriedem +2 on claudiub's fix now https://review.openstack.org/#/c/543971/
17:09:39 mriedem gdi i'm annoyed that we run several tempest jobs for docs changes
17:09:55 mriedem and that irrelevant-files isn't overridable
17:12:57 mriedem mdbooth: are you around?
17:13:16 mdbooth mriedem: I am!
17:13:30 mriedem mdbooth: can you take a look at https://review.openstack.org/#/c/542646/ ? it's related to a restriction / validation you put into the api
17:15:21 openstackgerrit Eric Fried proposed openstack/nova master: rp: GET /resource_providers?required= https://review.openstack.org/546837
17:15:21 openstackgerrit Eric Fried proposed openstack/nova master: Make _get_sharing_providers more efficient https://review.openstack.org/547008
17:15:23 efried edleafe: ^^
17:15:39 efried I mean ^
17:15:55 efried Anyone else have an IRC client that converts emoticons to graphics?
17:18:57 mdbooth mriedem: So it's my understanding that your reproducer *should* fail.
17:19:26 efried That *would* be an awkward dict situation
17:19:28 mdbooth mriedem: I have always taken min_disk to mean: the stuff this image does requires at least this much space.
17:20:05 mdbooth Which is obviously different to the image size.
17:22:37 mriedem mdbooth: i don't think that's the original image min_disk though
17:23:04 mriedem we say that's the min_disk b/c of the flavor root_gb
17:23:06 mdbooth mriedem: Reading back a bit more I was just going to ask exactly that.
17:23:30 mdbooth It sounds to me like the bug is in the setting of min_disk in the first place, then.
17:23:36 mdbooth It should be the min_disk of the original image.
17:23:57 mriedem so on my devstack right now, the single cirros image has min_disk=0
17:24:11 mriedem i can create a 1GB volume as the boot disk for my server that has that image in it,
17:24:21 mriedem snapshot it, and nova says the min_disk for the snapshot is now equal to the flavor.root_gb
17:24:27 mriedem 20 in my case
17:24:30 mriedem 20GB
17:25:01 mdbooth IMHO the bug is " and nova says the min_disk for the snapshot is now equal to the flavor.root_gb"
17:25:02 mriedem lemme create a volume from the image and see what the image meta shows in the volume
17:25:09 mriedem i agre
17:25:10 mriedem *agree
17:25:36 mdbooth IOW, I think the bug happens before the proposed fix
17:26:04 claudiub mriedem: regarding your comment on https://review.openstack.org/#/c/543971/5/nova/compute/manager.py , you mean the docstring which is on finish_resize (L4444), or a new one explaining the allocation cleanup part?
17:26:21 mriedem mdbooth: ok so i created a 1GB volume from a cirros image in devstack http://paste.openstack.org/show/682285/
17:26:30 mriedem | volume_image_metadata | {u'container_format': u'bare', u'min_ram': u'0', u'disk_format': u'qcow2', u'image_name': u'cirros-0.3.5-x86_64-disk', u'image_id': u'9491dd49-665c-46f4-9dbf-9a94aa1c9843', u'checksum': u'f8ab98ff5e73ebab884d80c9dc9c7290', u'min_disk': u'0', u'size': u'13267968'} |
17:26:42 mriedem the volume image meta shows the internal image min_disk is 0
17:27:16 mriedem mdbooth: what i'm not sure about, is if nova should say the volume-backed snapshot image min_disk should be the original image min_disk, or the volume size?
17:27:32 mriedem or, maybe we just don't care
17:27:38 mriedem and shouldn't even put min_disk in the snapshot
17:27:58 mriedem because if you use that thing to create another server, nova is going to boot from volume using the volume snapshot metadata embedded in the snapshot image
17:28:16 mriedem ie block_device_mapping='[{"guest_format": null, "boot_index": 0, "delete_on_termination": false, "no_device": null, "snapshot_id": "1158debc-9922-458d-9b46-8ef688334d65", "device_name": "/dev/vda", "disk_bus": "virtio", "image_id": null, "source_type": "snapshot", "tag": null, "device_type": "disk", "volume_id": null, "destination_type": "volume", "volume_size": 1}]'
17:28:31 mriedem so nova shouldn't really care about min_disk in this case
17:28:53 mriedem we're not going to create an ephemeral disk for the server, we're going to create a volume
17:29:28 mriedem here https://github.com/openstack/nova/blob/master/nova/virt/block_device.py#L684
17:29:33 openstackgerrit Merged openstack/nova-specs master: Explain how filters differ for /r_p vs. /a_c https://review.openstack.org/547056
17:29:58 mriedem claudiub: i meant on the new _finish_resize_helper method,
17:30:07 mdbooth I think min_disk should stay, and be the min_disk of the original image, because it's image metadata.
17:30:33 mdbooth Same as hw_* says the image needs certain hardware
17:30:42 mdbooth min_disk says it needs so much disk
17:31:06 mriedem for the image sure,
17:31:17 mriedem so the min_disk on the image could be 100MB, and the volume could be 1GB
17:31:38 mriedem anyway, that also works for me i think
17:31:38 mdbooth min_disk is about the *workload*, not how big the disk is
17:31:49 mdbooth We already know exactly how many bits are physically required to unpack it.
17:32:06 mdbooth min_disk says how much space is required to operate
17:32:33 mdbooth IOW we shouldn't be arbitrarily setting it where it wasn't set before.
17:33:55 mdbooth If we're going to exclude it, it should be in the context of a generic ability to filter arbitrary image metadata when creating a snapshot.
17:34:13 mdbooth Or entirely disabling copying image metadata, more likely.
17:35:11 mdbooth But I don't think min_disk can be treated specially by excluding it if it's set.
17:35:34 mriedem well we do have the non_inheritable_image_properties config option, which sucks since it's config-based API behavior
17:36:18 mdbooth Yeah
17:36:26 mriedem i guess that's just more about stuff you put into your images which shouldn't apply across snapshots, like signatures
17:36:42 mdbooth For now, I think the solution is to find whatever's setting it where it wasn't set before and kill that.
17:37:01 mdbooth Hopefully it's not cinder :/
17:37:44 mriedem not following you - setting it where it wasn't set before?

Earlier   Later