| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 15:14:36 | mriedem | cdent: jay owns it now | |
| 15:14:39 | melwitt | mriedem, dansmith, johnthetubaguy: did anyone want to do a hangout to get an overview of the quotas patches to help make review easier? | |
| 15:14:52 | cdent | thanks mriedem | |
| 15:15:20 | mriedem | melwitt: i probably could in awhile, but wanted to get started on this chinese business visa paperwork before leaving for a week | |
| 15:15:28 | mriedem | b/c i imagine this is going to be worse than reviewing those quotas patches :) | |
| 15:16:09 | melwitt | heh, okay | |
| 15:18:54 | openstackgerrit | Roman Podoliaka proposed openstack/nova master: neutron: re-raise 401 for a non-admin client https://review.openstack.org/422696 | |
| 15:22:27 | bauzas | cdent: FWIW, I tried to explain the problem with the current method only accepting one RC | |
| 15:22:57 | bauzas | of course, we can merge that change as it is, but I think it will probably need some modifications later | |
| 15:23:00 | bauzas | dansmith: ^ | |
| 15:24:12 | cdent | "needing some modification later" is what I was trying to explain above. That's expected. | |
| 15:24:37 | dansmith | bauzas: tbh, I'm not sure I see a ton of value in complicating these methods, which are effectively wrappers around sql statements, with lots of conditional behavior | |
| 15:25:08 | bauzas | dansmith: the problem is not with that method, the problem is that that method executes the code | |
| 15:25:32 | dansmith | um, what? | |
| 15:25:46 | bauzas | dansmith: I tried to explain the problem in the last comment | |
| 15:25:47 | dansmith | executing code is usually what I'm looking for when shopping for a method | |
| 15:26:08 | dansmith | I've read your comments | |
| 15:26:11 | bauzas | dansmith: say that I want to ask placement for two resource amounts, each of them being for a specific RC | |
| 15:26:44 | bauzas | then I would need to verify both RCs separately, instead of using the DBRM execution plan to make my single query faster | |
| 15:27:02 | bauzas | and we *already* do that | |
| 15:27:18 | bauzas | the SQL statement is just below | |
| 15:27:47 | bauzas | also, the provided test is frankly positive | |
| 15:27:58 | bauzas | it's just expecting something correct | |
| 15:28:34 | bauzas | I'd rather be interested to verify negative things, like having a shared RP that doesn't have enough disk | |
| 15:28:54 | bauzas | ideally, two shared RPs, one having allocations | |
| 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 :) | |