| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 14:30:02 | dansmith | can't backport that though | |
| 14:31:10 | johnthetubaguy | so us triggering a 401 just changes the error code from 400, rather than make the delete work? | |
| 14:31:16 | johnthetubaguy | or is this something deeper? | |
| 14:31:31 | johnthetubaguy | heat does a retry, which fixes things? | |
| 14:33:20 | dansmith | no, the instance is state locked at that point for some reason | |
| 14:33:26 | dansmith | like, you can't retry the delete | |
| 14:33:32 | dansmith | have to get more details on why | |
| 14:33:53 | dansmith | anyway, my point is, I don't think that fixing what is in that patch actually helps me out of the box | |
| 14:34:19 | dansmith | I'm guessing it's still task_state=deleting or something | |
| 14:34:57 | johnthetubaguy | ah, dang | |
| 14:35:31 | johnthetubaguy | would have to rollback the task state, I guess the instance might go into the ERROR state (or should) | |
| 14:39:57 | mriedem | dansmith: the service token thing is in ocata | |
| 14:39:58 | mriedem | for neutron | |
| 14:40:32 | mriedem | unless you're talking about backporting to like, kilo | |
| 14:40:40 | dansmith | johnthetubaguy: looks like it goes into error, but maybe not resetting the task_state or something | |
| 14:40:48 | dansmith | mriedem: yeah, before ocata I think | |
| 14:41:13 | mriedem | failures in terminate_instance in the compute manager would hit @reverts_task_state | |
| 14:42:37 | dansmith | should, yeah, which is why I'm asking for detail from the bug filer | |
| 14:43:18 | dansmith | it might be that heat just doesn't handle the failure but the instance is actually delete-able, which is what I'm trying to find out | |
| 14:44:39 | mriedem | ha, reading jay's 2nd set of slides, disk_available_least, that's fun | |
| 14:44:44 | mriedem | compute.free_disk_gb | |
| 14:44:49 | mriedem | compute.disk_available_least | |
| 14:44:51 | mriedem | duh | |
| 14:45:30 | openstackgerrit | melanie witt proposed openstack/nova master: Fix unit test exception KeyErrors https://review.openstack.org/462764 | |
| 14:45:31 | openstackgerrit | melanie witt proposed openstack/nova master: Make NovaException format errors fatal for tests https://review.openstack.org/411615 | |
| 14:56:11 | bauzas | hah, jay isn't around and I have a question for him https://review.openstack.org/#/c/460798/6 | |
| 14:58:44 | cdent | bauzas: there's been discussion of that in the comments on the earlier patches | |
| 14:58:58 | cdent | the basic answer is that the code is going to evolve to one query over a series of patches | |
| 14:59:04 | cdent | (including traits) | |
| 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: DNM: Canary test against multi-cell devstack https://review.openstack.org/458537 | |
| 15:09:19 | openstackgerrit | Dan Smith proposed openstack/nova master: Make target_cell() yield a new context https://review.openstack.org/458635 | |
| 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 | 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:34:53 | bauzas | anyway, it's late for me | |
| 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 | |