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-re... [2] https://review.openstack.org/#/c/502306/26/specs/rocky/approved/bandwidth-re... [3] https://review.openstack.org/#/c/611088/ -- Thanks, Matt