| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-22 | |||
| 14:53:16 | sean-k-mooney | the pip code uses the first constratit it finds for a dep | |
| 14:53:58 | sean-k-mooney | cdent: the main issue i have found is that mocking does not work correctly with the lower constrating deps as i think things moved | |
| 14:54:14 | sean-k-mooney | cdent: ill pull down your version and rebase and see if i see the same issue | |
| 14:54:15 | mgoddard | dansmith: at this point it's just been upgraded so will have restarted anyway - another restart won't hurt too much. I'll go with that, thanks | |
| 14:54:27 | dansmith | ack | |
| 14:54:43 | sean-k-mooney | i can combine both patches and see if i can get something that works fully | |
| 14:57:33 | sean-k-mooney | smcginnis: and yes the cinder code https://opendev.org/openstack/cinder/src/branch/master/tox.ini#L15-L18 is what we do in os-vif and it almost right they should also have upper-constriatns listed here https://opendev.org/openstack/cinder/src/branch/master/tox.ini#L198 after lower. | |
| 14:59:02 | smcginnis | sean-k-mooney: I don't think we want upper constraints listed in the lower constraints job on line 198. The point of that is to make sure it only uses the lower constraints. | |
| 14:59:12 | sean-k-mooney | smcginnis: the way os-vif placement and cinder execute lower-constraitns is actully probably fine 99% of the time as the are using lower-constraints for all there direct dependcies | |
| 14:59:57 | sean-k-mooney | smcginnis: if lower constratits does not list every dep we install then not having it in line 198 will leave deps uncapped | |
| 15:00:11 | sean-k-mooney | assuming those deps are capped in upper | |
| 15:00:20 | sean-k-mooney | that is the only case where it would be needed | |
| 15:00:24 | smcginnis | But lower should list every dependency. | |
| 15:00:39 | sean-k-mooney | it should but i dont think we enforce that | |
| 15:00:41 | smcginnis | Anything in requirements.txt should be in lower-constraints.txt | |
| 15:01:04 | smcginnis | The point of the job is to capture and validate all the requirement's minimum allowable versions. | |
| 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 | (before we expose it I mean) | |
| 15:47:14 | dansmith | or | |
| 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 | mriedem | he's fitting right in | |
| 15:52:01 | gmann | :) | |
| 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 | |