| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2018-06-13 | |||
| 14:49:07 | cdent | still confused on how it is currently working for nova? | |
| 14:49:20 | mordred | cdent: that is honestly a fascinating question | |
| 14:49:22 | kmalloc | yeah. i am inclined to say we should ksa2 at this point and drop some behavior | |
| 14:49:24 | cdent | s/still/I am still/ | |
| 14:49:43 | kmalloc | well... if it has links but no self | |
| 14:49:46 | kmalloc | it doesn't skip | |
| 14:49:53 | cdent | no links at all | |
| 14:49:57 | kmalloc | hm. | |
| 14:50:08 | cdent | so maybe the problem is not at the ksa level? | |
| 14:50:20 | kmalloc | maybe. | |
| 14:50:23 | cdent | in which case I'd argue against changing placement /, perhaps? | |
| 14:50:41 | mordred | cdent: yah - it might be that sdk is triggering version discovery where nova + plain ksa is not | |
| 14:50:51 | kmalloc | likely | |
| 14:51:07 | kmalloc | sdk does stuff in some more indirect ways. | |
| 14:51:15 | mordred | cdent: I would prefer to not special-case placement in sdk with a "please don't try version discovery on placement" - also, it'll break discovering available microversions | |
| 14:51:45 | kmalloc | mordred: ftr, i would love to stop skipping no-link items | |
| 14:51:47 | cdent | i'm not suggesting to avoid version discovery | |
| 14:51:56 | kmalloc | i just see some "this might actually break someone" | |
| 14:52:00 | kmalloc | potential | |
| 14:52:03 | cdent | well if you both feel that way I'll add a link | |
| 14:52:14 | mordred | cdent: yah - but nova's use of ksa is really only likely able to work by using ksa in such a way that version discovery is being avoided | |
| 14:52:14 | cdent | I don't feel strongly about it | |
| 14:52:24 | kmalloc | cdent: i don't want you to add a link, i don't think we can change ksa's behavior | |
| 14:52:30 | mordred | yah - I think the link is silly | |
| 14:52:33 | kmalloc | cdent: so, adding a link might be the correct way | |
| 14:52:45 | kmalloc | just not the desirable one | |
| 14:53:08 | cdent | what I do feel strongly about is having version numbers in urls and double endpoints | |
| 14:53:13 | kmalloc | mordred: i dislike the volume of inherited ick we carried over from ksc.session | |
| 14:53:18 | mordred | kmalloc: yah | |
| 14:53:23 | cdent | but we are in the reality we are in so... | |
| 14:53:23 | mordred | cdent: totally. | |
| 14:53:28 | kmalloc | cdent: please please please don't do version numbers in urls if you can avoid. | |
| 14:53:38 | kmalloc | cdent: so.. i am with you there | |
| 14:53:58 | kmalloc | mordred: how bad would it be to cut a ksa2 and drop compat behaviors [for sdk] | |
| 14:54:00 | cdent | kmalloc: yeah, don't plan to: placement was designed from the outset to never do that | |
| 14:54:10 | kmalloc | and start iterating on that instead of ksa1 continues? | |
| 14:54:21 | kmalloc | cdent: ++ and i appreciate that so very much! | |
| 14:54:23 | mordred | kmalloc: I don't think it would be _bad_ - but I think it would take us a few months | |
| 14:54:49 | kmalloc | mordred: well we'd need to ksa2 work anyway. i'm inclined to talk to lbragstad about it | |
| 14:54:56 | mordred | kk | |
| 14:55:04 | kmalloc | ksa1 is pretty stable, and we could make ksa2 just without all the icky compat stuff. | |
| 14:55:23 | cdent | can you guys speculate what ksa or the sdk would do if I set the href for the self link to '' | |
| 14:55:41 | mordred | I'll show you the code | |
| 14:56:04 | cdent | I want to do that because no where in placement does placemet know its own scheme, hostname, port, base url. And it is very important that it stay that way | |
| 14:56:36 | cdent | sorry about my n key | |
| 14:56:40 | mordred | cdent: https://github.com/openstack/keystoneauth/blob/master/keystoneauth1/discover.py#L406-L426 | |
| 14:57:42 | mordred | (although there is a bug in that that needs fixing) | |
| 14:57:50 | cdent | I read that as "should be okay": it will result in the discovery url, which is what we want | |
| 14:58:22 | mordred | cdent: yes. I believe that is true - also, we can make mnaser deploy it and we can find out for sure ;) | |
| 14:58:43 | kmalloc | cdent: it should work afaict. | |
| 14:59:08 | kmalloc | but, honestly,sometimes discovery stuff is wonkier than expected -- we have a lot of carry-over magic in lots of places. | |
| 14:59:23 | cdent | yeah, we shall see | |
| 15:00:39 | mordred | cdent, kmalloc: yup. this is the result of having to support the epic amount of incompatible pre-existing mush | |
| 15:01:01 | mnaser | i don't mind making changes if they're not too wild | |
| 15:02:50 | mordred | mnaser: btw - placement_enpdoint_override just worked for me | |
| 15:02:57 | mordred | placement_endpoint_override: http://placement-ca-ymq-1.vexxhost.net/ | |
| 15:03:07 | mnaser | maybe because i'm using os_client_config i guess | |
| 15:03:11 | mnaser | wait wait wait | |
| 15:03:18 | mnaser | mordred: remember the bug i mentioned about endpoint override not working?! | |
| 15:03:26 | mnaser | you fixed it but we might not had a release since | |
| 15:03:29 | mordred | yah | |
| 15:03:31 | mordred | ah | |
| 15:03:37 | mordred | yah - we need a release | |
| 15:03:47 | mnaser | https://review.openstack.org/#/c/565489/ | |
| 15:08:10 | frickler | mordred: nothing obvious on https://review.openstack.org/#/c/557935/ for me, either. though I seem to remember having seen this fail earlier with what I suspected to be race conditions, but on the detach side | |
| 15:09:03 | frickler | mordred: might either be an unstable test and the two in a row just unlucky, or some subtle change in timing somewhere | |
| 15:09:36 | mordred | frickler: nod. I'll recheck again just to be sure | |
| 15:10:26 | frickler | mordred: yeah, if that still has the same failure, I'll start comparing versions with the passing runs earlier | |
| 15:14:40 | cdent | woot on 565489 | |
| 15:19:45 | cdent | mordred, mnaser, kmalloc https://review.openstack.org/575117 updated to add links | |
| 15:22:30 | mnaser | if that seems okay to mordred and other reviews i can push that up to test | |
| 15:25:09 | mordred | mnaser: I think it's worth trying - also, we totally need to make an sdk release | |
| 15:27:22 | mnaser | mordred: ill wait for some nova reviews to roll in before i make that change | |
| 15:32:57 | kmalloc | cdent: ++ | |
| 16:16:49 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Add test to verify behavior on '' in self link https://review.openstack.org/575158 | |
| 16:17:21 | mordred | cdent, kmalloc, mnaser: ^^ that satisfies me that the link of '' from placement will do the right thing from sdk | |
| 16:17:47 | kmalloc | mordred: are we cross-gating sdk and ksa? | |
| 16:17:54 | kmalloc | if not, lets get that encoded in a test in KSA too | |
| 16:18:01 | kmalloc | i don't want to accidently break someone down the line. | |
| 16:18:15 | mordred | we are | |
| 16:18:17 | kmalloc | because the behavior changes because of some other "fix" that we miss. | |
| 16:18:20 | kmalloc | ok cool | |
| 16:18:23 | kmalloc | wfm then | |
| 16:18:24 | mordred | and yes - I think a test for that case in ksa is a good idea | |
| 16:18:31 | mordred | but also we cross-gate :) | |
| 16:18:51 | kmalloc | cross-gate makes me happy. i wont ever say no to more quality behavior tests ;) | |
| 16:19:33 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Add tests to verify behavior on '' in self link https://review.openstack.org/575158 | |
| 16:19:49 | mordred | cdent: ^^ added a second test just to verify microversion info is found correctly | |
| 16:22:38 | cdent | mordred: Imma have to take your word for it, that last test its hard to tell what it is actually doing | |
| 16:22:48 | cdent | but overall sanity seems to be in place | |
| 16:25:45 | mordred | cdent: the last test is basically just showing that the metainfo about the service's endpoint (placement.get_endpoint_data()) contains the microversion info that's in the discovery document ... namely that keystoneauth properly discovered the microversion ranges | |
| 16:25:53 | mordred | cdent: but - taking my word for it is also fine :) | |
| 16:27:04 | cdent | yeah, i figured that out once I read the json files, but the assert_calls remains dark | |
| 16:40:39 | mnaser | i like that test, ill keep an eye on it | |
| 16:46:48 | mordred | cdent: yah - for that you have to know a bit more about all of the requests_mock fixtures in that test suite | |
| 16:47:30 | mordred | cdent: the point of "assert_calls()" is to make sure that at the end of the test the http calls you told it to mock were all called and nothing was called that you didn't tell it to mock | |
| 16:47:52 | cdent | ✔ | |
| 16:48:04 | mordred | keeping in mind that the base class already mocks out the "get me a token" interaction from keystone | |
| 19:36:32 | openstackgerrit | Lance Bragstad proposed openstack/python-openstackclient master: Implement support for registered limits https://review.openstack.org/574475 | |
| 19:36:32 | openstackgerrit | Lance Bragstad proposed openstack/python-openstackclient master: Implement support for project limits https://review.openstack.org/575211 | |