| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-06 | |||
| 16:08:16 | melwitt | efried, stephenfinI think you're right :/ I didn't notice that. commenting | |
| 16:08:37 | openstackgerrit | sean mooney proposed openstack/nova master: [DNM] testing with new lable. https://review.opendev.org/680738 | |
| 16:08:37 | openstackgerrit | sean mooney proposed openstack/nova master: [DNM] testing with old lable https://review.opendev.org/680739 | |
| 16:08:44 | efried | melwitt: looking again, I think the one I commented on is okay, but the one a couple lines down should be access_url, not access_url_base. | |
| 16:08:46 | sean-k-mooney | ignore those by the way ^ | |
| 16:09:04 | sean-k-mooney | im trying to debug stuff. | |
| 16:09:28 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Find instance in another cell during floating IP re-association https://review.opendev.org/656594 | |
| 16:09:37 | melwitt | efried: I think they should both be access_url. the second one works because it's just checking the scheme, which would be the same on both access_url_base and access_url | |
| 16:09:56 | efried | melwitt: access_url is a @property tho, not a field. | |
| 16:10:10 | melwitt | yeah, but I think you need to get the access_url first and if None, raise | |
| 16:11:02 | melwitt | that check is supposed to be checking whether there is an access_url available and the property can return None | |
| 16:11:09 | stephenfin | efried: Yeah, what melwitt is saying. I've replied on the patch | |
| 16:11:24 | melwitt | but I dunno, maybe it doesn't really matter because all that method is doing is validating the scheme | |
| 16:11:49 | melwitt | it's never going to look at anything beyond the "https" or whatever the scheme is | |
| 16:12:27 | efried | ah, that makes sense | |
| 16:12:44 | efried | arguably a bit unrelated, but sokay I guess. | |
| 16:13:32 | melwitt | yeah, I'd agree that using the actual access_url here would be unrelated to what this method is doing | |
| 16:14:08 | efried | melwitt: If you're still good, I'll +A | |
| 16:14:12 | melwitt | (realized that as I typed the next two sentences) | |
| 16:15:14 | melwitt | efried: yeah I think it's fine after thinking through | |
| 16:16:10 | efried | ack, thanks for bearing with me melwitt & stephenfin | |
| 16:16:13 | melwitt | probably did | |
| 16:16:20 | stephenfin | thanks for the review (y) | |
| 16:22:59 | efried | sean-k-mooney: can you please ack this https://review.opendev.org/#/c/680094/ ? | |
| 16:23:54 | sean-k-mooney | oh the os-vif request | |
| 16:24:02 | sean-k-mooney | yes i wanned to double check one thing | |
| 16:24:08 | sean-k-mooney | but i think its fine | |
| 16:24:16 | efried | sean-k-mooney: ...per http://lists.openstack.org/pipermail/openstack-discuss/2019-September/009182.html | |
| 16:24:26 | efried | thank you | |
| 16:24:39 | sean-k-mooney | we did 2 train release already for m1 and m2 so this tecnically is not the first os-vif release of train | |
| 16:24:52 | sean-k-mooney | but i think 1.17.0 still make sense for other reasons | |
| 16:25:19 | efried | sean-k-mooney: right, that was the main thing I was concerned about. It looked like there were two bugfixes since the last release, but one of them seemed like it might be feature-y. | |
| 16:25:56 | efried | I'm sure it's not the end of the world if we go feature bump when we should have gone bugfix bump... is it smcginnis? | |
| 16:25:59 | sean-k-mooney | one of them was a securiy bug which has been backported | |
| 16:26:33 | sean-k-mooney | honestly version number are cheap so i would rather bump more then we neeeded too then not so 1.17 looks good to me | |
| 16:27:41 | sean-k-mooney | efried: no if we bump major it would be an issue but bumping feature should not be | |
| 16:27:56 | sean-k-mooney | stien is on the 1.15.x seriese anyway | |
| 16:28:30 | efried | I guess the only downside could be confusion of having .16 and .17 both in train. But meh. | |
| 16:28:39 | smcginnis | efried: So that one wasn't a new feature? I didn't go much further than the commit message. | |
| 16:29:15 | efried | smcginnis: the commit message sounded feature-y for sure, but there was a bug (not a bp) associated. This is what I wanted sean-k-mooney to confirm for us. | |
| 16:29:31 | efried | I could look at the code, but... | |
| 16:29:38 | efried | jangutter: you have an opinion here? | |
| 16:30:29 | efried | specifically: is this https://review.opendev.org/#/c/658786/ a "feature" or a "bugfix", for purposes of release numbering? | |
| 16:31:16 | jangutter | efried: it leans more to "feature" than bugfix, especially since the original idea was "feature". | |
| 16:31:31 | efried | jangutter: Okay, good enough for me, thanks. | |
| 16:31:37 | efried | sean-k-mooney, smcginnis: ^ cool? | |
| 16:31:48 | smcginnis | Works for me. | |
| 16:32:11 | jangutter | efried: os-vif uses bugs to track features (like neutron) | |
| 16:32:39 | efried | smcginnis: acked, thanks for your patience. | |
| 16:32:53 | efried | jangutter: okay, gtk, then I'm going to have this question every release :P | |
| 16:33:53 | jangutter | efried, sean-k-mooney: is there a way to distinguish between [RFE] bugs and your run of the mill annoyances? | |
| 16:33:58 | smcginnis | Thanks for following up on that. | |
| 16:34:39 | efried | jangutter: With careful use of tagging, I'm sure there is. You may have even had that in place in this case - I admit I didn't look. | |
| 16:34:45 | smcginnis | It's been pretty common in my experience that new "features" also fix bugs, so I've never really used BPs vs bugs as an indicator. | |
| 16:35:03 | efried | But that would also require an understanding of whatever tag scheme by whoever's doing the looking | |
| 16:35:11 | efried | and yeah, more broadly, what smcginnis says. | |
| 16:36:13 | efried | it's on a team by team basis, so really we need team members to make this call one way or another (either by proposing the release patches themselves and noting this reasoning in the commit message, or by being pestered like this when releases are proposed "automatically" or otherwise externally to the team) | |
| 16:37:15 | sean-k-mooney | jangutter: we use RFE bugs in os-vif to track feature | |
| 16:37:26 | sean-k-mooney | jangutter: because we dont want to have to use specs or blueprints | |
| 16:37:40 | sean-k-mooney | so RFE bugs cant be backported as they are features | |
| 16:37:43 | sean-k-mooney | other then that no | |
| 16:40:04 | efried | aspiers: I'm looking at https://review.opendev.org/#/c/680065/8/nova/tests/unit/scheduler/test_utils.py -- I was digging for this answer myself, but am being pulled in multiple directions -- can you confirm that there are other tests covering the behavior when hw_machine_type is absent (which is to allow it through) ? | |
| 16:40:19 | efried | I think tests above those are doing it, yah? | |
| 16:40:25 | aspiers | efried: checking | |
| 16:41:03 | efried | iow I wanted to say "hey, instead of doing this to all three, just do it to one or two, so it proves the other thing" -- but don't want to block on that if other tests are already covering that. | |
| 16:41:06 | aspiers | efried: testing hw_machine_type being absent was missing until very recently, I just added it like 2 days ago | |
| 16:41:15 | aspiers | let me find where | |
| 16:41:23 | aspiers | it's mentioned in a commit message somewhere | |
| 16:41:46 | aspiers | oh yeah it's this review | |
| 16:42:07 | aspiers | let me find the lines | |
| 16:42:35 | aspiers | efried: https://review.opendev.org/#/c/680065/8/nova/tests/unit/virt/test_hardware.py@3837 | |
| 16:43:09 | efried | that's asserting failure due to no uefi... | |
| 16:43:14 | aspiers | doh | |
| 16:43:19 | aspiers | brain fried | |
| 16:43:23 | efried | where's a case that succeeds with no mach type | |
| 16:43:31 | aspiers | ah OK | |
| 16:43:45 | aspiers | checking | |
| 16:43:55 | efried | perhaps L3926 could have a None in it too? | |
| 16:44:09 | aspiers | ah that has to be tested in the driver | |
| 16:44:20 | aspiers | since with no machine type in the image, it relies on pulling it from utils | |
| 16:44:28 | aspiers | which reads nova.conf on compute node | |
| 16:44:36 | aspiers | I think it's maybe in a later patch | |
| 16:44:39 | aspiers | checking ... | |
| 16:46:58 | aspiers | efried: https://review.opendev.org/#/c/644565/52/nova/tests/unit/virt/libvirt/test_driver.py@2700 | |
| 16:47:21 | aspiers | although maybe there is a test missing for when CONF.libvirt.hw_machine_type is set to x86_64=q35 | |
| 16:47:58 | efried | aspiers: that one is *also* a failure scenario | |
| 16:48:05 | aspiers | yeah that's what I'm saying | |
| 16:48:23 | aspiers | CONF.libvirt.hw_machine_type set to x86_64=q35 would be the non-failure scenario | |
| 16:48:27 | aspiers | I can add that | |
| 16:48:48 | aspiers | efried: kudos for eagle eyes as usual :) | |
| 16:49:07 | efried | where are you going to add it? | |
| 16:50:25 | efried | aspiers: is it adequate to undo one of those three lines of delta here https://review.opendev.org/#/c/680065/8/nova/tests/unit/scheduler/test_utils.py ? | |
| 16:50:37 | efried | and then maybe also add the case next to the one in the fourth patch? | |
| 16:51:35 | efried | aspiers: there's one more comment in there, in case you agree and want to make that change at the same time. | |
| 16:51:46 | aspiers | efried_afk: ok thanks! | |
| 17:45:31 | mriedem | dansmith: so linkedin told me it was a big day for you and i was looking at the suggested comments and i thought this one was a good fit https://photos.app.goo.gl/WrC1wjQYu7Btfz2g8 | |
| 17:48:10 | dansmith | heh | |
| 17:48:21 | dansmith | yeah, I wish linkedin wouldn't ask people to do that | |
| 17:48:34 | dansmith | and/or I wish people would ignore all that crap like I do :) | |
| 17:49:55 | sean-k-mooney | i think mose people do | |