Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-22
15:07:03 openstackgerrit Surya Seetharaman proposed openstack/nova master: [WIP] Add 'power-update' external event to listen to ironic https://review.openstack.org/645611
15:19:35 mriedem dansmith: do you see any reason to not add a locked_reason to the instances table vs system_metadata or something? https://review.openstack.org/#/c/638629/1/specs/train/approved/add-locked-reason.rst@42
15:20:20 mriedem i get a bit of the heeby jeebys shoving more non-filterable/indexable columns in the instances table for just system metadata type purposes, but if it doesn't cause any issues for indexing and the other locked/locked_by columns are already in there, then i suppose it's fine
15:20:56 mriedem i'd ask jay but he's gone
15:20:58 dansmith and requires rewriting the instances table, for what is just metadata
15:21:10 mriedem right - it's a schema migration on a large table
15:21:12 dansmith i.e. non-critical information that won't be there for some instances anyway
15:21:13 dansmith right
15:21:41 mriedem i'm not advocating a big ass data migration to move the existing locked/locked_by to another table as a blocker on this, but seems we shouldn't pile on
15:21:49 dansmith yeah
15:22:01 mriedem despite it will be confusing to have a new locked_* field in a different place now, but the Instance object abstracts that
15:22:09 dansmith moving it just means we have a collapse we have to do later (or leave it dangling) so moving is less good than just putting new things in the right-er place
15:22:50 dansmith and really, you only need that info in the api, right? you put it on the instance and you're loading it all the time...
15:22:57 mriedem correct
15:23:07 dansmith how long of a reason are we proposing?
15:23:10 mriedem purely non-queryable meta
15:23:13 mriedem 255
15:23:17 dansmith because we get a free 255 char limit with sysmeta
15:23:18 dansmith ack
15:23:19 mriedem which is exactly what system_metadata value is
15:23:21 dansmith yeah
15:25:29 dansmith seems all around better to put it in sysmeta to me, aside from the purely-developer reason of wanting them to look like they're next to each other in one data model at the bottom of the stack
15:25:39 dansmith ...which I get of course
15:25:40 dansmith but.
15:26:22 dansmith the argument of putting it instances because the other locked-by is there, is like saying we should have stored the pre-resize vm_state in instances because that's where vm_state is, which would be insane reasoning
15:26:30 dansmith and since most instances are unlocked for most of their life....
15:31:13 mriedem ok are you going to -1? if not i'll just leave a comment with a link to this conversation
15:31:50 tssurya mriedem, dansmith: I'll update the spec to add it to the metadata table
15:32:39 dansmith tssurya: I think that's the way to go, especially since it's just changing the spec to remove the db migration, schema migration and just say "stuff it into sysmeta" :)
15:33:04 dansmith it'll be much easier to merge that way too
15:33:46 tssurya dansmith: haha sure, for some reason I wanted to have all the lock related info in the same place, but yea sure we could split it to the metadata
15:33:57 tssurya hopefully I won't have to move the locked_by right ?
15:34:01 tssurya let that stay there ?
15:34:05 dansmith tssurya: no don't move the existing stuff
15:34:10 tssurya cool! :)
15:35:43 mriedem tssurya: i'm leaving comments on PS3
15:35:45 mriedem so hold up
15:36:06 tssurya mriedem: okay
15:44:40 mriedem tssurya: done
15:44:46 tssurya thanks
15:45:02 mriedem elbragstad: so we've got this silly locked_by field on the instance object in our db which is an enum of 'admin' or 'owner',
15:45:24 mriedem https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4016
15:46:05 mriedem i guess in this case admin is a non-project admin, so like a system scope admin
15:46:20 mriedem anyway, there is a proposal to expose that out of the rest api https://review.openstack.org/#/c/638629/3/specs/train/approved/add-locked-reason.rst@117
15:46:37 mriedem do you see any issues with future policy scope creep weirdness with that?
15:46:48 dansmith sure seems like that should be a userid and not an enum
15:47:14 dansmith or
15:47:14 dansmith (before we expose it I mean)
15:47:18 mriedem it's hella old
15:47:22 dansmith I know
15:47:45 gmann mriedem: dansmith as in future it can be locked by system.admin and project.admin as well we should make it user-id.
15:47:46 dansmith so when we expose it, maybe we should make it possible for it to be owner|admin|other, so that later if we store a user-id, we can say "other" there
15:48:06 gmann otherwise we will not know it is locked by project admin or system admin
15:48:19 mriedem gmann: today if you lock as project admin it's 'owner'
15:48:32 mriedem it's only 'admin' if the request comes from an admin in a different project
15:49:55 dansmith tssurya: just added a comment about that
15:50:04 gmann mriedem: no, it is 'admin' if project admin lock any non-admin user server of same project
15:50:07 tssurya dansmith: checking
15:50:18 mriedem looks like it was added in https://review.openstack.org/#/c/38196/
15:50:37 dansmith god the world was so much simpler in 2013
15:50:43 mriedem gmann: ? is_owner = instance.project_id == context.project_id
15:50:53 mriedem as long as the project id matches, it's owner
15:50:56 mriedem despite the role
15:51:01 gmann ohh yeah we only match project id. soory
15:51:03 gmann sorry
15:51:48 dansmith "soory" correct canadian for "sorry"
15:52:01 gmann :)
15:52:01 mriedem he's fitting right in
15:52:21 gmann so we assume 'admin' is system-admin which again might be confusing
15:52:36 mriedem hmm, i wonder if locked_by is still used on unlock https://review.openstack.org/#/c/38196/13/nova/compute/api.py@2490
15:52:51 dansmith it's what gates whether or not you can unlock right?
15:52:54 gmann I am adding lock as [system, project] scope_type in my PoC of scope_type - https://review.openstack.org/#/c/645452/2/nova/policies/lock_server.py
15:53:06 mriedem ah it is
15:53:49 gmann but we have overridden unlock also i think that is for admin to unlock owner server
15:53:52 gmann owner locked server
15:54:11 mriedem os_compute_api:os-lock-server:unlock:unlock_override
15:54:12 mriedem yuck
15:54:23 mriedem baking policy into the db
15:54:44 gmann yeah, we should remove that also. not sure any reason to keep that
15:54:44 dansmith ready for friday to be over already?
15:56:29 gmann are we going to expose 'locked_by' also ? not checked the spec yet
15:57:27 tssurya dansmith: umm you want to add "other" where ? as in locked_by enum schema needs to change ? or just in the response api terminology for future needs ?
15:57:42 dansmith tssurya: just the api schema for the future
15:57:54 tssurya okay
15:58:00 dansmith we should get some agreement on that (and the actual enum value) first though
15:58:06 tssurya yea
15:58:07 mriedem dansmith: i replied to that comment,
15:58:11 dansmith and mriedem is too busy barfing
15:58:13 mriedem Instance.locked_by is just a string
15:58:21 mriedem which will get spewed back out of the api response
15:58:36 mriedem so if we allow storing a user_id in there in the future, it's a db schema change but shouldn't really mean any change to the instance object or api
15:58:41 dansmith oh sorry, I assumed it was an enum in the json schema too
15:58:52 mriedem it might be in tempest...checking
15:59:06 mriedem oh nvm, we don't expose it today
15:59:08 gmann we only expose 'lock' as string in API response
15:59:09 dansmith it says admin or owner
15:59:12 mriedem right
15:59:16 gmann we do not expose 'locked_by'
15:59:22 mriedem tempest is the only thing that would have a response schema check
15:59:25 dansmith mriedem: right the thing she's adding says "admin or owner"

Earlier   Later