| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-30 | |||
| 14:09:27 | efried | So if all of that were in place, having a single function that checks sev *and anything else relevant* and either sets iommu on or raises an exception: sounds like the right plan. | |
| 14:13:30 | mriedem | artom: so https://review.opendev.org/#/c/678887/18 we're just waiting for another run after your rebase right? | |
| 14:21:49 | aspiers | efried: thanks, I think that's enough justification for me to start switching the driver iommu flags so they all get enabled from a single place | |
| 14:22:01 | aspiers | efried: right now the conflict is only a theoretical possibility | |
| 14:22:47 | aspiers | efried: so we may never need to worry about it, but yeah it makes sense what you say about there being enough places for us to proactively catch and handle any such conflicts if they did arise in future | |
| 14:25:23 | aspiers | efried: the only risk I can't see us easily avoiding is someone adding code which explicitly disables iommu but forgets to prevent the SEV code from overriding that. Moving the SEV iommu code away from each individual chunk responsible for building driver configuration means that it's "hidden" out of sight from that code, and therefore easily missed | |
| 14:25:44 | aspiers | efried: but that seems like a small risk, and in return we probably get much cleaner code enabling iommu for SEV | |
| 14:25:54 | aspiers | alex_xu: ^^^ | |
| 14:29:27 | mriedem | efried: dustinc: couple of questions in the ironic get_node sdk change https://review.opendev.org/#/c/642899/44/nova/virt/ironic/driver.py | |
| 14:33:29 | dansmith | artom: I think the version checking in here is wrong and that you'll fail spectacularly if you have an rpc version pin and have to roll back a migration: https://review.opendev.org/#/c/634606 | |
| 14:37:10 | efried | mriedem: responded | |
| 14:44:02 | mriedem | sean-k-mooney: this is all host agnostic right? https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L212-L228 | |
| 14:44:26 | mriedem | sean-k-mooney: meaning, if an instance has non-sriov pci requests, we're going to hit that same code for each alternate host during live migration and just fail on all of them since that validation doesn't involve the host | |
| 14:44:38 | mriedem | seems that should be pulled up higher in the conductor task before we even ask the scheduler for dest hosts | |
| 14:44:50 | mriedem | like https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L85 | |
| 14:45:20 | sean-k-mooney | we check for non neutron pci device requets in the conductor already | |
| 14:46:13 | sean-k-mooney | mriedem: here https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L216-L221 | |
| 14:46:48 | sean-k-mooney | mriedem: dan wanted us to check again in the compute node to be extra safe | |
| 14:47:23 | sean-k-mooney | mriedem: so if you have pci deivce requested by the flavor alias we never get to artoms code | |
| 14:48:58 | mriedem | sean-k-mooney: i think you're missing my point | |
| 14:49:29 | mriedem | let's say i have conductor configured to retry up to 100 hosts for live mgiration and the scheduler gives me back 100 possible dests, | |
| 14:49:43 | mriedem | if the instance has flavor-defined pci requests, i'm going to hit that same block of validation code 100 times | |
| 14:49:46 | mriedem | with the exact same results | |
| 14:50:28 | mriedem | also, did we check with any of the non-libvirt driver maintainers that flavor-defined pci requests aren't supported in their driver for live migration? | |
| 14:50:45 | mriedem | i'm assuming they don't, hyper-v would be the only one remotely close probably, but wondering if anyone asked or just assumed libvirt is the only driver in nova | |
| 14:51:25 | stephenfin | mriedem: No, we don't | |
| 14:51:35 | stephenfin | We assume libvirt is the only driver for a lot of these things | |
| 14:52:03 | stephenfin | We've also done a bad job of documenting the features and config options that are libvirt only, fwiw | |
| 14:53:23 | mriedem | https://bugs.launchpad.net/nova/+bug/1842087 | |
| 14:53:24 | openstack | Launchpad bug 1842087 in OpenStack Compute (nova) "_check_can_migrate_pci in the LiveMigrationTask has host agnostic validation that is redundant/expensive" [Low,Triaged] | |
| 14:54:30 | stephenfin | mriedem: fwiw, I've suggested aspiers adds similar checks for the SEV feature (based on his docs, you can't live migrate those instances) | |
| 14:57:26 | mriedem | efried: ok so https://review.opendev.org/#/c/642899/44 - looks like quite a few unrelated things in there, but i'm just grumbling i think b/c it seems to be that kind of morning | |
| 14:57:38 | mriedem | efried: can you sniff my latest comments and if i'm not crazy then i'll +W | |
| 14:58:57 | shilpasd | mriedem: Hi, i am in the process of understanding fixture client created at novaclient https://opendev.org/openstack/python-novaclient/src/branch/master/novaclient/tests/unit/fixture_data/client.py#L21 | |
| 14:59:46 | shilpasd | can you help me how this COMPUTE_URL is derived (endpoint >> http:// |
|
| 15:02:15 | mriedem | shilpasd: i'm not sure what you're asking, but i guess i'd follow this code: https://opendev.org/openstack/python-novaclient/src/branch/master/novaclient/tests/unit/fixture_data/client.py#L39-L40 | |
| 15:03:12 | artom | sean-k-mooney, wat. OK, I removed some ansible lines because the devstack patch was doing it, looks like I'll have to re-add them | |
| 15:03:42 | artom | mriedem, what I said to sean-k-mooney ^^ | |
| 15:03:58 | sean-k-mooney | mriedem: i did think we check but i cant recal specificlly | |
| 15:04:04 | efried | mriedem: Yeah, it's not terribly surprising that | |
| 15:04:05 | shilpasd | mriedem:COMPUTE_URL is used for creating endpoint in fixture, and if you check endpoint for compute is 'http:// |
|
| 15:04:08 | sean-k-mooney | e.g. with other dirvers | |
| 15:04:31 | efried | mriedem: ...that a six-part series with two authors and four dozen patch sets got a bit of intra-series bleed :( | |
| 15:04:57 | efried | mriedem: If you want we can get dustinc to ack those comments. | |
| 15:06:16 | shilpasd | mriedem: so my question, how what's the relation COMPUTE_URL as endpoint here in Fixture | |
| 15:07:28 | artom | dansmith, ah, I think I see what you mean. I do a 'if src_supports_numa_live_migration' check here: https://review.opendev.org/#/c/634606/62/nova/compute/manager.py@6511 | |
| 15:08:58 | dansmith | artom: ...but... | |
| 15:09:08 | artom | dansmith, so I should move that if into post_claim_migrate_data, right? | |
| 15:09:26 | artom | To make sure the new field got through RPC version pinning to the dest | |
| 15:09:43 | dansmith | the field will make it through regardless of the rpc pin | |
| 15:09:45 | dansmith | they're unrelated | |
| 15:09:46 | dansmith | entirely | |
| 15:09:53 | dansmith | that's my point | |
| 15:10:03 | artom | Oh. | |
| 15:10:06 | dansmith | you can't use the presence of a field in an object to know what the version pin is | |
| 15:10:32 | artom | Wait, so how does the conductor know when to backlevel an object? | |
| 15:11:33 | dansmith | artom: conductor backlevels an object when asked. when an object arrives at a compute node and that compute node sees it's too new, and asks conductor to backlevel it to a specific version (i.e. its latest for that object) | |
| 15:12:27 | artom | dansmith, aha, so I had it completely wrong. OK, need to rethink and update then. | |
| 15:12:29 | dansmith | but compute->compute there is no conductor involvement for the actual rpc call, and the version pin is not related to object versions | |
| 15:12:35 | dansmith | artom: yep | |
| 15:16:31 | mriedem | artom: left my comments in the claims patch as well https://review.opendev.org/#/c/635669/47 | |
| 15:16:39 | artom | mriedem, ack, much thanks. | |
| 15:17:49 | artom | And we want to see https://review.opendev.org/#/c/678887/ passing before we start the +W Train (zing!) going, right? | |
| 15:18:10 | mriedem | speaking of that, | |
| 15:18:11 | artom | (OK, train may have been optimistic - snail?) | |
| 15:18:15 | mriedem | why did you pull it away from the nova series? | |
| 15:18:24 | mriedem | it's a nova change that depends on a devstack change that depends on a nova change...but why? | |
| 15:18:38 | artom | It became a PITA to work on both at the same time in the same stack? | |
| 15:18:49 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 15:18:58 | artom | Specifically, it needed a rebase, but no the stack below it | |
| 15:19:01 | artom | *not | |
| 15:19:06 | mriedem | oh yeah ok | |
| 15:19:13 | dansmith | artom: I would really like to see that working, unless there's some fundamental reason it can't | |
| 15:19:22 | mriedem | as for the +W train yeah ^ what dan said | |
| 15:19:22 | dansmith | artom: it looks like it's still not even able to start an instance though right? | |
| 15:19:49 | artom | sean-k-mooney would know more about the fundamental reasons, so if thinks it can work, let's keep trying. | |
| 15:20:19 | artom | I removed some lines I thought were no longer needed (beause of the devstack nuke) when I rebased, looks like I'll have to put them back int | |
| 15:20:40 | artom | And put it on top again, why not, I'm respinning the series anyways | |
| 15:21:56 | mriedem | you don't have to put it back on top, | |
| 15:22:17 | mriedem | but can if you want | |
| 15:22:32 | sean-k-mooney | artom: the temp job? we jsut don have the right setting in the config. | |
| 15:23:12 | sean-k-mooney | dansmith: mriedem do ye just want to see a gate job using numa migrtaion | |
| 15:23:18 | sean-k-mooney | or does it have to be that gate job | |
| 15:23:58 | artom | *snerk* let's try and hack something in grenade! | |
| 15:24:01 | sean-k-mooney | because at this point if adding the options artom remvoed does not work it would be simpler form me to just make my nfv job work | |
| 15:24:01 | dansmith | well, especially given the upgrade issues, it'd also be nice to run the grenade LM job against this, even though it probably wouldn't have covered the situation I found earlier | |
| 15:24:23 | dansmith | sean-k-mooney: if it runs some LMs then that's better than noting | |
| 15:24:43 | dansmith | I'm also going to be tempted to hack the job to pin the rpc version at 5.1 to check his work | |
| 15:24:52 | sean-k-mooney | dansmith: well i have my https://review.opendev.org/#/c/652197/ nfv job | |
| 15:24:55 | dansmith | because breaking LM across versions is icky | |
| 15:24:59 | sean-k-mooney | that i was going to rework | |
| 15:25:16 | sean-k-mooney | so i was going to stop using fedora and jsut use ubunut with nested vert | |
| 15:25:29 | sean-k-mooney | the multinode version runs live migratio tests | |
| 15:26:00 | dansmith | the closer it is to a standard job, the easier it will be to verify, but anything is better than nothing | |
| 15:26:10 | sean-k-mooney | sure | |
| 15:26:21 | sean-k-mooney | whcih is why im swapping ot ubuntu | |
| 15:26:39 | sean-k-mooney | but we also need to move the nova-migration job to non legacy at some point | |
| 15:27:17 | sean-k-mooney | anyway i was planning to finish up early today given how late i have been working lately | |
| 15:28:02 | dansmith | understandable, and holiday weekend here for lots of people | |
| 15:28:47 | sean-k-mooney | dansmith: donnyd provide a mulit numa flavor we can use on FN and we can also use vexhost. so ill create a job on monday | |