| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-22 | |||
| 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: Add PCI NUMA policies https://review.openstack.org/527472 | |
| 16:00:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Modify signature of _filter_non_requested_pfs https://review.openstack.org/527473 | |
| 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 | 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:47 | finucannot | leakypipes: I assume the irony of that has already been pointed out multiple times | |
| 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 | 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:35 | mdbooth | No, that's exactly what I'm testing. | |
| 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 | 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:19 | mdbooth | However, I could be convinced to test only the simpler consecutives calls to _create_uuid() | |
| 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(). | |
| 16:15:35 | mdbooth | So we have a main thread of execution, which is interrupted by another thread of execution. | |
| 16:17:14 | mdbooth | artom: You have absolutely convinced me to simplify that test ;) | |
| 16:17:32 | artom | mdbooth, I really am just trying to understand, honest :) | |
| 16:17:35 | mdbooth | The most important aspect of it was to test that the compare-and-swap works. I can do that without the mocking. | |
| 16:17:58 | artom | mdbooth, think of it as teaching me :) | |
| 16:19:53 | figleaf | mriedem: you can't remove a zip tie without cutting it (and usually the hose) | |
| 16:20:01 | artom | mdbooth, a mock side_effect... | |
| 16:20:09 | artom | Maybe that's the part I'm not getting | |
| 16:20:17 | artom | Does it still call the original function? | |
| 16:20:31 | artom | And call the side_effect before/after/at the same time? | |
| 16:20:41 | mdbooth | It calls the side effect both times | |
| 16:20:45 | mdbooth | That's why flip is required | |
| 16:20:49 | artom | mdbooth, no, in general I mean | |
| 16:21:03 | mdbooth | flip makes it call the race first time only | |
| 16:21:16 | mdbooth | That prevents infinite recursion by the race functino | |
| 16:21:21 | artom | Like, if I mock foo() and mock.side_effect = bar, and I call foo(), does the real foo() still get called, or only bar()? | |
| 16:21:37 | mdbooth | No, only the side effect is called | |
| 16:21:40 | mdbooth | It's a bad name | |
| 16:21:42 | mriedem | figleaf: found a problem in our devstack setup for the alternate hosts stuff :) | |
| 16:21:50 | artom | mdbooth, So it's effectively replacing the mocked object | |
| 16:21:53 | figleaf | oh joy! | |
| 16:22:07 | mdbooth | It's replacing the mocked function in this case | |
| 16:22:19 | mdbooth | That's why we explicitly store a reference to the original | |
| 16:22:22 | artom | mdbooth, gotcha. Back to looking at the code | |
| 16:22:27 | mdbooth | So we can still call it. | |
| 16:22:36 | mdbooth | artom: I'm really going to simplify it. | |
| 16:22:46 | mdbooth | I no longer think it's worth it myself ;) | |
| 16:22:48 | artom | mdbooth, sure, but I still want to understand this | |
| 16:22:49 | mriedem | figleaf: pretty simple | |
| 16:22:50 | mriedem | http://logs.openstack.org/89/527289/1/check/ironic-tempest-dsvm-ipa-wholedisk-agent_ipmitool-tinyipa-multinode/570a3c9/logs/screen-n-cond-cell1.txt.gz#_Dec_21_23_49_28_813934 | |
| 16:23:09 | mriedem | figleaf: the ironic job failed the first node and was rescheduling, but the cell conductor couldn't talk to placement b/c placement isn't configured in nova_cell1.conf in devstack | |
| 16:23:17 | artom | mdbooth, I shall wear your down with the stubbornness of my ignorance ;) | |