| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-11 | |||
| 09:50:51 | kashyap | gibi: Aaah, I see. | |
| 09:51:28 | gibi | kgube: I will try | |
| 09:51:47 | kgube | thanks! | |
| 10:01:35 | gibi | kashyap: left feedback in https://review.opendev.org/c/openstack/nova/+/869587 | |
| 10:02:19 | gibi | kashyap, sean-k-mooney: I vaguely remember that you discussed removing the this check ^^ before. What was the outcome of that? | |
| 10:02:48 | auniyal | Hi gibi | |
| 10:02:55 | auniyal | can you please review these also | |
| 10:02:55 | gibi | auniyal: hi! | |
| 10:02:57 | auniyal | https://review.opendev.org/c/openstack/nova/+/864006 | |
| 10:02:59 | kashyap | gibi: Thanks! Will check in abit | |
| 10:03:04 | auniyal | https://review.opendev.org/c/openstack/nova/+/864006 | |
| 10:03:13 | auniyal | https://review.opendev.org/c/openinfra/openstack-map/+/866568 | |
| 10:09:25 | gibi | bauzas: do you want to check https://review.opendev.org/c/openstack/nova-specs/+/855490 (Use extend volume completion action) or I can +A it? | |
| 10:09:48 | gibi | you have review prio +1 on it | |
| 10:09:51 | gibi | hence my question | |
| 10:11:26 | opendevreview | Lukas Piwowarski proposed openstack/nova stable/zed: DNM: Test change in run-tempest role https://review.opendev.org/c/openstack/nova/+/869804 | |
| 10:14:00 | bauzas | gibi: sorry, discussing with Uggla but sure I can do it | |
| 10:15:59 | gibi | bauzas: it is more like do you want to? we have 2 +2s on it | |
| 10:16:36 | kashyap | gibi: Responded. Thanks for the review. I hope I have answered at least 60% of your questions :) | |
| 10:40:43 | pslestang | Hello all, dansmith will you have some time to review https://review.opendev.org/c/openstack/nova/+/867832, I pushed an other patchset after your +2 review | |
| 10:42:11 | gibi | kashyap: responeded | |
| 10:43:34 | opendevreview | Lukas Piwowarski proposed openstack/nova stable/zed: DNM: Test change in run-tempest role https://review.opendev.org/c/openstack/nova/+/869804 | |
| 10:52:34 | kashyap | gibi: Thank you. On the 2nd point, I'm wondering if there's a fairly uncomplicated way to return the flags | |
| 10:53:09 | kashyap | (And no your first point, will drop that dead variable) | |
| 11:05:58 | gibi | kashyap: my point is that the original function did not return that information, so that information was only use locally there, but the usage of it is removed by you | |
| 11:06:26 | gibi | so I think the generation of that information can be removed too | |
| 11:07:23 | kashyap | gibi: Okay, let me play a little more with it. I only want to make sure people can still specify extra flags and we propagate that | |
| 11:08:03 | kashyap | gibi: Ah, we still consider the extra flags in _get_guest_cpu_model_config() method | |
| 11:08:21 | kashyap | So we should be good | |
| 11:08:30 | gibi | I believe this was not the place where we actually handled the extra flags | |
| 11:09:04 | gibi | yepp _get_guest_cpu_model_config seem to be the real place | |
| 11:09:50 | kashyap | Yep, indeed we handle it _get_guest_cpu_model_config(). I myself added that ... seeing the note <emabarassed emoji> | |
| 11:11:00 | kashyap | gibi: Thanks! Respinning. | |
| 11:21:52 | opendevreview | Aaron S proposed openstack/nova master: Add further workaround features for qemu_monitor_announce_self https://review.opendev.org/c/openstack/nova/+/867324 | |
| 11:22:59 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: Remove compareCPU() check in _check_cpu_compatibility() https://review.opendev.org/c/openstack/nova/+/869587 | |
| 11:24:42 | kashyap | gibi: (While you still have the context. Hope I got that right) --^ | |
| 11:26:35 | opendevreview | Aaron S proposed openstack/nova master: Add further workaround features for qemu_monitor_announce_self https://review.opendev.org/c/openstack/nova/+/867324 | |
| 11:30:51 | opendevreview | Merged openstack/nova stable/ussuri: Adds a repoducer for post live migration fail https://review.opendev.org/c/openstack/nova/+/864006 | |
| 11:31:35 | opendevreview | Merged openstack/nova stable/ussuri: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866196 | |
| 11:32:17 | opendevreview | Artom Lifshitz proposed openstack/nova master: [Broken WIP] 2.94: FQDN in hostname https://review.opendev.org/c/openstack/nova/+/869812 | |
| 11:32:29 | sean-k-mooney | gibi: basically i thing its wrong for nova not to enfore the cpu compatiablity itself ideally at teh schduleing point and failures at the live migrate call to livert are too late. however apprently the libvirt folks are advising that we delegate this check to libvirt alone. so if we remove this i want us to add something to our backloag to go replace it in the future | |
| 11:33:19 | sean-k-mooney | currently we attempt to do that in pre-livemigrate using the old compareCPU api | |
| 11:34:28 | opendevreview | Balazs Gibizer proposed openstack/nova stable/yoga: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859312 | |
| 11:34:29 | opendevreview | Balazs Gibizer proposed openstack/nova stable/yoga: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859313 | |
| 11:35:13 | gibi | sean-k-mooney: the current patch is removing the compatibility check at the compute startup, it does not change the check at live migration | |
| 11:36:05 | sean-k-mooney | i have not looked at the current patch just the ones that were created before | |
| 11:36:13 | sean-k-mooney | why would we want to remove it at compute start up | |
| 11:36:17 | sean-k-mooney | looking at it now | |
| 11:38:16 | sean-k-mooney | as an unconditional change i think this is wrong | |
| 11:41:32 | gibi | sean-k-mooney: I think the renewed interest for this is coming from https://review.opendev.org/c/openstack/nova/+/869536/ | |
| 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 | |