| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-03 | |||
| 19:24:48 | artom | Hence this discussion on how do we achieve that without exposing rpcapi internals, or having a dummy method for the source to attempt to call | |
| 19:25:32 | sean-k-mooney | mriedem: ok will do. but im concerned that if the mechanium we use for sriov livemigration and multipel port binidngs is not enough for numa migriton all 3 feature would be incorect | |
| 19:26:30 | mriedem | i have no idea how well sriov live migration handles rolling upgrades | |
| 19:26:49 | sean-k-mooney | it handesl exactly as well as multiple portbindings | |
| 19:26:55 | sean-k-mooney | as we did exeactly the same thing | |
| 19:27:22 | mriedem | checking the source and dest compute service versions yeah | |
| 19:27:51 | sean-k-mooney | we have service version checkes here https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L35-L58 and we call them here https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L317-L344 | |
| 19:28:03 | sean-k-mooney | mriedem: yes | |
| 19:28:10 | artom | I think SRIOV was different because it would outright forbid it everything wasn't updated | |
| 19:28:23 | artom | NUMA LM has to work across N/N-1 boundaries | |
| 19:28:25 | sean-k-mooney | so did the multiple port bindign yes | |
| 19:28:33 | dansmith | well, that doesn't necessarily mean it's good, | |
| 19:28:45 | dansmith | but if it didn't have the same amount of rpc involvement as this it would be different | |
| 19:29:13 | artom | Dunno about RPC, but actually yeah, they could all have the new service version, but still be pinned when sending | |
| 19:29:14 | sean-k-mooney | the numa migration should have the same rpc workflow | |
| 19:30:17 | mriedem | sriov live migration rolls back the claim on dest here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L7496 | |
| 19:30:21 | dansmith | it's not the workflow | |
| 19:30:22 | mriedem | which the source will call | |
| 19:30:28 | mriedem | so it's not really the same thing artom is dealing with, | |
| 19:30:32 | mriedem | since artom is calling a new method | |
| 19:30:39 | dansmith | probably better to just read the scrollback | |
| 19:30:41 | sean-k-mooney | mriedem: ah good point | |
| 19:30:56 | sean-k-mooney | we dont intoduce new calls jsut now object for sriov | |
| 19:31:07 | sean-k-mooney | and multiple port bindings | |
| 19:33:49 | mriedem | brinzhang: when it comes to your microversion API changes, you should probably focus on https://review.opendev.org/#/c/673133/ since i think it's closer and less complicated | |
| 19:38:05 | sean-k-mooney | i have read half the scrollback but dumb question. since the extra complexity comes form trying to support cases other that both host are upgraded to the new version. could we just prevent that. we could add a workaoruds config options to allow people to opt in and state if you set this you need to ensure the correct rpc versions are set. | |
| 19:38:30 | sean-k-mooney | it wont just magically work but it avoid this issue entirely. | |
| 19:40:15 | artom | sean-k-mooney, no, because in a rolling upgrade we want to allow old computes to be "live-evacuated" to new ones | |
| 19:40:34 | sean-k-mooney | artom: right im saying why dont we not support that | |
| 19:40:53 | sean-k-mooney | but different question | |
| 19:41:02 | artom | sean-k-mooney, because it's a pretty crucial thing, IMHO :) | |
| 19:41:05 | sean-k-mooney | if we dont call the cleanup function on rollback | |
| 19:41:07 | artom | Anyways, the complexity comes from thinking about it | |
| 19:41:22 | artom | Which you need to do regardless | |
| 19:41:29 | sean-k-mooney | will the RT on the dest clean it up when the migration is rolled back | |
| 19:41:31 | artom | The code itself is clean-ish | |
| 19:41:37 | artom | sean-k-mooney, yeah, next periodic run | |
| 19:42:00 | sean-k-mooney | artom: ok so if we did not call the cleanup funtion it would get cleaned up eventually | |
| 19:42:06 | artom | Eventually | |
| 19:42:19 | mriedem | can i get another core to approve this simple refactor https://review.opendev.org/#/c/679651/ | |
| 19:49:16 | mriedem | dansmith: btw on https://review.opendev.org/#/c/633293/46 i gave a paste for what it would look like to use the existing prep_resize | |
| 19:50:27 | mriedem | at this point we can defer caring about that series until U i guess | |
| 19:55:54 | artom | dansmith, btw, I have a func test for RPC pinning, and it did find 1 bug. Wonder why it didn't pick up drop_move_claim() raising an RPC version error | |
| 19:56:31 | dansmith | artom: I dunno, probably worth looking though | |
| 19:56:39 | artom | Yep. | |
| 19:56:57 | dansmith | mriedem: okay, sorry I didn't really mean to block everything on that, I just wanted to ponder a bit but I never got anywhere | |
| 19:57:32 | openstackgerrit | Dustin Cowles proposed openstack/nova master: Provider Config File: Update provider tree with new custom resources https://review.opendev.org/676522 | |
| 20:03:31 | sean-k-mooney | ok i think im following what ye are suggesting | |
| 20:03:51 | sean-k-mooney | i also like mriedem function name but pep8 would hate it | |
| 20:21:02 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: NUMA live migration support https://review.opendev.org/634606 | |
| 20:21:03 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Functional tests for NUMA live migration https://review.opendev.org/672595 | |
| 20:21:03 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Deprecate CONF.workarounds.enable_numa_live_migration https://review.opendev.org/640021 | |
| 20:21:38 | artom | dansmith, mriedem, ^^ and at this point I need to done for a bit, because it's dad taxi and supper time | |
| 20:22:24 | artom | But if there's anything I'll try to address it later tonight | |
| 20:26:09 | sean-k-mooney | aspiers: som of the sev functional test are asserting behavior im not sure is correct or rather is failing when in integrate my image-metadata-prefilter series | |
| 20:26:44 | sean-k-mooney | aspiers: you seam to be asserting the ablit for the sev support to be removed while the agent is running. | |
| 20:27:19 | sean-k-mooney | the commen stats that thsi coudl happen as a result of a reconfiguration or kernel downgrade which woul nomally inlvoe the agent being resarted | |
| 20:28:45 | sean-k-mooney | aspiers: it looks like you are adding the sev tratis to the existing cpu tratis function which i am now cacheing as a property | |
| 20:28:58 | sean-k-mooney | https://review.opendev.org/#/c/666915/15/nova/virt/libvirt/driver.py | |
| 20:29:34 | sean-k-mooney | i can fix your functional test by reseting the property when you simulate teh restart of the compute service | |
| 20:29:52 | sean-k-mooney | but i wan to confim that you dont intend this to change outside of an agent restart | |
| 20:36:37 | sean-k-mooney | aspiers: im going to proceed with the assumetion i have interpred the test coorectly based on the comments and you are simulating an agent restart and just reset the chached traits. if you get a chance to review and confim that woudl be good. | |
| 21:12:49 | efried | nova FYI https://review.opendev.org/#/c/679862/ | |
| 21:14:20 | dansmith | +1 | |
| 21:24:19 | openstackgerrit | sean mooney proposed openstack/nova master: libvirt: use domain capabilities to get supported device models https://review.opendev.org/666915 | |
| 21:24:20 | openstackgerrit | sean mooney proposed openstack/nova master: Add transform_image_metadata request filter https://review.opendev.org/665775 | |
| 21:27:34 | mriedem | har har jk lol | |
| 21:30:51 | dansmith | heh | |
| 21:37:00 | aspiers | sean-k-mooney: the comments which explicitly say it's simulating agent restart are correct | |
| 21:37:15 | aspiers | https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_report_cpu_traits.py#L105 | |
| 21:37:22 | aspiers | https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_report_cpu_traits.py#L163 | |
| 21:38:14 | aspiers | efried: you around? need some quick advice on https://review.opendev.org/#/c/644565 | |
| 21:40:15 | efried | aspiers: ... | |
| 21:40:22 | aspiers | I think https://review.opendev.org/#/c/644565/49/nova/virt/libvirt/driver.py@5089 needs to be added to utils.py instead | |
| 21:40:46 | aspiers | since it turns out that blockinfo.py also needs to call it | |
| 21:41:14 | aspiers | or more precisely, blockinfo.py needs to know the same machine type, and in both cases SEV needs to be taken into account | |
| 21:41:37 | efried | aspiers: You want my advice for this why? | |
| 21:42:08 | aspiers | Well, just a sanity check that sounds reasonable, because that would mean passing more info into blockinfo.py | |
| 21:42:19 | aspiers | Adding SEV checks to the machine type calculation requires extra info | |
| 21:42:28 | aspiers | e.g. the Host object to check whether SEV is supported | |
| 21:42:41 | aspiers | or the sev_enabled variable if already calculated by the caller | |
| 21:43:07 | efried | At a glance, I agree it looks weird for a method with this generic a name to need anything sev-specific. | |
| 21:43:38 | efried | but I've repeatedly punted on this machine type stuff for a reason. | |
| 21:43:56 | aspiers | The key case here is when there is no machine type specified in the image | |
| 21:44:08 | aspiers | Maybe I can hassle kashyap about it | |
| 21:44:26 | efried | you need to default the machine type differently depending on whether SEV is enabled?? | |
| 21:44:30 | aspiers | Yes | |
| 21:44:38 | efried | that... doesn't sound right | |
| 21:44:40 | aspiers | Normally 'pc' for x86_64, but 'q35' if SEV is enabled | |
| 21:44:53 | aspiers | That's in the spec | |
| 21:45:21 | aspiers | Ahh, actually the spec is out of date on this point | |
| 21:45:47 | efried | SEV requires machine type Y, so if you want to SEV, you have to specify machine type Y as well as {other stuff to get SEV} | |
| 21:45:47 | efried | X doesn't work for SEV, so if you do that, SEV will be disabled. | |
| 21:45:47 | efried | If the machine type is not specified, we default it to X. | |
| 21:45:47 | efried | What sounds more likely to me is: | |
| 21:45:48 | aspiers | No, the spec is technically correct | |
| 21:46:34 | aspiers | AFAICS your proposal would have a big (negative) impact on usability of SEV | |
| 21:46:37 | efried | put another way, the "is SEV enabled" check gets an additional condition for machine type. And the above is how that's reflected in the documentation. | |
| 21:46:55 | aspiers | It would make it impossible to turn SEV on or off just by choosing a different flavor | |
| 21:47:09 | aspiers | You'd have to also choose an SEV-specific image to go alongside it | |