| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-22 | |||
| 15:39:10 | mriedem | cfriesen: @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED, | |
| 15:39:11 | mriedem | def evacuate(self, context, instance, host, on_shared_storage, | |
| 15:39:11 | mriedem | vm_states.ERROR]) | |
| 15:39:46 | mriedem | cfriesen: you'd likely want to revert the resize to get the instance back to the source host | |
| 15:41:47 | mriedem | mdbooth: +2 on https://review.openstack.org/#/c/242602/ | |
| 15:41:51 | mriedem | despite that terrible blank space | |
| 15:42:04 | mdbooth | mriedem: Thank you, Sir! | |
| 15:43:16 | mdbooth | Hah! I do that when I want to comment branches separately. Hadn't really thought about it too hard, tbh. | |
| 15:43:45 | mdbooth | I guess the comments can also live inside the block | |
| 15:46:10 | mdbooth | artom: So, that concurrency test | |
| 15:46:33 | mriedem | efried_cya_jan: how does one recheck the powervm CI? it's not on the wiki https://wiki.openstack.org/wiki/ThirdPartySystems/IBM_PowerVM_CI | |
| 15:46:34 | artom | mdbooth, right | |
| 15:47:21 | mriedem | "recheck powervm" i guess | |
| 15:47:45 | mdbooth | It sounded like I hadn't convinced you, yet. I could be persuaded to simplify it, but I'm still thinking about that. | |
| 15:47:46 | artom | mdbooth, so, I grok the Python threads stuff - they don't run concurrently, one will run when another yields to do IO or whatever | |
| 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 | |