| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-08 | |||
| 15:27:33 | artom | ... I think | |
| 15:28:35 | mriedem | yes i can read his comments and i think they align with what i just said, | |
| 15:28:56 | mriedem | "Nova then asks neutron to bind port on compute-0 again, at it happens:" - that's the migrate_instance_finish call on the source finish_revert_resize to update the port binding | |
| 15:29:15 | mriedem | "And after that, L2 agent on compute-0 wire port again:" - i assume that's the plug_vifs call from the driver's finish_revert_migration on the source host | |
| 15:30:35 | mriedem | artom: how recreateable is this? it would be a lot more clear if we just had some logging on the source that said, "updating port bindings to point at the source host" and then "finish revert migration in the driver which will plug vifs" | |
| 15:30:53 | mriedem | because if we saw the event come between those 2 messages we know it's the port binding change that is triggering the event before we're ready to wait for it | |
| 15:30:54 | artom | mriedem, like, 70% in our CI? | |
| 15:31:06 | mriedem | are you able to test with patches from upstream? | |
| 15:31:21 | artom | I'd need to check with the CI guys, but I could provide them test builds, yeah | |
| 15:31:55 | kashyap | aspiers: You about...? | |
| 15:32:43 | kashyap | aspiers: When you are -- at the risk of adding more work ... wonder if we should simply split out the addition of getDomainCapablities() method into its own patch. | |
| 15:33:06 | mriedem | artom: ew do you have to patch an rpm or something? | |
| 15:33:13 | kashyap | It is just my OCD of "one logical change per-patch thing". And it allows quicker merge, too. As it'll be easier on the reviewers eyes | |
| 15:33:14 | artom | mriedem, scratch build | |
| 15:33:35 | artom | Backport a patch, build RPMs with that | |
| 15:34:04 | mriedem | artom: actually you should be able to determine this with existing logs | |
| 15:34:11 | kashyap | (A "scratch build" is something that is short-lived, and will be "scratched" from the build system) | |
| 15:34:54 | artom | (Backport a patch, build a SRPM without pushing anything, scratch build, I should have said) | |
| 15:35:04 | mriedem | artom: you should see this on the source https://github.com/openstack/nova/blob/4f9bc724010f0c935bf83a6d19bdd805e86b7086/nova/network/neutronv2/api.py#L3355 | |
| 15:35:23 | mriedem | with binding:host_id changing to point at the source host | |
| 15:35:43 | mriedem | and then you should see this from the driver https://github.com/openstack/nova/blob/4f9bc724010f0c935bf83a6d19bdd805e86b7086/nova/virt/libvirt/driver.py#L8965 | |
| 15:35:47 | artom | mriedem, ack, looking | |
| 15:36:43 | mriedem | granted there is some other stuff that happens in the driver after that before unplug_vifs happens, so it's a tight window | |
| 15:42:25 | aspiers | kashyap: yeah could do, I kind of like there being an incentive to get SEV stuff merged though ;-) | |
| 15:42:54 | aspiers | when do we fork for stein? | |
| 15:43:27 | kashyap | aspiers: Yeah, I hear you. But as we both know ... it can be used for multiple features :-) | |
| 15:43:57 | kashyap | (I won't insist on it, though. But if you appetite...) | |
| 15:43:59 | aspiers | we already hit feature freeze, right? | |
| 15:44:03 | aspiers | https://wiki.openstack.org/wiki/Nova/Stein_Release_Schedule | |
| 15:44:04 | kashyap | aspiers: Yes, yesterday | |
| 15:44:10 | kashyap | mriedem: ^ Right? | |
| 15:45:04 | mriedem | yes | |
| 15:45:13 | mriedem | please don't be approving anything that's not already approved | |
| 15:45:14 | kashyap | aspiers: Also think of it this way: splitting it out allows it to be merged while SEV bits get reviewed :-) | |
| 15:45:39 | aspiers | I guess | |
| 15:45:40 | kashyap | mriedem: This (AMD SEV work) was already approved for Stein | |
| 15:45:55 | aspiers | I think mriedem means W+1 | |
| 15:45:58 | kashyap | aspiers: But sorry to be "that guy"; you're allowed to hate me for 5 minutes. | |
| 15:46:04 | aspiers | haha | |
| 15:46:21 | sean-k-mooney | aspiers: we will fork stien at RC1 so master is feature frozen for the next 2 ish weeks until that is done i think | |
| 15:46:42 | aspiers | sean-k-mooney: OK thanks | |
| 15:47:04 | kashyap | aspiers: This work will spill over into "Train", yes? | |
| 15:47:10 | sean-k-mooney | rc1 will be around march 21st | |
| 15:47:26 | aspiers | kashyap: https://review.openstack.org/#/c/641994/ | |
| 15:47:43 | aspiers | Oh, you already saw that :) | |
| 15:47:46 | aspiers | I forgot | |
| 15:47:53 | kashyap | Yeah, no worries. | |
| 15:48:33 | aspiers | Anyway, ultimately I need to do whatever you guys think is best :) | |
| 15:48:39 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Documentation for bandwidth support https://review.openstack.org/642064 | |
| 15:48:42 | aspiers | If splitting it out helps then let's do that | |
| 15:49:14 | kashyap | aspiers: Yeah, it also will be less "daunting" to those who don't normally dwell on this area of code | |
| 15:49:28 | aspiers | Sure | |
| 15:49:29 | mriedem | what are you talking about splitting out? | |
| 15:49:45 | aspiers | the call to getDomainCapabilities | |
| 15:50:13 | kashyap | mriedem: Yeah, the adding of getDomainCapablities() in libvirt/host.py | |
| 15:50:34 | aspiers | kashyap: I was also thinking about splitting this code move out https://review.openstack.org/#/c/633855/11/nova/virt/libvirt/utils.py | |
| 15:50:49 | mriedem | aspiers: ok, i see you already put the -W back on your own change that i don't want merged in stein at this point, so you know where i am on this | |
| 15:50:56 | kashyap | mriedem: This bit (line 680): https://review.openstack.org/#/c/633855/11/nova/virt/libvirt/host.py | |
| 15:51:34 | mriedem | sure, split that out even if to just make that change smaller and easier to grok | |
| 15:51:40 | mriedem | too much code in one patch == hard to review | |
| 15:51:43 | aspiers | mriedem: yes, that's also why I submitted https://review.openstack.org/#/c/641994/ | |
| 15:51:44 | kashyap | (Yeah, I agree with mriedem on AMD SEV for Stein, I'm afraid.) | |
| 15:52:15 | kashyap | aspiers: Yes. The machine_type_mappings() in a separate change is good, too. | |
| 15:52:21 | aspiers | There's not even a debate on that, since feature freeze was yesterday :) | |
| 15:52:31 | mriedem | aspiers: you'd be surprised | |
| 15:52:56 | aspiers | Haha OK, well I'm sure some vendors are insane but not us | |
| 15:54:10 | kashyap | It's the same story with upstream _kernel_ as well :D | |
| 15:54:20 | aspiers | Yup | |
| 15:54:25 | aspiers | SUSE knows all about that ... | |
| 15:54:27 | kashyap | Once they start cutting long-term 'stable' release, everyone starts running around headless-chickens | |
| 15:54:41 | kashyap | ... "we need to get this into the "long-term" release!" | |
| 15:54:56 | sean-k-mooney | kashyap: honestly i think that is better the feature backports | |
| 15:55:00 | gibi | mriedem, melwitt: I'm wondering about how to close the https://blueprints.launchpad.net/nova/+spec/bandwidth-resource-provider the spec talks about the server move operations that was not implemented in Stein so if we use that as a scope then the bp cannot be marked implemented | |
| 15:55:18 | kashyap | sean-k-mooney: Not saying it's bad :-) Just the reality and the incentive structure I guess | |
| 15:55:31 | mriedem | gibi: i think we can complete the blueprint in stein and have a separate blueprint for move operations in train | |
| 15:55:39 | aspiers | OK I'm done for the week. I'll look at splitting that out into 3 reviews next week | |
| 15:55:56 | aspiers | Have a good w/e all! o/ | |
| 15:56:16 | kashyap | aspiers: Just a heads-up, next week the first three days I'm travelling; so, if I'm slow on responding ... you know why :-) | |
| 15:56:20 | kashyap | Have a good one! | |
| 15:56:22 | aspiers | OK np :) | |
| 15:56:22 | gibi | mriedem: shall I remove the move operation from the old spec when I update that now? | |
| 15:56:45 | mriedem | gibi: are you going to amend the stein spec for the microversion and all the other stuff that came up? | |
| 15:57:07 | mriedem | i guess i'm not sure what you're asking me :) | |
| 15:57:51 | gibi | mriedem: yeah I'm plannig to amend the spec | |
| 15:57:58 | gibi | mriedem: so I can remove the move from the scope there | |
| 15:58:53 | mriedem | ok | |
| 15:59:01 | gibi | mriedem: ok | |
| 16:03:01 | gibi | mriedem: there are two trailing patche for the bandwidth series both can be considered bugfixes, https://review.openstack.org/#/c/638711/ and https://review.openstack.org/#/c/639608/ what do you think, shall I push them forward before RC1? | |
| 16:09:33 | openstackgerrit | guang-yee proposed openstack/nova master: pass endpoint interface to Ironic client https://review.openstack.org/640879 | |
| 16:27:09 | mriedem | gibi: on https://review.openstack.org/#/c/639608/ i think that's already a potential bug because of not handling errors from claim_resources | |
| 16:27:27 | mriedem | so it's low priority for me, but i think it's a bug we can report and work on | |
| 16:27:57 | mriedem | as i said before that whole while loop needs to be refactored at some point (that whole damn build_instances method actually) | |
| 16:28:46 | mriedem | gibi: https://review.openstack.org/#/c/638711/ isn't really a bug per se, but it's a performance optimization, so again i don't know that we need to rush it, but it's still good to have | |
| 16:29:14 | mriedem | you could report a low priority bug for tracking that if you wanted - as a perf issue | |
| 16:29:28 | mriedem | like i said, if i'm multi-creating 100 instances in one request, that's 100 separate GET /allocations calls | |
| 16:29:49 | mriedem | although, can you multi-create with a pre-created port...? | |
| 16:29:55 | mriedem | i'm not sure that is supported | |
| 16:30:19 | mriedem | that would be like multi-create with the same pre-created volume | |
| 16:30:23 | mriedem | so that likely doesn't work anyway | |
| 16:31:35 | openstackgerrit | Eric Fried proposed openstack/nova master: update gate test for removal of force evacuate https://review.openstack.org/641986 | |