| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-21 | |||
| 17:56:00 | openstackgerrit | Eric Fried proposed openstack/nova master: Remove deprecated [neutron]/url option https://review.opendev.org/677755 | |
| 17:56:16 | aspiers | mriedem: I would happily move the corresponding code out from driver.py at the same time if that was feasible | |
| 17:57:03 | aspiers | I'm not sure why it would become a mess for git history tracking? even git blame has for a long time been able to figure out when stuff gets moved between files | |
| 17:57:17 | aspiers | efried: exactly | |
| 17:57:47 | mriedem | i'm not a fan of the tests for something in one module living in another module | |
| 17:57:48 | aspiers | mriedem: that said, pointers on stuff to deduplicate are welcome | |
| 17:58:00 | mriedem | and like i said, i think you can just refactor the duplication in test_driver.py for lots of tests to probably make it smaller | |
| 17:58:34 | aspiers | I think our messages crossed :) ^^^ | |
| 17:58:43 | mriedem | this isn't an exercise i'm the one that's wanting to take on, so that's up to you, | |
| 17:58:55 | mriedem | but i know there is a ton of just copy test foo and tweak 1-2 lines for test bar | |
| 17:59:12 | mriedem | i also have no problem with the thing in pycharm | |
| 18:00:00 | efried | aspiers: git history is an issue, unfortunately - afaik blame will show the new commit on the new file. You would have to back up to see the previous iteration, which is awkward. (I've run into this a number of times already in placement, and it's pretty debilitating.) | |
| 18:00:25 | mriedem | correct, it sucks | |
| 18:00:36 | mriedem | the mox removal series is a prime example | |
| 18:00:45 | mriedem | it also causes issues with merge conflicts for backports | |
| 18:00:53 | sean-k-mooney | well perhaps this is jsut somethin we need to be aware of and think should we be moving things out of the driver into modules used by the driver in the future | |
| 18:01:04 | aspiers | efried: no, blame really can show where it came from | |
| 18:01:25 | sean-k-mooney | aspiers: yes but git blame on github crashes on those files | |
| 18:01:29 | sean-k-mooney | so you have to do it locally | |
| 18:01:43 | aspiers | ? | |
| 18:01:49 | efried | what's the blame trick, without iterating backward across commits? | |
| 18:01:58 | aspiers | mriedem's point about backports is a much more convincing argument against | |
| 18:02:19 | aspiers | however, if our policy is "you can't move stuff around, ever" then we're in for a miserable future | |
| 18:02:50 | mriedem | i'm not against refactoring giant methods in the libvirt driver module, deduplicating test code in test_driver, etc. i just don't want to see random tests moving out of test_driver into some other module when the code being tested is still in driver.py | |
| 18:03:20 | aspiers | mriedem: sure, that's why I said <aspiers> mriedem: I would happily move the corresponding code out from driver.py at the same time if that was feasible | |
| 18:03:21 | mriedem | aspiers: no one said that was the policy | |
| 18:03:24 | sean-k-mooney | aspiers: try opening https://github.com/openstack/nova/blame/master/nova/virt/libvirt/driver.py | |
| 18:03:36 | aspiers | mriedem: ok good :) | |
| 18:03:50 | aspiers | was beginning to sound that way | |
| 18:04:19 | mriedem | as a nova troll i have a reputation to maintain | |
| 18:04:26 | aspiers | :) | |
| 18:04:27 | mriedem | as immovable object | |
| 18:04:28 | sean-k-mooney | i think we just dont want test to be move with out the code also moving and am not sure that patches that only move code are a good idea | |
| 18:04:37 | aspiers | efried: try git gui blame for instance | |
| 18:04:55 | aspiers | some tooltips include "Copied Or Moved Here By:" | |
| 18:05:12 | aspiers | can't remember if github or gitea do the same | |
| 18:05:31 | sean-k-mooney | aspiers: i think i makes sense to consider moving things if you are refacoritn the code already. | |
| 18:05:32 | aspiers | but local is way faster anyway IME | |
| 18:05:39 | aspiers | sean-k-mooney: definitely agree with that | |
| 18:06:02 | aspiers | well, it's an idea to bear in mind anyway | |
| 18:06:28 | aspiers | 23k LoC in one file does not seem sane to me ;-) | |
| 18:07:14 | sean-k-mooney | 23K of code to test 9.75K of code and we still miss things... | |
| 18:07:43 | sean-k-mooney | the driver is not that bad coverage wise but ther are other part that are less awsome... | |
| 18:08:17 | efried | aspiers: git gui appears to be... a GUI :( :( :( | |
| 18:08:30 | aspiers | efried: shocker! who'd have thought? | |
| 18:08:33 | efried | guess it would take some getting used to. | |
| 18:09:01 | sean-k-mooney | aspiers: that is not a standard git command is it a python extentions | |
| 18:09:10 | efried | anyway, I agree enormous files suck, and being stuck with them because of sucky reasons also sucks. | |
| 18:09:19 | efried | sean-k-mooney: apt install git-gui | |
| 18:09:40 | efried | that's if you're running ubuntu. | |
| 18:09:47 | efried | If you're running another distro... just shoot yourself and be done. | |
| 18:10:07 | sean-k-mooney | PopOS which is based on ubuntu yes | |
| 18:10:18 | aspiers | efried: watch your mouth! | |
| 18:10:19 | efried | (sorry, I had a bad experience with rhel a decade ago and still haven't recovered) | |
| 18:10:31 | aspiers | you ran RHEL as a desktop? eww | |
| 18:10:34 | sean-k-mooney | although i have a rhel8 system for the last while... not sure i will keep it but it currently works | |
| 18:10:34 | aspiers | Fedora is OK | |
| 18:10:37 | efried | yeah, that was my problem | |
| 18:10:43 | aspiers | try openSUSE :) | |
| 18:10:50 | efried | is that still a thing?? | |
| 18:10:59 | efried | Is it written in FORTRAN? | |
| 18:11:09 | sean-k-mooney | aspiers: i used tumbelweed for a while and mostly liked it | |
| 18:11:13 | aspiers | ok, good time to leave | |
| 18:11:32 | sean-k-mooney | not sure i liked yast but it was nice to have up to date packages | |
| 18:49:40 | mriedem | melwitt: a couple of things in the archive patch https://review.opendev.org/#/c/507486/ | |
| 18:49:46 | mriedem | and a couple of nits in the nova-next patch on top of that | |
| 18:50:06 | mriedem | both should be small | |
| 18:50:52 | mriedem | melwitt: efried: i think we're ok to merge https://review.opendev.org/#/c/675218/6 now yeah? | |
| 18:51:11 | melwitt | thanks | |
| 18:51:49 | melwitt | yeah I think all is well | |
| 18:55:40 | mriedem | onto openstack resource provider inventory set --aggregate | |
| 18:57:35 | melwitt | heh | |
| 19:10:05 | melwitt | I was panicking with the potential loss of my launchpad bug text after a 503 "TimeoutError" but found when I reloaded the page, it posted everything I had written | |
| 19:10:16 | melwitt | phhheeewwww | |
| 19:13:21 | mriedem | happens a lot, just have to keep refreshing | |
| 19:17:34 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 19:21:23 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract SEV-specific bits on host detection https://review.opendev.org/636334 | |
| 19:21:34 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 19:22:51 | mriedem | melwitt: i think i have an idea for how to make one of multiple rps fail to set inventory in aggregate in https://review.opendev.org/#/c/640898/11 | |
| 19:22:56 | mriedem | add allocations to one of them | |
| 19:24:04 | openstackgerrit | melanie witt proposed openstack/nova master: Make a failure to purge_db fail in post_test_hook.sh https://review.opendev.org/677806 | |
| 19:25:01 | melwitt | mriedem: nice! thanks, I was stumped about that | |
| 19:25:19 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 19:26:28 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract SEV-specific bits on host detection https://review.opendev.org/636334 | |
| 19:26:42 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 19:28:40 | mriedem | melwitt: same, until about 5 minutes ago :) | |
| 19:29:50 | melwitt | \o/ | |
| 19:29:56 | mriedem | looking at the placement code i'm not totally sure if it handles the "drop inventory below what's allocated but don't remove the resource class" or if it's just "can't remove a resource class inventory that has allocations" - i see the latter in the placement code but not sure about the former, but i'm pretty sure it enforces that | |
| 19:29:58 | mriedem | efried: ^ right?! | |
| 19:31:08 | mriedem | hmm | |
| 19:31:08 | mriedem | https://review.opendev.org/#/c/640898/11 | |
| 19:31:09 | efried | mriedem: You can reduce inventory below what's allocated. You can't set total to zero (I'm 95% sure of that, would have to check). | |
| 19:31:10 | mriedem | ops | |
| 19:31:22 | mriedem | efried: not before 1.26 anyway.. | |
| 19:32:09 | mriedem | yeah so if your allocations exceed capacity it only logs a warning | |
| 19:32:10 | mriedem | https://github.com/openstack/placement/blob/master/placement/objects/resource_provider.py#L619 | |
| 19:32:35 | mriedem | if you try to remove a resource class entirely that has allocations it'll fail https://github.com/openstack/placement/blob/master/placement/objects/resource_provider.py#L84 | |
| 19:33:01 | mriedem | so melwitt i guess one of the providers has to have some resource class inventory and a consumer is using that rc, and then in set inventories you drop/omit that rc, | |
| 19:33:05 | mriedem | it could just be some CUSTOM_FOO in your test | |
| 19:33:25 | melwitt | ah ok | |