| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-04 | |||
| 16:27:16 | bauzas | so, ops can upgrade whenever they want, and once they're done, they set configs | |
| 16:27:17 | mriedem | just the multi-VF on same PF thing? | |
| 16:27:37 | bauzas | gibi: planned to write some ops docs btw. ? | |
| 16:27:51 | bauzas | that would also help me reviewing all of this :) | |
| 16:27:55 | gibi | bauzas: yeah, rubasov already started it for neutron and I will chime in | |
| 16:28:19 | bauzas | I'd be more than happy to review it | |
| 16:28:23 | gibi | mriedem: good question, I think the SRIOV + bandwidth support is now become virt driver specify | |
| 16:28:41 | gibi | bauzas: here is a neutron WIP doc patch https://review.openstack.org/#/c/640390/ | |
| 16:28:45 | dansmith | mriedem: hmm, aren't the two things in runway slot 2 and 3 part of the same effort? | |
| 16:28:59 | mriedem | dansmith: yeah, spot instances | |
| 16:29:02 | dansmith | yeah | |
| 16:29:04 | mriedem | separate blueprints for whatever reason | |
| 16:29:10 | dansmith | hrm | |
| 16:29:37 | dansmith | they're part of the same stack in gerrit, so.. kinda seems like that's one thing:) | |
| 16:29:42 | gibi | mriedem: if there is a InstancePCIRequest then it will have the new tag included even if the compute host only has a single PF | |
| 16:29:48 | mriedem | gibi: but maybe a compute without a supporting virt driver wouldn't configure the neutron agent to report that inventory | |
| 16:29:52 | artom | mriedem, I wasn't checking the database instance_numa_topology | |
| 16:29:58 | artom | So the pins are updated and correct | |
| 16:30:14 | gibi | mriedem: SRIOV without bw is kept intact | |
| 16:30:15 | artom | But Windriver are telling me the DB isn't updated, despite apply_migration_context() being called | |
| 16:30:43 | gibi | mriedem: so if no resource request in the sriov port then we don't need the special key from the virt driver | |
| 16:30:57 | mriedem | artom: are you saving the instance changes after calling apply_migration_context()? | |
| 16:31:04 | mriedem | b/c ^ doesn't persist the changes, it just updates the object fields | |
| 16:31:15 | artom | mriedem, wait, seriously? | |
| 16:31:35 | artom | mriedem, huh, but instance.save() is still called after in post_live_migration | |
| 16:31:36 | mriedem | don't take my word for it | |
| 16:31:43 | bauzas | gibi: if that's virt-specific, you should document it in https://docs.openstack.org/nova/latest/user/feature-classification.html | |
| 16:31:53 | gibi | bauzas: thanks for the pointer | |
| 16:32:16 | artom | mriedem, ah, you're right, it's not saved | |
| 16:32:21 | mriedem | i'm more worried that we send a server create request to a compute that doesn't support it | |
| 16:32:33 | mriedem | and we don't have any kind of feature capability for this port requested resources stuff | |
| 16:32:44 | mriedem | gibi: bauzas: ^ | |
| 16:32:48 | mriedem | and we just drop the request on the floor | |
| 16:33:01 | dansmith | artom: so does that mean some testing gap exists because we never re-migrate the instance a second time or something? | |
| 16:33:09 | mriedem | thinking about https://review.openstack.org/#/c/538498/ coincidentally | |
| 16:33:11 | bauzas | mriedem: that's my -1 | |
| 16:33:18 | dansmith | artom: meaning.. how could we be failing to save that and think that this works? | |
| 16:33:22 | artom | But there's an instance.save 5 lines after https://review.openstack.org/#/c/634606/37/nova/compute/manager.py@6925 | |
| 16:33:23 | gibi | mriedem: yeah if neutron agent is configured to report bandwidth but the nova virt driver does not support it then it will be NoValidHost after a bunch of reschedule | |
| 16:33:35 | bauzas | mriedem: but the question is : should we leave it as docs, or enforce it ? | |
| 16:33:37 | artom | dansmith, I was just testing the XML is updated correctly | |
| 16:33:52 | dansmith | artom: oh :( | |
| 16:34:03 | artom | dansmith, yeah, testing coverage gap :( | |
| 16:34:52 | mriedem | gibi: bauzas: at this point it's probably best to just doc it and clean it up in train with a capability tag a la https://review.openstack.org/#/c/538498/ | |
| 16:35:16 | bauzas | I'm good with that | |
| 16:35:19 | gibi | bauzas, mriedem, sean-k-mooney, stephenfin: this is the cons of the auto detection of the pf_interface_name. It makes the feature virt driver specific. If we go with the a new tag in the passthrough_whitelist then the solution is virt driver agnostic | |
| 16:35:19 | mriedem | aspiers: efried: btw who is on the hook for documenting ^ now that it's going to be codified? | |
| 16:35:55 | mriedem | but then you have to configure more crap | |
| 16:36:17 | gibi | mriedem: exactly | |
| 16:36:29 | mriedem | this is pretty magical unicorn and i expect 99% of openstack users for nfv stuff are using libvirt anyway | |
| 16:36:32 | openstackgerrit | Merged openstack/nova stable/rocky: Fix race in test_volume_swap_server_with_error https://review.openstack.org/640595 | |
| 16:36:32 | gibi | mriedem: I'm OK to add the capability in Train | |
| 16:38:11 | mriedem | dansmith: regarding the spot instance stuff i have skimmed some of it in the past (over a month ago) noting some obvious concerns, but haven't been back on it since, nor has anyone else from the core team it looks like, so that one is too risky at this point for me, especially without any integration (tempest) testing | |
| 16:38:33 | bauzas | mriedem: gibi: I'm not opposed to have this feature be virt-driver specific for the moment and provide a new abstract trait later | |
| 16:38:49 | mdbooth | lyarwood: I was looking into your tempest failures but got distracted. Just getting back into it. Did you manage to look by any chance? | |
| 16:39:04 | dansmith | mriedem: yeah, same, I was just skimming over it, thinking that since it was two pieces, maybe half of it was simple stuff we could merge, but it seems all tied together | |
| 16:40:43 | mdbooth | Looking at one of them in particular, I note that the type is deleted after a volume of that type is deleted, but before cinder logs that the delete completed. However, it could be, for eg, that cinder has already deleted the volume from the db but doesn't log until some backend stuff is done? I'm still looking. Looks weird. | |
| 16:40:46 | dansmith | artom: so yeah, I dunno why that save() doesn't get you there, but the migration context stuff is complex, so you might just be tripping over some assumption | |
| 16:41:13 | dansmith | artom: honestly, I'd start trying to reproduce (or disprove) that with a functional test before rattling a fix | |
| 16:41:25 | artom | dansmith, yeah, first order is modifying my tests to catch that | |
| 16:41:41 | artom | (Or writing new func tests for the same purpose) | |
| 16:41:42 | artom | Then debugging | |
| 16:41:43 | dansmith | artom: ++ | |
| 16:41:59 | mriedem | btw i do a lot of ^ type stuff in the cross-cell resize functional testing to make sure the db is what i expect it to be | |
| 16:42:11 | mriedem | since i'm farting around with 2 dbs | |
| 16:42:17 | gibi | bauzas: cool, then your current -1 on the patch is just procedural while you reviewing the rest of the patch? | |
| 16:43:59 | artom | mriedem, I think the DB asserting part will be relatively easy, it's the running 2 services part that's finicky | |
| 16:44:43 | artom | dansmith, while I have you, can we context switch to your thoughts on https://review.openstack.org/#/c/639396/? | |
| 16:45:57 | dansmith | artom: ugh, I really don't want to... I know that is going to be ugly because of whatever has moved out of that scope, but I'm really (really) not in favor of changing how that works.. like REALLY. | |
| 16:46:04 | bauzas | gibi: I left a new comment as a soft -1 | |
| 16:46:18 | gibi | bauzas: looking | |
| 16:46:21 | bauzas | gibi: but I'm continuing to review so I can +1 once I'm done | |
| 16:46:26 | gibi | bauzas: sure | |
| 16:46:30 | bauzas | gibi: just askign docd | |
| 16:46:43 | gibi | bauzas: I will let you go through it before I start updating the patch | |
| 16:46:57 | artom | dansmith, yeah :/ | |
| 16:47:20 | artom | dansmith, as I said in the review, I don't see a way around *some* change, because we race on setting up the event waiter | |
| 16:47:21 | gibi | bauzas: would you accep the documentation in a separate patch? | |
| 16:47:32 | artom | If we don't get to the driver code by the time Neutron sends us the event, it's lost | |
| 16:48:11 | dansmith | artom: yeah, but neutron is sending the event in response to things we're doing, which just means whatever we do to trigger it needs to happen within the context | |
| 16:48:26 | bauzas | gibi: yup | |
| 16:48:32 | gibi | bauzas: coolio | |
| 16:48:36 | bauzas | I can turn into a +1 if you prefer | |
| 16:48:55 | gibi | bauzas: don't worry, I will respin anyhow | |
| 16:49:06 | artom | dansmith, yeah - but we're poking Neutron in the compute manager, and setting up the waiter in the virt driver... | |
| 16:49:12 | gibi | bauzas: just let me know if you spot any other issues | |
| 16:49:19 | dansmith | artom: I understand | |
| 16:49:21 | artom | From the virt driver, I should say. It's a call to the compute manager | |
| 16:49:46 | artom | Hrmm, there's no RPC, mind you | |
| 16:49:56 | artom | Just... a whole bunch of intermediary steps | |
| 16:50:07 | bauzas | gibi: I'm on it | |
| 16:50:13 | gibi | bauzas: thanks a lot | |
| 16:50:29 | bauzas | I wish I could help better, that's it | |
| 16:51:45 | artom | dansmith, anyways, I'll try to come up with a least bad way of doing it, we'll have a strawman to pick apart | |
| 16:52:03 | artom | At least you're on board with the need for change ;) | |
| 16:54:06 | mriedem | artom: so we likely need 2 context managers to wait for separate events right? | |
| 16:54:15 | mriedem | one for the port binding host change, and one for the vif plug | |
| 16:54:29 | mriedem | that port binding host change happens in several move operations we'd care about (at least resize and evacuate) | |
| 16:54:37 | mriedem | and unshelve i think... | |
| 16:54:58 | artom | mriedem, hrmm, you lost me, how would 2 context managers solve this? | |