| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-10 | |||
| 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 | |
| 15:59:47 | mriedem | i <3 DB2 | |
| 15:59:52 | dansmith | I know you do | |
| 16:00:00 | mriedem | i heard ms azure rolled out a dbaas service and i noticed it didn't include db2 | |
| 16:00:03 | mriedem | i was hurt | |
| 16:00:14 | dansmith | and shocked, I'm sure | |
| 16:00:22 | mriedem | it does include pg | |
| 16:02:07 | mriedem | ok so if we had an 'active' column on the bdms table, we could set that to true when we do something like set the connection_info on it | |
| 16:02:11 | mriedem | which means it's attached | |
| 16:02:29 | mriedem | but still, | |
| 16:02:40 | mriedem | you could have >1 bdm on the same volume which are both 'active' | |
| 16:02:58 | mriedem | if that volume is multiattach=true | |
| 16:04:57 | mriedem | wonder if there is something that can be done on the cinder side, i.e. a rule saying, you can't have >1 attachment record to the same volume for different instances if the volume is multiattach=false | |
| 16:05:15 | mriedem | or if that's already the rule they have in place | |
| 16:14:46 | mriedem | lee bugged out, but i might have a fix on the cinder side | |
| 16:14:51 | smcginnis | mriedem: I think we can't due to things like migration. | |
| 16:14:52 | mriedem | glory hallelujah | |
| 16:15:13 | mriedem | smcginnis: i'll poke you with the patch when it's up, and i'll hope lee can apply and see if it solves his issue | |
| 16:15:25 | smcginnis | mriedem: OK, sounds like a plan. | |
| 16:19:31 | openstackgerrit | Lee Yarwood proposed openstack/nova master: rbd: flatten images when creating/unshelving an instance https://review.openstack.org/457886 | |
| 16:21:25 | melwitt | mriedem: thanks for adding the neutron stuff to the forum ideas etherpad | |
| 16:21:32 | mriedem | np | |