| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 17:25:51 | jaypipes | mdbooth: close enough to a public API in my book... | |
| 17:25:53 | mdbooth | And it's clearly called out | |
| 17:26:01 | mdbooth | It's got test_ in the name ;) | |
| 17:26:57 | jaypipes | mdbooth: sorry, I just disagree on that. from a style and a code structure perspective, I think it's better to explicitly create test fixtures and expectations within the test itself. | |
| 17:27:00 | mdbooth | jaypipes: We've also similar in a bunch of places. | |
| 17:27:19 | mdbooth | jaypipes: Meh, ok. | |
| 17:27:21 | jaypipes | mdbooth: can you point me to something similar please? | |
| 17:27:50 | mdbooth | jaypipes: I'm thinking class reset methods | |
| 17:28:01 | mdbooth | functions which only exist so they can be unit tested | |
| 17:28:06 | openstackgerrit | Chhavi Agarwal proposed openstack/nova master: Volume detach should have instance uuid https://review.openstack.org/511804 | |
| 17:28:07 | mdbooth | functions which only exist so that can be mocked | |
| 17:28:28 | jaypipes | mdbooth: that's a separate function. it's not changing a real function just for the purpose of testing. | |
| 17:28:44 | mdbooth | I'm pretty sure I've also seen test only args | |
| 17:28:51 | jaypipes | mdbooth: and we also discourage creating functions that exist only to be mocked. | |
| 17:28:52 | dansmith | I too am interested, | |
| 17:29:01 | mdbooth | It's a wart, but it allows code reuse. | |
| 17:29:02 | dansmith | because I don't think that we do that anywhere I'm familiar with | |
| 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 | |