| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 14:20:20 | johnthetubaguy | yeah, that would work | |
| 14:21:16 | openstackgerrit | Daniel Pawlik proposed openstack/nova master: Added kvm_hidden feature https://review.openstack.org/459753 | |
| 14:21:16 | johnthetubaguy | although that implies you would just check "if not context.auth_token" | |
| 14:21:25 | dansmith | or is_internal_request | |
| 14:21:30 | dansmith | or is_nova_admin | |
| 14:21:54 | mriedem | so in the long ago we used to just get the 401 from neutronclient and we didn't handle it, and we got a 500 in the compute api, | |
| 14:22:05 | mriedem | then we handled the 401 from neutron and converted that to a nova 401 to fix the 500 | |
| 14:22:28 | johnthetubaguy | then we made the config error back into a 500? | |
| 14:22:37 | dansmith | in the long ago, we had nova-network. period. | |
| 14:22:40 | mriedem | and we get the 401 now if your token expired (came in through the user context or you used nova.conf?) | |
| 14:23:01 | johnthetubaguy | so its not 401 if you got the token from nova.conf right? | |
| 14:23:11 | johnthetubaguy | should be 500 I think | |
| 14:23:21 | mriedem | NeutronAdminCredentialConfigurationInvalid turns it into a 400 | |
| 14:23:39 | johnthetubaguy | oh... | |
| 14:23:50 | mriedem | i think this came up in the original review on sahid's change, because sdague thought the 500 was correct | |
| 14:24:00 | johnthetubaguy | that feels like it should be 500 | |
| 14:24:02 | mriedem | i.e. if you don't have nova.conf [neutron] configured correctly | |
| 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 | |