Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-11-12
19:50:48 mriedem b/c the object is using the wrong column name
19:51:07 sean-k-mooney sure but i was wondering if you planned to chagne that
19:52:03 mriedem i think it would be useful to at least expose to the non-admin owner of the server the exception type, like we do for a fault message
19:53:37 sean-k-mooney ya maybe. as long as its just the type and not the body of the excption its proably safe. although im not sure all operator would be ok with that
19:53:55 sean-k-mooney e.g. it might affect there sla depening on what the falut was
19:54:13 mriedem if the instance goes to ERROR status we're already showing the exception type in the fault message
19:54:25 mriedem if not the formatted message
19:54:30 sean-k-mooney ya that is ture
19:54:47 sean-k-mooney i normally am logged in as root but we get the message in that case at least
19:54:56 sean-k-mooney * admin
19:55:05 sean-k-mooney so im not sure what a non admin sees
20:10:55 sean-k-mooney artom: :) looks like it worked https://openstack.fortnebula.com:13808/v1/AUTH_e8fd161dc34c421a979a9e6421f823e9/zuul_opendev_logs_3ae/691062/59/check/whitebox-multinode-devstack/3ae547c/testr_results.html.gz
20:11:29 sean-k-mooney we still need to adress the skips
20:12:14 sean-k-mooney but the ssh keys worked correctly and it looks liek the hugepages allocation did not cause memory issues
20:23:15 mriedem dansmith: your suggestions here to use the tuples for comparing times doesn't work in all cases https://review.opendev.org/#/c/636224/50/nova/compute/api.py@4956
20:23:51 mriedem and i don't really want to bake a bunch of conditional logic into unreadable tuples
20:25:22 dansmith aight, well, do what you want
20:25:32 dansmith inlining all that in a closure seems bad,
20:25:46 dansmith especially for testability, which has clearly manifested itself already
20:26:09 dansmith that could be a
20:26:16 dansmith def newer_than(obj)
20:26:42 dansmith method on NovaObject and be usable for other things, as well as easier to test
20:27:11 dansmith is there not some way we could do this without times?
20:27:22 dansmith because that's kinda smelly in general,
20:27:33 sean-k-mooney i was going to suggest a before or after method or something but ya the tuple way would be nice if it was not for the value error
20:27:37 dansmith assuming they're all time-synced, you don't have any delays, delayed transactions, etc
20:27:45 sean-k-mooney *type error
20:29:01 sean-k-mooney oh this is in get_newer_job already
20:29:31 sean-k-mooney you could define a lessthan operator on the migration object
20:29:45 sean-k-mooney although i guess that would have the same proably with None
20:29:57 dansmith no, it wouldn't
20:30:08 dansmith but also, less-than can mean lots of things other than time
20:30:20 sean-k-mooney for a migration object
20:30:26 sean-k-mooney and ya i could
20:30:42 mriedem we could just say f it and do like get_all and filter out by uuid after we've already processed one
20:31:13 mriedem this https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2883
20:31:49 dansmith mriedem: yeah
20:32:12 mriedem the migration from the first cell processed in the results is always the one that is returned,
20:32:20 mriedem which could be wrong, but i don't really care at this point
20:32:40 dansmith does it matter/
20:32:51 dansmith you just want to not return dupes right?
20:33:09 mriedem yes, but also figure that since we can know which is "newer" we should return that,
20:33:29 mriedem because the migration in the target cell is the one that's going to be getting updated as the resize progresses
20:33:42 mriedem including when it goes to 'finished' status and the server is in VERIFY_RESIZE status
20:34:16 sean-k-mooney mriedem ya i think the nestest makes sense
20:34:17 mriedem which is kind of important since i think i have functional tests later in the series asserting the migratoins api returns 1 and it's the correct status
20:34:26 dansmith mriedem: once you create the new one, couldn't you set the hidden flag on the old one or something/
20:34:48 mriedem sure, that's the alternative mentioned in the code and the commit message
20:34:59 mriedem but it has implications for the api because that hidden field has never been set before
20:35:01 dansmith the *newest* only makes sense if we never update the old migration
20:35:41 dansmith mriedem: I don't understand what the problem with that is, as I said.. api ignores migrations with the hidden field already ...
20:35:50 mriedem i don't think we do update the source cell migration unless we revert the resize
20:36:13 dansmith we just leave it pending forever?
20:36:36 mriedem no, when you confirm the resize in the target cell we hard destroy the source cell instance which will hard destroy it's related migratoin in the source cell db,
20:36:44 mriedem same (in reverse) for revert
20:37:02 dansmith what I meant is.. we leave it in the half-finished state until that time comes
20:37:04 mriedem otherwise you can never resize back to that cell b/c of the duplicate instance uuid
20:37:13 mriedem pretty sure yeah
20:37:21 sean-k-mooney so in that case cant we have teh db return the latest version based on update or created time
20:37:29 mriedem sean-k-mooney: different dbs
20:37:30 mriedem so no
20:38:10 sean-k-mooney but we only need to look at the one of the dbs if we only want the latest one right? i gues we would have too look at all of them during a migration
20:38:18 sean-k-mooney ya ok
20:38:25 dansmith sean-k-mooney: maybe you should read the patch we're discussing?
20:38:43 sean-k-mooney i am but im also about to head off for the night
20:39:51 sean-k-mooney the reason i changed my mind half way true was the Verify_resize comment
20:43:37 mriedem so it's not just the active migration that is a duplicate either, it's all migrations linked to the instance - those get copied into the target cell db early in TargetDBSetupTask, because once we flip the instance mapping that's where the api is going to go to get migrations for that server,
20:43:59 mriedem so if we started setting the hidden field on migrations, we'd have to do it on all of them until we're sure where the server is going to ultimately be
20:44:38 efried If I initiate an operation as an admin user, does the context have is_admin set, or does that only happen when nova generates an admin context explicitly?
20:44:51 mriedem is_admin is set
20:45:23 mriedem the difference is one has a token/user/project and the one nova creates for some periodics and stuff doesnt
20:46:35 mriedem https://github.com/openstack/nova/blob/master/nova/context.py#L141 https://github.com/openstack/nova/blob/master/nova/policy.py#L197 https://github.com/openstack/nova/blob/master/nova/policies/base.py#L24
20:48:25 sean-k-mooney mriedem: without your patch do we ignore the cell maping and simple cast to all cells by the way or just the cell where the instnce is
20:48:55 mriedem for what api?
20:49:01 mriedem GET /os-migrations iterates all cells
20:49:49 efried mriedem: perhaps the better question is: can I create an instance as an admin user?
20:49:50 sean-k-mooney based on what you jsut said i would not need too right since we would be copying all the migrations to the new cell
20:50:10 artom sean-k-mooney, yeah I am le much flabbergast
20:51:46 sean-k-mooney efried: an admin user is just a normal user with extra privladtes so it can have a project and a quota
20:52:20 sean-k-mooney efried: so the admin use that gets set up with devstack for example can
20:52:37 efried So I think you're saying that I should be able to tell based on whether project_id is set?
20:52:37 efried While generating libvirt xml, I need a context. The way the code is set up right now, sometimes the context comes from the user and sometimes it comes from get_admin_context(). I want to detect (and reject) the latter.
20:52:37 efried or maybe I should just say what I'm actually trying to do:
20:53:13 sean-k-mooney why do you want to reject the later
20:53:24 sean-k-mooney admin users shoudl be able to create vms with vtpm
20:53:45 sean-k-mooney just not on behalf of another user/project
20:53:54 mriedem sean-k-mooney: we need to copy the migrations to the target cell db because, like i said above, if the user confirms the resize there we hard-destroy the instance and it's old migrations in the source cell db
20:54:31 sean-k-mooney mriedem: yes im just wondering of we can optimese the /os_migration api to only look at the cell where the vm is currently
20:54:46 mriedem efried: a context created by nova.context.get_admin_context won't have a project_id, correct
20:54:57 sean-k-mooney that info should be in the instnace maping in the api db right?
20:54:59 efried sean-k-mooney: Like we were talking about yesterday, I need to be able to bounce on code paths where $not_the_owner_of_the_instance is trying to generate xml with a vtpm secret in it.
20:55:25 mriedem sean-k-mooney: what info?
20:55:27 efried I guess I could just let the key retrieval fail.
20:55:37 sean-k-mooney mriedem: which cell the vm is in
20:55:41 dansmith mriedem: sean-k-mooney has not read the patch and doesn't realize we're listing all migrations, not just for a specific instance
20:55:43 mriedem correct
20:55:50 sean-k-mooney dansmith: yes i have
20:56:07 sean-k-mooney oh i though you were listing all migrtion for the instace
20:56:09 mriedem sean-k-mooney: but the instance mapping saying the instance is in cell X doesn't mean the migration records from cell Y are in cell X unless we copy them there
20:56:12 sean-k-mooney not all migrtions in general

Earlier   Later