<div dir="ltr">Hi Bob, Miguel<br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 14, 2015 at 5:19 PM, Robert Kukura <span dir="ltr"><<a href="mailto:kukura@noironetworks.com" target="_blank">kukura@noironetworks.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I haven't had a chance to review this patch in detail yet, but am wondering if this is being integrated with ML2 as an extension driver? If so, that should clearly address how dictionaries are extended and input values are validated. If not, why?<br></blockquote><div>This was my initial idea as well. But with this approach we had to split QoS extension into two parts, once for QoS policies and rules managements and other for QoS policy association with network/port.</div><div>The first one is realized by oS service plugin and the latter could be realized as ML2 extension.</div><div>With current approach, QoS Plugin takes care of both parts. As Miguel mentioned, the port/network QoS policy association information can be added by enhancing current callback mechanism. It works fine when need to propagate details from ML2 to the QoS Plugin for configuration, but lacks support to add QoS policy details to port attributed when get_port is invoked.</div><div>With approach we took, all QoS functionality managed by single plugin and required less integration pieces in several places.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-Bob<div class="HOEnZb"><div class="h5"><br>
<br>
On 7/13/15 10:50 PM, Miguel Angel Ajo wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Inline reply (I've added to CC relevant people for ml2/plugin.py<br>
port_update extension<br>
handing -via git blame-) as they probably have an opinion here<br>
(specially the last<br>
two options).<br>
<br>
Kevin Benton wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This sounds like a lot of overlap with the dict extend functions. Why<br>
weren't they adequate for the QoS use case?<br>
</blockquote>
<br>
Let me explain, I believe Mike exceeded the proposal with AFTER_READ,<br>
that's not the plan,<br>
even if we could do as he proposed,<br>
<br>
AFTER_READ dict extension is just a temporary workaround until we have<br>
a separate explicit<br>
api @armax is working on. Making explicit that your service is going<br>
to extend resources,<br>
and handled that in an ordered way is a good thing.<br>
<br>
Afterwards, the source of this review has came from ML2 implementation of<br>
AFTER_CREATE/AFTER_UPDATE notification for ports/nets.<br>
<br>
Let me explain:<br>
<br>
As a decoupled, "mixinless" service extending core resources, we<br>
need to do two things:<br>
<br>
1) Extending the core resources as other extensions would do, adding<br>
stuff to the port/network<br>
dicts, here's where it comes the current AFTER_READ dict extension,<br>
and future API making<br>
that more explicit and more controlled.<br>
<br>
2) Verifying the extended values we receive on core resources, by<br>
registering to BEFORE_*<br>
callbacks. For example, if a tenant is trying to use a qos_profile_id<br>
he doesn't have access to,<br>
or that doesn't exist, we can cancel the operation by throwing an<br>
exception.<br>
<br>
We need to extend the notifications for ports and networks, as<br>
that's not notified currently.<br>
Mike will work on that too in a separate patch.<br>
<br>
<br>
3) Taking the finally extended values and store associations in database<br>
(AFTER_UPDATE/AFTER_CREATE) so any later reads of the<br>
port/network will get the right<br>
qos_profile_later in "after read".<br>
<br>
<br>
We have found that AFTER_CREATE/AFTER_UPDATE happens at plugin level<br>
(neutron/plugins/ml2/plugin.py / update_port) and that information<br>
passed down is<br>
very brief to our case (basically a "None" port if no ml2-know<br>
attribute is changed), and<br>
ml2 has to be aware of every single extension.<br>
<br>
Here there are two options:<br>
a) we make ml2 also aware of qos_profile_id changes, complicating<br>
the business logic down<br>
there, or...<br>
<br>
b) we send the AFTER_CREATE/UPDATE events, and we listen to the<br>
callback<br>
listeners to such notification, and they will tell us if there's any<br>
relevant field which must<br>
be propagated down to agents. Then it's up to the agents to use such<br>
field or not.<br>
<br>
<br>
Mike's patch proposal is in the direction of (b), he's a long term<br>
thinker, I was proposing him<br>
to just go (a) for now, but let's discuss and find what's more right.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Mon, Jul 13, 2015 at 7:58 AM, Mike Kolesnik<<a href="mailto:mkolesni@redhat.com" target="_blank">mkolesni@redhat.com</a>><br>
wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
I sent a simple patch to check the possibility to add results to<br>
callbacks:<br>
<a href="https://review.openstack.org/#/c/201127/" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/201127/</a><br>
<br>
This will allow us to decouple the callback logic from the ML2<br>
plugin in<br>
the QoS scenario where we need to update the agents in case the<br>
profile_id<br>
on a port/network changes.<br>
It will also allow for a cleaner way to extend resource attributes as<br>
AFTER_READ callbacks can return a dict of fields to add to the original<br>
resource instead of mutating it directly.<br>
<br>
Please let me know what you think of this idea.<br>
<br>
Regards,<br>
Mike<br>
<br>
<br>
__________________________________________________________________________<br>
<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe:<br>
<a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br>
<br>
</blockquote>
<br>
<br>
__________________________________________________________________________<br>
<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe:<br>
<a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote>
<br>
</blockquote>
<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div></div>