[ovs-dev] [ACL Meters 6/7] ofproto: Add support for specifying a meter in controller actions.
jpettit at ovn.org
Tue Jul 31 00:55:12 UTC 2018
> On Jul 30, 2018, at 12:49 PM, Ben Pfaff <blp at ovn.org> wrote:
> On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit <jpettit at ovn.org>
> In ofproto_check_ofpacts(), the other checks, if they fail, return an
> error and prevent the flow from being added. The new one doesn't; is
> the difference intentional?
Yes. My thought is that those controller actions are likely important, so if the meter doesn't exist, it's still safer to set up the flow and log a warning.
> Similarly, is there anything that prevents
> a meter from being deleted while still in use and, if that happens, does
> anything particularly bad happen?
Nothing prevents it. However, if it happens, then the metering just stops being enforced, and the other actions are unaffected.
> In ovs-ofctl.8.in, the wording seems a little vague because "associate"
> is such a weak word. Maybe change
> Associate packets sent to the controller with meter \fIid\fR.
> Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
> that this action sends to the controller.
> or something similarly descriptive
Yes, that is better.
> Do only "drop" actions make sense for controller metering?
Probably. I added some documentation to that effect to the description of the "meter" argument to the log() action in ovn-sb.xml.
> Please add some tests for the new action to the test "ovn -- action
> parsing" in ovn.at.
> Please document the new action in ovn-sb.xml.
Which new action? I've extended the log() action documentation to add "meter", which was missing before. There was no existing log action parsing test. I'll add that as a separate patch, since I think the existing log() parsing code could use a little cleanup in that area, too.
More information about the dev