Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-01-30
16:01:53 mriedem mdbooth: it's not a regression right?
16:02:06 mdbooth mriedem: Nope, but it's a bug.
16:02:14 mriedem mdbooth: sure, but we have lots of bugs
16:02:23 mdbooth mriedem: Right, but this one has a well-tested fix.
16:02:23 mriedem mdbooth: having said all of that, i was planning on trying to get that one in yet
16:02:29 mdbooth Thanks
16:07:56 ildikov johnthetubaguy: mriedem: so just to ensure we're on the same page, the plan now is to get that remove check_attach patch landed, when master opens for Pike?
16:08:21 mriedem ildikov: that's what i would prefer
16:08:27 mriedem since we're 4 days from rc1
16:09:01 ildikov I guess as the new Cinder API changes will be tied to a version etc. we don't necessarily have to land it a cycle earlier
16:09:17 ildikov as it makes those changes easier to do and follow
16:10:12 ildikov mriedem: so I can annoy you to hell with this, when master opens then?
16:10:27 ildikov mriedem: as it's been two cycles now... :/
16:10:41 ildikov mriedem: and it's really just a bugfix on its own
16:11:26 mriedem yeah, but to be fair, it's also super latent behavior
16:12:37 ildikov mriedem: I see this as one more reason to remove :)
16:13:04 artom mriedem, so I wrote the spec for the device role tagging bug: https://review.openstack.org/#/c/426030/
16:13:40 artom I poked johnthetubaguy to review it, dunno how much bandwidth he has to spare - it'd be awesome if we can fix this for ocata to avoid a cycle without device tagging (yeah, the client the can version pinned, I know)
16:13:51 artom dansmith ^^ same to you :)
16:13:59 artom Many thanks in advance, gentlemen :)
16:14:16 openstackgerrit Dan Smith proposed openstack/nova: Allow scheduler to run cell host discovery periodically https://review.openstack.org/426826
16:14:51 mriedem artom: i know, i have it open in a tab
16:16:08 dansmith do we really have to add another microversion to bring that back in?
16:16:56 mriedem jordanP: left a question in https://review.openstack.org/#/c/426259/ - i don't really see how that doesn't break the case where the admin is listing instances for a specific tenant
16:17:41 mriedem dansmith: i wrestled with that
16:18:01 mriedem it is an api change though
16:18:33 mriedem i.e. trying to tag bdms on server boot with 2.33 in newton wouldn't work but if we bug fix it in ocata, then it will work
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

Earlier   Later