| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-09 | |||
| 12:10:00 | gibi__ | and drop deps | |
| 12:10:36 | sean-k-mooney | https://github.com/openstack/os-vif/blob/master/tox.ini | |
| 12:10:38 | stephenfin | I'd keep 'deps' but only for test-requirements.txt and doc/requirements.txt | |
| 12:10:49 | sean-k-mooney | if we compare it to osvif we droped the install_command a while ago | |
| 12:11:09 | stephenfin | sean-k-mooney: you've got 'skipsdist = true' though | |
| 12:11:23 | sean-k-mooney | true | |
| 12:11:25 | stephenfin | so it won't actually install os-vif | |
| 12:11:30 | sean-k-mooney | but for nova we doint | |
| 12:11:36 | sean-k-mooney | https://github.com/openstack/nova/blob/master/tox.ini | |
| 12:11:40 | stephenfin | Yup, so it will install nova | |
| 12:12:20 | sean-k-mooney | ok that is why the os-vif packages were not being installed | |
| 12:12:20 | stephenfin | os-vif is working because Python adds $PWD to $PYTHONPATH | |
| 12:12:29 | stephenfin | or rather, was | |
| 12:12:39 | stephenfin | but I think tox 4 might have changed that, idk | |
| 12:12:43 | sean-k-mooney | we could have actully kept the two commits sepeate instaed fo squashihng them | |
| 12:13:27 | sean-k-mooney | stephenfin: i had to rebase this on top of gibis tox4 change https://review.opendev.org/c/openstack/os-vif/+/869500/2 because the os-vif package was not found | |
| 12:13:56 | sean-k-mooney | this combined one works https://review.opendev.org/c/openstack/os-vif/+/868420/3 | |
| 12:14:09 | sean-k-mooney | but we also could have used install command i guess | |
| 12:15:57 | stephenfin | gibi__: one other thing: instead of 'pip install' can we do 'python -m pip install' | |
| 12:16:17 | stephenfin | there's a reason for that that clarkb brought up on openstack-discuss recently (I think) but I don't recall what | |
| 12:16:24 | stephenfin | it's what tox itself does though | |
| 12:17:07 | sean-k-mooney | i have been pushing people to move to -m pip install and got that changed in devstack | |
| 12:17:43 | sean-k-mooney | there are some odd behaivors with user install/venvs when using pip that does not happen if you use python -m pip | |
| 12:18:12 | sean-k-mooney | bascailly you can have the pip package isntalled without the pip script being in your current path | |
| 12:19:10 | sean-k-mooney | also due to how setup tools console scripts work there can also be some funkyness in a mixed python2/python3 enve with the #! line | |
| 12:19:27 | sean-k-mooney | so the module apporch is just generally more robost | |
| 12:23:34 | gibi__ | ack | |
| 13:46:03 | opendevreview | Balazs Gibizer proposed openstack/placement master: Make tox.ini tox 4.0.0 compatible https://review.opendev.org/c/openstack/placement/+/868418 | |
| 13:46:43 | gibi | stephenfin, sean-k-mooney: ^^ | |
| 13:47:45 | kashyap | gibi: What do you think of workarounds like this? -- https://review.opendev.org/c/openstack/nova/+/869536/ (I'm not a fan of it...) | |
| 13:47:50 | kashyap | (Gibi or anyone :-)) | |
| 13:52:04 | kashyap | (You can comment directly on the review.) | |
| 13:54:06 | gibi | sean-k-mooney, stephenfin: I cannot switch os-vif to install_command from deps it results in https://paste.opendev.org/show/bkSH8jaL4WH04S2sge5t/ | |
| 13:54:47 | gibi | basically we have a constraing about os-vif in global requirements so the editable install building locally will conflict with the global constraint | |
| 13:55:11 | stephenfin | okay, I guess we need to continue using 'deps' there so | |
| 13:55:35 | gibi | stephenfin: even if it means we allow ignoring the upper constraint during the editable install? | |
| 13:55:52 | stephenfin | we'll still install the deps first, right? | |
| 13:58:01 | gibi | right | |
| 13:58:15 | gibi | but the package install can decided to upgrade the deps as there is no constraint there | |
| 13:58:45 | stephenfin | but would it, assuming the deps are already satisfied? | |
| 13:59:07 | gibi | I don't know but relying on that feels dirty :) | |
| 13:59:17 | stephenfin | It does, yeah :) | |
| 13:59:25 | gibi | kashyap: I'm not even sure I understand the problem statement in that patch | |
| 13:59:29 | stephenfin | I wonder if this is a pip bug? | |
| 14:00:37 | gibi | it feels like a tox shortcoming as tox calls pip install | |
| 14:00:46 | gibi | to install the deps of the package | |
| 14:02:40 | gibi | basically the install_package_deps step in tox ignores the deps | |
| 14:03:20 | gibi | so even if we put the constraint in the deps it has no effect | |
| 14:03:51 | gibi | install_package_deps does use the install_commmand hence our solution in placement to put the constraint there. | |
| 14:07:15 | kashyap | gibi: In this case, Intel "IceLake CPU" is not correctly recognized due to missing flag, "mpx". (And another problem is Nova's now-broken suboptimal CPU comparison code) | |
| 14:07:47 | kashyap | gibi: The quickest (and still valid) solution to this (and similar) problems is to _remove_ the CPU comparison that Nova does at all. As libvirt will do the Right Thing. | |
| 14:08:27 | kashyap | gibi: ... which can be done by reviving this short older patch (which you +1ed in the past). See the commit: https://review.opendev.org/c/openstack/nova/+/772917/ | |
| 14:09:56 | gibi | but that was abandoned in favor of https://review.opendev.org/q/topic:bp%252Fcpu-selection-with-hypervisor-consideration where https://review.opendev.org/c/openstack/nova/+/762330 needs substantial work | |
| 14:10:16 | kashyap | gibi: Yeah, indeed! That said: libvirt developers themselves now tell me that we (Nova) doesn't need to do that check anymore | |
| 14:10:45 | kashyap | gibi: Read this comment from Jiri here: | |
| 14:10:50 | kashyap | https://bugzilla.redhat.com/show_bug.cgi?id=2138381#c7 | |
| 14:11:02 | kashyap | Especially the 2nd paragraph | |
| 14:11:25 | kashyap | gibi: That substantial work is more fragile and I don't have cycles to baby-sit it. The best course is to remove the check w/ the shorter patch, which is still correct | |
| 14:11:40 | kashyap | As it provides most benefit with the shortest patch, IMHO. | |
| 14:11:55 | gibi | OK. do you suggest to revive https://review.opendev.org/c/openstack/nova/+/772917/ ? | |
| 14:12:27 | gibi | will that solve the issue behind https://review.opendev.org/c/openstack/nova/+/869536 too? | |
| 14:12:44 | kashyap | Yes, definitely. Based on that commit message rationale _and_ the advice of CPU modelling maintainer from libvirt | |
| 14:13:09 | kashyap | gibi: Yes | |
| 14:13:20 | kashyap | I'll comment there | |
| 14:14:56 | kashyap | Before removing that patch, we also have to deprecate (and remove later) this workaround: CONF.workarounds.skip_cpu_compare_on_dest | |
| 14:16:44 | gibi | I'm OK with this approach | |
| 14:17:00 | gibi | I don't believe we have the bandwidth to land https://review.opendev.org/q/topic:bp%252Fcpu-selection-with-hypervisor-consideration | |
| 14:17:02 | kashyap | gibi: Uh, I made a messy mistake in thinking: please ignore the above. Here's my correction: | |
| 14:17:39 | kashyap | gibi: We should actually get rid of _this_ compare_cpu() check in _ceck_cpu_compatibility() method - | |
| 14:17:42 | kashyap | https://github.com/openstack/nova/blob/8a476061c5e034016668cd9e5a20c4430ef6b68d/nova/virt/libvirt/driver.py#L991 | |
| 14:19:08 | kashyap | gibi: But the reason is the same. (The only correction is a different | |
| 14:19:13 | kashyap | ... method in Nova. | |
| 14:20:30 | gibi | ack | |
| 14:20:56 | gibi | feel free to ping me if I need to re-review the https://review.opendev.org/c/openstack/nova/+/772917/ | |
| 14:22:10 | kashyap | Nod, noted. Not that one, I'm sure we need to get rid of the check in _check_cpu_compatibility(). Thank you! | |
| 14:24:53 | sean-k-mooney | kashyap: in general im not sure i agree that we should relay on libvirt for the cpu compat check | |
| 14:25:06 | kashyap | sean-k-mooney: Why? What reason do we have? | |
| 14:25:24 | sean-k-mooney | i know libvirt will do it properly but i thing this is somethign that nova shoudl do not the hyperviros in general | |
| 14:25:33 | kashyap | FWIW, I think this is the right approach after thinking about it for a long time, and relying on the advice of SMEs on this topic. | |
| 14:25:41 | kashyap | Doing it ourselves is costly and error-prone at this point. | |
| 14:26:07 | sean-k-mooney | it may be the right approch form a libvirt point of view | |
| 14:26:11 | kashyap | When the hypervisor (in this case libvirt + QEMU) is _already_ doing it, let's please rely on it | |
| 14:26:23 | sean-k-mooney | but nova should have validateed the destination for compatibality before we invokve libvirt | |
| 14:26:26 | kashyap | sean-k-mooney: No, their advice is for management tools in general. | |
| 14:26:43 | sean-k-mooney | in fact form an nova point of veiw we shoudl have valdiated it entirly as part fo schudling | |
| 14:26:46 | kashyap | sean-k-mooney: That destination check is there (but we have added a workaround to skip it - as that's not required too) | |
| 14:26:58 | sean-k-mooney | form a nova point of view pre-livemigrate is already quite late | |
| 14:27:00 | kashyap | Yeah, "ideally..." scenarios are hard at this point :-) | |
| 14:28:10 | kashyap | sean-k-mooney: That's the one on destination skip: https://code.engineering.redhat.com/gerrit/c/nova/+/405286 | |
| 14:28:27 | kashyap | (Same reasoning in my commit applies for src check) | |
| 14:28:36 | sean-k-mooney | so form my perspective not validating all requirement in pre-livemigrateis incorrect but i understand why you want o delegate this to libvirt | |
| 14:29:03 | kashyap | Thank you. I'm getting tired of some these reports and playing whack-a-mole :( | |
| 14:30:09 | sean-k-mooney | so i think goign forward we may need to reqest a new feature in libvirt or desgin a new feature in nova | |
| 14:30:42 | kashyap | sean-k-mooney: What would the new RFE for libvirt be? | |
| 14:30:44 | sean-k-mooney | i consider it a bug to not validate all requiremnt like cpu compatiabliy so eventully i woudl like a more relyable way to do that validation | |
| 14:31:11 | sean-k-mooney | kashyap: im not quite sure | |
| 14:31:41 | kashyap | libvirt precisely did have several RFEs and it took a few years to work out all these issues and come to this point of "doing the right thing" on src + dest | |
| 14:31:47 | sean-k-mooney | in general i would like a more declaritive way to understand if live migratoin is possibel | |
| 14:32:06 | sean-k-mooney | so that we can model this in placment in some way but i dont know what that woudl look like | |
| 14:32:57 | sean-k-mooney | the imperitive check we have right now by invoking cpu_compare or the new apis is not really compatible with nova current schduling model | |