Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-21
16:57:36 mriedem ok. i'm alright with us being told that we shouldn't assume initialize_connection is idempotent and we should use the same connection_info to disconnect the volume as was used to connect the volume
16:57:37 jgriffith mriedem: and they get that from the connector
16:57:41 geguileo as long as you haven't changed the initiator name or something crazy like that (I've seen bugs around this)
16:58:05 mriedem i just don't want to find out later that someone relied on us always getting the latest info
16:58:30 jgriffith mriedem: +1 for NOT assuming idempotency, AND +2 for suggesting upgrades and using the new API's instead :)
16:58:32 mriedem with the new flow, we don't get the latest info anyway, it's all stored in cinder and we just re-use it
16:58:33 dansmith mriedem: "much later" we expect to always be using the new API that doesn't suffer from this right?
16:58:47 mriedem dansmith: yeah
16:58:48 jgriffith of course, assumign we're not broken in there as well somewhere
16:59:06 mriedem any existing attached volumes would have the old flow stuff until they get migrated
16:59:24 jgriffith because I think we may be now in our efforts to emulate the old behavior
16:59:26 mriedem which now this makes me paranoid about https://review.openstack.org/#/c/549130/
16:59:37 melwitt gibi: are you able to run the nova meeting tomorrow?
17:00:10 mriedem jgriffith: this is the problematic code https://github.com/openstack/nova/blob/2ec8c49f6cb4a0e7dba217e824c20d9c703d2105/nova/virt/libvirt/driver.py#L7743
17:00:29 mriedem that runs on the source host post-live migration
17:00:35 gibi melwitt: yes I can run it
17:00:38 mriedem for the new flow, we just get the connection_info out of the attachment record
17:00:45 melwitt gibi: cool, ty for confirming
17:00:55 jgriffith ahh, yeah
17:01:26 mriedem so the old flow thing there to call init_connectoin was a workaroudn
17:01:37 mriedem b/c at this point in the flow, in the nova db, the bdm.connection_info is actually for the dest host
17:01:37 gibi melwitt: will you update the agenda or shall I do it?
17:01:53 melwitt gibi: I'll update the agenda
17:02:54 gibi melwitt: OK, thanks
17:02:58 mriedem ok so to summarize, i think i'm hearing the majority of the cinder peeps in here saying doing https://github.com/openstack/nova/blob/2ec8c49f6cb4a0e7dba217e824c20d9c703d2105/nova/virt/libvirt/driver.py#L7749 is a bad workaround and only works for some drivers b/c we're getting lucky
17:02:58 jgriffith mriedem: yeah, I get ya
17:03:08 melwitt gibi: no, thank YOU :)
17:03:10 mriedem even though we've always done it....but anyway
17:03:25 dansmith mriedem: well, we've always done it which ended up in them changing a bunch of drivers to tolerate it
17:03:38 mriedem sure, good point
17:03:48 mriedem it's a fun incestual relationship
17:03:55 geguileo rofl
17:04:08 dansmith really? I don't enjoy any of it :)
17:04:09 dansmith but you're into some weird shit, granted
17:04:22 mriedem jgriffith: btw, we should remember to talk about this in vancouver about why the new flow is much better....
17:04:32 jgriffith mriedem: +1
17:04:53 mriedem alright i'll move onto reviewing the nova patch then, thanks geguileo jgriffith smcginnis
17:05:23 geguileo no problem, and thanks for fixing that :-)
17:05:52 jgriffith mriedem: just for the record one more time and to hope it sticks in peoples heads... "Never update/modify and attachment, just delete it and create a new one"
17:06:17 jgriffith That way you still have the ability to access the old data if you need it and nothing gets funky behind your back
17:06:23 jgriffith hmmm... well, anyway
17:06:42 jgriffith "funky behind your back" isn't the best phrase I guess
17:06:46 mriedem because that would be like calling initialize_connection again
17:06:57 jgriffith mriedem: exact-u-mundo
17:11:45 dansmith mdbooth: did you follow all that ^ ?
17:13:12 mriedem i put the link into the bug for the irc log and a summary
17:13:21 dansmith cool
17:14:13 mriedem while we're on the topic https://review.openstack.org/#/c/549130/2/nova/compute/manager.py@616
17:14:21 mriedem in case any cinder people are still here and can direct me
17:16:09 dansmith mriedem: btw, I changed this to fixes and left a snarky remark: https://review.openstack.org/#/c/554600/
17:16:17 dansmith if you say that's good I'll fix the pike backport
17:18:41 dansmith mriedem: melwitt: tssurya: do we need to have a cells meeting today? I know there are a bunch of tssurya's patches to review, but other than that, I don't know of anything else burning that justifies it
17:19:02 tssurya dansmith: not needed
17:19:22 dansmith tssurya: especially if it means I spend the time reviewing your stuff yeah? :)
17:19:35 tssurya yeaaaaa :D
17:20:24 melwitt +1 to skipping today
17:20:46 cdent the whole day
17:20:48 dansmith woot, even if mriedem really wants a meeting, he's outnumbered
17:21:44 ildikov jgriffith: you need to repeat those that thousands of times so they stick in people's heads :)
17:22:32 cfriesen tssurya: you pinged earlier?
17:25:21 mriedem ildikov: or push a patch to nova to add something to the docstring for the attachment_update method so we don't forget
17:25:26 mriedem i'd +2 that
17:26:21 ildikov mriedem: I can when I'm off booth duty and got to the next hotel today
17:26:22 mriedem dansmith: we've changed 'features' to 'fixes' in release notes in backports for other nova-manage changes for optional things, in order to fix bugs, so i think that's fine
17:27:07 tssurya cfriesen: yea just to ask about the third point in the bug description, since you explicitly stated delete service/compute node, I wanted to just confirm we are still doing a soft delete
17:27:49 tssurya cfriesen: saw you review btw, will change it to cascade, working on some tests and then will update the patch
17:27:51 ildikov mriedem: It won't replace the need of repeating the phrase a thousand times everywhere else
17:28:48 cfriesen tssurya: I think the service/node portion can remain as it is currently. what you have looks reasonable to me with the cascade change
17:29:54 tssurya cfriesen: ack, however I think maybe we need to provide a new command at some point to allow compute node deletions ? like we have osc placement allowing resource provider deletions ?
17:29:57 cfriesen tssurya: dansmith is the go-to guy in this area. :)
17:30:18 cfriesen tssurya: you mean compute node deletions without service deletion?
17:30:27 dansmith mriedem: fine, fine
17:30:28 tssurya cfriesen: yes he is, he just promised he will review ^^ ;)
17:30:42 tssurya cfriesen: yes
17:31:06 sean-k-mooney mriedem: just looked at https://review.openstack.org/#/c/554917 and yes i think that is fine for stable/queens
17:31:17 tssurya I mean deletion of cn, along with its dependencies
17:32:08 tssurya the record as such does not get removed right ?
17:32:26 tssurya or I don't know when its moved to shadow tables
17:34:08 dansmith in the nova database,
17:34:09 dansmith deleting a thing marks it as deleted (deleted=row['id'])
17:34:11 mdbooth dansmith: Thanks for the ping, caught up.
17:34:20 dansmith "nova-manage db archive-deleted rows" moves it to shadow tables
17:34:26 dansmith "nova-manage db purge" removes it from shadow tables
17:34:27 dansmith tssurya: ^
17:34:42 dansmith mdbooth: ack, I assume we're to expect a review from mriedem forthwith
17:34:54 tssurya ah yes was just checking this, I don't know why for some reason I though only the instance* tables were archived
17:35:00 dansmith \o/
17:35:00 mdbooth dansmith: Fortunately the above is consistent with my understanding having spoken to Gorka the other week.
17:35:09 dansmith mdbooth: ack, that's good :)
17:35:14 mdbooth dansmith: Heh, I do that, except I tend to google first :)
17:35:30 dansmith mdbooth: this is 'merica.. fire from the hip and ask questions later
17:35:52 cfriesen tssurya: under what scenario would we want to delete only a compute node and not a service?
17:36:08 dansmith cfriesen: ironic nodes outnumber the service(s) they're owned by
17:36:20 dansmith cfriesen: and, ironic nodes balance between services, and could potentially be orphaned
17:36:41 tssurya cfriesen: nope, I was talking about the delete records in the compute node table
17:36:42 dansmith cfriesen: I would guess maybe the same could happen if you change the hostname on a virt host
17:36:50 tssurya but that is done by archive_deleted_rows
17:36:56 tssurya already so taken care of :)
17:37:01 tssurya sorry for the confusion
17:37:04 mdbooth mriedem: Incidentally lyarwood pointed out that we're not running volume tests in the live migration job by default, so on his suggestion I hacked a run based on another patch he pointed me at. Link is in a review comment.

Earlier   Later