| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-22 | |||
| 15:01:38 | sean-k-mooney | smcginnis: yes i know but we dont validate that every file listed in pip feeze is in lower-constratis | |
| 15:02:03 | sean-k-mooney | the original file was generated that way but there is notheing to enforce it so it could be out of sync | |
| 15:02:41 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix return param docstring in check_can_live_migrate* methods https://review.openstack.org/645610 | |
| 15:02:44 | sean-k-mooney | lower-constratins also needs to have our depencies dependcies listed as we do not use our deps lower-constraitns recursivly | |
| 15:03:22 | sean-k-mooney | anyway ill email the list once i have a working fix for nova that work with master | |
| 15:04:39 | sean-k-mooney | i have a partial fix but as i said our unit tests mocking does not work with lower constratints as we end up using the oslo messaging amqp driver instead of the fake driver in some tests and the mocking does not work | |
| 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 | |