| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-10 | |||
| 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 | |
| 13:48:59 | mriedem | but if i do 'nova list --status DELETED' i get a 403 | |
| 13:49:06 | mriedem | but it's essentially the same filter | |
| 13:49:13 | alex_xu | yes... | |
| 13:50:12 | mriedem | this is all the api guide says about the deleted filter: "deleted returns (or excludes) deleted servers" | |
| 13:50:30 | mriedem | so we could expand on that if we wanted to include a note about the difference in behavior between --status DELETED and --deleted | |
| 13:50:39 | mriedem | and that it's admin-only either way | |
| 13:50:51 | efried | stephenfin: Then should the non-systemd mount be run under privsep too? | |
| 13:51:00 | mriedem | tssurya: so if you're going to update the api guide, that's what i'd update | |
| 13:52:45 | alex_xu | mriedem: nova list --deleted consistent with the query parameter behaviour. nova list --status DELETED consistent with the behavour of valid value of query parameter. | |
| 13:53:15 | tssurya_ | mriedem: okay, | |
| 13:53:25 | mriedem | alex_xu: i'm not sure i get your point | |
| 13:53:30 | alex_xu | mriedem: we also return 400 if you fill invalid date format for --change-sinces, we won't ignore that invalid value | |
| 13:53:49 | stephenfin | efried: Based on silvans comments, the umount is more generic and therefore needs the additional permissions | |
| 13:54:03 | stephenfin | This is where I'd normally have lyarwood weigh in but I think he's not around this week | |
| 13:54:04 | mriedem | alex_xu: i mean this https://github.com/openstack/nova/blob/b67a9a111ab0cf68084f62017a74f9a41377244f/nova/api/openstack/compute/servers.py#L194 | |
| 13:54:08 | mriedem | for nova list --status DELETED | |
| 13:54:15 | stephenfin | Maybe mdbooth would have an opinion though? ^ | |
| 13:54:38 | stephenfin | mdbooth: RE: efried's second comment here https://review.openstack.org/#/c/554195/14/nova/privsep/libvirt.py Any ideas? | |
| 13:54:43 | alex_xu | mriedem: yes, DELETED is invalid value for status parameter for non-admin | |
| 13:54:53 | tssurya | mriedem: alex_xu :wouldn't the right way to deal with --deleted actually be to add the default rule:is_admin so that it behaves sanely ? | |
| 13:55:02 | tssurya | like mriedem said above | |
| 13:55:25 | mdbooth | stephenfin: Reading... | |
| 13:55:43 | alex_xu | mriedem: it is same with the other parameters. If you put an invalid date format in changes-since parameter you will get a 400 also | |
| 13:55:57 | mriedem | but deleted=cookies is ok | |
| 13:56:12 | mriedem | i found the change that added that too, and it was a compromise on existing behavior | |
| 13:56:37 | alex_xu | mriedem: we didn't ignore deleted=cookikes, I guess, it is just map to a bool | |
| 13:56:58 | mriedem | it defaults to False if it's an invalid bool | |
| 13:57:25 | mriedem | https://review.openstack.org/#/c/173296/ | |
| 13:58:11 | mriedem | there was a young alex_xu fighting the good fight on that too https://review.openstack.org/#/c/173296/2/nova/api/openstack/compute/plugins/v3/servers.py | |
| 13:58:37 | alex_xu | in my childhood | |
| 13:58:41 | mriedem | ha | |
| 13:59:27 | mriedem | looks like the TODO from that review was never added | |
| 13:59:55 | mriedem | we could start an etherpad to track various gross little api issues like this if we wanted to tackle those in a single microversion in stein | |
| 14:02:20 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix hugepages memory usage per intances https://review.openstack.org/580657 | |
| 14:02:21 | openstackgerrit | sahid proposed openstack/nova master: hardware: remove duplicate memory check https://review.openstack.org/581365 | |
| 14:02:22 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix memory check usage for small/large pages https://review.openstack.org/532168 | |
| 14:10:27 | tssurya | mriedem, dansmith: another question I had was regarding https://review.openstack.org/#/c/557369/5/specs/rocky/approved/handling-down-cell.rst@74 , for updating queued_for_delete, since I can't do upcalls, can I just go ahead and mark this in the compute API before it is casted, even if the deletion fails ? and we document this specifically since its only "queued" for deletion ? | |
| 14:10:55 | tssurya | (although it doesn't seem very right/ its kind of stale) | |
| 14:11:08 | mriedem | tssurya: i commented on that specific issue in one of my earlier reviews | |
| 14:11:17 | mriedem | yes we have to set the flag in the api before casting to compute to do the soft_delete | |
| 14:11:20 | tssurya | which is what I am pointing at :) | |
| 14:11:32 | mdbooth | stephenfin efried: What are you asking specifically? | |
| 14:11:57 | dansmith | tssurya: mriedem right, and only look at that if the cell is down, otherwise honor what is in the cell db right? | |
| 14:12:06 | stephenfin | mdbooth: Why it's necessary to get root privileges to unmount volumes yet not to mount | |
| 14:12:08 | mdbooth | stephenfin: That doesn't look like a robust way to determine if systemd is functional to me. | |
| 14:12:22 | mdbooth | stephenfin: I assume that's because quobyte is weird? | |
| 14:12:30 | mriedem | dansmith: this is setting queued_for_delete=True when the cell is up | |
| 14:12:41 | mriedem | which you have to do in the api before casting to compute | |
| 14:12:56 | dansmith | mriedem: *setting* for sure, but then you don't *look* at it unless the cell is down and have to decide whether or not to show the shell instance yeah? | |
| 14:13:00 | mdbooth | stephenfin: Not being facetious, btw. A quobyte driver seems like the right place to hide that sort of stuff. | |
| 14:13:01 | mriedem | and there are 3 places that can happen (1) normal delete (Cast to compute), (2) local delete (compute is down), and (3) soft delete | |
| 14:13:04 | tssurya | dansmith: at the time of deletion | |
| 14:13:09 | stephenfin | mdbooth: Neither do I, but I guess that's a separate issue and someone will eventually report it if not | |
| 14:13:22 | dansmith | tssurya: I know, at time of deletion, you have to set it in the api when they first request it | |
| 14:13:36 | mdbooth | stephenfin: I couldn't work out from the bug report what the actual problem was. | |
| 14:13:42 | tssurya | dansmith: right we will not look at this unless a cell is down | |
| 14:13:44 | tssurya | got it | |
| 14:14:18 | dansmith | tssurya: mriedem: right, I'm just trying to explain, that's why we set it presumptively, because even though the delete hasn't happened, we only use this as a backup, so the fact that it might be a little aggressive (set before the delete happens), it's only for the emergency cas | |
| 14:14:20 | dansmith | *case | |
| 14:14:43 | dansmith | and why we _can_ set it that way, because it won't affect normal get/list calls | |
| 14:15:28 | stephenfin | mdbooth: Yeah, I was working on the same assumption but I was curious if there was something I was missing | |
| 14:15:39 | tssurya | dansmith: absolutely, thanks for confirming | |
| 14:15:43 | tssurya | thanks mriedem | |
| 14:16:04 | mdbooth | stephenfin: Although... it could be that root isn't required. | |
| 14:16:41 | mdbooth | stephenfin: kaisers blames it on the (nova) volume rather than quobyte | |
| 14:16:48 | mdbooth | s/volume/driver/ | |
| 14:17:02 | stephenfin | mdbooth: I think it's systemd itself at fault here https://github.com/systemd/systemd/issues/3388 | |