| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-30 | |||
| 16:19:02 | dansmith | it's pretty much the suck though | |
| 16:20:12 | jordanP | mriedem, I replied. Not sure I understood the question though. Sorry if I misunderstood | |
| 16:20:13 | mriedem | yes i know | |
| 16:20:13 | mriedem | http://lists.openstack.org/pipermail/openstack-dev/2017-January/110798.html | |
| 16:20:33 | mriedem | jordanP: before your change, an admin is listing instances for a specific tenant, right? | |
| 16:20:46 | jordanP | only if all_tenants=1 is passed | |
| 16:20:53 | jordanP | in addition to tenant_id=XXX | |
| 16:21:49 | mriedem | but in your patch, you're removing tenant_id from search_opts if all_tenants is False | |
| 16:22:04 | mriedem | so as an admin, i want to specifically list instances for a single tenant, not all tenants | |
| 16:22:51 | jordanP | yeah.. I know it sucks, but that's how the current api behaves | |
| 16:23:27 | jordanP | it's "documented" here: https://bugs.launchpad.net/nova/+bug/1185290 | |
| 16:23:27 | openstack | Launchpad bug 1185290 in OpenStack Compute (nova) "nova list's --tenant flag also requires --all-tenants" [Undecided,Fix released] - Assigned to Ghanshyam Mann (ghanshyammann) | |
| 16:25:06 | mriedem | should be documented here http://developer.openstack.org/api-ref/compute/?expanded=list-servers-detail#id4 | |
| 16:26:28 | mriedem | jordanP: your reply in the patch still doesn't make it clear to me *why* all_tenants has to be true | |
| 16:26:36 | mriedem | is it because the if all_tenants block elevates the context? | |
| 16:27:10 | mriedem | because if an admin is making the request, we shouldn't need to elevate the contexzt | |
| 16:27:27 | mriedem | is it something in compute_api.get_all()? or something in the DB API? | |
| 16:29:56 | jordanP | I don't understand the question. All I am enforcing is that tenant_id option is dropped if all_tenants doesn"t evaluate to "true" | |
| 16:30:19 | jordanP | why, the API was designed this way, I don"t know | |
| 16:31:31 | mriedem | jordanP: i'd like to understand why filtering by tenant_id does not work unless all_tenants=1 | |
| 16:31:46 | jordanP | ah ok | |
| 16:32:42 | jordanP | I think it should work but then in the get_all() method | |
| 16:33:16 | jordanP | we have this filter_mapping dict that maps tenant_id to project_id | |
| 16:33:50 | mriedem | right, which could overwrite project_id in filters to point at the tenant_id, rather than context.project_id | |
| 16:34:04 | jordanP | and if we pass both tenant_id and project_id (project_id is automatically added (search_opts['project_id'] = context.project_id), then it becomes unpredictable | |
| 16:34:21 | jordanP | yeah exactly | |
| 16:35:01 | jordanP | maybe there's a better fix for the issue | |
| 16:36:15 | mriedem | why not just sort search_opts.items() in get_all() if it's a random order issue? | |
| 16:36:42 | mriedem | i don't really enjoy hacking stuff in the REST API because of some behavior in the compute API code, | |
| 16:36:46 | mriedem | making it all totally tightly coupled | |
| 16:38:18 | jordanP | so the get_all() method would have these search_opts : {'tenant_id': XX, 'project_id': YY}. This is conflicting | |
| 16:39:37 | jordanP | sorting would work because we would be able to control, for sure, whether tenant_id takes precedence over project_id, but the overwritting would still be here and unexpected | |
| 16:40:19 | mriedem | so if tenant_id is handled first, it maps filters['project_id'] to XX | |
| 16:40:38 | mriedem | and then handles project_id and sets filters['project_id'] to YY | |
| 16:41:20 | jordanP | something like this, the logic inside the "for opt, value in search_opts.items():" is not trivial to say the least | |
| 16:41:40 | mriedem | so if both tenant_id and project_id are in search_opts, you want to handle project_id first | |
| 16:41:57 | mriedem | so that the final value is whatever was specified for tenant_id in the request, XX in your example | |
| 16:42:29 | mriedem | sorting search_opts.items() would fix that right? | |
| 16:42:38 | jordanP | yes | |
| 16:43:20 | jordanP | but that's more than a hack to me, no one will understand that the order on which items are iterated upon is important | |
| 16:43:55 | jordanP | we shouldn't be able to call get_all() with both tenant_id and project_id to begin with | |
| 16:44:53 | BobBall | mriedem: Sorry for the delay - I believe it was a regression in ocata; see comment on the bug. | |
| 16:45:36 | BobBall | Speaking of which, I'm very confused - anyone got a thought why Gerrit shows https://review.openstack.org/#/c/424428/2..3/nova/tests/unit/virt/xenapi/test_driver.py as a change in the comment at the top of the file? | |
| 16:46:45 | jordanP | BobBall, did you edit this file on a Windows OS ? | |
| 16:47:02 | mriedem | jordanP: if filtering by tenant_id is only randomly not working when all_tenants=0, then your change would definitely make it never work | |
| 16:47:02 | jordanP | looks like a \n vs \r\n issue | |
| 16:47:03 | BobBall | oh dear... I hope that's not the casue... Let me check... (It wasn't me) | |
| 16:47:42 | jordanP | mriedem, that's the point, it should never work | |
| 16:47:54 | mriedem | jordanP: i don't see why | |
| 16:48:02 | mriedem | jordanP: we don't say anything about that in our api docs | |
| 16:48:13 | mriedem | jordanP: and if it randomly DOES work today, then breaking it is worse | |
| 16:48:34 | jordanP | it randomly work today only on Py35 and it never works on Py27 | |
| 16:48:45 | jordanP | the goal of this patch is to make it never work, also on Py35 | |
| 16:49:57 | jordanP | the doc doesn't say anything about it, I agree. So I can make it work on both py27 and py35 but that would require a Tempest change too. Because Tempest has a test to check that filtering by tenant when all_tenant is 0 doesn't work | |
| 16:51:50 | BobBall | jordanP: Indeed, you are right. | |
| 16:52:19 | openstackgerrit | Bob Ball proposed openstack/nova: Fix live migrate with XenServer https://review.openstack.org/424428 | |
| 16:53:33 | mriedem | jordanP: what's different about py27? | |
| 16:53:43 | mriedem | the dict ordering is random on py27 too isn't it? | |
| 16:53:56 | jordanP | the order on which the items in the search_opts dict are iterated upon | |
| 16:54:20 | BobBall | mriedem: There was a CRLF / LF issue causing https://review.openstack.org/424428 to look huge. I'm not arguing that it's small, but the diff is really 600 lines shorter than patchset 3... | |
| 16:54:29 | mriedem | BobBall: ha | |
| 16:54:49 | mriedem | jordanP: again, i thought dict.items() was random in py27 too isn't it? | |
| 16:55:51 | jordanP | I can't confirm. Maybe it is, maybe someone/something set PYTHONSEED (whatever its name) to ° | |
| 16:55:53 | jordanP | 0 | |
| 16:56:22 | edleafe | mriedem: not random, but not guaranteed to stay in order after a change | |
| 16:57:27 | mriedem | so dict.items() in py27 will return the tuples in the order that they were added? | |
| 16:57:42 | mriedem | because in the REST API code we're adding tenant_id first and then project_id | |
| 16:58:19 | edleafe | mriedem: no, not in the order they were added. But if you call items() once, and then call it again without changing the dict, the order will be the same | |
| 16:58:32 | openstackgerrit | Maciej Szankin proposed openstack/nova: conf: resolved TODOs in compute configuration https://review.openstack.org/426793 | |
| 16:58:41 | mriedem | edleafe: we only call items() once in this case | |
| 16:58:45 | mriedem | as far as i can tell | |
| 16:58:55 | edleafe | mriedem: yeah, so no guarantee of ordering | |
| 16:59:15 | jordanP | we need to distinguish guarantee and what actually happens | |
| 16:59:16 | mriedem | ok, so i'm not really buying the 'this never worked on py27' argument | |
| 16:59:32 | mriedem | jordanP: i agree we need consistency | |
| 16:59:35 | bauzas | shit, the bottom change for the scheduler upcall will be -2 | |
| 16:59:46 | bauzas | because of a cells job | |
| 16:59:53 | mriedem | bauzas: so recheck | |
| 17:00:11 | jordanP | this never works on py27 and the proof is that the Tempest job for that behavior passes on py27 but sometimes fails on py35 | |
| 17:00:11 | bauzas | mriedem: it will stop the gate jobs ? | |
| 17:00:17 | bauzas | I thought not | |
| 17:00:34 | bauzas | unless rebasing my change | |
| 17:01:00 | bauzas | I'll actually wait for the grenade change to land | |
| 17:01:08 | bauzas | and then recheck | |
| 17:01:30 | openstackgerrit | Gábor Antal proposed openstack/nova: Transform instance.trigger_crash_dump notification https://review.openstack.org/396225 | |
| 17:02:42 | bauzas | mriedem: FYI, I'll also write a bit of explanation about cellsv2 in the prelude section | |
| 17:02:55 | mriedem | bauzas: don't do the cells v2 prelude | |
| 17:03:06 | mriedem | bauzas: unless you plan on updating this https://review.openstack.org/#/c/424737/ | |
| 17:03:13 | mriedem | ^ will be the reno for any cells v2 stuff that's missing | |
| 17:03:36 | bauzas | mriedem: the prelude section works differently from the other sections | |
| 17:03:55 | bauzas | mriedem: so I'd like to only have one change for the prelude section | |
| 17:04:01 | bauzas | because that's not bullets | |
| 17:04:08 | bauzas | I mean bullet points | |
| 17:04:58 | bauzas | mriedem: but I can do the prelude change that would be a follow-up of https://review.openstack.org/#/c/424737/8/releasenotes/notes/ocata-cellsv2-support-4b3b5e70e76bc756.yaml | |
| 17:05:09 | bauzas | so we would be sure that we don't miss them | |
| 17:05:16 | bauzas | mriedem: WFY ? | |
| 17:05:27 | mriedem | sure | |
| 17:14:20 | openstackgerrit | Jordan Pittier proposed openstack/nova: Fix unspecified bahavior on GET /servers/detail?tenant_id=X as admin https://review.openstack.org/426259 | |
| 17:16:17 | jordanP | the question remains: why do we ignore tenant_id if all_tenants=0, I can't answer to this, it's probably unspecified, undefined but it's been like this for a long long time. | |
| 17:16:20 | jordanP | I have to go | |