| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-01-31 | |||
| 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 | |
| 14:55:46 | sean-k-mooney | this is the mail i was refering too http://lists.openstack.org/pipermail/openstack-discuss/2019-February/002285.html what would be your preference? keep the config option undeprecated | |
| 14:56:03 | dansmith | sean-k-mooney: that's what I just said, yes :) | |
| 14:56:40 | sean-k-mooney | well what i should of asked is that long term or just for stien | |
| 14:57:07 | dansmith | I don't think there's any reason to deprecate it at all | |
| 14:57:22 | dansmith | there's a potential use for it, it's not blocking other things, nor is it a large maintenance burden, etc | |
| 14:58:42 | sean-k-mooney | it is exposing host info to the guest in a non obficated way. | |
| 14:59:26 | dansmith | it is, which is a good reason to default it to unique, but it's not useful data.. meaning the only thing you could really use it for is to determine if two instances are on the same host | |
| 14:59:31 | dansmith | which you can already do | |
| 14:59:36 | sean-k-mooney | i dont really care i guess one way or another if there is a config but i do think it would be nice to have uniqe id be the default | |
| 14:59:43 | dansmith | agree | |
| 15:07:49 | mriedem | the point of the flavor extra spec and image property were for users that really needed the unique serial to be able to get that regardless of the underlying config, which most operators likely don't even know this exists. in a public cloud, the flavor/image are probably controlled by the operator anyway, so having said that, if you don't have the spec/image property, you could still control this with aggregates if needed, i. | |