Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-01-30
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 jordanP looks like a \n vs \r\n issue
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: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 bauzas mriedem: it will stop the gate jobs ?
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: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
17:19:57 mriedem artom: comments inline https://review.openstack.org/#/c/426030/
17:20:22 artom mriedem, woot!
17:20:41 mriedem artom: mostly ok, just some nits
17:21:10 artom mriedem, sweet, lemme fix those and respin
17:33:04 openstackgerrit Artom Lifshitz proposed openstack/nova-specs: Fix tag attribute disappearing https://review.openstack.org/426030
17:37:46 mriedem artom: +2
17:38:53 openstackgerrit Stephen Finucane proposed openstack/nova: console: Move proxies to 'console/proxy' https://review.openstack.org/408192
17:38:54 artom mriedem, phast
17:40:22 mdbooth I didn't notice this one missed -3: https://review.openstack.org/#/c/423392/
17:40:36 mdbooth It's trivial, but it needs to go in to Ocata because it's upgrade related.
17:41:15 mdbooth mriedem: Is there anywhere I can add stuff like ^^^ in your etherpad?
17:41:26 mdbooth mriedem: Or maybe somewhere else?
17:45:00 jaypipes mriedem: k, doing final run of tests now for the Ironic inventory patch.
17:45:11 jaypipes mriedem: hopefully will be pushed in <30 minutes
17:45:21 dansmith artom: link me to the actual fix?
17:45:41 artom dansmith, https://review.openstack.org/#/c/424759/
17:51:05 openstackgerrit Sylvain Bauza proposed openstack/nova: Add prelude section for Ocata https://review.openstack.org/426864
17:51:55 bauzas shit, forgot the dep
17:52:01 openstackgerrit David Medberry proposed openstack/nova: Explain how allow_resize_to_same_host is useful https://review.openstack.org/426531
17:54:13 dansmith artom: okay, no +1s on that from the API folks
17:54:30 mriedem dansmith: api folks are pretty much all out right now

Earlier   Later