| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 14:59:43 | cdent | bauzas: in other words, it's important to read all the comments on all the patchsets in the entire stack to get a good idea of what's going on | |
| 14:59:49 | cdent | that's a pain, but that's how it is | |
| 14:59:50 | bauzas | cdent: I see PS2 comments | |
| 15:00:07 | cdent | yeah, look elsewhere in the stack too | |
| 15:00:32 | cdent | I had the same concerns and got rather confused because I was expecting things to be more complete in one patch, but that's not the path being taken | |
| 15:01:37 | bauzas | cdent: okay, I just think he missed a verification | |
| 15:01:54 | bauzas | cdent: about returning RPs having all the RCs | |
| 15:02:08 | cdent | yeah, that maybe, but some of the verification is intentionally being done later | |
| 15:02:12 | cdent | (in the stack) | |
| 15:02:16 | bauzas | that's the problem when you duplicate code | |
| 15:02:26 | cdent | yes, my complaint exactly | |
| 15:02:27 | bauzas | you reintroduce bugs | |
| 15:02:32 | cdent | but jay's aware of it | |
| 15:09:14 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: De-complicate some of the instance delete path https://review.openstack.org/458636 | |
| 15:09:15 | openstackgerrit | Dan Smith proposed openstack/nova master: Move to proper target_cell calling convention https://review.openstack.org/458634 | |
| 15:09:19 | openstackgerrit | Dan Smith proposed openstack/nova master: Make target_cell() yield a new context https://review.openstack.org/458635 | |
| 15:09:19 | openstackgerrit | Dan Smith proposed openstack/nova master: DNM: Canary test against multi-cell devstack https://review.openstack.org/458537 | |
| 15:12:06 | bauzas | cdent: okay confirmed that we need the HAVING count() for RCs, because we could return RPs not having the asked RC in their inventories | |
| 15:12:37 | cdent | mriedem, jroll: have we lost jroll on this https://blueprints.launchpad.net/nova/+spec/custom-resource-classes-in-flavors or is he staying in view? | |
| 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 | |