| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-29 | |||
| 18:17:25 | dansmith | / | |
| 18:18:46 | dansmith | kashyap: that works and is done elsewhere, but I also don't care that you assert that the warn was called, but that you executed that code. If you had a typo in anything inside that if, you wouldn't hit it in your tests unless you cause the if to be true, which is the real goal | |
| 18:19:11 | openstackgerrit | Artom Lifshitz proposed openstack/nova-specs master: NUMA-aware live migration https://review.openstack.org/552722 | |
| 18:19:42 | kashyap | dansmith: Yep; the typo inside the 'if' is taken care by the config "choices", right | |
| 18:19:49 | dansmith | kashyap: no | |
| 18:19:50 | kashyap | That's why we moved it there | |
| 18:19:52 | dansmith | no | |
| 18:20:00 | kashyap | Err, I mean, let me rephrase | |
| 18:20:07 | dansmith | if condition: LOG.warrrrrning('foo') | |
| 18:20:17 | dansmith | that would not fail tests unless you cause condition to be true | |
| 18:20:18 | jaypipes | artom: aaaaannndddd... +2. :) | |
| 18:20:32 | artom | jaypipes, yey! Thank you | |
| 18:21:01 | dansmith | kashyap: no failure: https://pastebin.com/A2muKWxU | |
| 18:21:24 | mriedem | dansmith: is that "you want to" or "you going to"? | |
| 18:21:27 | artom | I was worried we'd "hit" an asymptote and never get there ;) | |
| 18:21:35 | mriedem | i can't parse that southern drawl | |
| 18:21:48 | kashyap | dansmith: Okido; noted. | |
| 18:21:49 | dansmith | mriedem: you want to, although I assume you're going to | |
| 18:22:18 | mriedem | dansmith: i already -1ed for the other stuff | |
| 18:22:24 | mriedem | oh you replied | |
| 18:22:25 | mriedem | sec | |
| 18:24:26 | kashyap | dansmith: Alright; to make you double-happy, I'll slap in another test for the 'host-passthrough'. As we have two conditions there | |
| 18:24:54 | dansmith | kashyap: if it's not related to your change, then save it for another patch | |
| 18:25:05 | dansmith | kashyap: but yes as I noted, we don't have tests for the other conditions there | |
| 18:25:07 | kashyap | dansmith: It's actually related, because: | |
| 18:25:09 | mriedem | dansmith: replied in the docs patch | |
| 18:25:32 | dansmith | mriedem: tanks | |
| 18:25:39 | mriedem | that's also why i called out my specific points for the -1 | |
| 18:25:41 | openstackgerrit | Dan Smith proposed openstack/nova master: Documentation for tenant isolation with placement https://review.openstack.org/557490 | |
| 18:25:45 | mriedem | since the other was just discussion | |
| 18:25:56 | kashyap | dansmith: I wasn't sure if you wanted to me to test for *both* 'host-model' + 'host-passthrough' in this conditional: | |
| 18:25:58 | mriedem | coffee talk | |
| 18:25:59 | kashyap | + elif ((mode == "host-model" or mode == "host-passthrough" or | |
| 18:26:02 | kashyap | + model is None) and extra_flags): | |
| 18:26:02 | dansmith | mriedem: heh | |
| 18:26:23 | kashyap | dansmith: The second test would just identical, with the difference being: s/host-mode/host-passthrough/ | |
| 18:26:26 | dansmith | kashyap: oh I see, I mostly care that the condition is true (there's only one condition) so you run the stuff in the body of the if.. that's all | |
| 18:26:43 | kashyap | Okido; I'll stick with one test | |
| 18:26:52 | kashyap | And I get to write more tests when we lift the choices in future. | |
| 18:26:53 | dansmith | kashyap: it doesn't matter that you check that == works a second time, we can trust python on that :) | |
| 18:26:58 | kashyap | LOL | |
| 18:27:23 | dansmith | kashyap: although mriedem is a stickler, he might want both flags checked, so it's not wrong to do both | |
| 18:27:25 | kashyap | One more: a preference question -- do you care betwee assertTrue vs assert_called_with | |
| 18:27:35 | dansmith | assertTrue only for booleans | |
| 18:27:45 | kashyap | I apprecite his stickliness (is it a word?). Because he did caught somethings I didn't like in my phrasing | |
| 18:27:55 | kashyap | And I was getting sick of reading my own text again and again | |
| 18:27:56 | dansmith | assertTrue(foo.called) is okay with me for LOG.warning just to make sure it was called, | |
| 18:28:08 | dansmith | but mriedem tends to do a looooot of string compare assertions that I do not | |
| 18:28:21 | dansmith | so he would do assert_called_with | |
| 18:30:15 | mriedem | assertTrue(foo.called) is so 2015 | |
| 18:30:22 | mriedem | foo.assert_called() | |
| 18:30:28 | mriedem | foo.assert_not_called() | |
| 18:31:15 | mriedem | just because i commented on the release note, does that mean you expect me to go back and re-review this all again?! | |
| 18:31:40 | kashyap | mriedem: No. I'll actually nicely summarize in the main comment as to what all I addressed. | |
| 18:31:59 | mriedem | that can't be trusted | |
| 18:32:03 | kashyap | LOL | |
| 18:32:05 | kashyap | E.g. | |
| 18:32:06 | kashyap | PS21: | |
| 18:32:06 | kashyap | - Undid the whitespace change in driver.py | |
| 18:32:07 | kashyap | - Changed the 'exception' to a LOG.warning in driver.py | |
| 18:32:07 | kashyap | - Added an additional test in test_driver.py | |
| 18:32:08 | kashyap | - Clarify the release note that we're talking about libvirt where appropriate | |
| 18:32:12 | kashyap | - Remove the confusing comment about a future change & instead update the 'conf' file | |
| 18:32:14 | kashyap | [...] | |
| 18:32:17 | mriedem | omg | |
| 18:32:20 | kashyap | Of course you should review, and not trust me | |
| 18:32:23 | mriedem | you know you can diff patch sets in gerrit right? | |
| 18:32:27 | kashyap | I know; just kidding | |
| 18:32:34 | mriedem | i'm not sure you are, | |
| 18:32:39 | mriedem | you're used to the ML style reviews in qemu | |
| 18:32:44 | kashyap | mriedem: Damn | |
| 18:32:45 | kashyap | You caught me | |
| 18:34:04 | kashyap | As you caught me, that's indeed a thing in ML land; that goes under the three dashes below a patch (---): which *won't* go into Git commit, obviously | |
| 18:34:17 | kashyap | s/below a patch/below the main commit message/ | |
| 18:35:04 | openstackgerrit | Arvind Nadendla proposed openstack/nova master: Update ImageMetaProp object to expose traits https://review.openstack.org/557795 | |
| 18:35:55 | melwitt | stephenfin: I've been meaning to ask you if you could take a look at this https://bugs.launchpad.net/nova/+bug/1750672 it's a failure to be able to generate nova docs under py3. something to do with sphinx and we couldn't tell if it's a blockdiag bug or if it's something we need to fix on our side | |
| 18:35:55 | openstack | Launchpad bug 1750672 in OpenStack Compute (nova) "failure to generate Nova's doc in Python 3.6" [Medium,Confirmed] | |
| 18:36:14 | mriedem | dansmith: now that looks pretty http://logs.openstack.org/90/557490/8/check/build-openstack-sphinx-docs/413d6d9/html/admin/configuration/schedulers.html#aggregates-in-placement | |
| 18:36:27 | openstackgerrit | melanie witt proposed openstack/nova-specs master: Document Rocky review priorities https://review.openstack.org/557796 | |
| 18:37:52 | dansmith | mriedem: hell to the yeah | |
| 18:39:59 | dansmith | fwiw, | |
| 18:40:06 | dansmith | runways are doing a good job of keeping me jumpin' | |
| 18:40:25 | melwitt | like, jumpin' for joy? | |
| 18:40:42 | dansmith | like jumpin' busy responding to all the feedback firehose | |
| 18:40:55 | melwitt | ah yeah | |
| 18:41:29 | jmlowe_ | mriedem: looks like _update_ports_for_instance does update device_owner while _update_port_binding_for_instance doesn't | |
| 18:41:49 | jmlowe_ | no chance of confusion there! | |
| 18:43:22 | mriedem | jmlowe_: get familiar enough with the nova.network.neutronv2.api.API code, and you'll find there are at least 3 similarly named methods for about every operation that all do slightly different things and are all called in tightly-coupled ways from different places in nova | |
| 18:43:25 | mriedem | it's, great | |
| 18:44:21 | mriedem | see the amount of talking-to-myself i've done in https://review.openstack.org/#/c/551371/ | |
| 18:46:12 | jmlowe_ | bug 1759924 is away | |
| 18:46:12 | openstack | bug 1759924 in OpenStack Compute (nova) "Port device owner isn't updated with new host availability zone during unshelve" [Undecided,New] https://launchpad.net/bugs/1759924 | |
| 18:46:51 | mriedem | thanks | |
| 18:48:18 | jmlowe_ | I need about 6 more cups of coffee before I can muster the strength to propose a half assed copy and paste patch | |
| 18:49:18 | jmlowe_ | very tempted to just do the one line search opts fix because why do it the right way when you can do it with sed | |
| 18:50:04 | kashyap | mriedem: That's nice, "consciousness stream" writing on the review. (If only there's an easier for people to "discover" easily all the sluething you've done.) | |
| 18:50:15 | kashyap | (Barring you having to mention here on IRC, live.) | |
| 18:50:23 | openstackgerrit | Merged openstack/nova stable/pike: Save admin password to sysmeta in libvirt driver https://review.openstack.org/550498 | |
| 18:50:30 | openstackgerrit | Merged openstack/nova stable/pike: Add functional regression test for bug 1746509 https://review.openstack.org/555058 | |
| 18:50:30 | openstack | bug 1746509 in OpenStack Compute (nova) pike "TypeError: Can't upgrade a READER transaction to a WRITER mid-transaction" [High,In progress] https://launchpad.net/bugs/1746509 - Assigned to melanie witt (melwitt) | |