Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-03-25
15:00:43 mriedem nightmare_unreal: ok comments inline
15:00:56 nightmare_unreal thanks :)
15:01:03 mriedem gibi: it's also nice to review something outside of github too
15:01:58 brinzhang sean-k-mooney: gmann: If we re-using the os-isntance-actions: events policy, we want to expose noValidHost and other information to the non-admin, which cannot be changed by modifying the policy, is it?
15:02:25 mriedem nightmare_unreal: as to your original question, this put_allocations method is the one that overwrites the allocations for an instance https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L1948
15:02:27 brinzhang we dont want to expose the traceback to the non-admin user
15:02:28 gibi mriedem: I don't have too much experience with github but I imagine gerrit is a nicser interface
15:02:33 gibi nicer
15:02:43 mriedem gibi: just...different
15:03:02 mriedem unified diff in github reviews isn't terrible
15:03:12 nightmare_unreal new to gerrit though
15:03:31 nightmare_unreal thanks mriedem , I will work on it and submit again.
15:03:54 mriedem err i should say split diff i guess to be like how i used gerrit
15:05:05 mriedem nightmare_unreal: so the way heal_allocations works is we determine if an instance needs healing and the conditional for that is here https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L1922
15:05:08 gmann brinzhang: os-isntance-actions: events policy is admin by default
15:05:21 mriedem bypassing that is essentially a --force option or something like that
15:05:22 sean-k-mooney i prefer gerrit for revew also. im not really a fan of the pull request workflow but its still beter then doing things by email
15:05:27 gmann so it would not expose those info to non-admin. until override to do so
15:06:41 nightmare_unreal mriedem: ah okay thanks for the guidance .
15:06:53 mriedem nightmare_unreal: the thing that gets tricky is probably the network port allocations logic in there since you can tell the command to skip that, so we could have some problems if you skip healing port allocatoins but then forcefully overwrite the existing allocations to match the current flavor
15:07:25 mriedem so that probably means if you add a --force option or something it probably needs to be mutually exclusive with --skip-port-allocations
15:07:48 brinzhang We need a policy to expose the details to non-admin (from modify the default policy), if we just use de admin default, maybe the traceback can do anything, we dont need to populate details
15:07:50 mriedem this is why functional tests are better for changes to this command because there are a lot of moving parts
15:08:00 brinzhang gmann
15:08:07 nightmare_unreal okay
15:09:23 brinzhang gmann:https://review.opendev.org/#/c/699669/3/specs/ussuri/approved/action-event-fault-details.rst@54
15:11:08 brinzhang gmann: My use case is for non-admin, but at the end need to add system_reader role default, but we should allow user change the default poliy to show the details for non-admin user
15:12:08 brinzhang gmann: That's why I insist on using the new policy for show details
15:15:24 brinzhang gmann: as gibi comment in https://review.opendev.org/#/c/699669/2/specs/ussuri/approved/action-event-fault-details.rst@128
15:16:34 luyao lyarwood: Hi, I replied on https://review.opendev.org/#/c/714593/ in detail, as you commented, new cleanup method might be better, but it's really huge change, I think we need a bp, I'm willing to do this but later, my target is vpmem live migration in this release.:)
15:16:51 luyao lyarwood: thanks again for your comments. :)
15:18:12 brinzhang dansmith: I replied your comment in https://review.opendev.org/#/c/693828/, we have more details disscussed with the destroy-instance-with-datavolume using PATCH API in the SPEC
15:18:56 brinzhang damsmith: The spec https://review.opendev.org/#/c/580336/
15:19:01 lyarwood luyao: if you just need to clean vpmem things up then you can add them to the existing cleanup methods outside of cleanup for live migration
15:19:24 lyarwood luyao: post_live_migration or rollback_live_migration_at_destination etc
15:22:14 luyao lyarwood: I'd like to set do_cleanup True if there are vpmems, do you think it's OK?
15:22:43 gmann brinzhang: gibi so this is bit we are missing. os-isntance-actions: events policy is admin only and if operator want to show it to non-admin then traceback and host name etc can be seen to non-admin which is all good because operator want to do so
15:24:04 gmann and now with new field 'details' we are showing in 'events' dict so if policy os-isntance-actions: events pass then only we will add 'details'. so operator has to make os-isntance-actions: events for non-admin first and then only non-admin can see new field 'details'
15:24:52 gmann even we have new policy for 'details' field, operator has to enable 'events' policy for non-admin.
15:25:27 gmann i mean operator cannot do 1. keep os-isntance-actions: events for admin only and 2. new policy os-isntance-actions: events:details for non-admin
15:25:32 lyarwood luyao: I'd rather not change the semantics for vpmems at all and just add cleanup for them directly in the required places as we do with vifs and volumes
15:25:41 lyarwood luyao: I'll add a comment once I've finished something
15:26:27 brinzhang gmann: in traceback recorded the sensitive information, why expose this to the non-admin?
15:26:29 gmann brinzhang: we are embedding new field 'details' in 'events' dict which is already policy-configurable for non-admin.
15:26:58 gmann brinzhang: yeah, i am saying if event policy is admin how operator make 'details' to show to non-admin
15:27:18 gmann it is inside 'events' dict not outside
15:27:51 gmann https://review.opendev.org/#/c/694430/13/nova/api/openstack/compute/instance_actions.py@183
15:28:38 brinzhang gmann: no, if the microversion 2.51, we can see the events dict https://opendev.org/openstack/nova/src/branch/master/nova/api/openstack/compute/instance_actions.py#L171
15:29:14 brinzhang s/ if the microversion 2.51/ if the microversion >= 2.51
15:30:53 luyao lyarwood: Thanks. I understand you mean we can add a separate method to cleanup vpmem, acctually I had such one solution, but that means we need to add an rpc api to cleanup destination vpmem like rollback_live_migration_at_destination, alex_xu comments that it's very vpmem and libvirt specific, he hoped we can utilize current cleanup method
15:32:16 gmann brinzhang: you are right on that. i missed that microversion change.
15:32:19 brinzhang gmann: so we don't need to have to pass os-isntance-actions: events to show the 'events' dict.
15:32:35 gmann brinzhang: and hostId is also shown always to non-admin
15:32:56 brinzhang gmann: yes, hostId always shown to non-admin
15:34:27 gmann when and how operator will decide that he/she does not want to show traceback to non-admin but want to show 'details' which is nothing but error message for nova exception and exception names for other to non-admin.
15:35:20 gmann i mean we want to guard the 'details' field with admin by default but tell operator to enable for non-admin if he want to do with keeping traceback for admin only
15:35:56 brinzhang Yes, right
15:35:57 gmann actually that use case I am not getting. if new policy is non-admin by default then it make sense
15:36:26 brinzhang gmann: thanks ^^
15:36:26 gmann but we cannot make it non-admin by default because it may be info leak
15:36:52 brinzhang yes, that why we set system_reader by default
15:36:59 lyarwood luyao: I'm confused, we already have these within the libvirt driver? You wouldn't need to add any RPC calls.
15:37:20 gmann brinzhang: i mean i cannot get the use case that is why i am finding difficulty to understand the use of new policy
15:38:17 gmann or question is like: how operator can decide the that 'details' which has admin related info expose to non-admin but not traceback
15:38:23 brinzhang yeah, in the spec if there is not have the policy limit, I think you will get that case firstly
15:39:23 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: Support boot from volume stable device instance rescue https://review.opendev.org/701431
15:39:24 openstackgerrit Lee Yarwood proposed openstack/nova master: api: Introduce microverion 2.83 allowing boot from volume rescue https://review.opendev.org/701430
15:40:42 brinzhang if the operator want to expose the details to the non-admin, they just need to change the default policy, is it?
15:41:22 brinzhang because the BASE_POLICY_NAME % 'events:details' just limit show details in 'events' dict
15:41:45 luyao lyarwood: we already have vpmem cleanup logic inside libvirt driver, driver.cleanup will invoke vpmem cleanup.
15:42:37 lyarwood luyao: so why can't we just call that specific logic from other places instead of calling the entire cleanup method?
15:43:21 luyao lyarwood: if I want to cleanup vpmems on destination host but do_cleanup is False, I need a rpc call for vpmem cleanup
15:43:47 gmann brinzhang: yeah i get that but my point is it is difficult for operator to decide that 'details' (which can have non-nova exception so does leak the infa info) can be shown to non-admin and traceback not.
15:44:46 luyao lyarwood: alternatively we can set do_cleanup to True, then rpc call rollback_live_migration_at_destination will be invoked, then vpmem cleanup will be called inside that
15:45:02 brinzhang gmann: if the details is non-nova exception, it will be just only show the Exception class name to the details
15:45:16 gmann brinzhang: if any operator ask how to use these two policy in different way (one allowed for admin and one for non-admin) then we would not have clear answer right ?
15:45:36 brinzhang gamnn: pls see https://review.opendev.org/#/c/712697/
15:48:13 luyao lyarwood: so I asked could I set do_cleanup to True if there are vpmems
15:48:18 brinzhang gmann: 'traceback' show the exception details info, contains python path, and the all details. but 'details' just only show the format message if it's a nova exception, but if that is an non-nova excetption, we just show the simple info the the non-admin
15:48:52 brinzhang I donnot think it no clear
15:49:04 luyao lyarwood: Do I make it clear?
15:49:14 lyarwood luyao: yeah okay, that might be okay in the short term but I think after this we really need to clean this interface up
15:49:36 gmann brinzhang: that is what i was thinking to add in API side but serialize_args does.
15:49:49 lyarwood luyao: cleanup within libvirt is actually looking at migrate_data so why we are making the call to cleanup dependent on it is weird
15:49:51 gmann to handle the non nova exception details
15:49:53 luyao lyarwood: yeah agree
15:52:23 brinzhang gmann: you mean, something need I add in os-instance-action API?
15:54:53 gmann brinzhang: no i mean hiding detail about non nova exception but exception name itself can leak few info about driver used etc.
15:55:17 gmann can non-admin take action based on non-nova exception ?
15:56:10 brinzhang maybe try to do something that they can do, nothing else
15:56:11 gmann i am thinking if we hide the non- nova exception from 'details' field and only expose the nova exception which is what use case of 'details' is for non-admin
15:56:39 luyao lyarwood: we only have instance path file to cleanup previously but now we have other devices needs cleanup
15:56:54 gmann dansmith: ^^ ? any use case of keeping non-nova exception name in action event 'details' field.
15:57:07 gmann admin anyways can see all details from traceback
15:58:23 brinzhang gmann: thanks, I am sorry it's too later for me, I have to go.
15:58:31 gmann so that we can keep new field 'details' usable and no info leak for non-admin
15:58:58 gmann brinzhang: ah sorry. yeah. I will reply on review. thanks for discussion and late night.
15:59:38 luyao lyarwood: we can also add a flag in libvirt migrate data to tell if there are vpmems needs cleanup, I'm not sure is it necessary?
15:59:45 brinzhang gmann:We only show non-nova exception class name to users, I don't think it will cause serious information leakage.
16:01:03 brinzhang gmann: this serialize_args change comes mriedem and dansmith, if they are around, I think you can get more.

Earlier   Later