Open Stack

On 12/19/2018 9:03 AM, Balázs Gibizer wrote:
> * the next two patches [6][7] implement rejecting use cases that was
> decided to out of scope for the feature (interface attach with resource
> requst, booting with network having resource request)

I really thought these would go *before* any new feature changes since 
we currently do not support these types of ports, so if a user were to 
create a port (or network) with QoS rules and attach it to a server in 
nova, we're just ignoring it, which is essentially broken. I think of 
this like how we rejected attempts to rebuild a volume-backed server 
with a new image (we used to just let that slide with no errors but it 
didn't actually rebuild the root volume) or trying to attach SR-IOV 
ports (again, not supported in compute). I think of the "reject" patches 
more as bug fixes for existing broken, or at least misleading, code.

> * The next patch [8] add support for removing resource allocation
> during interface detach if the detached port has resource request

This is one example of how the implementation is backward. This is 
something that would be necessary plumbing before enabling a new feature 
in the API (assuming this cleanup happens in nova-compute?).

> * The rest of the series up until [9] implement support for selecting
> VFs form the same PF that the bandwidth was allocated from. This allows
> handling bandwidth and VF resources properly even if there are more
> than one PFs on the same compute connected to the same physnet.

This is really kind of news to me. Maybe I glossed over this or didn't 
understand it in the spec, but the spec doesn't really say much about 
this either. There is only one mention of VFs in the spec: "When 
multiple ports, having QoS policy rules towards the same physical 
network, are attached to the server (e.g. two VFs on the same PF) then 
the resulting allocation is the sum of the resource amounts of each 
individual port request." Anyway, it seems like a sub-feature and I just 
didn't realize it.

> 
> What is still missing from the patch series:
> * Support for any kind of server move operations (resize, migrate,
> evacuate, live-migrate, unshelve)
> * Upgrade support: Bound SRIOV ports might already have QoS policy that
> would need healing resource allocation in placement.

This wouldn't be a problem if we were already rejecting ports with QoS 
rules applied as mentioned above.

> 
> Order of the patches
> --------------------
> 
> One comment I got from Matt is that the patches are in wrong order. The
> current order allows booting server with bandwidth request_before_  
> every other server operation is supported (or rejected). This could
> lead to the violation of the resource view of the system. For example
> booting a server with a QoS aware port will result in bandwidth
> allocation on the compute host. But then later on migrating that server
> to another host does not properly handle moving the allocation to the
> target host.
> 
> In my view reordering the patches to only allow booting the first
> server with bandwdith after every other operation is supported is not
> feasible. The current chain is already 17 patches long and it does not
> touch server moving operations at all. Also if I cannot boot a server
> with bandwidth then I cannot test that such server can be moved
> properly. So all the functional test would end up in the last patch of
> the series.

When I say wrong order, I mostly mean from the bottom up, not from a 
feature parity perspective. The normal way a long complicated feature 
series like this that involves the entire stack would be to start 
plumbing code into nova-compute at the bottom layer and then work your 
way up to the control layer services (conductor/scheduler) and finally 
the API where the feature is turned on (usually with a microversion). I 
am semi OK with deferring move operation support in v1 and building that 
on later given the complicated nature of this feature, but my real issue 
in the order is the backward nature of it (I don't want to approve 
patches that add code to the API for a feature just to leave it fall on 
the ground if the compute code isn't already merged).

> 
> However I think we can use the feature flag approach. We can implement
> and verify everything in the current order without the end user seeing
> or causing any kind of inconsistencies. The nova implementation is only
> activated if a neutron port has a resource_request field. This field is
> added to the neutron API as an API extension. We can mark this
> extension experimental while we are working through the missing pieces
> of the feature. But we can also turn on that extension in our test
> envirnoment to verify each and every step during the implementation
> process.

I'm definitely not crazy about the feature flag idea, but I need to read 
and reply to the other responses in the thread first.

> 
> 
> Which RP fulfills the request of a neutron port
> -----------------------------------------------
> 
> Neutron expects a single RP uuid sent by nova in the port binding that
> tells neutron about which RP providers the resources for that port.
> This is the famous problem to find which request group is fulfilled by
> which resource provider in an allocation candidate. There are 3
> possible solutions:
> 
> * Neutron does the mapping. This would require Neutron to see each port
> binding for a given server as a single request. This is not the case
> today and not feasible to hack around in neutron.

Before I started reviewing the code, I read the spec again to get the 
context in my head and I really thought this is what was proposed - that 
the allocation candidates that we used to claim resources in the 
scheduler would be passed through to the port binding request and 
neutron would sort out whatever it needed to from that. This is the 
section of the spec I'm thinking of:

https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/bandwidth-resource-provider.html#mapping-between-physical-resource-consumption-and-claimed-resources

Did something change drastically since the spec was written such that 
nova needs to do this now, at least temporarily?

-- 

Thanks,

Matt