ovs_lib.OVSBridge flow managment methods poor design

[Aleksandr Chirko]

Unlike 'set_db_attribute()' and 'clear_db_attribute' methods which don't restrict our choice of desired attributes, flow managment methods such as 'add_flow', 'mod_flow' and 'delete_flows' internally make use of '_build_flow_expr_arr()' method, which validates and limits flow parameters. Beside from obvious inconsistency I see the following problems:

- If we misspell flow parameter it just will be ignored, and instead of exception (this is what we would expect) wrong flow will be defined.
- To add more flow options we have to keep hardcoding them in '_build_flow_expr_arr()'

My proposition is to use similar to 'set_db_attribute()' method approach - don't restrict which options we can use, just pack them and execute the command.

[Mark McClain (markmcclain)]

I think the better approach is to be more specific in the kwargs that the methods accepts. With explicit kwargs, misspellings will be detected very explicitly via run time errors vs attempting to execute a command that may fail.

[OpenStack Infra (hudson-openstack)]

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

[Aleksandr Chirko (achirko)]

Hi, all.

To Mark - I don't agree that in case of ovslib, run time error is better than failed command. Because as with 'set_db_attribute()', user must know what he's doing. And my other desire is to keep ovslib as low level and small as possible, so if we would need validation for flows - define Flow Manager, for DB attributes - Attribute Manager and so on.

My complain was about 3 things - inconsistency, hardcoded flow parameters and 'silent' mistakes. I actually have spotted another problem (bug!), but I didn't have time to post it, so I probably would soon and I'm going to propose my own fix.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/58533
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=3c00dd43f613c838f713a7cbf3cedb6767a8c52a
Submitter: Jenkins
Branch: master

commit 3c00dd43f613c838f713a7cbf3cedb6767a8c52a
Author: Aleks Chirko <email address hidden>
Date: Tue Nov 26 16:22:57 2013 +0200

    Bugfix and refactoring for ovs_lib flow methods

    Remove hardcoded flow parameters from
    '_build_flow_expr_str' method, so we can
    define any flows we want and can rely on 'ovs-ofctl'
    command to verify flow arguments correctness.
    When building flow string inside _build_flow_expr_str
    use the following approach:
    1. Build prefix and remove prefix params from flow_dict.
    2. Build postfix (actions) and remove 'actions' from
    flow dict.
    3. Inside the loop build flow array from everything
    what's left in flow_dict.
    4. Append postfix (actions) to the flow array.
    5. 'Join' flow array into flow string.

    Change _build_flow_expr_str() to be a function
    instead of an object method because 'self'
    parameter wasn't used.

    Remove 'add_or_mod_flow_str' method because
    we have to use separate logic when bulding flow
    strings for 'add_flow' and 'mod_flow' methods.

    Add more unit tests for OVSBridge class.

    Closes-Bug: #1255058
    Closes-Bug: #1240572

    Change-Id: Ic89221d006a626aa2fc40314a9acffc0ea6fd61c