Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-10-14
16:11:44 efried stephenfin: cool, I've got a few similar ones...
16:11:46 efried thanks
16:13:18 melwitt zzzeek: I would appreciate your review on this fix I proposed related to the questions I asked you on friday https://review.opendev.org/688206
16:13:38 zzzeek melwitt: yah, saw the link, will look now
16:13:46 melwitt (and thanks again for helping me out with that)
16:15:02 efried https://review.opendev.org/#/c/688250/ https://review.opendev.org/#/c/688235/ https://review.opendev.org/#/c/688087/ if you care stephenfin
16:17:22 cdent mriedem: my to do list has just reminded me to prompt folk about https://review.opendev.org/#/c/623558/ which is jay's patch to improve instance info fetch in the host manager. A while back I added some unit tests and did the tweaks that eric requested
16:18:49 openstackgerrit Merged openstack/os-resource-classes master: Bump the openstackdocstheme extension to 1.20 https://review.opendev.org/688249
16:24:10 zzzeek melwitt: before I look silly on the review because I'm missing something, do i read correctly that security_group_ensure_default is called twice inside of instance_create() ?
16:25:02 melwitt zzzeek: that is correct. the first time, it intends to create it if it doesn't exist, the second time, it's really only doing it to read it back (was my conclusion)
16:25:36 zzzeek melwitt: what condition would casue the return value to have changed in the intervening 15 or so lines of code?
16:25:39 melwitt are you thinking another way would be to just use the returned object from the first time, later on?
16:25:55 openstackgerrit Eric Fried proposed openstack/nova-specs master: Add 'Feature Liaison' spec process https://review.opendev.org/685857
16:25:56 openstackgerrit Eric Fried proposed openstack/nova-specs master: Add spec for vm scoped sriov numa affinity https://review.opendev.org/683174
16:26:01 efried bauzas: ^
16:26:19 zzzeek melwitt: beacsue i dont know the concepts being worked with here, from what im seeing i dont immediately see why a. the top call should use the context, not independent transaction and b. what's wrong with default_gruop assigned at the top there
16:26:58 openstackgerrit Ghanshyam Mann proposed openstack/nova-specs master: Re-propose policy-defaults-refresh spec for Ussuri https://review.opendev.org/686058
16:27:02 zzzeek melwitt: then there's this _Get_Sec_group_models closure, which appears to be called only once, three lines later, no recursion, no passing it onwards, so why is it like that
16:27:02 gmann melwitt: ^^^ re-proposed policy spec for ussuri
16:27:04 mriedem cdent: without digging into the code change yet, i dropped a few comments in the commit message
16:27:15 mriedem soft -1, i know it's an adopted patch
16:27:17 zzzeek melwitt: it makes it look like this used to be very different and was refactored, but not all the way
16:27:21 cdent mriedem: thanks
16:27:50 zzzeek melwitt: if you agree thatim reading it right i can post my comments to this effect and folks can discuss
16:28:36 melwitt zzzeek: indeed, I could only assume similar because I'm not _that_ familiar with the method and definitely not familiar with the history. I'm not quite parsing your question a) though, why the top call should not use a separate transcation? did you mean why does it?
16:29:10 mriedem note that we don't even need that call if using neutron https://review.opendev.org/#/c/653065/
16:29:20 melwitt zzzeek: and I agree that it looks partially refactored
16:29:22 zzzeek melwitt: yes, got my negatives reversed.
16:29:29 zzzeek melwitt: yes why use a separte trans there
16:30:37 zzzeek melwitt: when the separate trans is used, that means that you either want the thing you're doing to modify the DB even if your method fails, or you want the thing you're doing to modify the DB while your mehtod is still running (like logging a slow operation), or, otehrwise you're trying to read some state that occurs within the middle of your ongoing, slow transaction
16:31:26 zzzeek melwitt: this instance_create doesn't look like a slow function and it looks like the originator of the transaction that it commits. but i dont know
16:31:51 melwitt zzzeek: ok, yeah, I went through thinking about it and I think what that comment is saying is that if we don't fork out a separate transaction, two racing server creates would try to insert the default group (the first time) and then one will succeed and the second would fail the COMMIT and then raise a DBError and fail the second server create [to the user]. using a separate transaction avoids that. now, I think an alternative way to
16:31:51 melwitt have handled that would have been to try-except the DBError and call instance_create again in that case, but that's not what was done
16:32:59 zzzeek melwitt: only if this function is overall slow-ish is there a race created that wouldn't also exist in the security_group_ensure_default() call itself
16:33:01 melwitt *fail the COMMIT because of the unique constraint on project_id
16:33:06 zzzeek like, those can race anyway right?
16:33:31 zzzeek oh it has the retry on it
16:33:38 zzzeek I see so its independent so that it can run and do a retry
16:34:07 zzzeek OK but...it returns the value at least, so, you can use it at the top? the return value that is, and not call it a second time
16:36:16 melwitt yeah... thinking... on the surface I don't see why we couldn't. I was trying to think if there would be any implications for racing requests to instance_create, but there shouldn't be right..
16:37:19 zzzeek melwitt: I did a blame. this code is oldish
16:37:51 zzzeek Pavel Kholkin in 2015 seems to be most of what I'm asking about, is that person still around
16:38:15 zzzeek e.g. i can put up my qeustions and bring them onto a review if they still work for openstack
16:38:20 melwitt mriedem: yeah, I remember that patch. I'm not opposed to working around it but I landed on this because I was genuinely puzzled about how dupe groups were being created when the logic is clearly "get or create". once I found the root cause of how two identical groups were being created, I went ahead and proposed a way to address that directly
16:38:26 zzzeek haha "work for openstack"
16:39:57 zzzeek melwitt: is a "security group" kind of an infrequently created concept and the race here is the kind of thing that happens only during some kind of installation? a "Security group" sounds like the kind of thing an administrator creates just once
16:40:11 melwitt zzzeek: yeah, the code is old and as mriedem mentioned, it doesn't come into play (default group not ever used) if running with neutron, and neutron is the only networking we support as of... a few releases ago. this code is dealing with legacy nova-network security group record
16:40:44 melwitt and I don't think Pavel Kholkin is still around working on openstack
16:41:10 zzzeek melwitt: OK ill comment to these effects
16:42:58 melwitt zzzeek: yeah, this code only creates a group once per project and the only time it becomes a problem is when it's done with an anonymous RequestContext which has project_id=NULL, which is happening because nova-manage is inserting a "dummy" instance record to support a marker/paging (don't ask, haha) and the unique constraint on project_id won't "work" for NULL values
16:45:08 openstackgerrit Eric Fried proposed openstack/nova master: Stop using NoAuthMiddleware in tests https://review.opendev.org/687416
16:45:10 openstackgerrit Eric Fried proposed openstack/nova master: Repro bug 1845530: versioned discovery is authed https://review.opendev.org/685180
16:45:10 openstack bug 1845530 in OpenStack Compute (nova) "Versioned discovery endpoint should not require authentication" [Undecided,In progress] https://launchpad.net/bugs/1845530 - Assigned to Eric Fried (efried)
16:45:10 openstackgerrit Eric Fried proposed openstack/nova master: Allow versioned discovery unauthenticated https://review.opendev.org/685181
16:45:13 efried mriedem: ^
16:48:23 openstackgerrit Dan Smith proposed openstack/nova master: Add image caching API for aggregates https://review.opendev.org/687140
16:52:16 openstackgerrit Eric Fried proposed openstack/nova-specs master: Add 'Feature Liaison' spec process https://review.opendev.org/685857
16:52:16 openstackgerrit Eric Fried proposed openstack/nova-specs master: Add spec for vm scoped sriov numa affinity https://review.opendev.org/683174
16:52:49 efried bauzas, gmann: Had to respin to fix the title on gibi's s/Core/Feature/ Liaison section ^
16:53:07 efried (gibi thanks for adding that section preemptively :)
16:55:27 gmann efried: +1
16:55:33 efried thanks gmann
17:26:10 openstackgerrit Matt Riedemann proposed openstack/nova master: Add prep_snapshot_based_resize_at_dest compute method https://review.opendev.org/633293
17:26:10 openstackgerrit Matt Riedemann proposed openstack/nova master: Add PrepResizeAtDestTask https://review.opendev.org/627890
17:26:11 openstackgerrit Matt Riedemann proposed openstack/nova master: FUP for I66d8f06f19c5c631e33208580428aa843abb38d2 https://review.opendev.org/678951
17:26:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Add prep_snapshot_based_resize_at_source compute method https://review.opendev.org/634832
17:26:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Add PrepResizeAtSourceTask https://review.opendev.org/627891
17:26:13 openstackgerrit Matt Riedemann proposed openstack/nova master: Add finish_snapshot_based_resize_at_dest compute method https://review.opendev.org/635080
17:26:13 openstackgerrit Matt Riedemann proposed openstack/nova master: Add FinishResizeAtDestTask https://review.opendev.org/635646
17:26:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Execute CrossCellMigrationTask from MigrationTask https://review.opendev.org/635668
17:26:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Plumb allow_cross_cell_resize into compute API resize() https://review.opendev.org/635684
17:26:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Filter duplicates from compute API get_migrations_sorted() https://review.opendev.org/636224
17:26:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Start functional testing for cross-cell resize https://review.opendev.org/636253
17:26:16 openstackgerrit Matt Riedemann proposed openstack/nova master: Handle target host cross-cell cold migration in conductor https://review.opendev.org/642591
17:26:16 openstackgerrit Matt Riedemann proposed openstack/nova master: Validate image/create during cross-cell resize functional testing https://review.opendev.org/642592
17:26:17 openstackgerrit Matt Riedemann proposed openstack/nova master: Add zones wrinkle to TestMultiCellMigrate https://review.opendev.org/643450
17:26:17 openstackgerrit Matt Riedemann proposed openstack/nova master: Add negative test for cross-cell finish_resize failing https://review.opendev.org/643451
17:26:18 openstackgerrit Matt Riedemann proposed openstack/nova master: Refresh instance in MigrationTask.execute Exception handler https://review.opendev.org/669012
17:26:18 openstackgerrit Matt Riedemann proposed openstack/nova master: Add negative test for prep_snapshot_based_resize_at_source failing https://review.opendev.org/669013
17:26:19 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Add confirm_snapshot_based_resize_at_source compute method https://review.opendev.org/637058
17:51:00 openstackgerrit Son Pham proposed openstack/nova-specs master: [WIP]Add blueprint snapshot-vms-with-state of-processes https://review.opendev.org/688522
18:00:37 openstackgerrit Son Pham proposed openstack/nova-specs master: [WIP]Add blueprint snapshot-vms-with-state-of-processes https://review.opendev.org/686948
18:01:31 mriedem bauzas: here are a set of queens changes that are good to go: https://review.opendev.org/#/c/676285/ https://review.opendev.org/#/c/675355/ https://review.opendev.org/#/c/682722/ https://review.opendev.org/680873 https://review.opendev.org/#/c/676500/
18:11:21 dansmith mriedem: I guess you didn't squash that fixup patch into your second patch when you rebased just now eh/
18:12:10 dansmith doesn't look like anything is running yet, so you could do that without losing much still I think
18:12:36 mriedem https://review.opendev.org/#/c/678951/ you mean?
18:13:27 mriedem at some point the thing it was FUP'ing ( https://review.opendev.org/#/c/627890/ ) had +2s on it a couple of times so i figured it was easier to not squash for those +2s to come back as a diff check
18:14:30 mriedem i can if you want, i'm sure at this point a diff isn't trivial for gibi or stephenfin anymore anyway
18:14:32 dansmith mriedem: yeah, but rebased on master now so there's a ton of churn in the patches from there now
18:14:41 dansmith yeah
18:14:49 mriedem none of this code is impacted much from master...
18:14:58 mriedem well, this conductor task code
18:14:58 dansmith I think it makes sense to just do it now
18:15:14 dansmith oh, fair I guess. I just eye-diffed the bottom one and there was a lot of churn, but true about this one
18:15:35 mriedem https://review.opendev.org/#/c/627890/53..59 it's not really trivial anyway
18:15:37 dansmith anyway, whatever, the reason for doing it was a +2 on there in August, so seems safe to squash now but whatever
18:15:49 mriedem yeah i will
18:16:13 mriedem i'll take most any excuse to take a break from reviewing these queens backports...
18:22:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Add PrepResizeAtDestTask https://review.opendev.org/627890
18:22:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Add prep_snapshot_based_resize_at_source compute method https://review.opendev.org/634832

Earlier   Later