Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-08-25
14:23:49 sean-k-mooney so i did it in one pathc
14:24:15 sean-k-mooney i was not swaping to spwan to fix tehm i was fixing them so we could swap to spawn form spawn_n
14:25:19 sean-k-mooney i am fine to proceed with your leak fix for the final test
14:25:25 sean-k-mooney and then rebase the spawn change on top
14:25:33 sean-k-mooney so it clearer what is required for that
14:25:45 sean-k-mooney your fixes are less invasive then mine
14:26:23 gibi cool
14:26:36 gibi in the long run I have no problem removing the spawn_n usage from nova
14:26:52 gibi as far as I remember there was no good reason to have it in the first place
14:27:14 gibi it is less resource to use spawn_n but I don't think it is significant compared to spawn
14:28:16 sean-k-mooney so thats the thing
14:28:34 sean-k-mooney technially since its just a greenlet it shoudl be
14:28:55 sean-k-mooney but i dont think in reality it has any really difference
14:29:04 sean-k-mooney and melwitt pointed out a thing i forgot
14:29:11 sean-k-mooney when we were talking to the eventlet maintainers
14:29:25 sean-k-mooney they mentioned that there was some undefiend behavior with raw greenlets
14:29:40 sean-k-mooney so im hoping there will be no effective difference performance wise
14:29:55 sean-k-mooney and correctness wise im hopeing for less edgecases by this change
15:23:25 gibi it is less resource to use spawn_n but I don't think it is significant compared to spawn
15:23:31 gibi sorry
15:24:06 gibi yeah I agree with the overall direction
15:33:07 dansmith auniyal: is it passing tests now?
15:34:05 auniyal yes CI passing :)
15:47:48 dansmith auniyal: one very small (but weird) formatting nit
15:47:58 dansmith auniyal: if you can fix that real quick I'll +2
15:48:46 opendevreview Dan Smith proposed openstack/nova master: VERYWIP: Support setting alias on libvirt disks https://review.opendev.org/c/openstack/nova/+/892800
15:48:46 opendevreview Dan Smith proposed openstack/nova master: VERYWIP: Set libvirt device alias for volumes https://review.opendev.org/c/openstack/nova/+/892801
15:49:00 dansmith oh I guess kashyap isn't here
15:56:11 sean-k-mooney any reason for the ua- prefix for the volume id
15:56:19 dansmith auniyal: comments on the reproducer too
15:56:28 dansmith sean-k-mooney: required by libvirt (as noted in the commit message)
15:56:47 dansmith sean-k-mooney: interestingly if you omit that, it just ignores what you gave it and uses its own value with no warning :/
15:57:07 sean-k-mooney .. of couse it does
15:57:48 sean-k-mooney im debating in my head the value fo doint that in the call site as you did vs prefixing it here https://review.opendev.org/c/openstack/nova/+/892800/1/nova/virt/libvirt/config.py#1205
15:58:38 dansmith well, one reason is that libvirt gives everything auto-generated aliases if you don't specify one,
15:59:06 dansmith and so we'd need to strip out the ua- from the parsing if we do it on the input, and then there's no way to tell from the higher layers if it's one we set or libvirt set
15:59:23 dansmith default looks like:
15:59:23 dansmith <alias name='virtio-disk0'/>
15:59:36 sean-k-mooney ok thats good to know.
16:00:20 sean-k-mooney then one suggestion would be add a is a uuid_to_alais funciton
16:00:28 sean-k-mooney so we can just use taht everywhere
16:00:32 sean-k-mooney but thats a minor nit
16:01:06 sean-k-mooney i guess we wont alwasy have a uuid
16:01:13 dansmith right
16:01:18 sean-k-mooney for pci devie form flavor alaises for example
16:01:29 dansmith and root disk, if we wanted to set that
16:01:46 dansmith anyway, I mostly just wanted to make sure we could plumb that through and it would stick in libvirt
16:01:58 sean-k-mooney im not sure if we dont because we dont supprot detaching that but it would help for swap on reize perhaps
16:02:36 sean-k-mooney ya its good to get this basic proof of concept and find the "ua-" issue
16:02:37 dansmith I don't know that we need it at all, I'm just saying if we set the alias for those devices, we won't have a uuid
16:03:02 sean-k-mooney related question
16:03:20 sean-k-mooney do we know if libvirt has an api to get/remove a device via the alais
16:03:29 sean-k-mooney it would be nice if it did but probaly does not
16:03:45 dansmith idk, but it's not required I think
16:04:29 sean-k-mooney ya its likely not but it would be nice just to detach a volume by doing libvir.detach_device(alias=<volume uuid>)
16:04:39 dansmith https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceAlias
16:04:49 dansmith that will be *very* nice, IMHO
16:04:52 sean-k-mooney oh perfect
16:05:01 sean-k-mooney ya so that is what i was hoping for i think
16:05:32 sean-k-mooney it also renforice the idea that this is how libvirt want use to name/specify things to detatch ectra
16:05:43 dansmith up
16:05:48 dansmith also yup
16:07:01 sean-k-mooney so this was added in 3.9.0 which was released in late 2017 so when we first started lookign at teh lun issue it did not exist and when we looed at them last i dont know if we have a high enough min libvirt
16:07:16 sean-k-mooney but for master this looks like a nice cleanup over all
16:07:33 sean-k-mooney thanks for starting a poc of this
16:08:53 dansmith yeah, I'll get going on a spec just to lay some of this otu
16:09:10 kashyap dansmith: Hi! I hear you were looking for me :)
16:09:11 dansmith I'll have to survey where all we look up devices to see where we need to convert them to use this
16:09:26 dansmith kashyap: was just going to point you at this: https://review.opendev.org/q/topic:libvirt-dev-alias
16:09:35 dansmith and sean-k-mooney and I were chatting about it briefly just now
16:10:11 kashyap Yeah, just found 'em
16:13:46 kashyap dansmith: Just added a small comment here - https://review.opendev.org/c/openstack/nova/+/892800
16:14:00 kashyap My guess is you already know it
16:14:12 dansmith kashyap: it's in the commit message
16:15:43 dansmith well, the ua- part is anyway
16:17:01 kashyap Ohh, I read the commit message, but I didn't see the "ua-" part explicitly called out. Blind me
16:19:10 kashyap (Oh, it's in the other commit message. Just saw it)
16:21:28 auniyal dansmith, ack, will update , thanks
16:22:18 sean-k-mooney dansmith: looking out our downstream multi attach volume docs it says "Live migration of multi-attach volumes is not available" that cinder volume migration right
16:22:36 sean-k-mooney ill check our upstrema docs but as far as i am aware we supprot live migration of vms with multi attach
16:22:38 dansmith sean-k-mooney: no idea
16:23:37 kashyap sean-k-mooney: dansmith: I just read your brief discussion on the upstream log. (I agree that we should indeed retain the prefix or else ... the odd behavior Dan noted is not nice.)
16:28:16 opendevreview Merged openstack/nova master: introduce global greenpool https://review.opendev.org/c/openstack/nova/+/873061
16:33:28 opendevreview Dan Smith proposed openstack/nova-specs master: WIP: Add libvirt-dev-alias spec https://review.opendev.org/c/openstack/nova-specs/+/892806
16:34:07 dansmith sean-k-mooney: kashyap^ just brain dump level to get it started
16:41:11 kashyap Nice; the outline looks already quite good to me. Cced myself, will do a concetrated read on Mon
16:41:37 kashyap Heh, how plainly you write the alternative "We could maintain our own mapping in our database for those device types"
16:41:47 kashyap (I mean, I like the matter-of-factly stating)
16:46:14 opendevreview Amit Uniyal proposed openstack/nova master: Add function to get all attachments in Cinder.API module https://review.opendev.org/c/openstack/nova/+/881457
16:46:14 opendevreview Amit Uniyal proposed openstack/nova master: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/889947
16:46:15 opendevreview Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284
16:50:26 dansmith auniyal: looks like you committed a bunch of extra changes into that base patch?
16:52:25 auniyal yes, that was a pep8 fix I have to do after rebase
16:52:37 auniyal I added a note in commit msg for same
16:52:46 dansmith auniyal: they're in files you haven't touched
16:53:07 dansmith they should not be in that patch
16:53:40 auniyal ack, will remove them,
17:03:14 opendevreview Amit Uniyal proposed openstack/nova master: Add function to get all attachments in Cinder.API module https://review.opendev.org/c/openstack/nova/+/881457
17:03:15 opendevreview Amit Uniyal proposed openstack/nova master: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/889947
17:03:15 opendevreview Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284

Earlier   Later