[ovs-discuss] bug: Adding or deleting port results to flush of QoS policy for all bridge ports

[Andrey Korolyov]

On Tue, Aug 19, 2014 at 8:40 PM, Andrey Korolyov <andrey at xdel.ru> wrote:
> On Tue, Jul 29, 2014 at 8:46 PM, Ben Pfaff <blp at nicira.com> wrote:
>> On Mon, Jul 28, 2014 at 10:25:27PM +0400, Andrey Korolyov wrote:
>>> On Mon, Jul 28, 2014 at 9:44 PM, Ben Pfaff <blp at nicira.com> wrote:
>>> > On Fri, Jul 25, 2014 at 07:29:24PM +0400, Andrey Korolyov wrote:
>>> >> Assuming we have trivial case: libvirt, its QoS limits (for which
>>> >> appropriate rules are executed for each port) and more than one VM
>>> >> sharing the same OVS bridge. Port configurations are initially empty
>>> >> and addition going through libvirt` internal command execution by
>>> >> calling ovs-vsctl binary, so no QoS configuration is performed through
>>> >> existing vswitchd mechanisms.
>>> >>
>>> >> Since OVS decides to flush all disciplines every time when port added
>>> >> or removed from the switch, one will not be able to keep more than one
>>> >> port with externally set tc disciplines.
>>> >>
>>> >> At least this bug affects 0fe1d7f39de9836fea01c560a6fdbfd1405096ea
>>> >> (2.1.3 effectively) in Openflow mode, though probably cover area is
>>> >> wider. FAQ suggests tc as a fallback method for complex
>>> >> configurations, so I can guess that the bug is definitely a bug but
>>> >> not an expected behavior.
>>> >
>>> > It does sound like a bug.  Do you have time to fix it?
>>>
>>> Hi Ben,
>>>
>>> I`ll take a detailed look a bit later. The problem probably is in
>>> incorrect netdev infrastructure calling in the
>>> tc_add_del_ingress_qdisc()
>>>
>>> CCing colleague who may issue the fix earlier than me.
>>
>> Thanks.  I hate working on QoS, so I really appreciate someone else
>> taking a lead on this.
>
> Hi Ben,
>
> sorry for a huge delay, it looks like that the condition from below is
> unnecessary because it flushing QoS on ports without policy in the db.
> It cames from your c1c9c9c4 so I think you may decide what shape
> should this chunk take.
>
>
> vswitchd/bridge.c
> @@ -3630,7 +3630,6 @@ iface_configure_qos(struct iface *iface, const
> struct ovsrec_qos *qos)
>      ofpbuf_init(&queues_buf, 0);
>
>      if (!qos || qos->type[0] == '\0' || qos->n_queues < 1) {
> ->      netdev_set_qos(iface->netdev, NULL, NULL);
>      } else {
>          const struct ovsdb_datum *queues;
>          struct netdev_queue_dump dump;

Ping?

I can rewrite the condition causing the misbehaviour and send out a
patch, but it`s better to know initial reasons for putting this logic
there.