| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-22 | |||
| 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(). | |
| 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 ;) | |
| 16:23:22 | mriedem | figleaf: working on a devstack patch | |
| 16:24:22 | figleaf | mriedem: yeah, that would hose things | |
| 16:27:56 | artom | mdbooth, maybe it'd be easier if you point out the error in http://paste.openstack.org/show/629605/ ? | |
| 16:29:28 | mdbooth | artom: The error is that the invocations of get_by_instance_uuid overlap | |
| 16:29:45 | mdbooth | The one top left goes all the way to top right | |
| 16:30:01 | mdbooth | There's another invocation of get_by_instance_uuid in the middle of it | |
| 16:30:15 | mdbooth | IOW, they are executing 'concurrently' | |
| 16:31:42 | artom | Heh, maybe discussing ASCII art pseudo sequence diagrams on IRC wasn't the best idea | |
| 16:31:56 | artom | A debugger then... | |
| 16:32:05 | artom | Confessions: I have never used a Python debugger | |
| 16:32:14 | mdbooth | So we've got a big call to get_by_instance_uuid() | |
| 16:32:47 | mdbooth | We stick a mock somewhere in the middle of it which interrupts the flow to call get_by_instance_uuid() again, before continuing with the original call, which still hasn't finished. | |
| 16:34:03 | artom | So far so good. | |
| 16:40:19 | openstackgerrit | Merged openstack/nova master: objects: Add PCI NUMA policy fields https://review.openstack.org/527470 | |
| 16:42:31 | mriedem | figleaf: i think this should do the trick https://review.openstack.org/529857 | |
| 16:47:04 | artom | mdbooth, is that all we're testing? That if one get_by_uuid starts up, but while it's running another get_by_uuid is called, they're get the same value in the end? | |
| 16:47:19 | mdbooth | artom: Yep | |
| 16:47:23 | artom | mdbooth, christ | |
| 16:47:30 | artom | mdbooth, ok no, please get rid of it :) | |
| 16:47:31 | openstackgerrit | Merged openstack/nova master: Make conductor pass and use host_lists https://review.openstack.org/511358 | |
| 16:47:36 | mriedem | woot ^ | |
| 16:48:04 | mriedem | #success nova merged alternate hosts support for server build | |
| 16:48:06 | openstackstatus | mriedem: Added success to Success page | |