Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-06
17:29:16 sean-k-mooney2 mdbooth: test only args kindo of defets the reason for testing
17:29:25 mdbooth Otherwise we just have to duplicate code
17:29:25 dansmith yeah
17:29:30 sean-k-mooney2 mdbooth: you are not testing what runs in production
17:29:47 mdbooth sean-k-mooney2: The use is to create something only for use in testing
17:29:53 mdbooth So no, it doesn't run in production :)
17:30:13 mdbooth sean-k-mooney2: In this case, it's to tweak a function to not default something which was formally not defaulted.
17:30:57 mdbooth Anyway, if it's not going to pass muster I'll change it
17:32:26 sean-k-mooney2 mdbooth: can you send me a link?
17:32:48 mdbooth https://review.openstack.org/#/c/242603/25/nova/db/sqlalchemy/api.py
17:33:53 mdbooth That function is small enough the cut/pasting it into a test isn't entirely evil
17:35:06 sean-k-mooney2 and why do you want to not see the uuid automatically?
17:35:27 mdbooth sean-k-mooney2: So as to test upgrade of legacy objects which don't have it
17:36:23 sean-k-mooney2 and you cant mock the call to uuidutils.generate_uuid() in that case to retrun None?
17:36:54 mdbooth sean-k-mooney2: Cut/paste is less evil than that.
17:47:51 mdbooth dansmith: jaypipes So, I'm going to recut that second patch. No happy faces for https://review.openstack.org/#/c/242602/24, though ?
17:47:59 mdbooth While I'm respinning anyway, that is.
17:48:17 mdbooth It's the patch immediately before.
17:52:41 jaypipes mdbooth: gimme a few
17:53:00 mdbooth jaypipes: Thanks.
17:53:26 mdbooth jaypipes: That patch version is mostly dansmith with some stuff deleted that we don't need any more.
17:53:48 jaypipes mdbooth: in that case, automatic -2 from me
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 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.

Earlier   Later