| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-11 | |||
| 12:03:23 | kashyap | sean-k-mooney: Also, libvirt developers themselves are suggesting that management tools should let libvirt do the work here. | |
| 12:03:33 | kashyap | I have already mentioned that in the commit message. If that doesn't convince you; nothing else will :) | |
| 12:03:44 | sean-k-mooney | kashyap: they may be experts in libvirt but not the managemnet tools | |
| 12:03:57 | sean-k-mooney | failures in live_migrate are expensive | |
| 12:04:25 | sean-k-mooney | we have already plugged the networkign and done other operatiosn on the destination that need to be roled back | |
| 12:04:32 | kashyap | sean-k-mooney: Well, I totally realize that as someone who debugs a crap ton of live migration problems | |
| 12:04:41 | sean-k-mooney | the reason we have the check early in pre-livemeigrate is to prevent that setup | |
| 12:05:07 | kashyap | sean-k-mooney: Well. I would prefer if you don't block this on theoretical objections or "ideally X" scenario | |
| 12:05:40 | kashyap | Also, on the destination, removing the API check is still correct. I put the workaround as a compromise. | |
| 12:07:15 | sean-k-mooney | we can agree to disagree | |
| 12:10:44 | kashyap | Sure. FWIW, I heard no complaints from the folks using the workaround to skip the check on dest. (As I expected.) | |
| 12:12:46 | kashyap | gibi: Also on that Intel "mpx" saga: the issue is due to two reasons: (a) Intel is phasing out MPX; and consequently (b) QEMU removed that feature from all CPU models (Skylake, Icelake, Cascadelake) starting from QEMU 4.0 - https://gitlab.com/qemu-project/qemu/-/commit/ecb85fe48cacb | |
| 12:13:50 | sean-k-mooney | kashyap: libvirt adressetd that by adding a new cpu model without mpx https://gitlab.com/lvoytek/libvirt/-/commit/39f5bcd48323e814c910a92bf8ef76dd0166680d | |
| 12:14:42 | kashyap | sean-k-mooney: It is rejected; it was just merely submitted as a patch. I (and libvirt devs) reviewed that upstream libvirt list | |
| 12:14:59 | kashyap | I even linked to that patch discussion in my URLs | |
| 12:15:20 | sean-k-mooney | i see | |
| 12:16:01 | sean-k-mooney | apparently the cpu_model_extra_flags is not working properly to work around that by the way | |
| 12:16:03 | sean-k-mooney | https://bugzilla.redhat.com/show_bug.cgi?id=2158181 | |
| 12:16:11 | kashyap | sean-k-mooney: Let me get the URL from the upstream libvirt folks (from last year). It was spread over months so the archives are split | |
| 12:16:22 | kashyap | sean-k-mooney: Indeed, that's because the model is evaluated _before_ the flags | |
| 12:16:46 | sean-k-mooney | i got that form https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1978064 | |
| 12:16:57 | sean-k-mooney | it was marked as fix released | |
| 12:17:15 | sean-k-mooney | kashyap: shoudl we not fix that then instead of removing the check | |
| 12:17:31 | sean-k-mooney | we should be validating the modifed one no? | |
| 12:17:45 | kashyap | sean-k-mooney: The problem here is entirely due the older API, hence the recommendetaion to remove that now-buggy check | |
| 12:18:08 | sean-k-mooney | that really feels like a regression/bug to remove it | |
| 12:18:30 | kashyap | Maybe they're carrying downstream Ubuntu-specific patch? | |
| 12:18:44 | sean-k-mooney | ya maybe im not sure | |
| 12:19:05 | sean-k-mooney | its confusing give i knwo libvirt uses bugzilla for trackign right | |
| 12:19:22 | sean-k-mooney | so this libvirt tracker is presumabel for the ubunu package | |
| 12:19:58 | kashyap | sean-k-mooney: Upstream libvirt uses gitlab now | |
| 12:20:24 | kashyap | (E.g. the "mpx" issue was discussed here, also filed by the Ubuntu person: https://gitlab.com/libvirt/libvirt/-/issues/304) | |
| 12:21:09 | kashyap | (Also notice that is a private libvirt branch that you linked to) | |
| 12:21:12 | sean-k-mooney | kashyap: so looking at https://gitlab.com/libvirt/libvirt/-/issues/304#note_1065798706 | |
| 12:21:24 | sean-k-mooney | do we use check=full or generate that check string today | |
| 12:21:45 | sean-k-mooney | this is not related to the start up check | |
| 12:22:02 | sean-k-mooney | but i think we leave that to libvirt to set in most if not all cases | |
| 12:23:03 | kashyap | sean-k-mooney: Here was the rationale that libvirt rejected that adding extra named model. Which I fully agree with: | |
| 12:23:09 | kashyap | Adding a new CPU model is not that serious, but it's not good either as | |
| 12:23:09 | kashyap | [quote] | |
| 12:23:10 | kashyap | libvirt. Especially adding a new CPU model which does not exist in QEMU | |
| 12:23:10 | kashyap | it causes unnecessary compatibility issues with older versions of | |
| 12:23:11 | kashyap | something else when starting QEMU. | |
| 12:23:11 | kashyap | does not make any sense, as libvirt would need to translate it to | |
| 12:23:14 | kashyap | [/quote] | |
| 12:23:16 | kashyap | https://listman.redhat.com/archives/libvir-list/2022-August/233717.html | |
| 12:24:01 | sean-k-mooney | ya that makes sense why they would not want to do that | |
| 12:24:23 | kashyap | sean-k-mooney: Also almost since 4 years ago, QEMU and libvirt have stopped adding explicit named models like that "-noTSXAndThat_AndThisFeature" | |
| 12:24:26 | kashyap | Right. | |
| 12:24:38 | kashyap | sean-k-mooney: No, we don't use "check=full" | |
| 12:24:39 | sean-k-mooney | i think i would still prefer to move the cpu check to include the extra flags | |
| 12:24:50 | kashyap | (Answering the earlier question) | |
| 12:24:53 | sean-k-mooney | kashyap: ya i didnt think we did but wantted to confirm | |
| 12:25:11 | kashyap | sean-k-mooney: I'll let some actual tests with real CPU models be done by QE to see the existing patch's impact | |
| 12:25:16 | kashyap | That way we have clear evidence. | |
| 12:25:55 | sean-k-mooney | ok i need to go take my blood pressue medication and do a few bits. brb | |
| 12:26:08 | kashyap | Sure, take care! And thanks for the discussion | |
| 12:54:51 | zigo | one colleague of mine says it's because the image was deleted first. Is he right? | |
| 12:54:51 | zigo | https://paste.opendev.org/show/bRFOvaXxQxs5vVdwzVRV/ | |
| 12:54:51 | zigo | Last night, we had a case of crash of nova when deleting a VM: | |
| 12:55:20 | sean-k-mooney | no i dont think so | |
| 12:55:39 | sean-k-mooney | this is related to the nvram not the glance/vm disk image | |
| 12:55:43 | zigo | How come Nova can't "undefine domain with nvram" then? | |
| 12:56:39 | zigo | Was this fixed in a version higher than Victoria? | |
| 12:56:41 | sean-k-mooney | it might be related to https://bugs.launchpad.net/nova/+bug/1785123 | |
| 12:57:06 | zigo | Oh, thanks. | |
| 12:57:18 | sean-k-mooney | zigo: there have been some nvram/uefi issues fixed since then yes | |
| 12:57:52 | sean-k-mooney | https://review.opendev.org/q/project:openstack/nova+nvram | |
| 12:58:29 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/335512 that seams to be the most driectly related | |
| 12:59:29 | zigo | It's abandonned though ... :/ | |
| 12:59:35 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L298 | |
| 12:59:44 | sean-k-mooney | yes but the fix was condtionaly added on master | |
| 12:59:48 | sean-k-mooney | if uefi is supported | |
| 12:59:56 | sean-k-mooney | so we fixed it in a differnt patch | |
| 13:00:11 | sean-k-mooney | https://github.com/openstack/nova/commit/539d381434ccadcdc3f5d58c2705c35558a3a065 | |
| 13:00:33 | sean-k-mooney | hum apparently in ocata | |
| 13:00:51 | sean-k-mooney | zigo: what version of qemu are you using | |
| 13:00:55 | sean-k-mooney | sory libvirt | |
| 13:01:14 | zigo | 7.0.0 | |
| 13:01:19 | zigo | (the one from Bullseye) | |
| 13:02:28 | sean-k-mooney | i wonder what support_uefi is set too | |
| 13:02:52 | opendevreview | Merged openstack/nova master: Support unshelve with PCI in placement https://review.opendev.org/c/openstack/nova/+/854616 | |
| 13:04:32 | sean-k-mooney | zigo: so we do this https://github.com/openstack/nova/blob/70fd0cfc8ee2b541ffc1f9feb129314965d1670c/nova/virt/libvirt/driver.py#L1349-L1351 | |
| 13:05:54 | zigo | Victoria has the same code... | |
| 13:06:17 | zigo | So you see... instance.image_meta.properties.get | |
| 13:06:30 | zigo | Is this taken from the *image* ?!? | |
| 13:06:58 | zigo | Or is it a copy of the image meta? | |
| 13:07:23 | sean-k-mooney | a copy but i assume its uefi | |
| 13:09:39 | sean-k-mooney | you might be higing a kernel api change where some parmater changed for 1/0 to y/n | |
| 13:09:59 | sean-k-mooney | can you check the output of virsh domcapabilities | |
| 13:11:05 | zigo | https://paste.opendev.org/show/bEp66voE4chYd3lF3eme/ | |
| 13:11:10 | zigo | What am I looking for? | |
| 13:12:14 | zigo | The <os supported='yes'> bits? | |
| 13:12:22 | sean-k-mooney | <loader supported='yes'> | |
| 13:12:31 | sean-k-mooney | line 12 | |
| 13:12:51 | zigo | Is it supposed to be 0/1 instead ? | |
| 13:13:14 | sean-k-mooney | no i was thinkin of a change for secure boot | |
| 13:13:18 | sean-k-mooney | which is seperate | |
| 13:16:12 | opendevreview | Merged openstack/nova stable/train: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866201 | |
| 13:18:09 | zigo | sean-k-mooney: That bit of code is supposed to remove the disk containing the NVRAM values, right? | |