| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-11 | |||
| 11:41:36 | kashyap | Please see the commit message. We have already talked about it in detail before, and the rationale explains it there | |
| 11:41:47 | sean-k-mooney | i objected to the live migraiton workaround as i think that was also wrong fundementaly at a nova level so if we want to disabel this is think we shoudl have it behaind a workaround | |
| 11:42:07 | sean-k-mooney | kashyap: yep but you never convicend me it was right before | |
| 11:42:31 | sean-k-mooney | kashyap: i just agree to not block it because it was guarded behind a workaround and we still did the check by default | |
| 11:43:10 | sean-k-mooney | kashyap: really what i would liek to see is for you or someone else to complete swaping to the new api | |
| 11:43:47 | opendevreview | Balazs Gibizer proposed openstack/nova stable/xena: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859314 | |
| 11:43:48 | opendevreview | Balazs Gibizer proposed openstack/nova stable/xena: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859315 | |
| 11:44:06 | sean-k-mooney | gibi: yep proably however this was fixed in libvirt upstream with the intoduction fo new cpu models | |
| 11:44:19 | gibi | we tried the new API work that but is complicated enough that is stalls out | |
| 11:44:20 | sean-k-mooney | and i think we just have not backported that fix downsstream | |
| 11:48:18 | opendevreview | Manuel Bentele proposed openstack/nova master: libvirt: Add configuration options to set SPICE compression settings https://review.opendev.org/c/openstack/nova/+/828675 | |
| 12:00:50 | gibi | so moving this under a WA flag; would that mean that people with certain hw should always set the WA flag, i.e. in case of mpx https://review.opendev.org/c/openstack/nova/+/869536/ | |
| 12:01:33 | gibi | and we can only remove the WA flag after we resurrect https://review.opendev.org/c/openstack/nova/+/762330 and finish it? | |
| 12:02:31 | kashyap | sean-k-mooney: Hi, just reading back. As I noted in the commit, swapping out the APIs is not worth it at this point - and I don't have bandwidth for it | |
| 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 | [quote] | |
| 12:23:09 | kashyap | Adding a new CPU model is not that serious, but it's not good either as | |
| 12:23:10 | kashyap | it causes unnecessary compatibility issues with older versions of | |
| 12:23:10 | kashyap | libvirt. Especially adding a new CPU model which does not exist in QEMU | |
| 12:23:11 | kashyap | does not make any sense, as libvirt would need to translate it to | |
| 12:23:11 | kashyap | something else when starting QEMU. | |
| 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 | Last night, we had a case of crash of nova when deleting a VM: | |
| 12:54:51 | zigo | https://paste.opendev.org/show/bRFOvaXxQxs5vVdwzVRV/ | |
| 12:54:51 | zigo | one colleague of mine says it's because the image was deleted first. Is he right? | |
| 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* ?!? | |