Open Stack

On 12/20/2018 8:47 AM, Jay Pipes wrote:
> 
> The problem with feature flags is they are not discoverable. As much as 
> I hate API extensions, at least they are discoverable.

100% agree. Config-driven API behavior like this flies in the face of 
any kind of hope for interoperability of this feature. Can I do this on 
cloud X? Maybe? Can I do it on cloud Y? Let's find out! At the very 
least, I would think if you're going to add a config option to 
enable/disable this support in the compute API, at least temporarily, 
you would fail any requests which include ports with QoS resource 
requests because we're not going to honor them. That aligns more with 
what I was saying about how I thought the "reject" patches would come 
earlier in the series since we're currently not supporting this feature, 
and lying to users by allowing them to attach these kinds of ports. 
Although I might be conflating QoS ports in general and QoS ports that 
have the bandwidth resource request things on them.

During the spec review, we waffled a bit on whether or not this should 
have a new microversion with it [1][2]. The way this works definitely 
sounds like how volume multiattach is setup - you create a resource in 
another service with a special characteristic and then try to attach 
that thing to a server. Nova can either support that thing or not, and 
the only way for a user to find that out without a microversion is to 
try it and find out (does it fail? if it doesn't fail, is the thing 
working in my guest? if not, is it because it's not supported or because 
something failed or there is a bug?). With volume multiattach we added a 
microversion so that users could know if they can even make the request 
to attach those types of volumes so they didn't have to guess. So I 
could argue that QoS ports like this should get similar treatment, but 
things get fuzzy when it's implicit behavior on some external resource

> 
> Is there a reason why we can't just put code into Nova that, upon seeing 
> the resource requests, acts on it? Is there a reason it needs to be 
> behind a feature flag or even needs to query Neutron for extension 
> support? I mean, if it's in the response payload, just use it, right?

The main reason is how the code is written backward, as I've said 
elsewhere in this thread. If everything was properly plumbed in from the 
bottom up for at least creating servers with these kinds of ports (and 
cleaning them up properly), then it's just a matter of doing like you 
said - take action on the thing if it's there. Like [3] I think we could 
explicitly fail for things that are not supported until they do become 
supported, like any move operations. Whether those are considered bug 
fixes or new features (a la microversions) is debatable. It sucks adding 
stuff that works when creating a server but isn't supported when you 
perform actions on that server, and it sucks holding up features before 
everything is ready, and it sucks to need microversions for every little 
thing that ideally would have just be done up front, so I can't really 
say here on what we should do. I know in the golden days before 
microversions it was totally kosher to just feature flag our way to 
nirvana and we're still dealing with a lot of that fallout (like the 
fact that live migration with NUMA-affined instances still doesn't work 
properly).

[1] 
https://review.openstack.org/#/c/502306/17/specs/rocky/approved/bandwidth-resource-provider.rst@142
[2] 
https://review.openstack.org/#/c/502306/26/specs/rocky/approved/bandwidth-resource-provider.rst@99
[3] https://review.openstack.org/#/c/611088/

-- 

Thanks,

Matt