| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-25 | |||
| 13:08:51 | hrw | it is far from standard | |
| 13:08:55 | hrw | each arch has own way | |
| 13:09:07 | sean-k-mooney | yes but at least its a common location | |
| 13:09:39 | sean-k-mooney | otherwise you have to use more arcane cpuid checks and model specific registers | |
| 13:10:21 | hrw | yep | |
| 13:17:43 | kashyap | gibi: Yeah, the bitwise OR and logical OR of flags is always a bit confusing for me too; they look reasonable, see my comment: https://review.opendev.org/#/c/696834/12/nova/virt/libvirt/guest.py@773 | |
| 13:43:51 | gibi | kashyap, lyarwood: thanks. I'm +2 | |
| 13:44:59 | lyarwood | gibi: many thanks! | |
| 13:48:17 | dansmith | brinzhang_: I did, but I didn't understand what any of that had to do with why we need to use patch | |
| 14:23:39 | openstackgerrit | Maciej Kucia proposed openstack/nova master: SR-IOV passthrough: Check PF only if VF is enabled https://review.opendev.org/476642 | |
| 14:25:03 | openstackgerrit | Merged openstack/nova master: ksa auth conf and client for Cyborg access https://review.opendev.org/631242 | |
| 14:29:20 | openstackgerrit | Lee Yarwood proposed openstack/python-novaclient master: Microversion 2.83 - Stable device boot from volume rescue https://review.opendev.org/714956 | |
| 14:29:53 | openstackgerrit | Lee Yarwood proposed openstack/nova master: virt: Provide block_device_info during rescue https://review.opendev.org/700811 | |
| 14:29:54 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Report COMPUTE_RESCUE_BFV and check during rescue https://review.opendev.org/701429 | |
| 14:29:54 | openstackgerrit | Lee Yarwood proposed openstack/nova master: libvirt: Add support for stable device rescue https://review.opendev.org/700812 | |
| 14:29:55 | openstackgerrit | Lee Yarwood proposed openstack/nova master: libvirt: Support boot from volume stable device instance rescue https://review.opendev.org/701431 | |
| 14:29:55 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Extract _get_bdm_image_metadata into nova.utils https://review.opendev.org/705212 | |
| 14:29:56 | openstackgerrit | Lee Yarwood proposed openstack/nova master: DNM - Test stable device rescue tests with BFV instances https://review.opendev.org/710050 | |
| 14:29:56 | openstackgerrit | Lee Yarwood proposed openstack/nova master: api: Introduce microverion 2.83 allowing boot from volume rescue https://review.opendev.org/701430 | |
| 14:33:08 | huaqiang | hello. I see many '_from_dict' method in some NobaObject based classes, but not all classes, | |
| 14:33:23 | huaqiang | should I make it work for new field? | |
| 14:34:33 | openstackgerrit | Luigi Toscano proposed openstack/nova stable/ocata: Remove exp legacy-tempest-dsvm-full-devstack-plugin-nfs https://review.opendev.org/714958 | |
| 14:36:25 | nightmare_unreal | hey, how can one overwrite allocation for instance | |
| 14:38:53 | huaqiang | nightmare_unreal: cool name :D | |
| 14:39:36 | nightmare_unreal | huaqiang: thanks :D , it's just a nick I registered when I was more into gaming haha | |
| 14:42:25 | mriedem | nightmare_unreal: why do you want/need to? | |
| 14:42:45 | nightmare_unreal | working on this : https://bugs.launchpad.net/nova/+bug/1868997 | |
| 14:42:47 | openstack | Launchpad bug 1868997 in OpenStack Compute (nova) "option to overwrite allocations for instances" [Undecided,New] - Assigned to jayaditya gupta (jayssj11) | |
| 14:43:32 | mriedem | is that referring to the heal_allocations CLI? https://docs.openstack.org/nova/latest/cli/nova-manage.html#placement | |
| 14:43:51 | mriedem | we don't really need to track todos in the code with bug reports...so i'm not sure why someone opened that bug | |
| 14:43:59 | mriedem | or is that someone you? :) | |
| 14:45:26 | nightmare_unreal | yes that's me :) | |
| 14:46:02 | mriedem | ok https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L216 doesn't refer to a todo | |
| 14:46:19 | mriedem | oh wrong line, 2126 | |
| 14:46:37 | mriedem | https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L2126 | |
| 14:46:54 | nightmare_unreal | yup that one | |
| 14:47:35 | nightmare_unreal | I also did the --cell one : https://review.opendev.org/#/c/714459/ | |
| 14:47:39 | nightmare_unreal | but it needs review | |
| 14:47:57 | mriedem | are you on belmiro's team at cern? | |
| 14:48:29 | nightmare_unreal | yup | |
| 14:48:38 | nightmare_unreal | new joinee | |
| 14:49:00 | gibi | nightmare_unreal: I will get back to https://review.opendev.org/#/c/714459/ hopefully tomorrow | |
| 14:49:09 | nightmare_unreal | thanks gibi | |
| 14:49:10 | mriedem | cool. welcome. i can leave some quick comments on ^ | |
| 14:49:24 | nightmare_unreal | sure | |
| 14:50:15 | gibi | mriedem: thanks! | |
| 14:51:01 | gibi | mriedem: do you miss reviewing nova code ? :) | |
| 14:54:41 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP libvirt: Break up get_disk_mapping within blockinfo https://review.opendev.org/714962 | |
| 14:57:21 | mriedem | gibi: heal_allocations started as my baby so i'm partial | |
| 15:00:05 | gibi | :) | |
| 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 | |