Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-22
14:49:55 sean-k-mooney cdent: placement and os-vif only use lower-constratis in our lower-constratis job
14:50:31 cdent sean-k-mooney: and?
14:50:34 sean-k-mooney but if we have a transitive depency that is capped in upper-constratits but not in our lower-constratis then it will be uncapped
14:51:02 sean-k-mooney given os-vif and placement have relitivly few depencies we are proably fine
14:51:34 sean-k-mooney but for nova or larger project we actully should use both but lower constraints needs to be passed first
14:51:47 mgoddard dansmith: qq for when you return - would you recommend a full restart of all nova services rather than HUP, including those where we haven't seen issues with HUP (i.e. all except compute)?
14:52:08 cdent my changes to nova's lower-constraints job make it so the upper-constraints file is not included in the test (which it had been before), and that caused conflicts
14:52:52 sean-k-mooney yes and that is more correct but the actually fix should be to include lower-constratis first followed by upper
14:53:02 dansmith mgoddard: yes to restart of compute instead of hup, although it'll still cause in-flight boots to fail.. don't think it's really a concern for most of the other services, so probably okay to hup them
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

Earlier   Later