| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-04 | |||
| 16:17:50 | gibi | bauzas: I think you found a problem | |
| 16:18:18 | gibi | bauzas: we did not planned to check the compute version | |
| 16:19:08 | gibi | bauzas: right now if you boot a server and the scheduler select a host that is still Rocky then the pci claim fails as the new pf_interface_name key will be missing from the pci spec | |
| 16:19:25 | gibi | bauzas: so no resource inconsistency but more re-schedule will happen | |
| 16:20:41 | bauzas | gibi: wait | |
| 16:20:42 | gibi | bauzas: ohh no. the InstancePCIRequest will not request the new key either if the request lands on an old compute | |
| 16:20:58 | bauzas | gibi: yeah that | |
| 16:21:08 | bauzas | it's old code so it doesn't know about anything | |
| 16:21:31 | gibi | bauzas: yeah, the InstancePCIRequest change is also in the compute/manager | |
| 16:22:40 | gibi | BUT if we have an old compute then the old compute will not have bandwidth inventory | |
| 16:22:49 | gibi | so placement will not select it | |
| 16:23:07 | gibi | (assuming that nova-compute is updated along with the neutron agents) | |
| 16:23:10 | bauzas | gibi: that was the original reason why we didn't need a check | |
| 16:23:22 | bauzas | because placement checks it for free | |
| 16:23:23 | bauzas | but | |
| 16:23:49 | bauzas | for a classic move operation, you can end up with the late-check not being read | |
| 16:24:03 | mriedem | there is no move support for this in stein | |
| 16:24:30 | gibi | bauzas: ^^ | |
| 16:24:35 | mriedem | gibi: and we enforce that in the api right? | |
| 16:24:39 | mriedem | yeah i remember | |
| 16:24:47 | gibi | mriedem: yes, the moves are rejected | |
| 16:24:56 | gibi | mriedem: even after the 2.72 | |
| 16:24:56 | bauzas | mriedem: what happens with my instance ? am I able to migrate it with dropping the bandwidth check, or do i get some ERROR ? | |
| 16:25:07 | mriedem | you get a 400 or 409 | |
| 16:25:08 | bauzas | ah, that's an API enforcement | |
| 16:25:09 | mriedem | i can't remember which | |
| 16:25:10 | bauzas | good then | |
| 16:25:46 | mriedem | https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/evacuate.py#L123 400 | |
| 16:25:54 | bauzas | so that works, at least if neutron agent is upgraded with compute service, which is usually what's done | |
| 16:26:13 | mriedem | if the neutron agent is upgraded before nova-compute it could be reporting inventory before the compute is ready to handle those types of allocations... | |
| 16:26:18 | mriedem | but i'm not sure how realistic that is | |
| 16:26:41 | gibi | bauzas: even if they ugrade the neutron agent they can skip the bw config in the agent until nova-compute is upgraded | |
| 16:26:45 | bauzas | worth a note at least | |
| 16:26:54 | gibi | bauzas: the whole neutron bw inventory is config based | |
| 16:26:57 | bauzas | yeah | |
| 16:27:12 | mriedem | how much of the compute-level stuff is virt-driver specific? | |
| 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 | mriedem | aspiers: efried: btw who is on the hook for documenting ^ now that it's going to be codified? | |
| 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: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 | gibi | mriedem: I'm OK to add the capability in Train | |
| 16:36:32 | openstackgerrit | Merged openstack/nova stable/rocky: Fix race in test_volume_swap_server_with_error https://review.openstack.org/640595 | |
| 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 | |