Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-10-14
15:26:00 mriedem gmann: lots of things come up
15:26:16 mriedem idk if anyone (the tc?) documented test plans for this
15:26:22 mriedem or statements about testing this kind of upgrade
15:26:38 mriedem if there was previous agreement from the tc and/or qa or whatever that we as a community don't care about testing that scenario, then fine
15:27:05 gmann http://lists.openstack.org/pipermail/openstack-dev/2018-September/134592.html
15:27:32 mriedem yeah that's the thread i was thinking about when i said this came up earlier
15:30:41 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove unused 'nova-dsvm-base' job https://review.opendev.org/688389
15:30:41 openstackgerrit Stephen Finucane proposed openstack/nova master: Stop testing Python 2 https://review.opendev.org/687954
15:30:42 openstackgerrit Stephen Finucane proposed openstack/nova master: zuul: Make functional job inherit from openstack parents https://review.opendev.org/688425
15:30:47 gmann mriedem: i think there was agreement 'community don't care ' but documented anywhere
15:31:08 mriedem that's what doug seems to be saying http://lists.openstack.org/pipermail/openstack-dev/2018-September/134655.html
15:31:14 mriedem so *shrug*
15:31:28 gmann * not documented anywhere
15:33:44 gmann I think we can add that in TC doc/resolution. i will add that part as part of 24th TC office hour discussion
15:48:25 openstackgerrit Chris Dent proposed openstack/nova master: single pass instance info fetch in host manager https://review.opendev.org/623558
15:58:05 melwitt mriedem: I thought you might be interested in this, I wrote a regression test for the "run online_data_migrations twice fails" in the post test hook and a different fix on top https://review.opendev.org/688205
15:58:49 mriedem that would get around the mysql in functional test thing yeah
15:59:05 mriedem get your power back?
16:03:18 melwitt yeah, restored friday evening
16:03:50 melwitt nice to have internet again
16:03:54 mriedem heh, i bet
16:06:46 melwitt and yeah, I couldn't find another way to exercise the bug and explained briefly why in the commit message. lmk if you think the explanation could use more info, or if there are alternatives that I missed
16:07:14 openstackgerrit Merged openstack/nova master: Avoid using image with kernel in BDM large request func test https://review.opendev.org/688132
16:08:45 efried stephenfin: Does https://review.opendev.org/#/c/688249/ look right to you?
16:10:04 stephenfin sure does
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

Earlier   Later