| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-25 | |||
| 19:55:11 | openstackgerrit | Julia Kreger proposed openstack/nova master: ironic: add instance_uuid before any other spawn activity https://review.openstack.org/563722 | |
| 19:58:11 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: resource requests for nested providers https://review.openstack.org/554529 | |
| 20:00:52 | openstackgerrit | Jay Pipes proposed openstack/nova master: support multiple member_of qparams https://review.openstack.org/561315 | |
| 20:14:03 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use inspect.signature in nova.utils.expects_func_args https://review.openstack.org/564333 | |
| 20:14:04 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use inspect.signature in nova.network.base_api.refresh_cache https://review.openstack.org/564334 | |
| 20:22:12 | mriedem | yikun_: at this point, you should probably just incorporate https://review.openstack.org/#/c/563802/ into https://review.openstack.org/#/c/561819/ and https://review.openstack.org/#/c/557145/ respectively | |
| 20:22:31 | openstackgerrit | Jay Pipes proposed openstack/nova master: support multiple member_of qparams https://review.openstack.org/561315 | |
| 20:22:36 | mriedem | rather than a never-ending series of "address nits" patches at the end | |
| 20:25:05 | openstackgerrit | Merged openstack/nova master: Update docs for [keystone_authtoken] changes since Queens https://review.openstack.org/562812 | |
| 20:25:14 | openstackgerrit | Merged openstack/nova master: Make accept-language tests work with webob 1.8.x https://review.openstack.org/564255 | |
| 20:26:10 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/queens: Update docs for [keystone_authtoken] changes since Queens https://review.openstack.org/564336 | |
| 20:27:20 | melwitt | yikun_: seconded, especially the reno update. seems kinda weird to split that up | |
| 20:28:00 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: resource requests for nested providers https://review.openstack.org/554529 | |
| 20:28:31 | efried | mriedem: Is there a way to elevate the getargspec deprecation warning to error in py3, to prove those two patches you just proposed? | |
| 20:30:39 | mriedem | lbragstad: do you know the answer to this? https://review.openstack.org/#/c/524425/1/nova/api/openstack/placement/policies/base.py | |
| 20:31:00 | mriedem | efried: yeah i think so | |
| 20:31:12 | mriedem | https://docs.python.org/2/library/warnings.html#the-warnings-filter | |
| 20:33:00 | mriedem | https://docs.python.org/2/library/warnings.html#warnings.filterwarnings | |
| 20:33:19 | mriedem | like https://github.com/openstack/nova/blob/6d22567b9445e595f4c55627ea784801585a6e41/nova/tests/fixtures.py#L731 | |
| 20:33:46 | mriedem | err the ones below | |
| 20:33:50 | lbragstad | mriedem: i was just digging into that | |
| 20:33:51 | efried | mriedem: Would it be unreasonable to ask you to do that in those? | |
| 20:33:55 | mriedem | efried: i could | |
| 20:34:00 | lbragstad | one sec | |
| 20:34:18 | lbragstad | i want to say the answer is in the NovaKeystoneContext object | |
| 20:34:18 | mriedem | efried: note there is another inspect.getargspec usage in nova/cmd/common.py but it doesn't have any unit testing yet so i haven't tackled that one | |
| 20:34:30 | mriedem | lbragstad: nova.context.RequestContext? | |
| 20:35:17 | mriedem | efried: i'll throw something on top | |
| 20:35:22 | efried | ++ | |
| 20:36:53 | lbragstad | mriedem: first crack at working some documentation into oslo.policy's usage docs for newer services - https://review.openstack.org/#/c/564340/ | |
| 20:37:28 | lbragstad | mriedem: maybe - but nova has a special middleware that sits right behind auth_token | |
| 20:37:47 | lbragstad | it's job is to pull the auth token headers out of the request and generate a context object | |
| 20:38:00 | lbragstad | at least from what i can tell | |
| 20:38:22 | lbragstad | https://github.com/openstack/nova/blob/d124a0ed87cb99a06954ae7f3073cb859aa6808f/etc/nova/api-paste.ini#L80-L81 | |
| 20:38:47 | lbragstad | https://github.com/openstack/nova/blob/d124a0ed87cb99a06954ae7f3073cb859aa6808f/etc/nova/api-paste.ini#L32 | |
| 20:40:00 | mriedem | lbragstad: yeah i see it now | |
| 20:40:18 | mriedem | https://github.com/openstack/nova/blob/d124a0ed87cb99a06954ae7f3073cb859aa6808f/nova/api/auth.py#L72 | |
| 20:41:05 | mriedem | i don't think that has anything to do with rule:admin_api though | |
| 20:44:13 | lbragstad | yeah - digging through it and it doesn't look like it | |
| 20:44:31 | lbragstad | my bad | |
| 20:46:56 | lbragstad | it looks like rule:context_is_admin is really just an alias to role:admin | |
| 20:47:50 | lbragstad | https://github.com/openstack/nova/blob/master/nova/policy.py#L173 | |
| 20:49:31 | lbragstad | which is apparently used in nova's RequestContext https://github.com/openstack/nova/blob/d124a0ed87cb99a06954ae7f3073cb859aa6808f/nova/context.py#L151 | |
| 20:53:36 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add columns for generation to consumers https://review.openstack.org/557958 | |
| 20:53:37 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add the Consumer object https://review.openstack.org/561406 | |
| 20:53:38 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add Consumer objects to Allocations https://review.openstack.org/561407 | |
| 20:53:39 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add a microversion for consumer generation https://review.openstack.org/562000 | |
| 20:54:46 | lbragstad | so - i think the TL;DR is that rule:context_is_admin is used | |
| 20:56:26 | lbragstad | specifically when nova's middleware builds a context object, it'll parse the request environment headers, use oslo.context to get an object, then use oslo.policy's authorize methed (without a target?) to determine if context_is_admin is true | |
| 20:56:34 | lbragstad | based on the alias | |
| 20:56:56 | lbragstad | and then you set that to the ctx.is_admin attribute | |
| 20:58:51 | openstackgerrit | Merged openstack/nova master: Functional test: cold migrate to compute down https://review.openstack.org/496280 | |
| 20:59:06 | efried | mriedem: I'm not smart enough to understand why this changes anything: https://review.openstack.org/#/c/563732/2/nova/compute/api.py | |
| 21:01:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add a warnings filter for inspect.getargspec usage https://review.openstack.org/564343 | |
| 21:02:14 | openstackgerrit | Jay Pipes proposed openstack/nova master: support multiple member_of qparams https://review.openstack.org/561315 | |
| 21:03:51 | mriedem | efried: because before it you could specify bdms without a boot_index | |
| 21:04:31 | efried | mriedem: But those would be excluded from the boot_indexes list, nah? | |
| 21:04:49 | mriedem | right | |
| 21:05:10 | mriedem | for context you need to look at what _validate_bdm used to do https://review.openstack.org/#/c/524208/1/nova/compute/api.py | |
| 21:05:15 | mriedem | if 0 not in boot_indexes or not _subsequent_list(boot_indexes): | |
| 21:05:30 | mriedem | mdbooth's change was to clean up and prettify the right side of that conditional | |
| 21:05:44 | mriedem | and b/c we had 0 test coverage for the left side, it was dropped mistakenly | |
| 21:06:10 | lbragstad | mriedem: http://paste.openstack.org/raw/719958/ might be an idea of how to get rid of the context_is_admin alias | |
| 21:06:13 | mriedem | rather than revert that outright, i went with the suggestion from stephenfin in that change to keep the pretty right side, but still enforce the left side | |
| 21:06:25 | efried | mriedem: But range(0) is [] | |
| 21:06:49 | lbragstad | by bumping the processing of "is_admin" up in the chain and actually using the constructor of oslo.contexts RequestContext object | |
| 21:06:58 | lbragstad | (since it supports is_admin) | |
| 21:08:45 | efried | mriedem: So was "0 not in boot_indexes or" supposed to be added back in? | |
| 21:09:03 | mriedem | lbragstad: so that probably lets us drop this https://github.com/openstack/nova/blob/6d22567b9445e595f4c55627ea784801585a6e41/nova/tests/fixtures.py#L743 | |
| 21:09:18 | mriedem | efried: that's essentially what i'm adding back in yes | |
| 21:09:24 | mriedem | without a full revert | |
| 21:09:42 | lbragstad | maybe | |
| 21:09:51 | lbragstad | it looks like nova still uses is_admin | |
| 21:09:54 | lbragstad | on the context object | |
| 21:10:21 | lbragstad | my diff would be a way to remove the extra alias between context_is_admin and role:admin | |
| 21:10:33 | efried | mriedem: okay, but I don't think you are. Because if boot_indexes is [], that `any` will fail. | |
| 21:10:43 | efried | mriedem: | |
| 21:10:43 | efried | In [16]: boot_indexes = [] | |
| 21:10:43 | efried | In [17]: any(x != y for x, y in enumerate(boot_indexes)) | |
| 21:10:43 | efried | Out[17]: False | |
| 21:11:17 | lbragstad | instead, if you're hardcoding operations to a role name, you can check for it in the headers that auth_token gives you and then pass values to RequestContext's constructor directly | |
| 21:11:29 | mriedem | efried: and that's ok if you're not using bdms at all | |
| 21:11:41 | lbragstad | but you might be able to remove https://github.com/openstack/nova/blob/master/nova/policy.py#L172-L181 | |
| 21:12:13 | lbragstad | and save an enforcer trip | |
| 21:12:18 | efried | mriedem: Then the commit message isn't right. You're not changing anything logically afaict. | |
| 21:13:05 | mriedem | lbragstad: ok that's a bit over my head atm, since i'm doing a couple of things at once - this seems like something we could do separately and then i could build on it when adding the placement policy stuff | |
| 21:13:11 | mriedem | i'm just unsure of the implications of making that change | |
| 21:13:52 | lbragstad | does placement get context objecst from nova/context? | |
| 21:13:59 | lbragstad | or does it build those on it's own? | |
| 21:14:14 | mriedem | nova.context | |
| 21:14:18 | lbragstad | ok | |
| 21:14:58 | lbragstad | let me try something locally and i'll post a review with my thoughts, that might be easier to follow | |
| 21:16:37 | mriedem | efried: looks like in PS2 i changed something to fix one problem but broke what i was trying to do, so looking at it again | |
| 21:17:59 | efried | mriedem: PS1 looks samely broken to me. | |
| 21:18:47 | efried | mriedem: but the test failures are weird. Was there a separate fix for that? | |
| 21:19:37 | mriedem | the billions of test failures in PS1 were because it now made bdms required | |
| 21:19:52 | mriedem | i was fixing that in PS2, but apparently broke the test i was adding | |
| 21:19:58 | mriedem | fixing now | |
| 21:20:02 | mriedem | please hold dear caller | |
| 21:31:04 | arvindn05 | while you guys are on the topic of testings....are there any guidelines on test cases that they should test only one scenario?\ | |