Friday, 2017-04-28

clarkboh wait no it does00:00
clarkbendpoint = self._get_highest_endpoint(service_types, kwargs)00:00
mordredjamielennox: sigh. nope. and there it is00:00
clarkbwhere service types is service_types = [self.get_service_type(service_key)]00:00
clarkbso just broken cloud config?00:00
mordrednothing sets interface=admin00:00
mordredyup00:00
clarkband kwargs there has interface set00:00
mordredand becaues of how occ was using the previous ksc workaround, it was masking it00:00
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for operator_cloud functional tests  https://review.openstack.org/46078400:01
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for operator_cloud functional tests  https://review.openstack.org/46078400:02
mordredjamielennox: ok - a) thank you - I'm not sure how far down the rabbit hole I would have gotten before finding a missing interface :) and b) ^^ I think that should work around it for at least a sec until I can get a patch to devstack to write those clouds.yaml files properly00:03
clarkbmordred: you should update your devstack patch too for completeness00:03
mordredoh yah - I shall do that indeed00:03
clarkband then we also want to make sure clouds.yaml is using v3 by default00:03
* clarkb checks a master job00:03
jamielennoxmordred: yea, you've hit like the only section of openstack where setting an interface really matters00:03
clarkbmordred: http://logs.openstack.org/75/460675/1/check/gate-tempest-dsvm-neutron-full-ubuntu-xenial/76e7cab/logs/etc/openstack/clouds.yaml.txt.gz is master00:04
clarkbmordred: it uses v3 but the other issues are all there too00:04
jamielennoxafaik, that section of keystone v2 is the only thing that is only available on the admin url00:04
clarkbso you want to make change against master and backport00:04
mordredclarkb: yah00:04
jamielennoxmordred: i don't know the shade tests well, but wont that set a fairly global interface='admin'?00:05
jamielennoxlike you probably only want it for those keystone v2 admin calls00:05
mordredhttps://review.openstack.org/#/c/460786/00:09
clarkbmordred: reviewed I think bashate will trip on the extra tab there00:09
clarkbotherwise lgtm00:09
mordredjamielennox: we keep two cloud configs- one that's a normal user and one that's an admin user00:09
mordredjamielennox: so that'll set interface=admin for all the admin operations, which should be a reasonable fine thing to do, yeah?00:10
jamielennoxmordred: yep, i just mean even for like the nova admin calls you probably want to use public00:10
jamielennoxmordred: oh, you don't know the admin interface saga?00:10
mordredjamielennox: I think I've blocked it from my poor brain00:11
clarkbwhy have different interfaces if nevermind I don't want to know its 5pm and I am going for a walk00:11
mordredclarkb: it was an early "security" feature, iirc00:11
jamielennoxso basically the admin interface was only really ever used by keystone, it was decided that things like adding services, etc was only done by a cloud admin and so should be done on a different port and service to everything else00:12
jamielennoxbecause we don't trust RBAC? i don't know00:12
mordredbecause FEAR00:12
jamielennoxso really the whole :35357 port thing should never have actually been exposed to the public00:13
mordredjamielennox: so - we should not add interface: admin to the clouds.yaml files for the admin user in devstack - and instead just special case a thing in shade for those tests00:13
jamielennoxbut keystoneclient, a CLI used almost primarily by operators decided it should default to admin interface because it was silly to have people type it in every time00:13
jamielennoxdefaults stick...00:13
jamielennoxand the world burns00:13
* mordred sobs00:14
jamielennoxmordred: yea, special case like the 5 method calls that need the admin interface, because it's fairly undefined behaviour whether the adminURL in nova catalog can be hit from wherever you are00:14
mordredkk. cool00:15
mordredand we _really_ only need that for keystonev2 right?00:15
jamielennoxmordred: yep, one of the things for v3 was to completely remove the special admin cases00:16
jamielennoxrealistically if your deployment set up DNS correctly we could get the whole catalog thing down to 1 url00:17
jamielennox( per service/region)00:17
mordredjamielennox: but dns is scary00:17
jamielennoxright, that would be us as developers telling admins how to set up their deployment00:18
jamielennoxwhich is simultaneously heresy and what they complain about when there is no standard method00:18
mordredjamielennox: I kinda feel like we should also put in a patch to occ that is "if service_type=identity and api_version=2: interface=admin" - since it literally won't work otherwise00:23
jamielennoxmordred: it's limited to only those calls though, the OS-KSADM ones00:23
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for keystonev2 keystone tests  https://review.openstack.org/46078400:23
jamielennoxlist users etc will work fine on public00:24
mordredjamielennox: but v2 didn't haveany non-admin calls, did it?00:24
mordredah - gotcha00:24
mordredso it's just services and endpoints?00:24
jamielennoxand because admin is only expected for cloud admins it generally an internal ip00:24
mordredjamielennox: user password also seems to do OS-KSADM00:25
jamielennoxmordred: only the admin reset though00:26
mordredyah00:26
mordredwell - hopefully that patch will pass - 'm going to WIP the devstack patches for now00:26
mordredjamielennox: so really - the only reason this worked before is because the ksc hack was telling ksc to use the auth_url for the endpoint and not anything from the catalog (which is what the ksc hack was all about) COMBINED withthe fact that devstack puts the admin url into clouds.yaml as the auth_url00:28
jamielennoxbut yea, in for example your create_service() shade call i would expect you to force pass interface='admin' for v2 because otherwise it just doesn't work00:29
mordredjamielennox: yah - but we can't pass interface=admin on a call00:29
jamielennoxyep, and it's wrong that devstack is putting the admin_url into clouds.yaml00:29
mordredor, at least, not without a major restructuring00:29
jamielennoxmordred: you can't via client, you can via adapter00:29
mordredah - good point- we can make that more robust once we're done restifying00:30
mordredjamielennox: ok - I pushed up devstack patches for master, stable/ocata and stable/newton making the change to which endpoint goes into clouds.yaml00:37
mordredbetween that and the shade test fixes that _should_ be good - thank you very much for the hepl!00:38
jamielennoxmordred: i feel like i've fixed that in the past, someone always changes it back00:38
jamielennoxno worries00:38
mordredpeople LOVE unfixing fixes00:38
*** slaweq has joined #openstack-shade00:39
openstackgerritMonty Taylor proposed openstack-infra/shade master: Add in a bunch of TODOs about interface=admin  https://review.openstack.org/46079000:42
*** slaweq has quit IRC00:44
rodsmordred you around?01:07
rodsabout the idea of altering the message attribute of the exception we were talking before01:10
rodssomething like this seems to work(line 19-20) http://paste.openstack.org/show/608271/01:10
*** gouthamr has quit IRC01:44
*** gouthamr has joined #openstack-shade01:44
*** yolanda has quit IRC01:55
*** slaweq has joined #openstack-shade02:41
*** slaweq has quit IRC02:45
*** gouthamr has quit IRC03:50
*** gkadam has joined #openstack-shade03:51
*** gouthamr has joined #openstack-shade03:51
*** gouthamr has quit IRC03:56
*** slaweq has joined #openstack-shade04:42
*** slaweq has quit IRC04:42
*** slaweq has joined #openstack-shade04:44
*** gkadam__ has joined #openstack-shade04:48
*** gkadam has quit IRC04:49
*** slaweq has quit IRC04:49
*** gkadam__ is now known as regain04:49
*** regain is now known as gkadam04:49
*** gkadam has quit IRC04:50
*** gkadam has joined #openstack-shade04:51
*** slaweq has joined #openstack-shade05:45
*** slaweq has quit IRC05:49
*** yfried has joined #openstack-shade09:10
*** cdent has joined #openstack-shade09:14
*** purplerbot has joined #openstack-shade09:16
*** yfried has quit IRC09:22
*** cdent has quit IRC10:58
*** gkadam has quit IRC11:29
*** cdent has joined #openstack-shade11:35
*** slaweq has joined #openstack-shade11:50
*** slaweq has quit IRC11:54
mordredrods: ++ that looks great12:12
Shrewsmordred: rods: that paste doesn't make sense to me, actually. what are you trying to do? looks to me like you'd lose the original exception's message when the original intent was to let that info pass through12:30
mordredShrews: so - the problem here is that with restification we have OpenStackCloudExceptions that get thrown by the adapter without nice error messages12:31
mordred(they're things like "Not found")12:31
mordredso as we restify, we're actually now losing friendly error messages because we're letting adapter throw them on bad http codes12:32
mordredthis change would let us still wrap those calls in with shade_exceptions and inject a good error message, but keep the traceback in the exception intact12:32
Shrewsmordred: but with that change, you are now affecting code like this: https://github.com/openstack-infra/shade/blob/master/shade/openstackcloud.py#L795-L79612:35
Shrewsinstead of raising an exc saying "project not found", you replace the original exception message12:35
Shrewsand in that particular example, it's less descriptive12:35
mordredShrews: that's an excellent point12:36
mordredShrews, rods: how about we make a new context_manager that only does the message thing - and we add it only right around calls to the adapter?12:37
mordredthat way we're only doing it for places we know the error message from the adapter is lame and not accidentally for other places in shade thatuse the shade_exceptions context manager12:37
Shrewsyeah, i think you need the fix closer to the adapter12:38
Shrewsmordred: now that i look at that paste more closely, i don't think it would actually do anything. 'args' is only processed in __init__(), so i don't think that does what was intended12:49
mordredthe other idea I had (although I like rods use of the context_manager better) was to add an "error message" option to the adapter, but that feels clunky12:50
mordredShrews: args gets passed to the super ctr12:50
mordredand then __str__ does message = Exception.__str__(self)12:50
Shrewsmordred: yes, but only when first constructed. you aren't constructing a new exception12:51
mordredoh right. duh12:52
Shrewsthe 'raise' by itself seems wrong, too. in any case, i feel that's the wrong approach anyway12:52
Shrewsmordred: why can't you just use the context manager as-is within the adapter?12:53
mordredyah. piddle. I think you're right. darn. I liked the outcome of that approach code-wise12:53
mordredShrews: the adapter itself is long-lived, so we'd have to pass the error message we want produced to each call12:53
mordredOR - make a context manager that takes the adapter and an error message as an argument and returns us a thing that looks like the adapter ...12:54
mordredhang on - that didn't make sense- let me write that in non-working code real quick12:56
Shrewsmordred: where are the NOT FOUND exceptions being throw from (in shade)? gimme an example12:56
Shrewsfrom raw 'post' calls?12:56
mordredShrews: in shade/exc.py there is a function "raise_from_respose"12:56
mordredShrews: in _munch_response on ShadeAdapter in shade/_adapter.py, the first thing done is calling raise_from_response on the respose12:57
mordredresponse12:57
Shrews*nod*12:57
mordredShrews: if you look at https://review.openstack.org/#/c/460357/1/shade/openstackcloud.py12:58
mordredline 387912:58
Shrewsso...12:58
mordredyou can see how it kind of goes - the post call will throw an exception there because of raise_from_response, but it will not have any error message like the previous context manager provided12:58
Shrewsjust make raise_from_response throw normal Exception objects, then use shade_exceptions() to wrap the actual calls and give a useful error message?12:59
mordredwe could just make it post('/volumes', json=dict(volume=kwargs), error_msg="Error in creating volume") and we'd have something to pass to raise_from_response12:59
mordredShrews: well - the thing is - exception wrapping always makes following tracebacks harder, and I was hoping we could get out of the business if we can throw the right exception from the error site now that we'r enot using the python libs13:00
Shrewsactually, it seems the exceptions raised from raise_from_response are not useful anyway if the messages themselves aren't useful13:01
mordredthey're useful-ish in that there are a few different exception classes depending on status_code13:01
mordredso they give you some ways to handle the errors in code which are nice - but they depend on the servers for the human-readable error message13:02
mordredhrm. I also see some dead code in here13:04
Shrewswell, if you're going to always override the text of the exception (regardless of status code), then they aren't useful13:04
mordredyah13:04
Shrewsbut i like your post example above13:04
mordredI've got two ideas - let me sketch them up real quick and see what you think13:04
* Shrews should coffee more first13:05
rodsmordred Shrews hello13:11
*** Matias has quit IRC13:13
openstackgerritMonty Taylor proposed openstack-infra/shade master: Add optional error_message to adapter.request  https://review.openstack.org/46102513:13
mordredrods, Shrews: ^^ what about that? (completely untested)13:13
Shrewsmordred: that might work. if you only wanted it for certain status codes, you could instead pass in a mapping from code to message13:15
Shrews404 => "omg what went wrong????"13:16
Shrewse.g13:16
mordredShrews: yes - I agree - it would not be difficult to extend it in that way13:17
mordredalthough honestly if we have callsites that want to do that, I think it would be easier for those to just ask for the response un-munched and do the direct response processing themselves :)13:18
* Shrews must spitballing. you're obviously more familiar with what's needed than i13:19
Shrewss/must/just/13:19
rodsmordred I would be more inclined to test your first idea about the context_manager for the message to apply when needed13:21
rodsbut I'm not that familiar with the code base yet and I could be very wrong :)13:23
Shrewsrods: the context manager idea doesn't work b/c it was designed for existing shade exceptions to pass through unchanged13:25
Shrewsit's original purpose is to make sure that the code it wraps doesn't let a non-shade exception get through, AND to not alter any shade exceptions that may be thrown13:27
mordredShrews: would you  mind helping me figure outwhat's wrong with https://review.openstack.org/#/c/460784 ?13:27
mordredShrews: the errors say AttributeError: 'TestIdentity' object has no attribute 'config'13:27
Shrewsmordred: easy... the author!13:27
Shrewslooking13:28
mordredbut when I look at the chain of setUp methods, I clearly see self.config being set13:28
mordredso I'm very confused13:28
Shrewsmordred: could that be a reaction to the pep8 error there?13:29
Shrewshttp://logs.openstack.org/84/460784/3/check/gate-shade-pep8-ubuntu-xenial/f1fb982/console.html#_2017-04-28_00_26_19_40349313:29
mordredShrews: for the love of13:30
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for keystonev2 keystone tests  https://review.openstack.org/46078413:35
mordredShrews: maybe that'll be better13:35
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for keystonev2 keystone tests  https://review.openstack.org/46078413:36
*** cdent has quit IRC14:05
*** gouthamr has joined #openstack-shade14:44
Shrewsmordred: KeystoneBaseFunctionalTestCase should derive from BaseFunctionalTestCase, not base.TestCase (the cause of the current errors)14:46
mordredShrews: thank you sir14:47
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for keystonev2 keystone tests  https://review.openstack.org/46078414:48
*** cdent has joined #openstack-shade14:52
*** openstackgerrit has quit IRC15:17
*** yolanda has joined #openstack-shade15:43
*** slaweq has joined #openstack-shade16:22
*** cdent has quit IRC16:29
*** openstackgerrit has joined #openstack-shade16:45
openstackgerritMonty Taylor proposed openstack-infra/shade master: Set interface=admin for keystonev2 keystone tests  https://review.openstack.org/46078416:45
*** slaweq has quit IRC17:04
*** slaweq__ has joined #openstack-shade17:06
*** slaweq__ has quit IRC17:07
*** slaweq has joined #openstack-shade17:19
*** slaweq has quit IRC17:24
openstackgerritMonty Taylor proposed openstack/os-client-config master: Fix interactions with keystoneauth from newton  https://review.openstack.org/46111417:47
*** slaweq has joined #openstack-shade17:48
mordredShrews: oh - I was wrong, we ARE testing git tips of libraries properly in our gate-shade-dsvm-functional-legacy-libs-nv jobs17:55
mordredShrews: so I'm not sure why we didn't notice the occ-caused newton issue before17:55
*** slaweq has quit IRC18:06
*** slaweq has joined #openstack-shade18:06
*** yolanda has quit IRC18:07
mordredShrews: also, https://review.openstack.org/#/c/460784/ fixes the gate18:09
mordredclarkb: ^^ that's the fix for yesterday's fun games with newton18:09
clarkbmordred: you aren't going to fix in devstack?18:10
mordredclarkb: two different fixes it turns out18:10
mordredclarkb: the devstack fix won't actually fix the shade issue - it'll just make devstack correct18:10
clarkbisn't shade using the devstack clouds.yaml though which would set the interface?18:11
Shrewsmordred: something is terribly amiss in the nv job: http://logs.openstack.org/84/460784/7/check/gate-shade-dsvm-functional-legacy-libs-nv/f6da27a/console.html#_2017-04-28_17_10_06_18533918:11
*** slaweq has quit IRC18:11
mordredshade-side - for those operations in keystone v2 we should be explicitly requesting the admin interface18:11
mordredclarkb: nothing else should _ever_ use the admin interface by default it turns out18:11
mordredclarkb: so setting interface=admin in clouds.yaml would not be correct18:11
mordredShrews: yah - bug in latest occ release - patch just pushed up to fix that18:11
clarkbmordred: right so the issue is that to be an admin you have to know a shibboleth, have access to a special port, and change all your urls18:12
Shrewsi did not think 'shibboleth' was an actual word18:15
Shrewsheh18:15
*** yolanda has joined #openstack-shade18:16
clarkbits like securing your home by giving everyone that should have access keys, but then only letting them in if they know to go around the back and knock with a special knock18:17
clarkbmordred: I've +2'd the change but wonder if shade shouldn't also grow a more verbose error message if you are using admin actions that require the admin interface and admin isn't set?18:22
openstackgerritDavid Shrewsbury proposed openstack-infra/shade master: Add in a bunch of TODOs about interface=admin  https://review.openstack.org/46079018:23
mordredclarkb: ah - so - as soon as we migrate these calls to REST - we can enforce interface=admin18:23
mordredclarkb: I dropped in a bunch of todo comments here: https://review.openstack.org/#/c/460790/18:23
mordredclarkb: to remind us to do it18:23
mordredwe can't do it _before_ then, because python-keystoneclient is not flexible enough18:23
Shrewsjust approved that and the other thing18:23
mordredShrews: woot. thakns18:24
clarkbmordred: what we can do is if keystone admin and not interface=admin emit verbose error instead of 404ing?18:24
mordredclarkb: yes - we can do that now18:24
mordred"this call is about to fail, instead of making it, I'm going to go ahead nad stop and tell you so"18:24
openstackgerritMonty Taylor proposed openstack-infra/shade master: Add optional error_message to adapter.request  https://review.openstack.org/46102518:26
clarkbmordred: ya because as an end user a 404 in the bowels of the o[penstack api isn't super helpful18:27
mordredclarkb: ++18:27
*** slaweq has joined #openstack-shade18:33
*** slaweq has quit IRC18:55
openstackgerritMerged openstack-infra/shade master: Set interface=admin for keystonev2 keystone tests  https://review.openstack.org/46078419:04
mordredrods, slaweq_: ^^ tests should be working again19:05
*** slaweq has joined #openstack-shade19:20
openstackgerritSlawek Kaplonski proposed openstack-infra/shade master: Replace neutronclient with REST API calls in router commands  https://review.openstack.org/46051819:34
openstackgerritMerged openstack-infra/shade master: Add in a bunch of TODOs about interface=admin  https://review.openstack.org/46079019:52
rodsmordred cool, thx20:04
rodsanything I can do to help with https://review.openstack.org/#/c/461025/?20:04
mordredrods: I'm poking at it right now... let's see what stupid thing I've done now :)20:08
mordredwow. I'm good. what a silly typo :)20:09
rods:)20:13
openstackgerritMonty Taylor proposed openstack-infra/shade master: Add optional error_message to adapter.request  https://review.openstack.org/46102520:26
openstackgerritSlawek Kaplonski proposed openstack-infra/shade master: Remove neutronclient mocks from ports tests  https://review.openstack.org/46114920:41
openstackgerritMonty Taylor proposed openstack-infra/shade master: Add some more debugging to the post_test_hook  https://review.openstack.org/46077920:50
openstackgerritSlawek Kaplonski proposed openstack-infra/shade master: Remove neutronclient mocks from quotas tests  https://review.openstack.org/46115020:59
rodsmordred tests passed on https://review.openstack.org/#/c/461025/321:22
rodsonce that commit is merged, I'll push a new ps for https://review.openstack.org/#/c/460357/121:24
openstackgerritMonty Taylor proposed openstack/os-client-config master: Fix interactions with keystoneauth from newton  https://review.openstack.org/46111421:35
mordredawesome21:36
mordredShrews: if you're still around, I think you liked the approach in https://review.openstack.org/#/c/461025 - but do you want to chime in on it?21:37
rodsmordred functional tests run against a newton devstack, right?21:51
mordredrods: in the legacy tests, yes. in the normal tests it's normal devstack21:53
rodsok, thx21:53
*** cmurphy has quit IRC21:59
*** cmurphy has joined #openstack-shade22:05
*** gouthamr has quit IRC22:14
*** slaweq has quit IRC22:22
morganmordred: shade is doing something wrong with get domain22:55
morganmordred: trying to chase it down22:55
morganmordred: when you do a get_domain_by name it just does ".get_domain" still vs an actual search22:55
morganso https://identity.example.com/v3/domains/<domain_name>22:56
morganfound this out in the conversion away form mock22:56
mordredmorgan: oh good!22:58
*** slaweq has joined #openstack-shade23:00
morganyeaaaaah23:00
morgan#funtimes23:00
morganmy brain was trying to figure out how the heck we needed to mock domains/domain_name vs domains/domain_id23:01
morganit dawned on me something broken was happening23:01
morganit might be KSC broken fwiw23:01
openstackgerritMorgan Fainberg proposed openstack-infra/shade master: Convert test_role_assignments to requests mock  https://review.openstack.org/44831323:03
mordredmorgan: blasphemy!!!23:04
* morgan has 24 more tests to fix there and a follow up to fix the domain behavior23:05
morganonly 2400 LOC so far23:05
morgan... in short... Sorry!23:05
morganit is effectively un-reviewable except "do tests run"23:05
mordredyup23:06
*** slaweq has quit IRC23:06
mordredit's fun to read and be like "wow, that's not efficient" - but it's definitely what the original code does - yay testing frameworks23:07
*** slaweq has joined #openstack-shade23:07
morgani wish i could make it better... but really, i can't23:07
*** slaweq has quit IRC23:12
*** rods has quit IRC23:13
openstackgerritMonty Taylor proposed openstack-infra/shade master: Add some more debugging to the post_test_hook  https://review.openstack.org/46077923:15
*** rods has joined #openstack-shade23:17

Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!