qos wrong units in max-burst-kbps option (per-second is wrong)

[Slawek Kaplonski]

In neutron in qos bw limit rule table in database and in API extension parameter "max-burst-kbps" has got wrong units suggested. Burst should be given in kb (kilobits) instead of kbps (kilobits per second)

example of ovs configuration:
http://openvswitch.org/support/config-cookbooks/qos-rate-limiting/ it is "a parameter to the policing algorithm to indicate the maximum amount of data (in Kb) that this interface can send beyond the policing rate."

[Assaf Muller (amuller)]

The OVS documentation refers to documentation of its own implementation. The Neutron QoS API offers an abstraction that happens to have only one implementation at this time, but has your own LB patch (https://review.openstack.org/236210) as a second one. As long as something translates from the API's unit of measurement to the different implementations unit of measurement, we're fine. We're not bound to any one implementation's documentation.

Assigning to Miguel for further triaging.

[Slawek Kaplonski (slaweq)]

@Assaf: I know that there is LB patch also in progress because I'm doing it :) Please note that this unit which is currently in parameter can be misleading for users. It is not only inconsistency with ovs documentation. Same explanation of such parameter You will find also in tc documentation for example for tbf queue.
Second thing is: if it would be really kbps instead of kb - how to interpret that? If it is kb than it is (as docs said) number of bytes which can be send with higher speed (full speed) then limit.

[Miguel Angel Ajo (mangelajo)]

I agree with @assaf comments here,

We used kilobits to be uniform in the API.

We don't necessarily need to match the low level implementation details, this is a high level abstraction for any plugin to implement.

What I would look at is the discrepance in "per second"

OVS talks about kBs while we talk about kb*p*s

It's not the same 10 kilobits than 10 kilobits per second. It was my understanding when we designed the API that the burst was measured in <UNITS> per second, but that's probably wrong.

[Slawek Kaplonski (slaweq)]

@ajo: I understsand that You designed API that burst is given in kbps but I don't agree that You don't want to match low level implementation details. If You are telling to Neutron's user that he is configuring burst as kbps and in fact You are configuring in ovs "ingress_policing_burst" which is exactly: "a parameter to the policing algorithm to indicate the maximum amount of data (in Kb) that this interface can send beyond the policing rate." (according to http://openvswitch.org/support/config-cookbooks/qos-rate-limiting/) then IMHO user is misled.
It isn't so important that You named option with "kbps" - it still configure amount of data which can be send beyond policy rate".
If You don't want to change it, ok but IMHO it is not good now.

[Miguel Angel Ajo (mangelajo)]

Sorry slawek, I thought you were talking about kBytes, and not kbits.

You're right.

If your proposal is to remove the "per second" part, that's completely right.
We should fix documentation in that regard too, I believe it's worth to be fixed,
but we should accept the old parameter and provide some deprecation notice for at least 1 cycle.

[Miguel Angel Ajo (mangelajo)]

Work being done here: https://review.openstack.org/#/c/291633/

[Miguel Angel Ajo (mangelajo)]

As per last comments in the review, it seems that we don't have a reasonable way to change that
until we have microversioning for API.

Because this change implies that the dictionary of the rule changes the name of one of the fields
at read (we could allow both names at write) and that's backwards incompatible.

[Miguel Angel Ajo (mangelajo)]

As per review discussion, this can't really be done until we had API microversioning, deferring until then.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.openstack.org/291633
Reason: as decided on meeting: bug will be not fixed becuse we need something like API microversioning for that :/