| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-22 | |||
| 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 | |
| 16:48:11 | artom | mdbooth, I'd see the point if we let them both run to completion in different processes | |
| 16:48:27 | artom | mdbooth, but in the end, all your test does it call _create_uuid twice in successions | |
| 16:48:29 | mdbooth | artom: They do both run to completion! | |
| 16:48:35 | mdbooth | In effectively different processes. | |
| 16:48:52 | artom | ... | |
| 16:49:07 | artom | mdbooth, but when the second one is called via the mock, the first's flow is interreupted | |
| 16:49:11 | artom | Until the second one returns | |
| 16:49:12 | artom | No? | |
| 16:49:13 | mdbooth | By mocking _create_uuid specifically, it triggers an execution order which causes a race | |
| 16:49:30 | figleaf | mriedem: It's a xmas miracle! | |
| 16:49:38 | openstackgerrit | Merged openstack/nova master: doc: update supported drivers for cpu topology https://review.openstack.org/529294 | |
| 16:49:54 | openstackgerrit | Matt Riedemann proposed openstack/python-novaclient master: Add support for the 2.57 microversion https://review.openstack.org/528128 | |
| 16:50:14 | mdbooth | artom: Yes. That's an execution order which would cause incorrect behaviour if we didn't handle it. | |
| 16:50:34 | openstackgerrit | Matthew Booth proposed openstack/nova master: Make BlockDeviceMapping object support uuid https://review.openstack.org/242603 | |
| 16:50:35 | openstackgerrit | Matthew Booth proposed openstack/nova master: DriverBlockDevice: make subclasses inherit _proxy_as_attr https://review.openstack.org/524167 | |
| 16:50:35 | openstackgerrit | Matthew Booth proposed openstack/nova master: Add an online migration for BDM.uuid https://review.openstack.org/525599 | |
| 16:50:36 | openstackgerrit | Matthew Booth proposed openstack/nova master: Expose BDM uuid to drivers https://review.openstack.org/529037 | |
| 16:50:42 | mdbooth | artom: However, it's gone ^^^ ;) | |
| 16:50:51 | artom | mdbooth, dammit | |
| 16:51:03 | artom | mdbooth, I still want to understand you | |
| 16:51:17 | mdbooth | artom: It's a good test, but a totally agree that it's more complicated that it needs to be | |
| 16:52:00 | mdbooth | Anyway, I'm going to run for the hills! | |
| 16:52:13 | mdbooth | Merry Christmas, all! See you in the New Year. | |
| 16:55:06 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: libvirt: create vGPU for instance https://review.openstack.org/528832 | |
| 16:57:49 | artom | mdbooth, enjoy your ugly sweaters ;) | |
| 17:12:48 | mriedem | figleaf: heh my bash fu always fails the first few times | |
| 17:14:25 | figleaf | mriedem: don't feel bad - I felt uneasy +1'ing that because my bash-fu is so weak | |
| 17:14:37 | figleaf | it looked logical, thougn | |
| 17:14:39 | figleaf | though | |
| 17:15:29 | mriedem | it does do the correct thing for nova_cell1.conf | |
| 17:15:30 | mriedem | so that's good | |
| 17:19:22 | mriedem | someone was mentioning issues with the NumInstances filter yesterday, and i got thinking about that one, if you ran multiple filter scheduler processes, that filter would be totally racey | |
| 17:19:38 | mriedem | depending on which process gets a request, or if both are processing requests at the same time for the same host | |
| 17:20:26 | mriedem | if the limit is 10 and you've got 9 on a host already, and both workers get a request at the same time, they'd both think there is room for one more and could choose it | |
| 17:20:32 | mriedem | placement doesn't fix that | |