Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-03-25
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.
16:01:09 brinzhang gmann: thanks too, bye
16:01:32 sean-k-mooney luyao: we had to do host cleanup before for things other then the instnace files
16:02:04 sean-k-mooney luyao: like removing mounted volumes, cleaning up ports or other actions
16:03:08 brinzhang gmann: this is the original thought https://review.opendev.org/#/c/694428/9/nova/objects/instance_action.py@196
16:03:33 openstackgerrit Lee Yarwood proposed openstack/python-novaclient master: Microversion 2.83 - Stable device boot from volume rescue https://review.opendev.org/714956
16:04:01 lyarwood luyao: possibily, just need to jump on a call and I'll try to update the review again
16:04:11 sean-k-mooney brinzhang: for non admin i think they should only see the class name for nova exceptions too
16:04:53 brinzhang sean-k-mooney: yeah, agree, make sense to me too.
16:05:35 sean-k-mooney non admins ususally dont have the acess required to fix the cause of most nova excpetions
16:07:54 luyao sean-k-mooney: sorry, you mean post live migration?
16:08:41 sean-k-mooney luyao: yes we clean up those resouces ealier in the function
16:08:59 sean-k-mooney luyao: so we unplug the guest interface on the ovs bridge for example
16:09:20 sean-k-mooney and we have to unmount any cinder volumes that were mounted on the soucres node
16:09:30 luyao sean-k-mooney: yeah, invoking driver.cleanup will not cleanup them again
16:09:44 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: Add support for stable device rescue https://review.opendev.org/700812
16:09:44 openstackgerrit Lee Yarwood proposed openstack/nova master: virt: Provide block_device_info during rescue https://review.opendev.org/700811
16:09:45 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Extract _get_bdm_image_metadata into nova.utils https://review.opendev.org/705212
16:09:45 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Report COMPUTE_RESCUE_BFV and check during rescue https://review.opendev.org/701429
16:09:46 openstackgerrit Lee Yarwood proposed openstack/nova master: api: Introduce microverion 2.83 allowing boot from volume rescue https://review.opendev.org/701430
16:09:46 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: Support boot from volume stable device instance rescue https://review.opendev.org/701431
16:09:47 openstackgerrit Lee Yarwood proposed openstack/nova master: DNM - Test stable device rescue tests with BFV instances https://review.opendev.org/710050
16:10:09 sean-k-mooney luyao: ya i know
16:10:22 sean-k-mooney well with the flags you have set
16:10:23 gmann sean-k-mooney: brinzhang and that is what use case if actually. expose something a non-admin could fix. may be filter or whitelist the non-admin fixable exceptions can be better here ?
16:10:42 gmann or at least not expose the non-nova exception at all.
16:11:10 sean-k-mooney gmann: well i would geuss any 4xx errors should be actionable by them in some way
16:11:27 sean-k-mooney if we are identifying them as client issues
16:11:30 luyao sean-k-mooney: yeah, and now I need driver.cleanup to cleanup vpmems
16:12:28 gmann sean-k-mooney: yeah most of them yes. few 404 might not be but I have not checked all exceptions but overall 4xx is in their range
16:17:19 luyao sean-k-mooney, lyarwood: I'll offline and can't response promptly, so please left comments on patch https://review.opendev.org/#/c/687856 if you have any suggestion about vpmem cleanup during live migration. Many Thanks. :)
16:24:57 sean-k-mooney luyao: sure

Earlier   Later