| Message ID | 20210514205333.2836565-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-northd-ddlog: Document why projections are now output relations. | expand |
Bleep bloop. Greetings Ben Pfaff, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 112 characters long (recommended limit is 79) #49 FILE: Documentation/tutorials/ddlog-new-feature.rst:283: <https://github.com/vmware/differential-datalog/blob/master/doc/tutorial/tutorial.md#avoiding-weight-overflow>`_ Lines checked: 115, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Fri, May 14, 2021 at 4:54 PM Ben Pfaff <blp@ovn.org> wrote: > > Signed-off-by: Ben Pfaff <blp@ovn.org> > Suggested-by: Numan Siddique <numans@ovn.org> Thanks for the patch. Acked-by: Numan Siddique <numans@ovn.org> Numan > --- > Documentation/tutorials/ddlog-new-feature.rst | 31 +++++++++++++++++++ > northd/lswitch.dl | 12 +++++++ > 2 files changed, 43 insertions(+) > > diff --git a/Documentation/tutorials/ddlog-new-feature.rst b/Documentation/tutorials/ddlog-new-feature.rst > index 02876db66d74..de66ca5ada5c 100644 > --- a/Documentation/tutorials/ddlog-new-feature.rst > +++ b/Documentation/tutorials/ddlog-new-feature.rst > @@ -252,6 +252,37 @@ corresponding DDlog operations that need to be performed are: > At this point we have all the feature configuration relevant > information stored in DDlog relations in ``ovn-northd-ddlog`` memory. > > +Pitfalls of projections > +~~~~~~~~~~~~~~~~~~~~~~~ > + > +A projection is a join that uses only some of the data in a record. > +When the fields that are used have duplicates, the result can be many > +"copies" of a record, which DDlog represents internally with an > +integer "weight" that counts the number of copies. We don't have a > +projection with duplicates in this example, but `lswitch.dl` has many > +of them, such as this one:: > + > + relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) > + > + LogicalSwitchHasACLs(ls, true) :- > + LogicalSwitchACL(ls, _). > + > + LogicalSwitchHasACLs(ls, false) :- > + nb::Logical_Switch(._uuid = ls), > + not LogicalSwitchACL(ls, _). > + > +When multiple projections get joined together, the weights can > +overflow, which causes DDlog to malfunction. The solution is to make > +the relation an output relation, which causes DDlog to filter it > +through a "distinct" operator that reduces the weights to 1. Thus, > +`LogicalSwitchHasACLs` is actually implemented this way:: > + > + output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) > + > +For more information, see `Avoiding weight overflow > +<https://github.com/vmware/differential-datalog/blob/master/doc/tutorial/tutorial.md#avoiding-weight-overflow>`_ > +in the DDlog tutorial. > + > Write rules to update output relations > -------------------------------------- > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > index 8fbb313b9666..b98a9ea04c3c 100644 > --- a/northd/lswitch.dl > +++ b/northd/lswitch.dl > @@ -70,6 +70,8 @@ LogicalSwitchPortWithUnknownAddress(ls_uuid, lsp_uuid) :- > lsp in nb::Logical_Switch_Port(._uuid = lsp_uuid), > lsp.is_enabled() and lsp.addresses.contains("unknown"). > > +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +# is an output relation: > output relation LogicalSwitchHasUnknownPorts(ls: uuid, has_unknown: bool) > LogicalSwitchHasUnknownPorts(ls, true) :- LogicalSwitchPortWithUnknownAddress(ls, _). > LogicalSwitchHasUnknownPorts(ls, false) :- > @@ -116,6 +118,8 @@ LogicalSwitchStatefulACL(ls, acl) :- > LogicalSwitchACL(ls, acl), > nb::ACL(._uuid = acl, .action = "allow-related"). > > +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +# is an output relation: > output relation LogicalSwitchHasStatefulACL(ls: uuid, has_stateful_acl: bool) > > LogicalSwitchHasStatefulACL(ls, true) :- > @@ -125,6 +129,8 @@ LogicalSwitchHasStatefulACL(ls, false) :- > nb::Logical_Switch(._uuid = ls), > not LogicalSwitchStatefulACL(ls, _). > > +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +# is an output relation: > output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) > > LogicalSwitchHasACLs(ls, true) :- > @@ -170,6 +176,8 @@ LogicalSwitchWithDNSRecords(ls) :- > nb::DNS(._uuid = dns_uuid, .records = records), > not records.is_empty(). > > +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +# is an output relation: > output relation LogicalSwitchHasDNSRecords(ls: uuid, has_dns_records: bool) > > LogicalSwitchHasDNSRecords(ls, true) :- > @@ -186,6 +194,8 @@ LogicalSwitchHasNonRouterPort0(ls_uuid) :- > lsp in nb::Logical_Switch_Port(._uuid = lsp_uuid), > lsp.__type != "router". > > +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +# is an output relation: > output relation LogicalSwitchHasNonRouterPort(ls: uuid, has_non_router_port: bool) > LogicalSwitchHasNonRouterPort(ls, true) :- > LogicalSwitchHasNonRouterPort0(ls). > @@ -285,6 +295,8 @@ SwitchLBVIP(sw_uuid, lb, vip, backends) :- > var kv = FlatMap(vips), > (var vip, var backends) = kv. > > +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +# is an output relation: > output relation LogicalSwitchHasLBVIP(sw_uuid: uuid, has_lb_vip: bool) > LogicalSwitchHasLBVIP(sw_uuid, true) :- > SwitchLBVIP(.sw_uuid = sw_uuid). > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Sun, May 16, 2021 at 06:29:08PM -0400, Numan Siddique wrote: > On Fri, May 14, 2021 at 4:54 PM Ben Pfaff <blp@ovn.org> wrote: > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Suggested-by: Numan Siddique <numans@ovn.org> > > Thanks for the patch. > > Acked-by: Numan Siddique <numans@ovn.org> Thanks. I fixed the comment style (ddlog uses //, not #, silly me) and applied this to the main branch.
diff --git a/Documentation/tutorials/ddlog-new-feature.rst b/Documentation/tutorials/ddlog-new-feature.rst index 02876db66d74..de66ca5ada5c 100644 --- a/Documentation/tutorials/ddlog-new-feature.rst +++ b/Documentation/tutorials/ddlog-new-feature.rst @@ -252,6 +252,37 @@ corresponding DDlog operations that need to be performed are: At this point we have all the feature configuration relevant information stored in DDlog relations in ``ovn-northd-ddlog`` memory. +Pitfalls of projections +~~~~~~~~~~~~~~~~~~~~~~~ + +A projection is a join that uses only some of the data in a record. +When the fields that are used have duplicates, the result can be many +"copies" of a record, which DDlog represents internally with an +integer "weight" that counts the number of copies. We don't have a +projection with duplicates in this example, but `lswitch.dl` has many +of them, such as this one:: + + relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) + + LogicalSwitchHasACLs(ls, true) :- + LogicalSwitchACL(ls, _). + + LogicalSwitchHasACLs(ls, false) :- + nb::Logical_Switch(._uuid = ls), + not LogicalSwitchACL(ls, _). + +When multiple projections get joined together, the weights can +overflow, which causes DDlog to malfunction. The solution is to make +the relation an output relation, which causes DDlog to filter it +through a "distinct" operator that reduces the weights to 1. Thus, +`LogicalSwitchHasACLs` is actually implemented this way:: + + output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) + +For more information, see `Avoiding weight overflow +<https://github.com/vmware/differential-datalog/blob/master/doc/tutorial/tutorial.md#avoiding-weight-overflow>`_ +in the DDlog tutorial. + Write rules to update output relations -------------------------------------- diff --git a/northd/lswitch.dl b/northd/lswitch.dl index 8fbb313b9666..b98a9ea04c3c 100644 --- a/northd/lswitch.dl +++ b/northd/lswitch.dl @@ -70,6 +70,8 @@ LogicalSwitchPortWithUnknownAddress(ls_uuid, lsp_uuid) :- lsp in nb::Logical_Switch_Port(._uuid = lsp_uuid), lsp.is_enabled() and lsp.addresses.contains("unknown"). +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this +# is an output relation: output relation LogicalSwitchHasUnknownPorts(ls: uuid, has_unknown: bool) LogicalSwitchHasUnknownPorts(ls, true) :- LogicalSwitchPortWithUnknownAddress(ls, _). LogicalSwitchHasUnknownPorts(ls, false) :- @@ -116,6 +118,8 @@ LogicalSwitchStatefulACL(ls, acl) :- LogicalSwitchACL(ls, acl), nb::ACL(._uuid = acl, .action = "allow-related"). +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this +# is an output relation: output relation LogicalSwitchHasStatefulACL(ls: uuid, has_stateful_acl: bool) LogicalSwitchHasStatefulACL(ls, true) :- @@ -125,6 +129,8 @@ LogicalSwitchHasStatefulACL(ls, false) :- nb::Logical_Switch(._uuid = ls), not LogicalSwitchStatefulACL(ls, _). +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this +# is an output relation: output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) LogicalSwitchHasACLs(ls, true) :- @@ -170,6 +176,8 @@ LogicalSwitchWithDNSRecords(ls) :- nb::DNS(._uuid = dns_uuid, .records = records), not records.is_empty(). +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this +# is an output relation: output relation LogicalSwitchHasDNSRecords(ls: uuid, has_dns_records: bool) LogicalSwitchHasDNSRecords(ls, true) :- @@ -186,6 +194,8 @@ LogicalSwitchHasNonRouterPort0(ls_uuid) :- lsp in nb::Logical_Switch_Port(._uuid = lsp_uuid), lsp.__type != "router". +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this +# is an output relation: output relation LogicalSwitchHasNonRouterPort(ls: uuid, has_non_router_port: bool) LogicalSwitchHasNonRouterPort(ls, true) :- LogicalSwitchHasNonRouterPort0(ls). @@ -285,6 +295,8 @@ SwitchLBVIP(sw_uuid, lb, vip, backends) :- var kv = FlatMap(vips), (var vip, var backends) = kv. +# "Pitfalls of projections" in ddlog-new-feature.rst explains why this +# is an output relation: output relation LogicalSwitchHasLBVIP(sw_uuid: uuid, has_lb_vip: bool) LogicalSwitchHasLBVIP(sw_uuid, true) :- SwitchLBVIP(.sw_uuid = sw_uuid).
Signed-off-by: Ben Pfaff <blp@ovn.org> Suggested-by: Numan Siddique <numans@ovn.org> --- Documentation/tutorials/ddlog-new-feature.rst | 31 +++++++++++++++++++ northd/lswitch.dl | 12 +++++++ 2 files changed, 43 insertions(+)