| Message ID | cover.1625762235.git.lorenzo.bianconi@redhat.com |
|---|---|
| Headers | show |
| Series | respin CoPP series | expand |
On Thu, Jul 08, 2021 at 06:40:01PM +0200, Lorenzo Bianconi wrote: > This series respin CoPP support introduced here [0] by Dumitru rebasing on top > of ovn master branch and adding some missing meters (e.g. bfd or acl reject). > The main goal of this series is to continue the discussion about the proposed > approach and to align on CMS APIs. > For the moment DDLog is not supported yet and it will be added in a subsequent > series. I wrote a ddlog implementation on top of v7 and pushed it here: https://github.com/blp/ovs-reviews/commits/COPP-v7%2Bddlog There are just two patches on top of yours. I'd prefer if the first one "ovn-northd-ddlog: Optimize AggregatedFlow rules." be inserted somewhere in your series and if the second one "Implement CoPP for DDlog." were squashed in the right place in your series. It occurred to me that the new column controller_meter gets populated pretty predictably: if something in the flow will make ovn-controller process a packet, then it gets set to whatever that is. As a result, I don't know whether it's necessary to have the column. ovn-controller could just recognize that a particular flow would cause it to process a packet (it knows that, of course) and then assign that packet to the appropriate meter, if it exists.
On Jul 08, Ben Pfaff wrote: > On Thu, Jul 08, 2021 at 06:40:01PM +0200, Lorenzo Bianconi wrote: > > This series respin CoPP support introduced here [0] by Dumitru rebasing on top > > of ovn master branch and adding some missing meters (e.g. bfd or acl reject). > > The main goal of this series is to continue the discussion about the proposed > > approach and to align on CMS APIs. > > For the moment DDLog is not supported yet and it will be added in a subsequent > > series. > > I wrote a ddlog implementation on top of v7 and pushed it here: > https://github.com/blp/ovs-reviews/commits/COPP-v7%2Bddlog > > There are just two patches on top of yours. I'd prefer if the first one > "ovn-northd-ddlog: Optimize AggregatedFlow rules." be inserted somewhere > in your series and if the second one "Implement CoPP for DDlog." were > squashed in the right place in your series. Hi Ben, thx a lot for the series. Compiling the code I have this error: error: module 'copp' imported by northd/ovn_northd.dl not found. Paths searched: /home/lorenzo/workspace/ovn/northd/copp.dl /home/lorenzo/workspace/ddlog-v0.38/lib/copp.dl I think you have not committed copp.dl, correct? Regards, Lorenzo > > It occurred to me that the new column controller_meter gets populated > pretty predictably: if something in the flow will make ovn-controller > process a packet, then it gets set to whatever that is. As a result, I > don't know whether it's necessary to have the column. ovn-controller > could just recognize that a particular flow would cause it to process a > packet (it knows that, of course) and then assign that packet to the > appropriate meter, if it exists. >
On Fri, Jul 09, 2021 at 11:11:48AM +0200, Lorenzo Bianconi wrote: > On Jul 08, Ben Pfaff wrote: > > On Thu, Jul 08, 2021 at 06:40:01PM +0200, Lorenzo Bianconi wrote: > > > This series respin CoPP support introduced here [0] by Dumitru rebasing on top > > > of ovn master branch and adding some missing meters (e.g. bfd or acl reject). > > > The main goal of this series is to continue the discussion about the proposed > > > approach and to align on CMS APIs. > > > For the moment DDLog is not supported yet and it will be added in a subsequent > > > series. > > > > I wrote a ddlog implementation on top of v7 and pushed it here: > > https://github.com/blp/ovs-reviews/commits/COPP-v7%2Bddlog > > > > There are just two patches on top of yours. I'd prefer if the first one > > "ovn-northd-ddlog: Optimize AggregatedFlow rules." be inserted somewhere > > in your series and if the second one "Implement CoPP for DDlog." were > > squashed in the right place in your series. > > Hi Ben, > > thx a lot for the series. > Compiling the code I have this error: > > error: module 'copp' imported by northd/ovn_northd.dl not found. Paths searched: > /home/lorenzo/workspace/ovn/northd/copp.dl > /home/lorenzo/workspace/ddlog-v0.38/lib/copp.dl > > I think you have not committed copp.dl, correct? Oops. I am sorry about that. It was careless. I have pushed an additional patch that should add it properly. It would be best to fold that in as well.
> On Fri, Jul 09, 2021 at 11:11:48AM +0200, Lorenzo Bianconi wrote: > > On Jul 08, Ben Pfaff wrote: > > > On Thu, Jul 08, 2021 at 06:40:01PM +0200, Lorenzo Bianconi wrote: > > > > This series respin CoPP support introduced here [0] by Dumitru rebasing on top > > > > of ovn master branch and adding some missing meters (e.g. bfd or acl reject). > > > > The main goal of this series is to continue the discussion about the proposed > > > > approach and to align on CMS APIs. > > > > For the moment DDLog is not supported yet and it will be added in a subsequent > > > > series. > > > > > > I wrote a ddlog implementation on top of v7 and pushed it here: > > > https://github.com/blp/ovs-reviews/commits/COPP-v7%2Bddlog > > > > > > There are just two patches on top of yours. I'd prefer if the first one > > > "ovn-northd-ddlog: Optimize AggregatedFlow rules." be inserted somewhere > > > in your series and if the second one "Implement CoPP for DDlog." were > > > squashed in the right place in your series. > > > > Hi Ben, > > > > thx a lot for the series. > > Compiling the code I have this error: > > > > error: module 'copp' imported by northd/ovn_northd.dl not found. Paths searched: > > /home/lorenzo/workspace/ovn/northd/copp.dl > > /home/lorenzo/workspace/ddlog-v0.38/lib/copp.dl > > > > I think you have not committed copp.dl, correct? > > Oops. I am sorry about that. It was careless. I have pushed an > additional patch that should add it properly. It would be best to fold > that in as well. > ack, no worries. I folded the patch here: https://github.com/LorenzoBianconi/ovn/tree/CoPP-v7 however CoPP tests available in system-ovn.at and for ovn-northd/ovn-controller are failing with DDlog. Can you please double check if you are facing the same issues? Regards, Lorenzo
On Sat, Jul 10, 2021 at 02:56:53PM +0200, Lorenzo Bianconi wrote: > ack, no worries. I folded the patch here: > https://github.com/LorenzoBianconi/ovn/tree/CoPP-v7 > > however CoPP tests available in system-ovn.at and for ovn-northd/ovn-controller > are failing with DDlog. Can you please double check if you are facing the same > issues? I didn't run the system-ovn.at tests, but the ones for ovn-northd and ovn-controller are embarrassing. Sorry about that. I'll fix them, and I'll see if I can get the system-ovn tests set up and run here.
On Tue, Jul 13, 2021 at 12:00:13PM -0700, Ben Pfaff wrote: > On Sat, Jul 10, 2021 at 02:56:53PM +0200, Lorenzo Bianconi wrote: > > ack, no worries. I folded the patch here: > > https://github.com/LorenzoBianconi/ovn/tree/CoPP-v7 > > > > however CoPP tests available in system-ovn.at and for ovn-northd/ovn-controller > > are failing with DDlog. Can you please double check if you are facing the same > > issues? > > I didn't run the system-ovn.at tests, but the ones for ovn-northd and > ovn-controller are embarrassing. Sorry about that. I'll fix them, and > I'll see if I can get the system-ovn tests set up and run here. Here's a fix to fold in. With this, I get failures in the following tests. I'm going to assume that this isn't related since all the versions of the same test fail: 539: Symmetric IPv6 ECMP reply flows -- ovn-northd -- dp-groups=yes FAILED (ovn.at:23254) 540: Symmetric IPv6 ECMP reply flows -- ovn-northd -- dp-groups=no FAILED (ovn.at:23254) 541: Symmetric IPv6 ECMP reply flows -- ovn-northd-ddlog -- dp-groups=yes FAILED (ovn.at:23254) 542: Symmetric IPv6 ECMP reply flows -- ovn-northd-ddlog -- dp-groups=no FAILED (ovn.at:23254) I still haven't set up the system-ovn tests. It's on my to-do list. -8<--------------------------cut here-------------------------->8-- ovn-northd-ddlog: Fix CoPP behavior. In ovn_northd.dl, the new 'controller_meter' column needs to be included in the row UUID, otherwise replacing a row with one that is identical except for controller_meter will fail with an ovsdb-server transaction error. In ovn-controller.at, it's necessary to wait for flow table updates. --- northd/ovn_northd.dl | 4 ++-- tests/ovn-controller.at | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index d7b54be85644..71e868f05db2 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -1697,7 +1697,7 @@ for (f in AggregatedFlow()) { if (f.logical_datapaths.size() == 1) { Some{var dp} = f.logical_datapaths.nth(0) in sb::Out_Logical_Flow( - ._uuid = hash128((dp, f.stage, f.priority, f.__match, f.actions, f.external_ids)), + ._uuid = hash128((dp, f.stage, f.priority, f.__match, f.actions, f.controller_meter, f.external_ids)), .logical_datapath = Some{dp}, .logical_dp_group = None, .pipeline = pipeline, @@ -1710,7 +1710,7 @@ for (f in AggregatedFlow()) { } else { var group_uuid = hash128(f.logical_datapaths) in { sb::Out_Logical_Flow( - ._uuid = hash128((group_uuid, f.stage, f.priority, f.__match, f.actions, f.external_ids)), + ._uuid = hash128((group_uuid, f.stage, f.priority, f.__match, f.actions, f.controller_meter, f.external_ids)), .logical_datapath = None, .logical_dp_group = Some{group_uuid}, .pipeline = pipeline, diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 475528623b5c..e317b70aa55f 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -632,19 +632,19 @@ check ovn-nbctl ls-lb-add ls1 lb1 # controller-event metering check ovn-nbctl meter-add event-elb drop 100 pktps 10 -check ovn-nbctl ls-copp-add ls1 event-elb event-elb +check ovn-nbctl --wait=hv ls-copp-add ls1 event-elb event-elb AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.0f | grep -q meter_id=1]) # reject metering check ovn-nbctl meter-add acl-meter drop 1 pktps 0 check ovn-nbctl ls-copp-add ls1 reject acl-meter -check ovn-nbctl acl-add ls1 from-lport 1002 'inport == "lsp1" && ip && udp' reject +check ovn-nbctl --wait=hv acl-add ls1 from-lport 1002 'inport == "lsp1" && ip && udp' reject AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.16 | grep -q meter_id=2]) # arp metering check ovn-nbctl meter-add arp-meter drop 200 pktps 0 -check ovn-nbctl lr-copp-add lr1 arp-resolve arp-meter +check ovn-nbctl --wait=hv lr-copp-add lr1 arp-resolve arp-meter AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.00 | grep -q meter_id=3]) OVN_CLEANUP([hv1])
> On Tue, Jul 13, 2021 at 12:00:13PM -0700, Ben Pfaff wrote: > > On Sat, Jul 10, 2021 at 02:56:53PM +0200, Lorenzo Bianconi wrote: > > > ack, no worries. I folded the patch here: > > > https://github.com/LorenzoBianconi/ovn/tree/CoPP-v7 > > > > > > however CoPP tests available in system-ovn.at and for ovn-northd/ovn-controller > > > are failing with DDlog. Can you please double check if you are facing the same > > > issues? > > > > I didn't run the system-ovn.at tests, but the ones for ovn-northd and > > ovn-controller are embarrassing. Sorry about that. I'll fix them, and > > I'll see if I can get the system-ovn tests set up and run here. > > Here's a fix to fold in. With this, I get failures in the following > tests. I'm going to assume that this isn't related since all the > versions of the same test fail: > > 539: Symmetric IPv6 ECMP reply flows -- ovn-northd -- dp-groups=yes FAILED (ovn.at:23254) > 540: Symmetric IPv6 ECMP reply flows -- ovn-northd -- dp-groups=no FAILED (ovn.at:23254) > 541: Symmetric IPv6 ECMP reply flows -- ovn-northd-ddlog -- dp-groups=yes FAILED (ovn.at:23254) > 542: Symmetric IPv6 ECMP reply flows -- ovn-northd-ddlog -- dp-groups=no > FAILED (ovn.at:23254) > > I still haven't set up the system-ovn tests. It's on my to-do list. Hi Ben, thx a lot for the fix. Adding a small change system-ovn tests are fine now. I posted v7 squashing your changes. Regards, Lorenzo > > -8<--------------------------cut here-------------------------->8-- > > ovn-northd-ddlog: Fix CoPP behavior. > > In ovn_northd.dl, the new 'controller_meter' column needs to be > included in the row UUID, otherwise replacing a row with one that is > identical except for controller_meter will fail with an ovsdb-server > transaction error. > > In ovn-controller.at, it's necessary to wait for flow table updates. > --- > northd/ovn_northd.dl | 4 ++-- > tests/ovn-controller.at | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index d7b54be85644..71e868f05db2 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -1697,7 +1697,7 @@ for (f in AggregatedFlow()) { > if (f.logical_datapaths.size() == 1) { > Some{var dp} = f.logical_datapaths.nth(0) in > sb::Out_Logical_Flow( > - ._uuid = hash128((dp, f.stage, f.priority, f.__match, f.actions, f.external_ids)), > + ._uuid = hash128((dp, f.stage, f.priority, f.__match, f.actions, f.controller_meter, f.external_ids)), > .logical_datapath = Some{dp}, > .logical_dp_group = None, > .pipeline = pipeline, > @@ -1710,7 +1710,7 @@ for (f in AggregatedFlow()) { > } else { > var group_uuid = hash128(f.logical_datapaths) in { > sb::Out_Logical_Flow( > - ._uuid = hash128((group_uuid, f.stage, f.priority, f.__match, f.actions, f.external_ids)), > + ._uuid = hash128((group_uuid, f.stage, f.priority, f.__match, f.actions, f.controller_meter, f.external_ids)), > .logical_datapath = None, > .logical_dp_group = Some{group_uuid}, > .pipeline = pipeline, > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 475528623b5c..e317b70aa55f 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -632,19 +632,19 @@ check ovn-nbctl ls-lb-add ls1 lb1 > > # controller-event metering > check ovn-nbctl meter-add event-elb drop 100 pktps 10 > -check ovn-nbctl ls-copp-add ls1 event-elb event-elb > +check ovn-nbctl --wait=hv ls-copp-add ls1 event-elb event-elb > > AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.0f | grep -q meter_id=1]) > > # reject metering > check ovn-nbctl meter-add acl-meter drop 1 pktps 0 > check ovn-nbctl ls-copp-add ls1 reject acl-meter > -check ovn-nbctl acl-add ls1 from-lport 1002 'inport == "lsp1" && ip && udp' reject > +check ovn-nbctl --wait=hv acl-add ls1 from-lport 1002 'inport == "lsp1" && ip && udp' reject > AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.16 | grep -q meter_id=2]) > > # arp metering > check ovn-nbctl meter-add arp-meter drop 200 pktps 0 > -check ovn-nbctl lr-copp-add lr1 arp-resolve arp-meter > +check ovn-nbctl --wait=hv lr-copp-add lr1 arp-resolve arp-meter > AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.00 | grep -q meter_id=3]) > > OVN_CLEANUP([hv1]) > -- > 2.31.1 >
This series respin CoPP support introduced here [0] by Dumitru rebasing on top of ovn master branch and adding some missing meters (e.g. bfd or acl reject). The main goal of this series is to continue the discussion about the proposed approach and to align on CMS APIs. For the moment DDLog is not supported yet and it will be added in a subsequent series. Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1947913 https://bugzilla.redhat.com/show_bug.cgi?id=1946610 Changes since v5: - rebased on top of ovn master Changes since v4: - cosmetics - rebased on top of ovn master Changes since v3: - fix DDlog compilation errors - rebased on top of ovn master Changes since v2: - add sbctl checks in tests/ovn-northd.at unit tests - remove letfovers in utilities/ovn-nbctl.8.xml Changes since v1: - merge patch 3/5 and 4/5 - cosmetics - improve naming conventions - add more unit-tests/system-tests - remove duplicated flow - remove some leftover entries in ovn-nbctl.8.xml - add metering for sctp abort packets Changes since RFC: - drop per-port metering - add unit/system tests - add reject action metering Dumitru Ceara (4): ovn-controller: Add support for Logical_Flow control meters ovn-northd: Add support for CoPP. ovn-northd: Add CoPP policies for flows that punt packets to ovn-controller. NEWS: Add CoPP support. NEWS | 1 + controller/lflow.c | 40 ++- controller/ofctrl.c | 56 ++-- controller/ofctrl.h | 21 +- controller/physical.c | 9 +- include/ovn/actions.h | 3 +- lib/actions.c | 116 +++----- lib/automake.mk | 2 + lib/copp.c | 143 ++++++++++ lib/copp.h | 59 ++++ northd/ovn-northd.c | 567 ++++++++++++++++++++++++-------------- northd/ovn_northd.dl | 2 + ovn-nb.ovsschema | 16 +- ovn-nb.xml | 81 ++++++ ovn-sb.ovsschema | 4 +- ovn-sb.xml | 6 + tests/atlocal.in | 3 + tests/ovn-controller.at | 52 ++++ tests/ovn-northd.at | 96 +++++++ tests/ovn.at | 9 +- tests/system-ovn.at | 138 ++++++++++ utilities/ovn-nbctl.8.xml | 75 +++++ utilities/ovn-nbctl.c | 178 ++++++++++++ 23 files changed, 1364 insertions(+), 313 deletions(-) create mode 100644 lib/copp.c create mode 100644 lib/copp.h