| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 18:40:46 | melwitt | jeblair: I don't think limit info is cached | |
| 18:42:45 | sean-k-mooney2 | melwitt: is limit info still stored in nova or is that contolled by keystone now for cores,ram and insances? | |
| 18:42:58 | jaypipes | mdbooth: actually, decorating the _from_db_object() with the @writer context is actually a very *explicit* way of saying the expectations of that method. https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L902 | |
| 18:43:38 | jaypipes | mdbooth: and functional tests of the data online migration will blow up nicely if it's set to @reader instead of @writer. | |
| 18:43:49 | jaypipes | mdbooth: and blowing up is a good thing in this case... | |
| 18:43:50 | mdbooth | jaypipes: Absolutely, but it's unnecessary except in this really weird edge case. | |
| 18:43:56 | jaypipes | mdbooth: agreed | |
| 18:44:04 | mdbooth | Which will go away... | |
| 18:44:11 | jaypipes | mdbooth: thus the comments saying so :) | |
| 18:44:19 | jaypipes | mdbooth: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L904-L905 | |
| 18:44:19 | mdbooth | Except that now everything above it was unnecessarily promoted to writer | |
| 18:44:38 | mdbooth | And now you have to go audit all the things to find out after the fact which ones were really supposed to be writers. | |
| 18:45:03 | jaypipes | yeah, that's the tradeoff I suppose | |
| 18:45:40 | dansmith | sean-k-mooney2: still in our db | |
| 18:46:12 | mdbooth | jaypipes: That races, btw | |
| 18:46:21 | sean-k-mooney2 | dansmith: oh ok was i imagining that we wanted to move it to keystone at some point? | |
| 18:47:06 | mdbooth | jaypipes: Concurrent reads can create different uuids | |
| 18:47:29 | jeblair | melwitt: thanks, that's easy then :) | |
| 18:49:11 | sean-k-mooney2 | mdbooth in which case it needs to be a writer lock no? | |
| 18:49:49 | mdbooth | sean-k-mooney2: Doesn't matter, unless both prior reads are done with 'for update' (or equivalent) | |
| 18:50:08 | mdbooth | Which I don't think they are | |
| 18:50:57 | mdbooth | You'll get 2 reads of 2 null values. They'll race to update. The first will write a value and block the second one. When the first commits, the second will write its own different value. | |
| 18:51:07 | mdbooth | Both functions will return objects with different uuids. | |
| 18:51:44 | sean-k-mooney2 | mdbooth: what you effectivly want to do is have a cas so only one updates the record | |
| 18:52:07 | sean-k-mooney2 | mdbooth: im guessing thats what 'for update' will give you | |
| 18:52:13 | mdbooth | cas? | |
| 18:52:18 | jaypipes | compare and swap | |
| 18:52:23 | mdbooth | Ah, yeah | |
| 18:52:24 | jaypipes | but we're overthinking this I think. | |
| 18:52:27 | mdbooth | That's what I wrote | |
| 18:52:42 | jaypipes | yeah, the update_with_match thing is the compare and swap | |
| 18:52:46 | jaypipes | regardless... | |
| 18:52:50 | mdbooth | https://review.openstack.org/#/c/242603/25/nova/objects/block_device.py | |
| 18:54:19 | mdbooth | jaypipes: Thing is, if you overthink the building blocks, it means you don't need to overthink everything else. Not so much, anyway. | |
| 18:54:44 | mdbooth | The caller doesn't need to be concerned with all these weird edge cases. It just works. | |
| 19:00:26 | openstackgerrit | Merged openstack/nova master: Fix doubling allocations on rebuild https://review.openstack.org/521662 | |
| 19:01:51 | zzzeek | mdbooth / jaypipes the retry decorartor is wanting, IMO | |
| 19:02:05 | zzzeek | there was originally some idea to integrate one into enginefacade itself so that it knows how to nest | |
| 19:02:40 | mdbooth | zzzeek: Could you use a savepoint for this, btw? | |
| 19:03:10 | zzzeek | mdbooth: not if you're looking to respond to other transactions also making changes b.c. isolation may prevent you from seeing those changes | |
| 19:03:40 | zzzeek | for an UPDATE i think repeatable read would mean you can't see another transactions updates | |
| 19:05:31 | mdbooth | zzzeek: Anyway, my approach was to use an independent transaction context explicitly. I figured this would always be safe. | |
| 19:06:02 | mdbooth | Then I was convinced it wasn't worth writing slight weird code for an extremely obtuse edge case where failure was safe. | |
| 19:06:03 | zzzeek | mdbooth: i think you are correct assuming that record is not part of the external tranasction. pre-enginefacade nova was all independent transactions. | |
| 19:06:37 | mdbooth | But then I flipped again because I figured this is a read transaction which only needs to write in an edge case (migrating a legacy record). | |
| 19:07:10 | mdbooth | So having an independent transaction is useful to avoid forcing the enclosing transaction and all its potential parents to become writers. | |
| 19:08:06 | mdbooth | And that's where I am now. | |
| 19:10:38 | zzzeek | mdbooth: i think leave it and goad me into working up a better retry situation for enginefacade | |
| 19:11:08 | mdbooth | zzzeek: So all of the above is in the context of code we'd want to delete next cycle anyway, as it's only for a migration :) | |
| 19:11:48 | zzzeek | mdbooth: nova's magical migrations...sure | |
| 19:11:48 | mdbooth | Well cycle +1, because the online migration runs it too. | |
| 19:11:52 | zzzeek | mdbooth: sure | |
| 19:12:00 | zzzeek | mdbooth: this is not the most concerning thing to me :) | |
| 19:13:10 | mdbooth | I think it's fine, tbh. I also doubt that many of nova's uses of the retry wrapper are a problem today, because we don't nest many transactions. | |
| 19:13:35 | mdbooth | It would be a bigger problem if we made wider use of the enginefacade | |
| 19:14:29 | mdbooth | jaypipes: Thanks again | |
| 19:22:30 | openstackgerrit | Chris Dent proposed openstack/nova master: Optional separate database for placement API https://review.openstack.org/362766 | |
| 19:33:20 | cdent | can someone besides mriedem put some eyes on https://review.openstack.org/#/c/523403/ is an auth config error that would be useful to backport. dansmith, jaypipes ? | |
| 19:33:55 | efried | cdent Patches are passing with your grenade fix - thanks for that. | |
| 19:34:06 | cdent | huzzah | |
| 19:34:30 | mriedem | cdent: i think melwitt enjoys auth config bug fixes | |
| 19:34:59 | melwitt | I totally do | |
| 19:35:02 | cdent | ah yes melwitt would be a fine choice | |
| 19:35:28 | cdent | apologies had not hear you around recently | |
| 19:35:35 | cdent | whereas dansmith and jaypipes … | |
| 19:35:57 | efried | cdent Is the kwarg really called oslo_config_config? | |
| 19:36:19 | cdent | yeah, isn’t that awesome, there are a few different olso_config_* | |
| 19:36:54 | efried | whee | |
| 19:37:42 | cdent | I think it is to try to be explicit about the fact that it can also accept keyword arguments which are configuration keys and values | |
| 19:38:05 | cdent | a sort of namespacing | |
| 19:41:27 | sean-k-mooney2 | cdent: we were going to do something similar with networking-ovs-dpdk devstack setting then reallised that would haver ment setting OVS_DPDK_OVS_REPO AND OVS_DPDK_DPDK_REPO so decided not to prepend everyting with OVS_DPDK | |
| 19:42:08 | cdent | oh my | |
| 19:42:52 | sean-k-mooney2 | we still have pretty verbose setting without that https://github.com/openstack/networking-ovs-dpdk/blob/master/devstack/settings | |
| 19:42:53 | efried | cdent Left a comment on that patch. Is there a reason we would need to keep that arg? | |
| 19:43:49 | cdent | efried: because we don’t know who might have use the deploy() method in the past | |
| 19:44:01 | cdent | that’s how you build a custom wsgi application, if you’re so inclined | |
| 19:44:07 | efried | cdent Is it somehow public? | |
| 19:44:24 | cdent | it is public by virtue of being python | |
| 19:44:47 | efried | What I mean is, is it used by some known external entity that we care about? | |
| 19:44:52 | cdent | and in the early days of placement was the entry point to the wsgi application stack until sean added the stuff in wsgi.py | |
| 19:44:57 | cdent | the point is that we don’t know | |
| 19:45:09 | efried | But we care? | |
| 19:45:13 | cdent | yes, of course | |
| 19:45:18 | efried | Oh, not of course. | |
| 19:45:33 | cdent | the whole point of a wsgi application is to be able to integrate it in whatever way you want | |
| 19:45:36 | efried | Nova changes "public APIs" like ComputeDriver all the time without worrying about external consumption. | |
| 19:45:47 | sean-k-mooney2 | was it ever the entrypoint in a version of the placement api that shipped in a release | |
| 19:46:07 | efried | And when I've asked about that before, it was basically explained away to "we don't care". | |
| 19:46:15 | efried | So in this case, why do we care? | |
| 19:46:16 | cdent | sean-k-mooney2: a) I _think_ so but am not certain, b) it has always been my intent that deploy be available as a public interface | |
| 19:46:28 | cdent | because placement is not nova? | |
| 19:46:46 | cdent | it has a _much_ different attitude towards building the wsgi application than the nova-api ever did | |
| 19:47:13 | sean-k-mooney2 | cdent: well if that was the intent then we should treat it as public and document that fact. | |
| 19:48:13 | cdent | I think I probably made the mistake of assuming that was somehow obvious, by virtue of its position in the wsgi stack, but I think I may be one of the few people around here who think of the wsgi stack in that fashion | |
| 19:48:23 | cdent | we can certainly document it as such | |
| 19:49:09 | cdent | efried: would your preference be that I add that documentation to that patch now, or is it cool to worry about it later? | |
| 19:49:32 | efried | Later is fine. | |
| 19:50:50 | efried | cdent Changed sides. | |
| 19:51:25 | cdent | thanks, I’ll do the documenting likely tomorrow, I think I’m out of code mode for today | |
| 19:51:36 | cdent | also, all my promises to myself to not work late tonight seem to be for naught | |