Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-10
14:22:38 efried just so
14:34:07 openstackgerrit Matt Riedemann proposed openstack/nova master: Update wording in @safe_connect placement warnings https://review.openstack.org/560039
14:35:28 bauzas efried: jaypipes: stephenfin: sean-k-mooney: cfriesen_: thanks all for the reviews of https://review.openstack.org/#/c/552924/6/specs/rocky/approved/numa-topology-with-rps.rst
14:36:00 bauzas efried: jaypipes: stephenfin: sean-k-mooney: cfriesen_: now we have a new revision based on your comments https://review.openstack.org/#/c/552924/
14:36:04 openstackgerrit Merged openstack/nova master: Remove mox in unit/api/*/test_instance_actions.py https://review.openstack.org/559269
14:38:35 jaypipes bauzas: k, reviewing now
14:39:41 openstackgerrit Surya Seetharaman proposed openstack/nova master: [WIP] Delete orphaned req_spec/inst_mapp records of archived instances https://review.openstack.org/560042
14:55:58 openstackgerrit Chris Dent proposed openstack/nova master: Use nova.db.api directly https://review.openstack.org/543262
15:01:34 openstackgerrit Merged openstack/nova master: Remove mox in test_user_data.py https://review.openstack.org/559264
15:01:51 openstackgerrit Merged openstack/nova master: Remove mox in unit/api/*/test_server_metadata.py https://review.openstack.org/559673
15:02:02 openstackgerrit Merged openstack/nova master: Remove mox in unit/api/*/test_server_password.py https://review.openstack.org/559649
15:04:31 efried Spec cores (jaypipes dansmith because this spec is near and dear to your hearts) can we get https://review.openstack.org/#/c/556971/ approved now? Code is shaping up.
15:04:57 dansmith efried: wake me when jaypipes is +2 on it
15:05:02 efried ack
15:08:28 efried edleafe: You gonna rebase the rest of the consumer generation series to pick up that fix?
15:09:16 edleafe efried: already done (locally). Need to figure out one last bit before I push a new rev
15:09:22 efried coo
15:10:40 openstackgerrit Chris Dent proposed openstack/nova master: Use nova.db.api directly https://review.openstack.org/543262
15:18:50 jaypipes dansmith: I'm +2 on the spec now.
15:19:36 dansmith well, that was a short nap
15:20:03 mriedem lyarwood: at the ptg we talked about adding an online data migration to look for and remove duplicate bdm entries in the db, to eventually clear the way to adding a unique constraint on instance uuid and volume id in the bdm table - is that still on your radar, or something i can start hacking on?
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

Earlier   Later