| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-09-02 | |||
| 11:51:04 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/792364/8/nova/exception.py | |
| 11:51:06 | sean-k-mooney | in the followup | |
| 11:51:20 | stephenfin | sean-k-mooney: that's a different review | |
| 11:51:29 | sean-k-mooney | yes | |
| 11:51:45 | sean-k-mooney | hum | |
| 11:51:52 | stephenfin | ohhhhh | |
| 11:51:54 | sean-k-mooney | ok its also in INVALID_FLAVOR_IMAGE_EXCEPTIONS | |
| 11:52:01 | sean-k-mooney | so it was getting converted too | |
| 11:52:08 | sean-k-mooney | ok it should be good | |
| 11:52:17 | stephenfin | so because we were introducing this new helper in that change, you suggested changing it there | |
| 11:52:26 | stephenfin | that was not at all obvious :-D context, lad | |
| 11:52:45 | stephenfin | anyway, yeah, it was all getting translated to HTTP 400 in the end | |
| 11:52:54 | stephenfin | and even if it wasn't, this would be an okay change IMO | |
| 11:53:06 | sean-k-mooney | to a 400 | |
| 11:53:13 | sean-k-mooney | yes to a 500 no | |
| 11:53:26 | stephenfin | yup, fair | |
| 11:53:28 | sean-k-mooney | anyway ok it should be good again | |
| 11:53:34 | stephenfin | yup, should be | |
| 11:54:50 | sean-k-mooney | and no i was chaning the code in hte bottom change becasue i tought it was not being caught there | |
| 11:55:08 | stephenfin | gotcha | |
| 11:55:53 | sean-k-mooney | we have a top level excption handeler above that that catuche anything that inherits form NovaException | |
| 11:56:11 | sean-k-mooney | that is not caught else whwere i tought we were going all the way to that | |
| 11:56:31 | stephenfin | The whole nova.exception module needs a rework to either double down on exception inheritance or remove it entirely. It's all very confusing. That rework would be seriously tedious though and I doubt anyone sane will want to do it :) | |
| 11:57:07 | sean-k-mooney | ya espcially since we marshal between them in a few places too | |
| 11:57:16 | stephenfin | yup | |
| 11:57:36 | opendevreview | Balazs Gibizer proposed openstack/nova master: Add --sleep option for archive_deleted_rows --until-complete https://review.opendev.org/c/openstack/nova/+/771749 | |
| 12:10:07 | kashyap | sean-k-mooney: Heya, are you able to tell what's off with this negative test (only remaining failure from the CI for me) - It passed when I posted the patch locally; https://cpaste.org/?e49aa4ae738ca284#5xdtN3VCgvzp3P5fFPXDmYJ8gYxVRQvuGaKyfxsRjYfb | |
| 12:11:02 | sean-k-mooney | do you have a link to the review | |
| 12:12:00 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/794796? | |
| 12:12:04 | kashyap | Yep | |
| 12:12:11 | kashyap | Sorry, was just about to fetch it | |
| 12:12:30 | sean-k-mooney | simple | |
| 12:12:45 | sean-k-mooney | 1.30 is not less then 1.30 https://review.opendev.org/c/openstack/nova/+/794796/3/nova/objects/image_meta.py#190 | |
| 12:12:46 | kashyap | (I fixed the trivial notification object part; so ignore that test, pls) | |
| 12:12:59 | sean-k-mooney | so self.assertRaises(exception.ObjectActionError, | |
| 12:13:01 | sean-k-mooney | obj.obj_to_primitive, '1.30') | |
| 12:13:05 | sean-k-mooney | shoudl ne '1.29' | |
| 12:13:47 | kashyap | sean-k-mooney: Urgh, <embarassed> :D | |
| 12:14:55 | kashyap | sean-k-mooney: Thanks! | |
| 12:15:03 | sean-k-mooney | There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. | |
| 12:15:19 | kashyap | sean-k-mooney: LOL | |
| 12:16:55 | opendevreview | Merged openstack/nova master: workarounds: Remove rbd_volume_local_attach https://review.opendev.org/c/openstack/nova/+/805648 | |
| 12:18:33 | opendevreview | Federico Ressi proposed openstack/nova master: [WIP] Debug APIs call failures https://review.opendev.org/c/openstack/nova/+/806683 | |
| 12:18:57 | opendevreview | Federico Ressi proposed openstack/nova master: Debug Nova APIs call failures https://review.opendev.org/c/openstack/nova/+/806683 | |
| 12:27:06 | opendevreview | Federico Ressi proposed openstack/nova master: [WIP] Check Nova project changes with tobiko scenario test cases https://review.opendev.org/c/openstack/nova/+/806853 | |
| 12:31:28 | opendevreview | Federico Ressi proposed openstack/nova master: Check Nova project changes with Tobiko scenario test cases https://review.opendev.org/c/openstack/nova/+/806853 | |
| 12:40:55 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: Allow 'bochs' as a display device option https://review.opendev.org/c/openstack/nova/+/794796 | |
| 12:51:54 | gibi | sean-k-mooney: :D | |
| 13:13:06 | opendevreview | Takashi Natsume proposed openstack/nova master: Update compute rpc version alias for xena https://review.opendev.org/c/openstack/nova/+/807137 | |
| 13:20:54 | kashyap | sean-k-mooney: Oh, duh. In my revision I missed the extra test that stephenfin asked above. Adding that too | |
| 13:50:13 | opendevreview | Kevin Zhao proposed openstack/nova master: Support Cpu Compararion on Aarch64 Platform https://review.opendev.org/c/openstack/nova/+/763928 | |
| 14:00:17 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: Allow 'bochs' as a display device option https://review.opendev.org/c/openstack/nova/+/794796 | |
| 14:00:59 | kashyap | stephenfin: Fixed --^ That should address all you asked for. Let me know if I missed anything. And thanks for the review. | |
| 14:04:14 | opendevreview | Balazs Gibizer proposed openstack/placement master: Reproduce 404 when allocation queried with 1.38 https://review.opendev.org/c/openstack/placement/+/807154 | |
| 14:04:21 | opendevreview | Balazs Gibizer proposed openstack/placement master: Fix adding 'unknown' to the ConsumerTypeCache https://review.opendev.org/c/openstack/placement/+/807155 | |
| 14:14:57 | gibi | lyarwood, stephenfin, bauzas: can I get a second set of eyes on https://review.opendev.org/c/openstack/osc-placement/+/804458 (consumer_types osc-placement client support) | |
| 14:15:55 | bauzas | gibi: doing it now | |
| 14:17:06 | gibi | thanks you | |
| 14:24:54 | kashyap | gibi: WHen you get 20 minutes, mind have a look at this? - https://review.opendev.org/c/openstack/nova/+/763928 | |
| 14:31:58 | opendevreview | Merged openstack/osc-placement master: Add support for microversion 1.38 consumer types https://review.opendev.org/c/openstack/osc-placement/+/804458 | |
| 14:49:52 | gibi | kashyap: are you OK with that patch? | |
| 15:01:56 | kashyap | gibi: 80%; I just want to do a real migration test w/ that patch - that gives me more peace of mind. (I wanted to split it up for better reviewability but just couldn't get around to it.) | |
| 15:02:33 | kashyap | gibi: How about this: | |
| 15:03:02 | gibi | kashyap: I have no problem with the patch, as far as see it is specialized to arm64 so we cannot broke our main deployers with it :) | |
| 15:03:04 | kashyap | gibi: I'll setup a local env to do a bunch more tests on KVM (our CI tests only QEMU) | |
| 15:03:11 | kashyap | gibi: Oh, wait | |
| 15:03:24 | kashyap | gibi: I gave you the wrong patch; damn | |
| 15:03:25 | gibi | or you linked me something else :D | |
| 15:03:35 | kashyap | gibi: Yeah, grrrr me. | |
| 15:03:39 | gibi | no worries | |
| 15:03:47 | kashyap | I meant this chunky one (hence the "20 minutes") - https://review.opendev.org/c/openstack/nova/+/763928 | |
| 15:04:10 | kashyap | Feel free to say: "no, that is too much review at this point". | |
| 15:04:27 | kashyap | But I can answer any questions about that patch. | |
| 15:04:56 | kashyap | gibi: Incidentally, the commit message from the patch author is a verbatim copy/paste of what I wrote in one of the reviews :D | |
| 15:05:15 | gibi | :) | |
| 15:05:54 | gibi | you linked the wrong patch again https://review.opendev.org/c/openstack/nova/+/763928 | |
| 15:05:57 | kashyap | gibi: The short summary is - this patch will be replacing the two older libvirt CPU APIs that are now causing more problems in the new world of hardware CVE flaws | |
| 15:06:11 | kashyap | gibi: Gah, sorry, this one - https://review.opendev.org/c/openstack/nova/+/762330/ | |
| 15:06:19 | kashyap | "libvirt: Use improved guest CPU config APIs" | |
| 15:06:47 | gibi | ok that is chunky indeed :) | |
| 15:06:49 | kashyap | gibi: It's a chunky one. Actually, this hour is not good to review it. And will only make you grumpy, I'm afraid | |
| 15:07:03 | gibi | I will try and lets see how far I can get | |
| 15:07:25 | kashyap | gibi: I can split it up, if you like. (That's also the right thing in my view, and told that on the review a few times) | |
| 15:07:36 | kashyap | (And also in the original spec I wrote) | |
| 15:09:40 | kashyap | gibi: I'll copy/paste from my notes, the "outline" of the patch: | |
| 15:09:44 | kashyap | - libvirt: Introduce compare_hypervisor_cpu() method | |
| 15:09:44 | kashyap | - libvirt: Introduce baseline_hypervisor_cpu() method | |
| 15:09:44 | kashyap | - Rework _get_guest_cpu_model_config() method to take advantage of the new CPU APIs | |
| 15:09:48 | kashyap | - Update the get_capabilities() method to make use of baseline_hypervisor_cpu() | |
| 15:09:51 | kashyap | - Update the check_can_live_migrate_destination() method to use compare_hypervisor_cpu() | |
| 15:10:57 | gibi | thanks | |
| 15:38:01 | melwitt | gibi: thank you for reviewing my patch. I'm going to try a simpler fix and try to restore the original func test. if it works, I think reviewers might like it more | |
| 15:38:51 | bauzas | melwitt: you're on my plate | |
| 15:39:22 | bauzas | but I've seen gibi's concerns so I held a bit | |
| 15:39:59 | melwitt | bauzas: yes thank you for your review too, I'm going to try a simpler approach bc as you said the current patch looks pretty hacky | |
| 15:40:10 | gibi | melwitt: I have no problem with the logic of the current fix. Just had some nits about docs | |
| 15:40:36 | gibi | melwitt: but sure, if you have a simpler solution then I'm happy to review that too later | |
| 15:40:44 | melwitt | gibi: understood. I worry that it's too confusing | |
| 15:41:02 | melwitt | (for maintainability) | |
| 15:41:30 | gibi | OK | |