| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 15:29:01 | cdent | bauzas: there are some tests like that later in the stack | |
| 15:29:17 | cdent | but yes, having them not with the actual code change is a bit disconcerting | |
| 15:29:39 | bauzas | that's what we ask when reviewing | |
| 15:30:04 | figleaf | What's the point of testing behavior that you know is wrong/incomplete? | |
| 15:30:27 | cdent | fair point | |
| 15:30:36 | cdent | but what's the point of merging code that you know is wrong/incomplete? | |
| 15:30:44 | cdent | I thought changes were supposed to be atomic? | |
| 15:31:01 | cdent | (I don't agree with that position, but I thought that was what was expected) | |
| 15:31:26 | bauzas | figleaf: you're kidding, hopefully ? | |
| 15:31:27 | figleaf | cdent: well, I agree with that. I always thought that the imperative to keep changes small was taken too seriously | |
| 15:31:36 | figleaf | bauzas: no | |
| 15:31:50 | bauzas | I probably misunderstood your question then | |
| 15:32:13 | figleaf | If you are coding something that you a) know is not correct and b) plan on changing in the dependent patch, the test should be with the code that makes it work | |
| 15:32:53 | bauzas | figleaf: negative coverage can be small https://github.com/openstack/nova/blob/master/nova/tests/functional/db/test_resource_provider.py#L678-L694 | |
| 15:33:22 | figleaf | How big should a patch be? https://www.brainyquote.com/quotes/quotes/j/jdsaling184470.html | |
| 15:34:29 | bauzas | figleaf: so, just to make it clear, when you code and you know you're introducing bugs, you prefer to leave as it is for the proposal, and fix it later ? | |
| 15:34:53 | bauzas | anyway, it's late for me | |
| 15:34:53 | mriedem | so write the test asserting you know it's going to fail somehow, and then the later change fixes it once the code is in place - we do that with functional regressions bug tests all the time | |
| 15:35:03 | mriedem | write the test to show the bug, then the patch that fixes the bug also changes the test | |
| 15:35:16 | bauzas | mriedem: that happens when we merged code | |
| 15:35:18 | figleaf | bauzas: I prefer not to have patches that don't work. | |
| 15:35:32 | bauzas | mriedem: not when it's in the review state | |
| 15:35:37 | bauzas | unless it's super critical | |
| 15:35:38 | mriedem | none of this is used yet right? | |
| 15:35:48 | figleaf | Given that jay is saying this is strictly to reduce review size, I'm fine with the test being with the code that actually works | |
| 15:35:57 | bauzas | I agree, but I'd by far clarify *how* it would be used | |
| 15:36:12 | mriedem | if none of this is used, and it's all split up to ease the review burden, then yeah doing the tests at the end seems fine | |
| 15:36:29 | mriedem | we write new apis that plumb in the lower levels and we do'nt know that all works until we write the api tests at the end | |
| 15:36:32 | mriedem | e.g. tempest tests | |
| 15:36:39 | bauzas | and then we fix | |
| 15:36:51 | bauzas | either way, I'm +1 | |
| 15:36:55 | mriedem | so you want a single giant patch? | |
| 15:37:03 | mriedem | granted, i haven't looked at any of the latest series | |
| 15:37:14 | bauzas | so, seems we're diverting | |
| 15:37:27 | bauzas | I'm not advocating for large chunks of code | |
| 15:37:29 | mriedem | if it's complicated, we're going to be fixing parts of it later regardless | |
| 15:37:41 | bauzas | I'm just pointing out that this method is probably underperformant | |
| 15:37:43 | mriedem | i'm sure there is some law about LOC to bugs ratio | |
| 15:38:06 | bauzas | my original concern is not coverage | |
| 15:38:19 | bauzas | my concern is just, we already have code that does that | |
| 15:38:27 | bauzas | and we're adding a fresh new code | |
| 15:38:40 | bauzas | that new code doesn't include the multi-RC thing | |
| 15:38:59 | bauzas | so, fine by me with merging that one, but then I'm pretty sure we'll require to modify it later | |
| 15:39:19 | dansmith | that goes for every line of code we merge | |
| 15:39:22 | cdent | bauzas: everyone knows and accept that | |
| 15:39:25 | dansmith | I don't understand the problem at all | |
| 15:39:35 | cdent | this code is written with the intent of being modified later | |
| 15:39:36 | figleaf | yeah, we had this discussion two days ago | |
| 15:39:58 | bauzas | sorry, French card played here _o | |
| 15:40:00 | bauzas | \o | |
| 15:41:24 | bauzas | dansmith: the plan is to integrate it with get_all_by_filters() right? | |
| 15:41:43 | bauzas | dansmith: so, it's possibly asking for two or more resource classes | |
| 15:42:17 | bauzas | dansmith: if so, we need to call _get_providers_with_shared_capacity() twice, correct? | |
| 15:42:23 | bauzas | for each RC | |
| 15:43:11 | dansmith | I don't know that that's the plan | |
| 15:43:17 | dansmith | cdent and figleaf are the ones that said the knew of plans to change something down the road | |
| 15:44:23 | bauzas | when looking at the series, I can't find where that method is called, so I'm guessing given the commit msg | |
| 15:44:27 | cdent | bauzas: the end goal is to end up with optimized sql but to get there in steps so that more people understand what the queries are actually doing. if we did that in one step, it would be even more incomprehensible than it is now | |
| 15:44:40 | dansmith | that ^ | |
| 15:44:45 | dansmith | I mean, | |
| 15:44:49 | dansmith | the end goal is to merge this EVER. | |
| 15:45:32 | cdent | bauzas: which method do you mean when you say "that method". If you clear that up, I'll point you to where it will be used | |
| 15:45:57 | bauzas | then, I'm possibly biased by the fact I wrote the SQL-fu for get_all_by_filters() so I'm seasoned with the SQL statement (and the SQLA calls) he wrote | |
| 15:47:47 | bauzas | cdent: sorry, I wanted to explain that I don't see where _get_providers_with_shared_capacity() is used by get_all_by_filters() | |
| 15:47:52 | bauzas | cdent: in the series | |
| 15:48:44 | cdent | line 909 https://review.openstack.org/#/c/461574/9/nova/objects/resource_provider.py | |
| 15:49:09 | cdent | and yes, for now it is once per rc_id | |
| 15:50:43 | bauzas | cdent: I'm actually confused by https://review.openstack.org/#/c/461574/9/nova/objects/resource_provider.py | |
| 15:51:51 | bauzas | anyway, seems you're feeling me as a nitpicker, and that's the end of the week | |
| 15:52:14 | bauzas | so... okay, fair enough, let's merge that code | |
| 15:53:19 | openstackgerrit | Stephen Finucane proposed openstack/nova master: conf: Rename qemu_enable_gluster_storage_driver https://review.openstack.org/447091 | |
| 16:03:30 | mdbooth | johnthetubaguy: Got a second? I'm trying to work out where in the live migration flow nova tells cinder that a volume is going to be attached from a new host | |
| 16:03:43 | mdbooth | I can't find it, though. Is that because it doesn't exist, or because I'm blind? | |
| 16:04:01 | openstackgerrit | Alex Xu proposed openstack/nova master: Totally freeze the extension_info API https://review.openstack.org/462825 | |
| 16:05:44 | alex_xu | sfinucan: thanks for the review :) | |
| 16:06:16 | sfinucan | alex_xu: No problemo 👍 | |
| 16:07:46 | mriedem | mdbooth: pre_live_migration in the virt driver | |
| 16:07:56 | mriedem | mdbooth: that's run on the destination host | |
| 16:08:05 | mriedem | source compute rpc casts to the destination compute | |
| 16:08:40 | mdbooth | mriedem: So that calls vol_driver.connect_volume | |
| 16:08:46 | mriedem | mdbooth: sorry rpc call | |
| 16:09:02 | mriedem | mdbooth: yeah loops over the bdms and calls self._connect_volume(connection_info, disk_info) | |
| 16:09:12 | mdbooth | But I thought (and here I expect to find I'm wrong) that cinder didn't allow connections from unexpected hosts | |
| 16:09:26 | mdbooth | So I was expecting to see a cinder call in there | |
| 16:09:55 | mdbooth | That's just attempting to connect directly to the storage, and expects to be allowed already, right? | |
| 16:11:28 | mriedem | i'm not sure where it initializes the connection | |
| 16:11:34 | mriedem | btw, i think hemna has a flow chart for all of this | |
| 16:12:23 | mdbooth | I think I've got a good grasp of the nova flow, I just can't find this piece | |
| 16:12:34 | mdbooth | But that may be because it doesn't exist, or need to | |
| 16:12:45 | mdbooth | In which case I've misunderstood cinder | |
| 16:12:45 | johnthetubaguy | mdbooth: sorry stuck in a call right now | |
| 16:12:47 | hemna | yah I did | |
| 16:12:52 | hemna | let me see if I can find the url | |
| 16:12:53 | mriedem | mdbooth: looks like post_live_migration | |
| 16:13:30 | mdbooth | mriedem: Oh, interesting | |
| 16:13:59 | mdbooth | If the connection isn't initialised until post_live_migration, how can it expect to connect in pre_live_migration? | |
| 16:14:27 | mdbooth | Incidentally, I'm specifically looking for this because I've got logs where the connect_volume in pre_live_migration fails | |
| 16:14:55 | mdbooth | However, I'm starting from the assumption that this works, and I've just missed something | |
| 16:15:01 | hemna | https://drive.google.com/file/d/0B1Kp6K43HLHyRkFMZHVHWDJ3amM/view | |
| 16:15:07 | mriedem | oh post_live_migration is where it initializes the connection in cinder to get the connection_info, which it uses to disconnect the volume from the source host | |