| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 14:03:33 | johnthetubaguy | dansmith: true, although I assumed we just used the version cached in the instance for that | |
| 14:03:36 | mriedem | so basically context.get_admin_context() and pass that down | |
| 14:03:41 | mriedem | so it doesn't have a token, | |
| 14:03:42 | dansmith | mriedem: right | |
| 14:03:45 | mriedem | but is_admin=True | |
| 14:03:46 | mriedem | ok | |
| 14:04:54 | johnthetubaguy | I would tempted to have a re-raise_401=True? | |
| 14:04:55 | mriedem | so in the commit message where it says we get the 401 b/c the token is expired, | |
| 14:05:02 | mriedem | if we didn't have a token to begin with, | |
| 14:05:08 | mriedem | is that referring to the token that neutronclient creates? | |
| 14:05:29 | johnthetubaguy | I think its the user token passed into the API, that happens to be admin, but expires by the time we talk to neutron | |
| 14:05:58 | johnthetubaguy | ... now if you enable service token auth (the second token) that whole crazy goes away | |
| 14:06:21 | dansmith | johnthetubaguy: so that callers of this have to pass it to get proper behavior you mean? | |
| 14:06:44 | johnthetubaguy | so, its a configuration option right now, to enable the service token to get sent with any user token | |
| 14:07:12 | johnthetubaguy | (line 159 I guess) | |
| 14:07:39 | mriedem | yeah so no one is using that yet, | |
| 14:07:41 | mriedem | so we can ignore that part, | |
| 14:07:54 | mriedem | i'm also a bit lost where you said "I think its the user token passed into the API, that happens to be admin, but expires by the time we talk to neutron" | |
| 14:08:06 | mriedem | i thought if you did context.get_admin_context() there is no token | |
| 14:08:12 | dansmith | mriedem: correct | |
| 14:08:16 | mriedem | hence the "and not context.auth_token" check | |
| 14:08:24 | johnthetubaguy | oh wait, I kinda hate what this is doing | |
| 14:09:11 | johnthetubaguy | I don't like converting what the user passed in, into our own service token | |
| 14:09:14 | johnthetubaguy | I missed that bit | |
| 14:09:22 | johnthetubaguy | well, miss-read it | |
| 14:09:43 | dansmith | converting what? | |
| 14:09:45 | dansmith | the admin flag? | |
| 14:10:52 | johnthetubaguy | so if the user passes in a token, I think we should pass that through to the next service | |
| 14:11:12 | johnthetubaguy | what I think is happening here is we ignore the user token, and just use a service token instead, which sounds bad | |
| 14:11:32 | dansmith | no, I don't think so | |
| 14:11:52 | mriedem | if the user passes in a token, the "and not context.auth_token" is False | |
| 14:11:54 | johnthetubaguy | actually, my brain is fried, its not doing that, got my admin's mixed up here | |
| 14:12:35 | johnthetubaguy | admin=True means use service admin token, rather than context.auth_token, which we do anyways (for port binding reasons) | |
| 14:12:50 | dansmith | this unifies the logic for determining if we use admin_auth with the logic for the admin= flag on ClientWrapper.. so they match | |
| 14:14:30 | dansmith | it doesn't do anything other than make sure the ClientWrapper gets the right admin flag so that it does the right raise behavior I think | |
| 14:14:44 | dansmith | L108 only I think | |
| 14:15:03 | johnthetubaguy | yeah, sorry, I keep going around in silly loops | |
| 14:15:22 | johnthetubaguy | I wonder is that admin flag should be supress_401_error=True? | |
| 14:15:43 | dansmith | oh is that what you meant? | |
| 14:16:09 | dansmith | if ClientWrapper isn't used anywhere else, then that's probably an okay change | |
| 14:16:19 | mriedem | that's pretty leaky tight coupling | |
| 14:16:51 | mriedem | i'd rather change the admin kwarg in ClientWrapper to "use_service_credentials" or "using_service_credentials" or something | |
| 14:16:54 | dansmith | mriedem: but ClientWrapper is really only used from right there and for this purpose | |
| 14:17:09 | dansmith | mriedem: but that isn't right | |
| 14:17:10 | dansmith | mriedem: it doesn't change that behavior | |
| 14:19:10 | johnthetubaguy | looking at the exception name I am tempted by is_token_from_config=True/False | |
| 14:19:15 | johnthetubaguy | in the client wrapper | |
| 14:19:39 | dansmith | yeah that's more correct | |
| 14:19:41 | johnthetubaguy | is_admin=False when context.is_admin = True, just doesn't sit right | |
| 14:20:07 | dansmith | is_from_an_api_request=True makes most sense maybe | |
| 14:20:20 | johnthetubaguy | yeah, that would work | |
| 14:21:16 | johnthetubaguy | although that implies you would just check "if not context.auth_token" | |
| 14:21:16 | openstackgerrit | Daniel Pawlik proposed openstack/nova master: Added kvm_hidden feature https://review.openstack.org/459753 | |
| 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 | |