| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-10 | |||
| 12:53:35 | jangutter | jaypipes: for SR-IOV VF's there's a rough a proxy: Neutron Ports can under quota.... but it's very very very leaky at best. | |
| 12:54:22 | jangutter | jaypipes: very difficult to see how that model would translate to GPU, though. | |
| 12:57:38 | efried | cdent, jaypipes: +A on https://review.openstack.org/#/c/543262/ -- great stuff. | |
| 12:58:13 | efried | johnthetubaguy: FYI ^ | |
| 13:03:27 | cdent | efried: On self.compute thing, it's been long enoug since I made that change that I can't remember the details :( | |
| 13:03:49 | efried | no worries; gate-bound in any case. If it passes zuul, it's good enough for me. | |
| 13:04:33 | efried | cdent: But as I'm sure you're aware, mock.patch.object(actual object, 'method') is not quite the same thing as mock.patch.object(path to lib containing method, 'method') | |
| 13:04:47 | efried | cdent: But in this case I think both things were actually pointing to the lib ultimately. | |
| 13:04:53 | efried | so in this case I think it *was* the same. | |
| 13:05:08 | cdent | yes, that's why it ended up as it did, because the indirection of self.compute was meaningless | |
| 13:12:19 | openstackgerrit | Merged openstack/os-vif master: add noop plugin https://review.openstack.org/567924 | |
| 13:13:27 | tssurya | mriedem, dansmith: do you have some time now or shall I ping you back later ? | |
| 13:14:41 | mriedem | i'm here | |
| 13:15:50 | tssurya | mriedem: so is it documented somewhere that when a user does nova list --deleted, it silently ignores this | |
| 13:15:57 | tssurya | without complaining about policy ? | |
| 13:16:10 | mriedem | for non-admins? | |
| 13:16:21 | tssurya | yes | |
| 13:16:43 | mriedem | i'm not aware of something saying it's not an error for non-admins | |
| 13:16:54 | mriedem | https://docs.openstack.org/python-novaclient/latest/cli/nova.html#nova-list says (admin only) | |
| 13:17:02 | tssurya | hmm okay then I will open a bug | |
| 13:17:11 | mriedem | bug for what? | |
| 13:17:28 | tssurya | because currently if you do nova list --deleted as a non-admin | |
| 13:17:32 | tssurya | it just ignores the filter | |
| 13:17:39 | tssurya | and gives you all the active instances | |
| 13:17:39 | mriedem | it's been that way forever | |
| 13:18:03 | mriedem | changing that would likely be a non-trivial backward incompatible behavior change in the api | |
| 13:18:26 | tssurya | oh so why do the rest of the admin only filters give policy errors | |
| 13:18:32 | mriedem | see remove_invalid_options() | |
| 13:18:59 | mriedem | which filters for example? | |
| 13:19:09 | mriedem | also, you should probably include alex_xu and/or gmann in this discussion | |
| 13:19:16 | tssurya | --user or --all-tenants etcc | |
| 13:19:29 | tssurya | I mean all those filters which say (admin only) | |
| 13:19:38 | tssurya | shouldn't they all behave in the same way ? | |
| 13:19:59 | mriedem | i see | |
| 13:20:00 | mriedem | msg = _("Only administrators may list deleted instances") | |
| 13:20:00 | mriedem | raise exc.HTTPForbidden(explanation=msg) | |
| 13:20:24 | mriedem | but looks like that's only if you're filtering on vm_state='deleted' | |
| 13:20:39 | tssurya | yea | |
| 13:20:48 | openstackgerrit | Merged openstack/nova master: Remove duplicate parameter in API sample documents https://review.openstack.org/581203 | |
| 13:20:49 | tssurya | doing --status DELETED is caught correctly | |
| 13:21:00 | tssurya | thanks alex_xu | |
| 13:21:38 | mriedem | tssurya: it's also possible today to just pass random query params w/o an error | |
| 13:21:42 | mriedem | GET /servers?foo=bar | |
| 13:21:54 | mriedem | for backward compatibility reasons | |
| 13:22:03 | tssurya | mriedem: ah okay | |
| 13:22:15 | mriedem | if we made the query parameter whitelist a true whitelist and failure for unknown parameters, that would be a microversion | |
| 13:23:17 | tssurya | mriedem: hmm, yea I don't know if it needs to change, I noticed this when I was playing around with policy/filters | |
| 13:23:22 | alex_xu | the API behaviour is ignoring any invalid parameters | |
| 13:23:37 | mriedem | https://github.com/openstack/nova/blob/b67a9a111ab0cf68084f62017a74f9a41377244f/nova/api/openstack/compute/schemas/servers.py#L599 | |
| 13:24:26 | openstackgerrit | Matthew Booth proposed openstack/nova master: Avoid redundant initialize_connection on source post live migration https://review.openstack.org/551302 | |
| 13:24:27 | openstackgerrit | Matthew Booth proposed openstack/nova master: Restore connection_info after live migration rollback https://review.openstack.org/551349 | |
| 13:26:08 | openstackgerrit | Silvan Kaiser proposed openstack/nova master: Exec systemd-run with privileges in Quobyte driver https://review.openstack.org/554195 | |
| 13:27:35 | mriedem | also, for example, 2.26 adds the *tags* query params but they are only effective if using >= 2.26, if you pass tags=1,2 with 2.1, they are just ignored | |
| 13:27:39 | tssurya | alex_xu, mriedem: thanks for the pointer to the code/confirmation! it was just strange that nova list --deleted and nova list --status DELETED behaved differently. | |
| 13:27:41 | mriedem | because 'additionalProperties': True, | |
| 13:28:31 | tssurya | right | |
| 13:28:52 | mriedem | it also looks like if you pass deleted=cookies it will accept that and default deleted=False | |
| 13:29:10 | tssurya | heh yea | |
| 13:29:17 | mriedem | https://github.com/openstack/nova/blob/b67a9a111ab0cf68084f62017a74f9a41377244f/nova/api/openstack/compute/servers.py#L187 | |
| 13:30:15 | mriedem | why we treat --status DELETED and --deleted differently, idk | |
| 13:30:24 | mriedem | looks like that was pre-microversions | |
| 13:30:36 | tssurya | yea , well is this documented somewhere outside the code ? | |
| 13:31:05 | tssurya | (I couldn't find it) | |
| 13:31:10 | mriedem | tssurya: i'm not aware of something about this being documented off the top of my head - i'd check the compute API guide since that's where i'd expect something like this to exist, if at all | |
| 13:31:24 | alex_xu | tssurya: mriedem in the before, we have a proposal to change the all_tenants policy https://review.openstack.org/#/c/527019/3/specs/ocata/implemented/add-whitelist-for-server-list-filter-sort-parameters.rst@68 | |
| 13:31:30 | alex_xu | but we didn't implement it | |
| 13:31:46 | openstackgerrit | Silvan Kaiser proposed openstack/nova master: Exec systemd-run with privileges in Quobyte driver https://review.openstack.org/554195 | |
| 13:32:13 | tssurya | mriedem, alex_xu: ack | |
| 13:37:28 | mriedem | https://review.openstack.org/#/c/11469/ added the ability to do nova list --status DELETED | |
| 13:37:32 | mriedem | and fails for non-admins | |
| 13:38:12 | mriedem | which is so long ago there would have been no consideration of consistency with failing a non-admin passing deleted=True | |
| 13:39:16 | mriedem | it feels like changing that now to fail for non-admins would require a microversion but i could see the argument both ways | |
| 13:39:50 | mriedem | there is something to be said for explicit api behavior, i.e. if i'm a non-admin doing nova list --deleted and not getting back deleted instances, i might not realize i'm using the wrong rc file and think it's a bug | |
| 13:40:12 | mriedem | i'm sure we have crap like this all over the api | |
| 13:40:23 | tssurya_ | Haha | |
| 13:40:37 | mriedem | there are lots of "make this strict in a new microversion" notes in lots of that code | |
| 13:40:50 | mriedem | for example, lots of the schema validation that has additionalProperties: true | |
| 13:40:59 | mriedem | and fixing error response codes | |
| 13:41:26 | mriedem | if someone had the energy and care to wrangle a lot of those things into a spec we could fix them in a single microversion | |
| 13:42:16 | mriedem | validation just gets hard when you have 80 *known* query params and some of those have behavior characteristics depending on microversion and/or what other parameters are passed | |
| 13:42:37 | tssurya_ | Hmm okay so for now I just ignore this or should I open a bug / should we resurrect the whitelist spec next cycle ? | |
| 13:43:08 | mriedem | if you care to document it, that would be something | |
| 13:43:25 | mriedem | https://developer.openstack.org/api-guide/compute/server_concepts.html#server-query | |
| 13:43:43 | tssurya_ | Ack I will put some doc patch then | |
| 13:43:46 | mriedem | "Other options will be ignored by nova silently." | |
| 13:44:03 | mriedem | that first bullet basically says what you're hitting | |
| 13:44:04 | mriedem | "For general user, there is limited set of attributes of the servers can be used as query option. reservation_id, name, status, image, flavor, ip, changes-since, ip6, tags, tags-any, not-tags, not-tags-any are supported options to be used. Other options will be ignored by nova silently." | |
| 13:44:20 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix hugepages memory usage per intances https://review.openstack.org/580657 | |
| 13:44:21 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix memory check usage for small/large pages https://review.openstack.org/532168 | |
| 13:44:22 | openstackgerrit | sahid proposed openstack/nova master: hardware: remove duplicate memory check https://review.openstack.org/581365 | |
| 13:45:42 | mriedem | alex_xu: do you know why we have all_tenants in _get_server_search_options and then we just check a policy if you specify it and fail with a 403 if you're not an admin? | |
| 13:46:50 | alex_xu | mriedem: I don't know why, probably just a feature people want to control the visibility of all_tenant by the policy in the before | |
| 13:47:21 | mriedem | that's one way we could handle filtering on deleted - add a policy rule which defaults to rule:admin_api | |
| 13:48:11 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix hugepages memory usage per intances https://review.openstack.org/580657 | |
| 13:48:12 | openstackgerrit | sahid proposed openstack/nova master: hardware: remove duplicate memory check https://review.openstack.org/581365 | |
| 13:48:13 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix memory check usage for small/large pages https://review.openstack.org/532168 | |
| 13:48:21 | alex_xu | you mean the status=deleted? | |
| 13:48:38 | mriedem | well, that and deleted=True | |
| 13:48:44 | mriedem | we're inconsistent today, | |
| 13:48:52 | mriedem | for a non-admin, if i do 'nova list --deleted' it's ignored | |