| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 14:24:37 | mriedem | would have to dig back through the git history of time to find out how they changed sean's mind | |
| 14:24:42 | dansmith | so the place I'm seeing this is in instance delete, | |
| 14:24:48 | dansmith | which is failing and breaking the instance state | |
| 14:25:10 | johnthetubaguy | which instance delete path? | |
| 14:25:37 | dansmith | from compute manager while deallocating network | |
| 14:26:00 | johnthetubaguy | oh, it takes a while, so gives the token plenty time to have timed out? | |
| 14:26:05 | mriedem | johnthetubaguy: looks like this is our fault https://review.openstack.org/#/c/312014/ | |
| 14:26:30 | johnthetubaguy | mriedem: I probably should have expected that | |
| 14:27:04 | mriedem | https://review.openstack.org/#/c/312014/14/nova/network/neutronv2/api.py | |
| 14:27:41 | dansmith | johnthetubaguy: I dunno why it fails, it's from heat so heat might've gotten its token a long time ago or something | |
| 14:28:24 | johnthetubaguy | dansmith: might be the trust based token has a shorter lifetime | |
| 14:29:08 | johnthetubaguy | dansmith: the more general fix for this has merged already of course, you configure a service token to stop that expiry happening | |
| 14:29:44 | dansmith | yeah | |
| 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 | |