| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-25 | |||
| 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: Set libvirt device alias for volumes https://review.opendev.org/c/openstack/nova/+/892801 | |
| 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: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 | <alias name='virtio-disk0'/> | |
| 15:59:23 | dansmith | default looks like: | |
| 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: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/889947 | |
| 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: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: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 17:03:15 | opendevreview | Amit Uniyal proposed openstack/nova master: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/889947 | |
| 17:11:28 | auniyal | thanks dansmith | |
| 17:24:04 | sean-k-mooney | dansmith: this wont shock you but since we are discussin cinder voluems in varios place i cam acroos an unsupproted edgecase a while back | |
| 17:24:37 | sean-k-mooney | if you have an image of format=iso you can both a vm form it and then install to a cinder data volume | |
| 17:24:49 | sean-k-mooney | however you cant do that if you try to do bfv | |
| 17:25:14 | sean-k-mooney | as we assume all bfv case are block devices not "cd-roms" | |
| 17:25:43 | sean-k-mooney | so we try to use the volume contianign the iso data as a block device | |
| 17:25:57 | sean-k-mooney | so wehn the vm boot it cant actully load the bootload/root filesytem | |
| 17:26:39 | sean-k-mooney | i feel like that should be simple to fix if we ever had time to do it | |
| 17:27:44 | sean-k-mooney | as a result you cant actully use cidner volumes as "dirver disks" or install media today without somehow converting the iso to a bootable filesystem | |
| 17:30:27 | dansmith | why would you bfv with the root disk as the iso? | |
| 17:30:49 | dansmith | you want bfv with the root disk as a volume and the iso as a secondary non-root device right? same for driver disks | |
| 17:31:28 | sean-k-mooney | well that also wotn work volume created form iso just dont work when attach to a vm | |
| 17:31:54 | sean-k-mooney | but my usecase was installing openshift on openstack using the installer iso | |
| 17:32:11 | sean-k-mooney | my home cloud is configured ot only do bfv | |
| 17:32:51 | sean-k-mooney | i worked aroudn it by temproy addign a flavor with some local storage and then did the install to a cienr volume and then booting another vm form that data volume after the fact | |
| 17:33:08 | dansmith | yeah I think the iso-on-volume thing is known, | |