Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-06
16:45:31 jaypipes mdbooth: no.
16:45:52 jaypipes mdbooth: the retry decorator is designed to work with the enginefacade's transaction context...
16:46:08 jaypipes zzzeek: you around? need your expert opinion on something.
16:46:15 mdbooth jaypipes: Ah... lemme check that. I may have missed that.
16:46:48 efried jaypipes Argh, found a pep8 error in your REST API change.
16:46:59 efried jaypipes in_tree pushed a docstring line over 80c :(
16:47:02 jaypipes efried: that I did or that you did? :)
16:47:07 zzzeek jaypipes: gotta leave in about 3 minutes but whats up
16:47:08 jaypipes efried: ah
16:47:18 jaypipes zzzeek: sec, grabbing link
16:47:19 efried jaypipes You did. But let me fix it, since I've got the stack right here.
16:47:40 mriedem cdent: done https://review.openstack.org/#/c/521639/ thanks
16:47:55 jaypipes zzzeek: line 123, my comment here: https://review.openstack.org/#/c/242603/25/nova/objects/block_device.py
16:48:07 cdent mriedem: cool, yeah on the IRON_NFV cleanup, I didn’t want to disrupt the entire functional test
16:48:10 jaypipes zzzeek: AFAIK, there's no need to do that
16:48:30 openstackgerrit Eric Fried proposed openstack/nova master: placement: allow filter providers in tree https://review.openstack.org/377215
16:48:31 openstackgerrit Eric Fried proposed openstack/nova master: placement: adds REST API for nested providers https://review.openstack.org/384807
16:48:32 openstackgerrit Eric Fried proposed openstack/nova master: SchedulerReportClient._get_providers_in_tree https://review.openstack.org/520663
16:48:32 openstackgerrit Eric Fried proposed openstack/nova master: Scheduler set_inventory_for_provider does nested https://review.openstack.org/520643
16:48:32 jaypipes zzzeek: i.e. no need for the txtct.using(context) thing on line 125
16:48:32 openstackgerrit Eric Fried proposed openstack/nova master: placement: update client to set parent provider https://review.openstack.org/385693
16:48:33 openstackgerrit Eric Fried proposed openstack/nova master: ProviderTree.populate_from_iterable https://review.openstack.org/520756
16:48:33 openstackgerrit Eric Fried proposed openstack/nova master: SchedulerReportClient._get_providers_in_aggregates https://review.openstack.org/521097
16:48:34 openstackgerrit Eric Fried proposed openstack/nova master: WIP: ComputeDriver.update_provider_tree() https://review.openstack.org/521187
16:48:34 openstackgerrit Eric Fried proposed openstack/nova master: WIP: Scheduler[Report]Client.get_provider_tree https://review.openstack.org/521098
16:48:35 openstackgerrit Eric Fried proposed openstack/nova master: WIP: Use update_provider_tree from resource tracker https://review.openstack.org/520246
16:48:40 efried jaypipes Fixed. Very bottom of the stack, of course.
16:48:47 jaypipes efried: ack
16:48:49 jaypipes ty
16:48:59 mdbooth jaypipes: I'm looking at the code for wrap_db_retry, and I don't see anything which would create a new transaction.
16:49:25 mdbooth We normally use this decorator on a function which also has a decorator to create a transaction context, though.
16:49:38 jaypipes mdbooth: the whole *point* of the retry decorator is to catch the ROLLBACK and restart a transaction :)
16:49:53 mdbooth I don't think so, no.
16:49:58 mdbooth I think it just retries.
16:50:09 mdbooth And the way we always use it is that we also create a transaction.
16:50:22 zzzeek jaypipes: i belive your comment is accurate but the method itself has to be called before there's some larger transaction going on , unless the retry decorators know to propagate all the way up to the top one
16:50:59 efried bauzas gibi Would you please re-+A https://review.openstack.org/377215 (rebase & trivial pep8 change)
16:51:01 mdbooth There's no nested context stuff going on with the retry decorator
16:51:17 bauzas efried: did
16:51:31 efried bauzas Merci
16:51:43 mdbooth jaypipes: I'm looking at the code: it just runs it again
16:52:27 dansmith the transaction is in the decorator, so if the retry is above that, it'll do another transaction
16:52:36 mdbooth dansmith: Right.
16:52:53 mdbooth They're 2 separate things, but we normally use them together.
16:52:55 zzzeek jaypipes mdbooth i dont see the point of the retry decorator if you have a context manager inside to handle the thing anyway
16:53:21 mdbooth zzzeek: Well the context manager always has to be inside the retry decorator
16:53:24 jaypipes zzzeek: me neither, which is why I wrote that comment.
16:53:44 mdbooth However, we normally put it inside the retry decorator using another decorator
16:53:47 mdbooth So:
16:53:51 mdbooth @retry
16:53:55 mdbooth @transaction
16:53:58 mdbooth def my_foo():
16:54:05 zzzeek mdbooth jaypipes haven't looked at this stuff in awhile, the context manager looks unfamiliar to me, would want to see other places the rerty decorator is used and see if a. they also have a nested trans or b. they dont, and then how do these work?
16:54:38 gibi efried: done
16:54:43 efried gibi Thanks!
16:54:57 mdbooth jaypipes: Let me think again if there's a good enough reason not to use the decorator there.
16:55:13 dansmith sales meeting in 5
16:55:36 jaypipes mdbooth: I'm not saying don't use the decorator. I'm saying no need to use the inner tctxt.using() thing.
16:56:21 mdbooth jaypipes: So I'm also entirely sure we must have an enginefacade transaction scope there
16:56:34 mdbooth Which we normally do at function scope using a decorator
16:56:47 mdbooth I'm actually being super-defensive there, I think
16:57:01 openstackgerrit Balazs Gibizer proposed openstack/nova master: Add regression test for bug 1735407 https://review.openstack.org/526095
16:57:02 openstack bug 1735407 in OpenStack Compute (nova) "[Nova] Evacuation doesn't respect anti-affinity rules" [Medium,In progress] https://launchpad.net/bugs/1735407 - Assigned to Balazs Gibizer (balazs-gibizer)
16:57:43 mdbooth If the transaction fails on commit, it'll trigger a retry
16:57:56 mdbooth When we retry, we want a new transaction
16:57:57 mdbooth So we need to create one somehow
16:58:05 mdbooth I'm explicitly using an independent transaction
16:58:16 mdbooth So I'm explicitly *not* using any enclosing transaction scope
16:58:23 jaypipes mdbooth: ahhhh... sorry, yeah, I didn't realize there was no enginefacade decorator being used on any of these object entrypoints (BlockDeviceMapping.create()/save(), etc)
16:58:57 mdbooth So it definitely looks weird because I'm being super-defensive.
16:59:00 mriedem ildikov: replied in https://review.openstack.org/#/c/525787/
16:59:07 jaypipes mdbooth: my presumption was that there was an enginefacade trx context decorator on one of the BDM object methods. :(
16:59:08 mriedem ildikov: if you agree, i'll start on those changes after my next meeting
16:59:18 jaypipes mdbooth: and I see now there isn't...
16:59:27 mdbooth Actually, when I originally wrote that I was less defensive and jsut wrote a comment about how this might fail on multi-master galera
16:59:36 mdbooth But we don't care, because it almost definitely won't
16:59:57 mdbooth And that version looked more normal
17:00:12 jaypipes mdbooth: instead of using that independent context using() thing, I believe you can instead just decorate the _create_uuid() method with the @writer enginefacade decorator, in addition to the @wrap_db_retry decorator.
17:00:16 melwitt mriedem: replied on the review about power_on
17:00:18 mdbooth (but it failed safely)
17:00:43 mdbooth jaypipes: Note the 'independent' in there
17:01:10 mdbooth That's so a race there wouldn't cause a failure of the encompassing transaction
17:01:58 mdbooth jaypipes: Again, I could be convinced to roll back to my big comment explaining why this might fail sometimes.
17:02:16 jaypipes mdbooth: *what* encompassing trx though? I had originally thought there was an encompassing transaction managed by an enginefacade transaction context manager, but I see now there isn't any enginefacade decorator being used on any of the BDM object methods like create(), save(), etc
17:02:38 mdbooth jaypipes: Not today, no.
17:03:01 jaypipes mdbooth: you're implying there will be? :)
17:03:22 mdbooth jaypipes: The design of the enginefacade encourages it, and we have no way of knowing.
17:03:52 mdbooth e.g. if the thing *calling* save had its own transaction
17:03:59 mdbooth we'd be in an encompassing scope
17:04:36 mdbooth Do we ever call save() within a transaction scope?
17:04:51 mdbooth We never used to, because they were confined to db.api()
17:05:01 mdbooth However, they've been moving out of there for a while.
17:06:10 mdbooth jaypipes: Anyway, this is very defensive. I wrote it such that it shouldn't fail however you call it. That makes it look different, so it's a tossup whether it's worth it.
17:06:15 openstackgerrit Theodoros Tsioutsias proposed openstack/python-novaclient master: CommandError is raised for invalid server fields https://review.openstack.org/525110
17:06:17 jaypipes mdbooth: commented on the patch again...
17:06:31 jaypipes mdbooth: lemme know if that code makes more sense.
17:07:01 jaypipes mdbooth: sorry, I originally had missed the fact that no enginefacade trx context decorators were in use for BDM object
17:07:09 bauzas quick question for privsep specialists
17:07:41 openstackgerrit Theodoros Tsioutsias proposed openstack/python-novaclient master: CommandError is raised for invalid server fields https://review.openstack.org/525110
17:07:43 mdbooth jaypipes: Yeah, I think the only reason I didn't do that was to have an independent transaction

Earlier   Later