| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-01-31 | |||
| 21:25:57 | mriedem | but some joker nacked it | |
| 21:26:06 | mriedem | https://review.openstack.org/#/c/625092/ | |
| 21:26:35 | smcginnis | Part of the Cinder fix, IIRC, was getting rid of all those warnings. | |
| 21:27:26 | mriedem | ok that's a good tip though, thanks | |
| 21:34:35 | mriedem | this is probably the worst warning that shows up | |
| 21:34:35 | mriedem | b"/home/zuul/src/git.openstack.org/openstack/nova/.tox/py35/lib/python3.5/site-packages/sqlalchemy/orm/evaluator.py:99: SAWarning: Evaluating non-mapped column expression 'id' onto ORM instances; this is a deprecated use case. Please make use of the actual mapped columns in ORM-evaluated UPDATE / DELETE expressions." | |
| 21:34:41 | mriedem | 866 hits in one py35 unit test run | |
| 21:36:47 | stephenfin | mriedem: I'm gone to FOSDEM tomorrow, but I should be able to take on https://blueprints.launchpad.net/nova/+spec/remove-force-flag-from-live-migrate-and-evacuate if alex_xu and gibi aren't available. Mention it to me on Monday? | |
| 21:37:16 | mriedem | sure | |
| 22:01:12 | openstackgerrit | Merged openstack/nova stable/rocky: Fix incompatible version handling in BuildRequest https://review.openstack.org/631731 | |
| 22:01:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add microversion to expose virtual device tags https://review.openstack.org/631948 | |
| 22:39:11 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix race in test_volume_swap_server_with_error https://review.openstack.org/634334 | |
| 22:39:12 | mriedem | easy gate bug fix ^ | |
| 22:58:21 | openstackgerrit | Jack Ding proposed openstack/nova master: Flavor extra spec and image properties validation https://review.openstack.org/620706 | |
| #openstack-nova - 2019-02-01 | |||
| 00:19:00 | openstackgerrit | Merged openstack/nova master: Make 'plugin' a required argument for '_get_vif_instance' https://review.openstack.org/608279 | |
| 02:17:05 | openstackgerrit | Merged openstack/nova stable/rocky: Fix using template cell urls with nova-manage https://review.openstack.org/633086 | |
| 02:24:50 | openstackgerrit | Zhenyu Zheng proposed openstack/nova master: Don't change volume status to `in-use` when attach to shelved vm. https://review.openstack.org/627096 | |
| 09:29:56 | openstackgerrit | Zhenyu Zheng proposed openstack/nova master: Per-instance serial number https://review.openstack.org/619953 | |
| 12:07:15 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP libvirt: Avoid premature calls to disconnect_volume during LM https://review.openstack.org/634398 | |
| 12:07:53 | lyarwood | mdbooth: ^ if you wouldn't mind taking a look at this when you're back, from a downstream triage bug. | |
| 12:15:30 | mdbooth | lyarwood: Looking | |
| 12:17:42 | mdbooth | lyarwood: Just reading the commit message makes me sad :( That's so racy. | |
| 12:17:52 | mdbooth | Not your patch, the one you reference. | |
| 12:18:48 | mdbooth | That reference counting can only happen on the host, while holding a lock. | |
| 12:21:55 | lyarwood | mdbooth: ack yeah we've spoken about lock many many times in this area | |
| 12:22:00 | lyarwood | locks* | |
| 12:22:21 | lyarwood | mdbooth: tbh this is just a backportable fix, I'd like to follow up and refactor this entire method after this. | |
| 12:22:21 | sean-k-mooney | os-bricks? | |
| 12:22:40 | lyarwood | sean-k-mooney: up a level in n-cpu before we call down. | |
| 12:23:27 | lyarwood | sean-k-mooney: there's reference/connection counting going on before we call disconnect_volume in os-brick that could race in this example. | |
| 12:23:35 | sean-k-mooney | i was just asking as when you mentiond talking about locks several times previously the os-brick interaction conde was what jumped to mind | |
| 12:23:56 | lyarwood | yeah that's part of it, I don't think anything has happened there tbh | |
| 12:24:41 | sean-k-mooney | there was talk of intoducing a context manager to hanel the locking if i remmebre. not sure if mdbooth ever had time to work on it | |
| 12:26:58 | mdbooth | sean-k-mooney: That particular instance requires a lock across connect and disconnect | |
| 12:27:46 | mdbooth | So, for eg, a concurrent connect can't interfere with attachments during the check in disconnect | |
| 12:27:58 | sean-k-mooney | mdbooth: is this related to the down stream issue from yesterday? | |
| 12:28:09 | mdbooth | No. It's Lee's bug. | |
| 12:28:22 | sean-k-mooney | oh ok | |
| 12:28:48 | lyarwood | yeah just a triage bug | |
| 12:30:08 | sean-k-mooney | lyarwood: well it sould like it a valid one if ye are dicussing adding new locking | |
| 12:30:38 | mdbooth | Yeah, pretty sure it's valid. | |
| 12:32:27 | sean-k-mooney | lyarwood: mdbooth im not sure if we have any gate jobs that test livemigration with multiatached volumes | |
| 12:33:05 | mdbooth | Probably not | |
| 12:33:28 | lyarwood | yeah I don't think there's any multihost multiattach testing at the moment | |
| 12:34:18 | sean-k-mooney | is it just lvm that supports it currently or perhaps the nfs dirver? ceph doesnt right now right | |
| 12:35:57 | lyarwood | sean-k-mooney: yeah LVM/iSCSI does, rbd doesn't etc. | |
| 12:38:37 | sean-k-mooney | right i think we have an existing lvm based multinode job we could add a test too. i might take a crack at that. its been a while since i looked at tempest and kind of want to brush up on it | |
| 12:46:19 | mdbooth | lyarwood: There's also resize/cold migrate to consider | |
| 12:57:15 | openstackgerrit | sean mooney proposed openstack/nova master: PCI: do not force remove allocated devices https://review.openstack.org/626381 | |
| 13:03:15 | mdbooth | lyarwood: Comment in review. That assumption makes me uncomfortable. However, I perhaps haven't had enough coffee to see where it's incorrect yet. | |
| 13:03:31 | mdbooth | Throwing it to you to see if you can come up with anything yourself :) | |
| 13:08:21 | mdbooth | lyarwood: No, this is way way more complicated than this | |
| 13:08:45 | mdbooth | Because we're also potentially racing with other live migrations, resizes or cold migrations | |
| 13:09:02 | mdbooth | We're assuming that only *this* attachment might be in a special state | |
| 13:09:21 | mdbooth | But what if, for eg, we're in the middle of host-evacuate-live, and *everything* is currently live migrating | |
| 13:09:35 | mdbooth | This whole approach is just wrong | |
| 13:10:47 | mdbooth | My head is also too fuzzy to consider the case of cleaning up evacuated instances in init_host | |
| 13:21:15 | mdbooth | lyarwood: Sorry, I don't think we can spit-shine that turd. | |
| 13:25:36 | mdbooth | lyarwood: The design of the nfs volume driver handles both shared targets and multi-attach. I'm pretty sure we could mostly copy it. | |
| 13:25:54 | mdbooth | We also backport that, btw. | |
| 13:26:00 | mdbooth | backported | |
| 13:34:09 | lyarwood | mdbooth: back, reading | |
| 13:34:52 | mdbooth | lyarwood: TL;DR Your edge case has edge cases | |
| 13:35:18 | mdbooth | It just makes things edgier | |
| 13:37:42 | lyarwood | and now this is stuck in my head https://www.youtube.com/watch?v=H8dZwXnMrRU | |
| 13:38:43 | mdbooth | lyarwood: LOL | |
| 13:38:59 | lyarwood | mdbooth: right there's a bigger issue with this anyway that came to me during lunch, instance.host isn't updated until later in the post_live_migration flow. | |
| 13:39:28 | lyarwood | https://docs.openstack.org/nova/latest/reference/live-migration.html - post_live_migration_at_destination actually updates it | |
| 13:39:53 | mdbooth | lyarwood: And that will differ for cold migration, of course. | |
| 13:41:28 | mdbooth | Bill Bailey is awesome | |
| 13:43:12 | mdbooth | lyarwood: So you're suggesting we just overlay the edge cases with a massive effects library which makes them seem awesome? | |
| 13:43:45 | mdbooth | A truly generic solution. | |
| 13:45:04 | lyarwood | os-twiddly-dee | |
| 13:49:36 | mdbooth | lyarwood: Incidentally, I had to watch that twice just to convince myself he was playing Jingle Bells from the start. | |
| 13:50:52 | openstackgerrit | Eric Fried proposed openstack/os-resource-classes master: Add normalize_name utility https://review.openstack.org/634258 | |
| 14:22:34 | lyarwood | mdbooth: okay, found the actual issue causing _should_disconnect_target to return True, during post_live_migration we are fetching the old connection_info from c-api again - https://github.com/openstack/nova/blob/5a4863aa152f58f6de426b3908a75c2cc1b2efca/nova/virt/libvirt/driver.py#L7970-L7976 and that doesn't contain the multiattach key, that's only added by n-cpu during the attach flows | |
| 14:22:36 | lyarwood | https://github.com/openstack/nova/blob/5a4863aa152f58f6de426b3908a75c2cc1b2efca/nova/virt/block_device.py#L538-L549 | |
| 14:23:44 | lyarwood | I'll just grab the volume earlier in _should_disconnect_target and use it to tell if it's multiattach enabled or not. | |
| 14:23:56 | mdbooth | lyarwood: Ah, that one. Didn't I fix that already? | |
| 14:24:37 | mdbooth | lyarwood: Sec... lemme see if I can find those patches | |
| 14:24:40 | lyarwood | mdbooth: not that I can see in this flow | |
| 14:25:20 | mdbooth | lyarwood: Is it related to this patch: https://review.openstack.org/#/c/551302/ ? | |
| 14:27:15 | lyarwood | mdbooth: yeah | |
| 14:28:14 | lyarwood | mdbooth: https://review.openstack.org/#/c/551349/ was still WIP I think | |
| 14:28:36 | mdbooth | Right, but I think the two are somewhat independent | |
| 14:28:51 | mdbooth | Would the first one, appropriately rebased, fix your issue? | |
| 14:29:43 | lyarwood | mdbooth: yes AFAICT | |
| 14:29:54 | mdbooth | lyarwood: Would you like to take it? | |
| 14:31:29 | lyarwood | mdbooth: sure, I'll rebase and push this along. | |
| 14:32:05 | mdbooth | lyarwood: Thanks! | |
| 14:33:54 | mriedem | bauzas: can you hit this as stable core? v | |
| 14:33:54 | mriedem | https://review.openstack.org/#/c/632720/ | |
| 14:34:07 | bauzas | mriedem: of course yeah | |
| 14:42:00 | openstackgerrit | Eric Fried proposed openstack/os-resource-classes master: Add normalize_name utility https://review.openstack.org/634258 | |
| 14:43:08 | rha | Hello. Anybody here, who's willing to do a review on: https://review.openstack.org/#/c/420026/ ? It should be rather straightforward I hope, | |
| 14:50:14 | dansmith | mriedem: I hadn't saved my comments on that serial number patch, so I just did and see that a new rev was postd | |
| 14:50:44 | dansmith | mriedem: I -1d the new one because I don't think we really should or need to deprecate that thing, but I'm not going to die on that hill | |
| 14:52:05 | sean-k-mooney | dansmith: Zhenyu Zheng send an email this morning saying he would push a new version following the discusssion on the ml thread | |
| 14:52:24 | dansmith | sean-k-mooney: you mean the new rev I mention above/ | |
| 14:52:48 | sean-k-mooney | dansmith: i havent looked but i assume so | |