| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-30 | |||
| 13:40:50 | sean-k-mooney | how did i miss a full sentence... | |
| 13:40:53 | efried | I'll just abstain. | |
| 13:40:57 | stephenfin | \o/ | |
| 13:41:08 | stephenfin | that's a sane default | |
| 13:41:19 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/rocky: Add useful error log when _determine_version_cap raises DBNotAllowed https://review.opendev.org/679449 | |
| 13:41:23 | sean-k-mooney | litally ment to type "all i saw was" before "can you do..." | |
| 13:41:38 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/rocky: Add useful error log when _determine_version_cap raises DBNotAllowed https://review.opendev.org/679449 | |
| 13:41:51 | sean-k-mooney | stephenfin: proably because white space in lanagues like python matters | |
| 13:42:00 | sean-k-mooney | so changing indention could change behavior | |
| 13:42:09 | sean-k-mooney | or no one added it | |
| 13:42:11 | stephenfin | sean-k-mooney: Right, but make it toggleable | |
| 13:42:28 | stephenfin | Maybe 'git blame -W' to override whatever and re-enable it | |
| 13:42:35 | stephenfin | Hmm, that would be a decent patch | |
| 13:42:40 | stephenfin | Possible weekend project there | |
| 13:42:56 | sean-k-mooney | stephenfin: as long as you dont change the default behavior | |
| 13:43:05 | stephenfin | oh yeah, of cours | |
| 13:43:06 | stephenfin | *course | |
| 13:43:16 | sean-k-mooney | efried: speaking of which | |
| 13:43:44 | sean-k-mooney | efried: care to take a look at |https://review.opendev.org/#/q/status:open+project:openstack/nova+branch:master+topic:bp/image-metadata-prefiltering | |
| 13:44:07 | sean-k-mooney | there are more more important reviews but im hoping those merge soon | |
| 13:44:21 | efried | sean-k-mooney: been on the list, but got some stuff in front of it. | |
| 13:44:32 | sean-k-mooney | cool | |
| 13:45:38 | sean-k-mooney | stephenfin: by they way i tested and fixed https://review.opendev.org/#/c/675776/ | |
| 13:45:50 | stephenfin | the revert? | |
| 13:45:52 | sean-k-mooney | stephenfin: we do need that fix to avoid out issue with libvirt | |
| 13:45:54 | sean-k-mooney | yes | |
| 13:45:54 | stephenfin | yeah | |
| 13:47:09 | sean-k-mooney | i guess we are still waiting for the melonox ci to run on it too | |
| 13:49:10 | mdbooth | mriedem dansmith: I don't think I'm going to get better confirmation of the fix in https://review.opendev.org/#/c/677736/ . It *has* run on a real system, though, and didn't break it. | |
| 13:49:50 | sean-k-mooney | artom: https://review.opendev.org/#/c/678887/ failed because its using qemu again | |
| 13:50:45 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 13:51:27 | aspiers | alex_xu, stephenfin, efried: need your take on this debate https://review.opendev.org/#/c/636318/49/nova/virt/libvirt/config.py@2701 | |
| 13:52:12 | mriedem | mdbooth: dan is out today, | |
| 13:52:16 | mriedem | but are you going to address https://review.opendev.org/#/c/677736/3/nova/virt/libvirt/host.py@a236 ? | |
| 13:52:22 | mriedem | otherwise i'll +2 it | |
| 13:53:09 | aspiers | stephenfin, efried: if the question isn't easy to parse from those comments, I can help explain here | |
| 13:53:33 | efried | aspiers: Give me 10 mins | |
| 13:53:38 | aspiers | efried: np | |
| 13:53:49 | sean-k-mooney | mdbooth: you know the py37 hang i was seeing before when we tryied to boot vms could also be fixed by https://review.opendev.org/#/c/677736/3 | |
| 13:54:41 | sean-k-mooney | i had assumed when i saw it it was due to eventlest and python37 not playing nicely but that looks like it is more likely the actual cause | |
| 13:57:25 | mdbooth | sean-k-mooney: Why would the behaviour be different on py37, though? | |
| 13:58:36 | mdbooth | Bug call time | |
| 14:02:10 | sean-k-mooney | well i was febuary, eventlet did not offically test/support py37 and we have see issue with it on python3 in the pasted that looked like it | |
| 14:02:44 | sean-k-mooney | but honestly it was just a guess and it happend on python 3 but python 2.7 | |
| 14:09:25 | efried | aspiers: I don't have the bandwidth to dig into this all the way right now, so the following is a bit from the hip: | |
| 14:09:25 | efried | If it's possible for Thing A (via extra spec, image meta, config, etc) to ask for a switch to be flipped on, and Thing B (likewise) for it to be off, that's a conflict, and should be an error. | |
| 14:09:25 | efried | We have a place where we validate flavor+image compatibility for things like this: nova.compute.api.API._validate_flavor_image | |
| 14:09:25 | efried | As of yet, I think all of these ^ checks are just checking whether Thing A is on in flavor and off in image meta, kind of thing. | |
| 14:09:25 | efried | So this might be the first time we are cross-checking Thing A and Thing B for compatibility. | |
| 14:09:26 | efried | But that's fine - I believe that was one of the long-term goals of that infrastructure. stephenfin might be able to say more there. | |
| 14:09:26 | efried | Now, that's at the API layer. Once you get down to the compute, you can start vetting against things like config or whatever. Hopefully any conflicts there would have already been filtered out based on placement; but if not, late fail the spawn. | |
| 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 | |