| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-22 | |||
| 15:48:03 | artom | mdbooth, it's not so much convincing, as explaining :) | |
| 15:48:32 | artom | mdbooth, what's not clear to me, and it may be ignorance of Python's internals on my part, is how your code makes sure there are two threads | |
| 15:48:37 | mdbooth | artom: Well your point is around whether the complexity is worth it, right? | |
| 15:48:56 | mdbooth | I'm confident it's a valid test. I could be persuaded that a simpler test might be better, though. | |
| 15:49:08 | mdbooth | There aren't 2 threads. | |
| 15:49:22 | mdbooth | There's only 1 thread, but it acts like 2 threads. | |
| 15:49:45 | mdbooth | We use mock to intercept a function call in the 'main' thread. | |
| 15:50:05 | mdbooth | When the main thread executes that function call, we interrupt it and do something else first, before continuing. | |
| 15:50:23 | mdbooth | Does that make more sense? | |
| 15:50:55 | mriedem | seems pretty paranoid for something that we've already established a pattern of in several other objects | |
| 15:51:06 | mriedem | or are you actually seeing this race happening with something like the cellsv1 job? | |
| 15:51:40 | artom | mdbooth, let me look at the code again | |
| 15:51:53 | artom | I still can't wrap my head around how purely sequential execution can test a race | |
| 15:51:58 | mdbooth | mriedem: It's more that I can see the bug and I fixed it, and it's not *that* complicated. | |
| 15:52:25 | mdbooth | mriedem: artom is trying to get his head round the test, and I've never yet managed to write a unit test for a race which was easy to read. | |
| 15:52:31 | artom | If it's just about calling _create_uuid twice, surely can do that without the whole flip/race thing | |
| 15:53:28 | mdbooth | artom: That's where I could be persuaded. Except that my test is 1 step up from that. | |
| 15:54:00 | mdbooth | My test asserts that if the race happens whilst reading the bdm object, it will work fine. | |
| 15:54:20 | mdbooth | Your test would be much simpler, and we could possibly agree it's sufficient. | |
| 15:54:24 | artom | But... the race can only happen when writing | |
| 15:54:55 | artom | I guess if you go up one stop from that, it two reads happen at the same time on the same uuid-less BDM, both will attempt to write a UUID | |
| 15:54:59 | artom | *if two | |
| 15:55:02 | mdbooth | No, this is weird. If we *read* a bdm object with no uuid we create one before returning it. | |
| 15:55:28 | mdbooth | So it's a read operation, but we might have to migrate a legacy object during it. | |
| 15:55:32 | artom | Right, but the actual race is the writing part | |
| 15:55:37 | mdbooth | Yes | |
| 15:56:01 | artom | That's what the code, does, right? Read, if there's no UUID, create one and save it, then return | |
| 15:56:02 | mdbooth | My test is at the level of the read operation | |
| 15:56:04 | mdbooth | Your test would just be on the write bit | |
| 15:56:37 | mdbooth | artom: Yep. Notice there's a compare-and-swap in there, which avoids a race. | |
| 15:57:18 | mdbooth | I guess the most important thing is to test the compare-and-swap, which simply executing _create_uuid manually twice would do. | |
| 15:57:24 | mdbooth | The test would also be easier to read. | |
| 15:57:39 | artom | Ah, right, compare and swap is atomic at the DB level, right? | |
| 15:57:45 | mdbooth | Yes | |
| 15:58:02 | mdbooth | There are a couple of db-related hoops in that function which ensure it's atomic. | |
| 15:58:14 | openstackgerrit | Merged openstack/nova stable/ocata: Use instance.project_id when creating request specs for old instances https://review.openstack.org/529387 | |
| 15:58:24 | artom | So if there are two of more getting compare_and_swaps getting to the DB at the same time, only the first one will succeed | |
| 15:58:37 | artom | So, the DB-write race is handled for us | |
| 15:58:42 | artom | I may have been thinking about this wrong | |
| 15:58:43 | mdbooth | Correct. | |
| 15:59:27 | artom | So is there actually a race then? If we try a compare_and_swap and it fails, if we read after that, we're guaranteed to get the correct UUID | |
| 15:59:43 | artom | A read race, I should say | |
| 15:59:51 | mdbooth | There is no read race. | |
| 15:59:59 | mdbooth | Not in this patch, anyway. | |
| 16:00:04 | artom | ... | |
| 16:00:10 | artom | So what are we testing then? | |
| 16:00:16 | mdbooth | However, I think the original one just wrote the uuid to the db. | |
| 16:00:28 | mdbooth | That is a race, and if you executed my test against that version, it would fail. | |
| 16:00:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Modify signature of _filter_non_requested_pfs https://review.openstack.org/527473 | |
| 16:00:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add PCI NUMA policies https://review.openstack.org/527472 | |
| 16:01:08 | finucannot | leakypipes, bauwser: Voilà ^^ | |
| 16:01:13 | mdbooth | In fact, I think the original version didn't write it immediately at all, just generated it. | |
| 16:01:16 | leakypipes | mriedem: yeah, dinner was great. dishwasher is leaking, though... | |
| 16:01:44 | mdbooth | So there was a delay of unknown size between generation of the value and writing it to the db without a compare-and-swap. | |
| 16:01:45 | mriedem | leakypipes: "dishwasher" isn't code for one of the pugs is it? | |
| 16:01:47 | finucannot | leakypipes: I assume the irony of that has already been pointed out multiple times | |
| 16:01:47 | leakypipes | mriedem: so after $400 electrician bill I now have another $200 bill for the installers from Lowe's to come day after christmas to fix it. | |
| 16:01:53 | leakypipes | finucannot: yes sir. | |
| 16:02:00 | leakypipes | mriedem: no. | |
| 16:02:12 | artom | mdbooth, oh, so you're not testing that your code in the latest patchset handles a race properly | |
| 16:02:19 | mdbooth | artom: Yes. | |
| 16:02:29 | mdbooth | Erm... | |
| 16:02:33 | artom | Yes you're not? | |
| 16:02:35 | mdbooth | No, that's exactly what I'm testing. | |
| 16:02:35 | mriedem | leakypipes: oh lowes installers huh? i had to have lowes guys come back to fix our washing machine install which sprayed water all over the (thankfully unfinished) storage room in our basement | |
| 16:02:42 | mriedem | turns out zip ties are important for the drain hose | |
| 16:02:55 | artom | But you said there's no race in this patch | |
| 16:03:23 | mdbooth | The test asserts that the code correctly handles the situation where 2 threads both read a NULL uuid from the db, and both then try to populate it. | |
| 16:03:42 | mdbooth | The test passes. | |
| 16:03:51 | mdbooth | That doesn't mean there's no reason for the test, though. | |
| 16:04:59 | mdbooth | As it happens, the code will also work if the race happens during the db transaction, or on commit because both transactions occurred on different masters of a multi-master galera cluster. | |
| 16:05:04 | artom | SO I guess step one would be to remove all mentions of race from that test | |
| 16:05:07 | mdbooth | I didn't write tests for those, though, because.... | |
| 16:05:24 | mdbooth | I would only understand them very briefly myself. | |
| 16:05:39 | mdbooth | artom: No, because it's a race ;) | |
| 16:05:52 | mdbooth | It simulates a race | |
| 16:06:04 | mdbooth | It simulates 2 threads with 1 thread. | |
| 16:06:12 | artom | I'm going to look up the definition of race, I swear to God ;) | |
| 16:06:36 | mdbooth | A race is a bug caused by execution timing. | |
| 16:07:02 | mdbooth | It doesn't require real concurrency. | |
| 16:07:23 | artom | Right, when the outcome is determined by the order in which things are executed | |
| 16:07:47 | artom | The code is written so as to *not have that* | |
| 16:08:04 | artom | Because 1. compare_and_swap 2. if it fails, we handle it gracefully and read the correct value | |
| 16:08:08 | mdbooth | Yes, although order may be A->B, B->A, or A(a bit)->B(all of it)->A(the rest) | |
| 16:08:40 | figleaf | mriedem: not zip ties. Use these: https://images-na.ssl-images-amazon.com/images/I/71%2BzOxf-hBL._SX425_.jpg | |
| 16:09:19 | mdbooth | However, I could be convinced to test only the simpler consecutives calls to _create_uuid() | |
| 16:09:19 | artom | So given two reads A and B, we need a test that makes sure that, regardless of the sequence in which they're (partially) executed, the UUID read at the end is the same for both | |
| 16:09:39 | mdbooth | With a note that this simulates a race in the calling code, as this will only ever happen if there was a race. | |
| 16:10:07 | mdbooth | Rather than creating the race itself with fancy mocks. | |
| 16:10:15 | mriedem | figleaf: i used this https://s7d1.scene7.com/is/image/BedBathandBeyond/45894242895794p?$478$ to call lowes and tell them to get their guys out to fix the shit | |
| 16:10:48 | mriedem | ooo btw, where is my xmas list? | |
| 16:10:49 | mriedem | https://www.saksfifthavenue.com/main/ProductDetail.jsp?PRODUCT%3C%3Eprd_id=845524447125273&site_refer=CSE_GGLPRADS001&gclid=Cj0KCQiA9_LRBRDZARIsAAcLXjfz8bCutBnNrpZ-bKlG_1RRPKyVGX4G1WC1vjzOtoO6K81rPpJXYrgaAuXkEALw_wcB&gclsrc=aw.ds | |
| 16:11:01 | mdbooth | artom: Think of it like this. We've got code that does: if uuid is not populated: populate_uuid() | |
| 16:11:05 | mriedem | if i were a corporation with some sweet tax cuts i could buy that phone | |
| 16:11:27 | mriedem | oh it's not an actual phone | |
| 16:11:30 | mriedem | it's a makeup thingy | |
| 16:11:42 | mdbooth | artom: We're simulating the case where the above code is interrupted mid flow, so by the time populate_uuid is called, it's actually already populated. | |
| 16:11:51 | mdbooth | Even though we just checked that it wasn't. | |
| 16:12:27 | artom | mdbooth, ok, point me to the "interrupted mid flow" bit - how does it happen in the test? | |
| 16:14:34 | mdbooth | artom: The first time we call _create_uuid() (populate_uuid in my example above), we interrupt the flow by calling race(). race() reads the bdm itself, which populates the uuid, then continues execution of the main thread by calling orig_create_uuid(). | |