Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-01-09
12:08:34 stephenfin so we're relying on us having already installed the deps with constraints and hoping (I guess) that tox doesn't decide to reinstall them without constraints when installing the package
12:08:36 gibi__ stephenfin: we install our deps with constraints yes, but then we install the placement package's deps again without constriants
12:08:54 stephenfin so best to override the install_command
12:09:02 sean-k-mooney ok but woudl we not
12:09:13 sean-k-mooney ya would we not need to change that ^
12:09:22 stephenfin we should drop '-rrequirements.txt' from 'deps' too since it's now unnecessary and potentially confusing
12:09:51 gibi__ OK, I can move everything to install_command
12:09:51 sean-k-mooney this looks a littel differnt then normal actully
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 :(

Earlier   Later