| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 17:53:58 | mdbooth | jaypipes: Understood. | |
| 17:54:01 | jaypipes | hehe | |
| 17:54:03 | mdbooth | Probably for the best. | |
| 17:54:06 | jaypipes | :) | |
| 18:06:04 | mdbooth | Speaking of anti-patterns, btw: functions which modify their input parameters. I ran across a unit test last week which only passed because the method it called modified the dict it was given as well as its output, so the later assertion that the 2 were equal passed coincidentally. | |
| 18:06:37 | mdbooth | If it had actually mattered, that would be pretty obtuse. | |
| 18:07:30 | mdbooth | Common culprit is popping values from input dicts. | |
| 18:10:40 | sean-k-mooney2 | mdbooth: that depends on the fucntion. if the fucntion returned nothing the modifying the inpu arguments may be correct. like a sort fuction there are other exampels though | |
| 18:10:44 | openstackgerrit | Ed Leafe proposed openstack/nova master: Change RPC for select_destinations() https://review.openstack.org/516707 | |
| 18:10:45 | openstackgerrit | Ed Leafe proposed openstack/nova master: Move the claim_resources method to scheduler utils https://review.openstack.org/511357 | |
| 18:11:00 | edleafe | mriedem: ^^ Got these working. Now starting on your comments on https://review.openstack.org/#/c/511358/ | |
| 18:11:04 | mdbooth | sean-k-mooney2: Right, if the purpose of the function is to modify its input parameters, that's different. | |
| 18:11:13 | sean-k-mooney2 | mdbooth: though i do agreee that in general a fucntion should retrun someting or have sideefect but not both | |
| 18:17:25 | mdbooth | jaypipes: So, I just realised that another reason to use an independent transaction in _create_uuid is that if it were ever called in the future in the context of a read transaction (because the caller is just reading from the db), it's going to be a failure because we can't promote a read transaction to a write transaction. | |
| 18:17:53 | mdbooth | So basically, this only works robustly as long as we continue not using enginefacade | |
| 18:18:28 | jaypipes | mdbooth: all the more reason to use enginefacade, no? :) | |
| 18:18:40 | mdbooth | jaypipes: No, the exact opposite | |
| 18:19:01 | mdbooth | If we were using enginefacade the way it was intended, i.e. to create larger-scoped transactionss | |
| 18:19:08 | mdbooth | We'd be hitting bugs here | |
| 18:19:28 | mdbooth | We're only relatively safe because we don't do that | |
| 18:20:04 | mdbooth | Ideally something which might read a few bdms would have its own read transaction | |
| 18:20:12 | mdbooth | Which covered all of them | |
| 18:20:25 | mdbooth | Then suddenly one of them doesn't have a uuid and we need to do a write | |
| 18:20:36 | mdbooth | This would fail, because we're in a read transaction. | |
| 18:21:00 | mdbooth | If we always do the write in an independent transaction, that doesn't matter | |
| 18:21:32 | mdbooth | Actually, I wonder if we'd hit that in practise... | |
| 18:24:51 | mdbooth | No, we currently still have separate micro-transactions everywhere | |
| 18:26:31 | _ix | Hello again. I was curious about how to best segment my deployment. I'd like some hypervisors running off of rados block devices, and others running off of local ssd. Are availabiltiy zones what I'm looking for? | |
| 18:27:09 | mdbooth | But if, for eg, we annotated BlockDeviceMapping.get_by_volume_id with @reader so that it fetched the joined instance in the same transaction as the bdm, that would be a bug. It would be pretty obtuse, though, because it's not at all obvious that _from_db_object can write | |
| 18:30:11 | _ix | I must be looking for host aggregates. | |
| 18:31:33 | mdbooth | jaypipes: Nah, the comments are longer than the code. | |
| 18:31:58 | mdbooth | s/code/code to fix the problems requiring comments/ | |
| 18:33:50 | sean-k-mooney2 | every time i read bdms i keep wanting to change it to dbms as i assume i reversed the letter because of my dyslexia | |
| 18:38:45 | jeblair | hi, we're looking at having nodepool ask nova for its quota periodically to make it more responsive to changes. does nova cache limit information for users (cores, ram, instances)? if so, what's a typical cache period? | |
| 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 | mdbooth | Except that now everything above it was unnecessarily promoted to writer | |
| 18:44:19 | jaypipes | mdbooth: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L904-L905 | |
| 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 | mdbooth | Well cycle +1, because the online migration runs it too. | |
| 19:11:48 | zzzeek | mdbooth: nova's magical migrations...sure | |
| 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? | |