Posted	Nick	Remark
#openstack-nova - 2018-04-10
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	zzzeek_	mriedem: most mysql / mariadb variants ignore them
15:58:20	dansmith	wait,
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	dansmith	you have to do your own uniqueness checking in the contraint then
15:58:30	zzzeek_	mriedem: which is why you never see thme :)
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
16:22:37	openstack	bug 1732428 in OpenStack Compute (nova) "Unshelving a VM breaks instance metadata when using qcow2 backed images" [Medium,In progress] https://launchpad.net/bugs/1732428 - Assigned to Matt Riedemann (mriedem)
16:22:37	mriedem	lyarwood: does that also fix bug 1732428?
16:24:18	lyarwood	mriedem: no, flatten is specific to the rbd imagebackend
16:24:49	openstackgerrit	Eric Berglund proposed openstack/nova master: PowerVM: Add proc_units_factor conf option https://review.openstack.org/554688
16:26:16	openstack	Launchpad bug 1762687 in Cinder "Concurrent requests to attach the same non-multiattach volume to multiple instances can succeed" [High,New]
16:26:16	jgriffith	lyarwood: I added a note to https://bugs.launchpad.net/cinder/+bug/1762687
16:26:28	lyarwood	thanks ./me looks
16:26:33	jgriffith	I think the race is the condition check in _reserve_volume on the cidner side
16:26:37	jgriffith	mriedem: ^^
16:27:23	mriedem	jgriffith: just about got something here
16:27:46	jgriffith	mriedem: oh... so I guess that means I was wrong?
16:27:55	mriedem	haven't read the comment yet,
16:27:58	mriedem	just fixing tests
16:28:01	jgriffith	Oh... LOL
16:28:12	jgriffith	mriedem: so what you're saying is "there's a chance" :)
16:28:22	smcginnis	:)
16:28:28	jgriffith	if this were slack I'd insert stupid gif here
16:30:12	smcginnis	I think that's the whole reason why folks like slack over irc. :)
16:31:11	melwitt	dansmith: this is the patch we talked briefly about on friday at the ptg about flattening rbd images if not CONF.use_cow_images. I had asked the room if there was any usefulness in someone configuring that way and you had said some people would to get better performance https://review.openstack.org/#/c/457886
16:31:20	mriedem	lyarwood: can you test this out? https://review.openstack.org/560074
16:31:27	lyarwood	mriedem: sure can
16:33:17	mriedem	dansmith: melwitt: just fyi, i'll be out for a few hours this afternoon
16:33:56	melwitt	k
16:33:57	dansmith	melwitt: okay, was there more to that question?
16:36:18	melwitt	dansmith: lyarwood rebased it a little while ago and it reminded me that I had been meaning to ask if you could review it. I added the bit about using the CONF.use_cow_images config option as a toggle for flattening
16:36:28	dansmith	okay
16:40:59	lyarwood	mriedem: that appears to be enough but I'm just spamming requests from the cli again
16:43:23	mriedem	lyarwood: ok i'm updating it per jgriffith's comment
16:43:59	jgriffith	lyarwood: so just the refresh was enough?
16:44:17	lyarwood	mriedem: that's a different issue though right? That's allowing concurrent attach requests for multiattach volumes that are reserved?
16:44:34	jgriffith	lyarwood: if so that's great, and we can consider that if adding reserve to the status check has consequences (I still think it might)
16:46:19	lyarwood	jgriffith: yeah as above I can't see how adding reserved helps with this non-multiattach race tbh
16:46:50	lyarwood	jgriffith: we only expect available or downloading in that case right?

Earlier   Later