| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-10 | |||
| 15:20:29 | mriedem | L575 https://etherpad.openstack.org/p/nova-ptg-rocky | |
| 15:21:46 | lyarwood | mriedem: yeah, only just got around to starting yesterday when I hit https://bugs.launchpad.net/cinder/+bug/1762687 | |
| 15:21:46 | openstack | Launchpad bug 1762687 in OpenStack Compute (nova) "Concurrent requests to attach the same non-multiattach volume to multiple instances can succeed" [Undecided,New] - Assigned to Lee Yarwood (lyarwood) | |
| 15:21:51 | mriedem | or was it a unique constraint on device name? now i'm confused | |
| 15:22:21 | lyarwood | mriedem: https://blueprints.launchpad.net/nova/+spec/remove-and-block-duplicate-bdms created that a while ago, wanted to catch up with melwitt or you about getting it approved etc this week | |
| 15:22:27 | dansmith | efried: the first bullet under "if there is no record" mentions stuff about user_id and project_id, but I'm missing why that's related | |
| 15:22:44 | mriedem | lyarwood: we don't need a blueprint for a bug fix | |
| 15:22:59 | lyarwood | mriedem: right, even if it's across two cycles? | |
| 15:23:00 | mriedem | lyarwood: as for that new concurrent requests bug, i know what that's about, and why it's only since queens | |
| 15:23:06 | mriedem | lyarwood: sure | |
| 15:23:12 | lyarwood | mriedem: kk, I'll nuke the bp then | |
| 15:24:15 | efried | edleafe: --^ | |
| 15:24:58 | efried | dansmith: It's because of the goofiness we implemented wrt user and project IDs earlier, plus having no endpoints that manage consumers directly. | |
| 15:25:35 | efried | dansmith: At earlier microversions, proj/user IDs were optional, so we wanted to not create the consumer record if they weren't specified. | |
| 15:25:59 | efried | dansmith: But now we *always* want to set/maintain the generation, even at older microversions, so we *have* to create the consumer record. | |
| 15:26:25 | efried | dansmith: So we had to make proj/user ID fields nullable so that, at older microversions where they weren't required/specified, we could still create that record. | |
| 15:26:34 | efried | I think I've got that right - edleafe help me out here ^ | |
| 15:27:26 | dansmith | hmm, okay, that is.. odd, | |
| 15:27:31 | efried | it is indeed. | |
| 15:27:37 | dansmith | so we didn't initially have a consumer record and then added it for user/proj? | |
| 15:27:47 | efried | I think that's the case, yes. | |
| 15:27:50 | dansmith | I guess it seems weird that we didn't just start creating those records with null fields at that point | |
| 15:28:00 | efried | yeah. That would have been a thing to do. | |
| 15:28:02 | efried | Somewhere I tagged the IRC discussion edleafe and I had about this. | |
| 15:28:05 | mriedem | lyarwood: details https://bugs.launchpad.net/nova/+bug/1762687/comments/3 | |
| 15:28:05 | openstack | Launchpad bug 1762687 in OpenStack Compute (nova) "Concurrent requests to attach the same non-multiattach volume to multiple instances can succeed" [Undecided,New] - Assigned to Lee Yarwood (lyarwood) | |
| 15:28:08 | efried | possibly in a spec comment. | |
| 15:28:25 | dansmith | efried: so ... is it unreasonable to say that expecting the reader of the spec to have that context is.. unreasonable? | |
| 15:28:34 | efried | dansmith: Here's that IRC convo: http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2018-04-04.log.html#t2018-04-04T13:35:30 | |
| 15:28:36 | dansmith | because reading it from the top, I get stopped at that point with zero idea where it's going | |
| 15:29:09 | efried | dansmith: That's not unreasonable. But trying to explain all that context probably would be. How about a vague "for historical reasons" sentence? | |
| 15:29:23 | efried | or I suppose we could link that eavesdrop in | |
| 15:29:53 | dansmith | well, | |
| 15:30:01 | dansmith | I kinda think the context is worthwhile in here | |
| 15:30:04 | dansmith | more than an irc link | |
| 15:30:38 | efried | Or perhaps this is an implementation detail that doesn't need to be in the spec at all. Since I *think* the interface isn't changing. (edleafe said it was changing a teeny bit; I never understood how) | |
| 15:30:43 | lyarwood | mriedem: I think you've missed that this is with two different instances, not one. | |
| 15:31:39 | mriedem | oh.... | |
| 15:31:42 | mriedem | yes, dear | |
| 15:35:13 | dansmith | efried: edleafe: comments in there.. if you're really concernedand want to do those in a follow-in that's fine, but I think it's probably fine to just roll them in here | |
| 15:35:23 | dansmith | and I can fast approve if jaybird isn't around at that point | |
| 15:36:04 | kashyap | When anyone gets a moment later, I'm duking around a potentially stupid unit test mistake: http://paste.openstack.org/show/718840/. Corrections / snide remarks / rotten tomatoes welcome. | |
| 15:36:13 | kashyap | s/bbia/bbiab/ | |
| 15:36:57 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: Cleanup RP and HM records while deleting a compute service. https://review.openstack.org/554920 | |
| 15:41:40 | mriedem | lyarwood: i don't have great solutions off the top of my head | |
| 15:41:52 | dansmith | kashyap: L26 isn't a tuple | |
| 15:41:54 | mriedem | was thinking if we had a bdm.multiattach column that maybe that could somehow be used, but not really... | |
| 15:42:15 | dansmith | kashyap: so I think you're doing "(mock_getver,).return_value = " | |
| 15:42:29 | dansmith | kashyap: (foo,bar) and (foo,) are tuples.. (foo) is not | |
| 15:42:54 | mriedem | lyarwood: we have this... https://github.com/openstack/nova/blob/master/nova/objects/block_device.py#L327 but that doesn't alleviate the race | |
| 15:43:22 | dansmith | kashyap: and of course, you should be removing test.nested | |
| 15:47:38 | edleafe | dansmith: yeah, I'll push a new rev of that spec shortly | |
| 15:47:45 | dansmith | edleafe: ack thanks | |
| 15:47:55 | lyarwood | mriedem: yeah we can still race on our side, then again so could c-api when it's creating the attachment | |
| 15:48:08 | lyarwood | *attachments | |
| 15:48:34 | mriedem | so i can't remember what we said at the ptg the bdm unique constraint would be on, since it can't be volume_id and instance_uuid, since that would break multiattach | |
| 15:48:41 | mriedem | oh, nvm, | |
| 15:48:45 | mriedem | that would be ok | |
| 15:49:01 | dansmith | what if people want multiple attachments to the same vm and instance?? | |
| 15:49:14 | dansmith | s/??/?/ | |
| 15:49:37 | lyarwood | to the same volume and instance you mean? | |
| 15:49:50 | dansmith | heh yeah | |
| 15:49:51 | lyarwood | like during LM | |
| 15:50:05 | dansmith | or just for some sort of fictitious multipathing sort of thing | |
| 15:50:15 | dansmith | like they want multiple VFs on the same physnet today | |
| 15:50:39 | mriedem | via the cinder api, you can create multiple volume attachments to the same instance and volume | |
| 15:50:45 | mriedem | nova uses that for migrations | |
| 15:50:46 | lyarwood | we wouldn't model that using attachments, that's all within a single attachment and the connection_info it provides | |
| 15:51:10 | mriedem | only one attachment should be 'active' at any time | |
| 15:51:15 | mriedem | like what we're doing with the neutron port binding stuff | |
| 15:51:42 | mriedem | active = the attachment has a host connector and connection to the backend storage | |
| 15:52:10 | dansmith | lyarwood: I dunno, if we have multiple paths to the host from the volume provider, we'd need different attachments because of differing addresses right? | |
| 15:53:13 | dansmith | I'm just playing devil's advocate here to make sure we don't regret a decision later | |
| 15:53:20 | dansmith | removing a constraint is easy I guess | |
| 15:53:32 | mriedem | i don't even have a decision/solution for this race problem right now | |
| 15:53:33 | dansmith | although I'm not sure if mriedem is saying current live migration behavior would break there | |
| 15:53:37 | lyarwood | dansmith: hehe yeah I understand | |
| 15:54:10 | mriedem | the solution to fix the race with attaching the same volume to the same instance concurrently is a unique constraint on the bdms table over the volume_id and instance_uuid columns | |
| 15:54:17 | mriedem | but that doesn't fix lyarwood's new bug | |
| 15:55:11 | mriedem | you can't put a unique constraint on just the volume_id column since that would break multiattach | |
| 15:55:35 | mriedem | you almost need a conditional constraint, where volume_id must be unique if multiattach=False | |
| 15:55:46 | mriedem | but there is no such thing as a conditional unique constraint is there? | |
| 15:56:11 | mriedem | and jaypibbles ran off | |
| 15:56:29 | dansmith | mriedem: I think you need an active=$id column to do that | |
| 15:56:39 | dansmith | that's why we have deleted=$id I think | |
| 15:56:47 | mriedem | https://en.wikipedia.org/wiki/Check_constraint | |
| 15:57:38 | dansmith | hmm | |
| 15:57:45 | dansmith | I wonder what the performance of that is | |
| 15:57:50 | mriedem | i've always seen these in sqla-migrate but never played with one | |
| 15:58:00 | mriedem | zzzeek_: how terrible are check constraints? | |
| 15:58:20 | dansmith | wait, | |
| 15:58:20 | zzzeek_ | mriedem: most mysql / mariadb variants ignore them | |
| 15:58:25 | lyarwood | http://docs.sqlalchemy.org/en/latest/core/constraints.html#check-constraint | |
| 15:58:28 | lyarwood | Note that some databases do not actively support check constraints such as MySQL. | |
| 15:58:30 | zzzeek_ | mriedem: which is why you never see thme :) | |
| 15:58:30 | dansmith | you have to do your own uniqueness checking in the contraint then | |
| 15:58:33 | lyarwood | ^ yeah what zzzeek_ said | |
| 15:58:34 | mriedem | gdi | |
| 15:58:49 | zzzeek_ | lyarwood: mariadb 10.2 does now. oddly enough this creates more problems :) | |
| 15:59:35 | dansmith | mriedem: I'm sure DB2 supports them and is just hanging out by the punch bowl waiting for someone to care | |