RFE: QoS – rule's management (set,delete, show…) requires <qos-policy> parameter in addition to <rule-id>

[Arkady Shtempler]

 In order to create QoS rule we must provide policy name and that’s OK, because rule should be
associated to specific QoS policy.
 Once user wants to manage the existing QoS rules, for example: SHOW, DELETE or SET commands, <qos-policy> and <rule-id> are mandatory parameters and this doesn’t make sense as <rule-id> only should be enough. (Just like we have in other features, security groups for example)

The above is relevant for both: Rest APIs and CLI.
Documentation change will be required on fix.

Usage example for: SET,DELETE and SHOW

1) SET
(openstack) network qos rule set
usage: network qos rule set [-h] [--max-kbps <max-kbps>]
                            [--max-burst-kbits <max-burst-kbits>]
                            [--dscp-mark <dscp-mark>] [--min-kbps <min-kbps>]
                            [--ingress | --egress]
                            <qos-policy> <rule-id>

2) DELETE
(openstack) network qos rule delete
usage: network qos rule delete [-h] <qos-policy> <rule-id>
network qos rule delete: error: too few arguments

3) SHOW
(openstack) network qos rule show
usage: network qos rule show [-h] [-f {json,shell,table,value,yaml}]
                             [-c COLUMN] [--max-width <integer>] [--fit-width]
                             [--print-empty] [--noindent] [--prefix PREFIX]
                             <qos-policy> <rule-id>
network qos rule show: error: too few arguments

[Nate Johnston (nate-johnston)]

Since a QoS rule will only be associated with one QoS policy, I believe this is a valid change.

[Slawek Kaplonski (slaweq)]

But "problem" here is that QoS API is build this way. Please take a look at REST path e.g. for DELETE rule: /v2.0/qos/policies/{policy_id}/bandwidth_limit_rules/{rule_id}
So to make this call You need policy_id and that's why it is done like that.

I agree that maybe it's not the best solution but for some reason it was accepted to be like that some time ago and now IMO we will have to leave with it.
I'm not sure if doing such backward incompatible API change would be good idea.
But I think that final decision about this should be done by someone from drivers team.

[Brian Haley (brian-haley)]

So based on Slaweq's comment the policy_id is required by the API in the DELETE case. So this looks like an invalid bug. I'll wait for the submitters response before marking as such.

[Slawek Kaplonski (slaweq)]

@Brian: DELETE was only an example. In fact it's the same for all other QoS calls :)

[Arkady Shtempler (ashtempl)]

Guys, his is a valid bug, I talked to developers and QE before opening this issue. I don't know what was the reason for implementing such strange behavior, maybe it was quick and simple solution, but for the end user this behavior is not friendly.
I think that user experience is the key to make the decision.

[Miguel Lavalle (minsel)]

This is a tough one. I see Arkady's point about usability but at the same time Slawek and Brian point about backwards compatibility is also valid. Could we keep the current API and implement an extension that allows users to handle QoS rules directly by uuid?

[Miguel Lavalle (minsel)]

Added rfe tag and "RFE" to the description to highlight it to the drivers team

[Slawek Kaplonski (slaweq)]

Maybe we could add something like "get_policy_for_rule" API call to neutron? Then this call could be used in OpenStackSDK/Client to "discover" QoS policy for e.g. rule and do call in same way as it is done now.
That should be IMO easy to implement, backward compatible and UX for OSC/SDK should be better.

[Miguel Lavalle (minsel)]

@Arkady,

Please look at proposal by Slawek in #8. It would enable the OpenStackSDK/Client to discover the QoS policy for a given rule. Based on that, OpenStackSDK/Client would call the current API. However, from the point of view of the user, the policy id is not needed anymore. Does this address your usability concern?

[Arkady Shtempler (ashtempl)]

@Miguel

Yes, I'm totally agree with Slawek proposal as it solves user's problem and as mentioned by him it's easy to implement.

[Miguel Lavalle (minsel)]

@Arkady, @Slawek,

Thanks for your comments. Tagging this as rfe-triaged so we can discuss it in the drivers meeting

[YAMAMOTO Takashi (yamamoto)]

FWIW, while midonet qos api basically is mirroring neutron's,
it intentionally differed in this part for the exact reason.
https://docs.midonet.org/docs/latest-en/rest-api/content/qos-policy-dscp-marking-rules.html

maybe we can provide /v2.0/qos/bandwidth_limit_rules/{rule_id} as an alias
and handle "get_policy_for_rule" equivalent internally? that way we can save a REST round-trip.

[Miguel Lavalle (minsel)]

Yes, I like saving one ReST round trip

[Slawek Kaplonski (slaweq)]

I like YAMAMOTO's proposal. Alias would be really good idea IMO.

[Miguel Lavalle (minsel)]

This RFE was discussed by the Neutron drivers and approved on August 31st, following the proposal outlined in #12 above.

@Arakady,

Are you planning to implement the RFE? If not, I will. Please let us know

[Miguel Lavalle (minsel)]

@Arkady,

I assigned the RFE to me. If you want to implement it, please feel free to change the assignment

[Arkady Shtempler (ashtempl)]

@Miguel feel free to implement this RFE.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/608473

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/608473
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=eb2e5c96aee844102d2f5e2ad03f87df8664202e
Submitter: Zuul
Branch: master

commit eb2e5c96aee844102d2f5e2ad03f87df8664202e
Author: Miguel Lavalle <email address hidden>
Date: Sun Oct 7 19:20:09 2018 -0500

    Define qos-rules-alias extension

    This patch adds qos-rules-alias extension to enable users to perform
    GET, PUT and DELETE operations on QoS rules as though they are first
    level resources. In other words, the user doesn't have to specify the
    QoS policy ID

    Change-Id: I5366dfee9b760ff5c884981582cdd17245b4d16f
    Partial-Bug: #1777627

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/610666
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=0c476067f6406d549eb629f3b970e4a2e896aca4
Submitter: Zuul
Branch: master

commit 0c476067f6406d549eb629f3b970e4a2e896aca4
Author: Miguel Lavalle <email address hidden>
Date: Mon Oct 15 12:25:26 2018 -0500

    Add api-ref for qos-rules-alias extension

    Add api-ref for the qos-rules-alias extension.

    Change-Id: I6d2d32f89e04a8ad83fae2a1183d572221881b90
    Partial-Bug: #1777627

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/613820

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/620429
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=ab284169b2f4a859affc995dbd4cd4eebf7b96e1
Submitter: Zuul
Branch: master

commit ab284169b2f4a859affc995dbd4cd4eebf7b96e1
Author: Miguel Lavalle <email address hidden>
Date: Tue Nov 27 18:15:08 2018 -0600

    Fix QoS alias api definition

    Change [1] wrongly attempted to re-define bandwidth_limit_rules,
    dscp_marking_rules and minimum_bandwidth_rules as first level API
    resources, leading to conflicts in the QoS API. We now define
    alias_bandwidth_limit_rules, alias_dscp_marking_rules and
    alias_minimum_bandwidth_rules that will enable users to perform GET,
    PUT and DELETE operations on the corresponding QoS rules without having
    to specify the associated policy ID.

    [1] I5366dfee9b760ff5c884981582cdd17245b4d16f

    Change-Id: I978668ee073c26d6e05ea5e5a0a4e16a7e513fa1
    Partial-Bug: #1777627

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/613820
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=636f1b539446c27c703744159c10bc08739f8937
Submitter: Zuul
Branch: master

commit 636f1b539446c27c703744159c10bc08739f8937
Author: Miguel Lavalle <email address hidden>
Date: Sun Oct 28 20:12:29 2018 -0500

    Define qos-rules-alias extension

    This patch adds qos-rules-alias extension to enable users to perform
    GET, PUT and DELETE operations on QoS rules as though they are first
    level resources. In other words, the user doesn't have to specify the
    QoS policy ID.

    Change-Id: Ia7535d83e3ae874106e22652dfd97bd9250ad37b
    Partial-Bug: #1777627