Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-06-12
16:34:42 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: flatten rbd images when unshelving an instance https://review.opendev.org/457886
16:37:58 openstackgerrit Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420
16:42:45 mriedem stephenfin: this shouldn't be stable-only i don't think but you might want to verify if the bug is legit https://review.opendev.org/#/c/664838/1
16:48:59 aspiers efried: Slightly out of my depth here. So shall I go ahead and change spawn() to persist allocations as a new field in the Instance object?
16:50:42 efried aspiers: I wasn't following whatever conversation led up to this
16:51:00 efried Is something we're doing with SEV requiring us to be able to look at the allocation some time after spawn?
16:51:10 efried or is this related to some other topic?
16:51:11 aspiers Yes
16:51:18 efried Do tell
16:51:42 aspiers http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-06-12.log.html#t2019-06-12T12:50:55
16:52:14 aspiers There are multiple places scattered through the code for building the guest config XML which need to know whether SEV is required
16:52:40 aspiers but to perform the check in the way you requested, allocations need to be accessible at those points
16:53:00 efried say wha now?
16:53:13 efried you're telling me that every time libvirt attaches a volume, it rebuilds the whole XML from scratch?
16:53:20 aspiers no
16:53:31 aspiers it needs to know whether to use iommu
16:53:32 efried you're telling me that attaching a volume needs to be SEV aware?
16:53:45 aspiers not necessarily even attaching a volume
16:54:00 efried okay, I see.
16:54:02 aspiers just block devices and stuff at launch-time
16:54:18 aspiers and also to decide whether to include <locked />
16:54:30 aspiers the latest 3 commits at the top of the series
16:55:00 efried I think long term, having allocations on the instance object is a great idea. mriedem, dansmith, how do y'all feel about that?
16:55:00 efried However, in this scenario, you could also solve by commonizing (libvirt/utils?) the code that inspects the flavor/image meta.
16:55:19 dansmith efried: why?
16:55:25 aspiers Yes, that possibility occurred to me too
16:56:42 efried dansmith: example here is SEV. We were considering doing the flavor/image parsing and validation just once, in the request filter, to add resources=MEM_ENCRYPTION_CONTEXT. Then after that, e.g. in spawn(), just looking at the allocation for MEM_ENCRYPTION_CONTEXT so we don't have to redo that parsing/validation.
16:57:12 aspiers here's sean-k-mooney's take: http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-06-12.log.html#t2019-06-12T13:13:16
16:57:18 efried In spawn specifically, we are getting the allocation as a param, so that's fine. But there are (aspiers tells me) other places where we need the same information but don't (currently) have access to the allocations.
16:57:21 dansmith spawn having access to the allocations makes sense, but storing them on/with the instance doesn't really
16:57:52 aspiers here's an example: https://review.opendev.org/#/c/662558/5/nova/virt/libvirt/driver.py
16:58:08 dansmith I imagine bauzas would want access to allocations for gpu stuff as well, and I'm sure we discussed making that a param to spawn in the past
16:58:15 aspiers currently this is broken because it's insufficient to pass only the flavor to _sev_required()
16:58:23 efried allocations are already passed to spawn
16:58:33 dansmith ack, okay I was sure we had discussed
16:58:36 aspiers right, the problem is that they are not passed much further on
16:58:58 aspiers sean-k-mooney's other suggestion was to pass from spawn() to _get_guest_xml() and so on
16:59:21 dansmith yeah, pass them further if you want, but storing them .. no so much
16:59:33 aspiers but that doesn't solve things like attaching volumes to an SEV instance
16:59:39 efried you could cache a map of instance:allocations on the virt driver
16:59:47 dansmith efried: dude
16:59:53 efried but if you restart the compute driver you'd have to rebuild that by querying placement
16:59:57 dansmith aspiers: so look them up and pass them in if you need
17:00:09 efried we've said we don't like virt drivers talking to placement
17:00:19 dansmith yeah
17:00:31 efried though I guess `read` may be acceptable
17:00:32 dansmith and caching placement data is also smelly, IMHO
17:00:46 dansmith efried: no, make compute manager look them up and pass them in
17:01:04 efried dansmith: You mean augment every ComputeDriver method to accept allocations?
17:01:30 dansmith surely we don't need them on every call
17:01:40 efried creeping death of a thousand cuts
17:02:15 efried we already store other (pre-placement) allocation-like things on the Instance, don't we? PCI devices and such?
17:02:31 dansmith but we own those things
17:02:40 dansmith we cache the neutron port information and it's a disaster
17:03:02 efried why?
17:03:12 efried because that information can get changed via neutron?
17:03:19 efried and then we have to figure out how to sync it?
17:03:24 efried In this case nova does "own" the information.
17:03:30 efried the allocation should not change unless nova changes it
17:03:36 efried whereupon we're editing the Instance obj anyway.
17:03:58 aspiers so how would something like DriverVolumeBlockDevice.attach() get hold of allocations?
17:04:04 dansmith okay, whatever, you asked my my opinion.. I hate it. My opinion doesn't matter anyway, only mriedem's, so just do whatever he says
17:04:10 aspiers if not via the Instance?
17:04:43 aspiers (Yes, that is an honest stupid question)
17:04:54 efried aspiers: If I'm understanding dansmith correctly, he's suggesting adding an allocations param to attach_volume and then passing it down through the call stack to DVBD.attach()
17:05:17 efried doing that ^ as needed every time we encounter a ComputeDriver operation who(se guts) require access to the allocation
17:05:21 aspiers efried: so through attach_volume() in the manager?
17:05:40 efried yup. The compute manager would call placement to get the allocations for the instance before invoking the method.
17:05:54 aspiers it seems pretty much every instance lifecycle method is already passing context and instance through
17:06:00 efried yes
17:07:43 aspiers looks like adding allocations would bloat the already long parameter lists of many methods
17:08:26 aspiers long parameter lists seem like a code smell to me in general, but I don't really enough about nova to judge here
17:08:51 aspiers I'll do whatever you gurus think is best :)
17:08:57 efried let's see what mriedem thinks
17:09:00 aspiers OK
17:09:37 dansmith we have virtapi for compute drivers to ask compute manger for help. compute manager already has an in-memory cache of a bunch of this information, so letting the driver ask manager for allocation info for an instance when it needs it, which can be a readthrough cache operation would also be reasonable
17:10:05 efried ah, that could work. What's that virtapi called?
17:10:41 dansmith storing this on the instance is pointless to me because I think we always need to get it from placement to be sure it's right I think, and the instance record is already a massive thing we shoot across the stressed RPC bus with a ton of extra crap we don't need 90% of the time
17:11:02 efried ComputeDriver.virtapi...
17:11:06 dansmith but, as I said, it matters only what mriedem thinks, so ... no point in even discussing until he opines
17:11:28 aspiers RPC bloat sounds like a very valid concern
17:16:23 sean-k-mooney RPC bloat being the reason not to put it in the instance?
17:16:40 aspiers that's how I interpreted what dansmith said
17:16:47 efried This virtapi thing is pretty small right now, basically just has wait_for_instance_event
17:16:51 aspiers yeah
17:16:59 aspiers also, I don't see allocations cached in the manager
17:17:06 sean-k-mooney dansmith: the allcoation should not change over the lifetime of the instance without a server action to modify it
17:17:10 dansmith efried: it used to have a bunch of services the driver used, but has whittled down
17:17:29 sean-k-mooney e.g. it will only cahnge on move operattion/resize/rebulds/or attaches/detaches
17:17:57 dansmith sean-k-mooney: in some future where I can allocate non-compute resources for the instance, the instance's allocation can change not from nova right?
17:18:02 efried but if dansmith likes the idea of extending it with allocations_for_instance, which we can cache in the manager, I'm down with that idea.
17:18:15 dansmith aspiers: no, we cache inventory AFAIK, but not allocations at the moment
17:18:26 sean-k-mooney dansmith: im notsure about that
17:18:43 sean-k-mooney are you thinking baout things like changing the bandwith allcoation via neutron
17:18:51 sean-k-mooney e.g. by changing the qos policy
17:19:17 efried aspiers: The inventory (basically all the provider tree information for all the hosts managed by this compute) is cached in the SchedulerReportClient today. But yeah, not the allocations.
17:19:40 sean-k-mooney i guess that could happen in the future
17:19:55 dansmith sean-k-mooney: I'm thinking of allocating things like a new volume, or even ephemeral things like networked secure enclave keys or something
17:20:17 sean-k-mooney dansmith: well a new volume would be a volume attach right
17:20:19 efried as long as those ^ things happen in nova, we're good, but yeah, if they happen outside of nova, weirdness.

Earlier   Later