Message ID | 1515148996-1533-1-git-send-email-dalvarez@redhat.com |
---|---|
State | Rejected |
Headers | show |
Series | [ovs-dev] ovn-northd: Avoid duplicate logical flows in SB db | expand |
Acked-By: Miguel Angel Ajo <majopela@redhat.com> On Fri, Jan 5, 2018 at 11:44 AM Daniel Alvarez <dalvarez@redhat.com> wrote: > When there are two ACLs in a Logical Switch with same direction, > priority, match and action fields, ovn-northd will generate the > exact same logical flow for them into SB database. This will make > ovn-controller log messages (INFO) saying that the duplicate flow > is going to be dropped. > > This patch avoids adding duplicate lflows into SB database so that > ovn-controller doesn't have to process them. > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > --- > > This patch is needed as part of the consistency work we're doing in the > OpenStack integration [0]. In our effort to ensure consistency across > objects in Neutron and OVN databases we find some special cases like > security group rules which match OVN ACLs but not in 1:1 relationship. > Until now, two identical security group rules beloning each to a > different security group would generate a single ACL in NB database. > With this behavior, there's no way to map the ACL in OVN to the > corresponding Neutron object. > > By implementing [0] we're trying to ensure this mapping so we make use > of the external_ids column of every table for this purpose. It may happen > that we'll have two identical ACLs but each referencing a different > Neutron object in their external_ids field. However, this will make > ovn-northd to generate two duplicate lflows into SB database which will > make ovn-controller drop them when installing the actual flows. With this > patch we'll avoid duplicate flows to be inserted in SB database in such > cases. > > [0] > https://docs.openstack.org/networking-ovn/latest/contributor/design/database_consistency.html > > ovn/northd/ovn-northd.c | 11 +++++++++++ > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 7e6b1d9..cc64861 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -428,6 +428,13 @@ struct macam_node { > struct eth_addr mac_addr; /* Allocated MAC address. */ > }; > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > + struct ovn_datapath *od, > + enum ovn_stage stage, > + uint16_t priority, > + const char *match, > + const char *actions); > + > static void > cleanup_macam(struct hmap *macam) > { > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct > ovn_datapath *od, > const char *stage_hint, const char *where) > { > ovs_assert(ovn_stage_to_datapath_type(stage) == > ovn_datapath_get_type(od)); > + > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, actions)) { > + return; > + } > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > ovn_lflow_init(lflow, od, stage, priority, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 954e259..ba96c81 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > AT_CLEANUP > + > +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate > lflows]) > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > +ovn_start > + > +ovn-nbctl ls-add S1 > + > +# Insert a duplicate ACL into NB database. > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl @acl > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl @acl > + > +# Check that there are two entries in ACL table in NB database. > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > +grep _uuid | wc -l], [0], [2 > +]) > + > +# Now make sure that only one logical flow is added to SB database. > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ > +grep _uuid | wc -l], [0], [1 > +]) > + > +AT_CLEANUP > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
I suspect that this doesn't go far enough, because it includes actions in the hash, so that it would fail to deduplicate two identical ACLs with different actions (e.g. "drop" versus "allow"). On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote: > When there are two ACLs in a Logical Switch with same direction, > priority, match and action fields, ovn-northd will generate the > exact same logical flow for them into SB database. This will make > ovn-controller log messages (INFO) saying that the duplicate flow > is going to be dropped. > > This patch avoids adding duplicate lflows into SB database so that > ovn-controller doesn't have to process them. > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > --- > > This patch is needed as part of the consistency work we're doing in the > OpenStack integration [0]. In our effort to ensure consistency across > objects in Neutron and OVN databases we find some special cases like > security group rules which match OVN ACLs but not in 1:1 relationship. > Until now, two identical security group rules beloning each to a > different security group would generate a single ACL in NB database. > With this behavior, there's no way to map the ACL in OVN to the > corresponding Neutron object. > > By implementing [0] we're trying to ensure this mapping so we make use > of the external_ids column of every table for this purpose. It may happen > that we'll have two identical ACLs but each referencing a different > Neutron object in their external_ids field. However, this will make > ovn-northd to generate two duplicate lflows into SB database which will > make ovn-controller drop them when installing the actual flows. With this > patch we'll avoid duplicate flows to be inserted in SB database in such > cases. > > [0] https://docs.openstack.org/networking-ovn/latest/contributor/design/database_consistency.html > > ovn/northd/ovn-northd.c | 11 +++++++++++ > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 7e6b1d9..cc64861 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -428,6 +428,13 @@ struct macam_node { > struct eth_addr mac_addr; /* Allocated MAC address. */ > }; > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > + struct ovn_datapath *od, > + enum ovn_stage stage, > + uint16_t priority, > + const char *match, > + const char *actions); > + > static void > cleanup_macam(struct hmap *macam) > { > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, > const char *stage_hint, const char *where) > { > ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od)); > + > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, actions)) { > + return; > + } > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > ovn_lflow_init(lflow, od, stage, priority, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 954e259..ba96c81 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > AT_CLEANUP > + > +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate lflows]) > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > +ovn_start > + > +ovn-nbctl ls-add S1 > + > +# Insert a duplicate ACL into NB database. > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl @acl > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl @acl > + > +# Check that there are two entries in ACL table in NB database. > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > +grep _uuid | wc -l], [0], [2 > +]) > + > +# Now make sure that only one logical flow is added to SB database. > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ > +grep _uuid | wc -l], [0], [1 > +]) > + > +AT_CLEANUP > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Right! We didn't hit that issue, but it'd make sense to fix in this patch I guess. We could modify the hashing function to not include the action (not sure if it does now..), and also have a separate search function that ignores the action. On Mon, Jan 8, 2018 at 5:39 PM Ben Pfaff <blp@ovn.org> wrote: > I suspect that this doesn't go far enough, because it includes actions > in the hash, so that it would fail to deduplicate two identical ACLs > with different actions (e.g. "drop" versus "allow"). > > On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote: > > When there are two ACLs in a Logical Switch with same direction, > > priority, match and action fields, ovn-northd will generate the > > exact same logical flow for them into SB database. This will make > > ovn-controller log messages (INFO) saying that the duplicate flow > > is going to be dropped. > > > > This patch avoids adding duplicate lflows into SB database so that > > ovn-controller doesn't have to process them. > > > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > > --- > > > > This patch is needed as part of the consistency work we're doing in the > > OpenStack integration [0]. In our effort to ensure consistency across > > objects in Neutron and OVN databases we find some special cases like > > security group rules which match OVN ACLs but not in 1:1 relationship. > > Until now, two identical security group rules beloning each to a > > different security group would generate a single ACL in NB database. > > With this behavior, there's no way to map the ACL in OVN to the > > corresponding Neutron object. > > > > By implementing [0] we're trying to ensure this mapping so we make use > > of the external_ids column of every table for this purpose. It may happen > > that we'll have two identical ACLs but each referencing a different > > Neutron object in their external_ids field. However, this will make > > ovn-northd to generate two duplicate lflows into SB database which will > > make ovn-controller drop them when installing the actual flows. With this > > patch we'll avoid duplicate flows to be inserted in SB database in such > > cases. > > > > [0] > https://docs.openstack.org/networking-ovn/latest/contributor/design/database_consistency.html > > > > ovn/northd/ovn-northd.c | 11 +++++++++++ > > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > > 2 files changed, 35 insertions(+) > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index 7e6b1d9..cc64861 100644 > > --- a/ovn/northd/ovn-northd.c > > +++ b/ovn/northd/ovn-northd.c > > @@ -428,6 +428,13 @@ struct macam_node { > > struct eth_addr mac_addr; /* Allocated MAC address. */ > > }; > > > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > > + struct ovn_datapath *od, > > + enum ovn_stage stage, > > + uint16_t priority, > > + const char *match, > > + const char *actions); > > + > > static void > > cleanup_macam(struct hmap *macam) > > { > > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct > ovn_datapath *od, > > const char *stage_hint, const char *where) > > { > > ovs_assert(ovn_stage_to_datapath_type(stage) == > ovn_datapath_get_type(od)); > > + > > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, actions)) > { > > + return; > > + } > > > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > > ovn_lflow_init(lflow, od, stage, priority, > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 954e259..ba96c81 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > > > AT_CLEANUP > > + > > +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate > lflows]) > > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > > +ovn_start > > + > > +ovn-nbctl ls-add S1 > > + > > +# Insert a duplicate ACL into NB database. > > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl > @acl > > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl > @acl > > + > > +# Check that there are two entries in ACL table in NB database. > > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > > +grep _uuid | wc -l], [0], [2 > > +]) > > + > > +# Now make sure that only one logical flow is added to SB database. > > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ > > +grep _uuid | wc -l], [0], [1 > > +]) > > + > > +AT_CLEANUP > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
If both ACLs have same priority, match, ..., but different actions, it is a misconfiguration in NB. What could northd do here besides raising an error log? Another point, would this type of check increase the difficulty of probably future incremental-processing in northd? From my point of view, it might be better just keep northd simple, and let clients handle the correctness, and let ovn-controller to do the final check. In this case, could Neutron maintain multiple sg-rule ids in external-ids of the same ACL entry? Thanks, Han On Mon, Jan 8, 2018 at 8:52 AM, Miguel Angel Ajo Pelayo <majopela@redhat.com > wrote: > Right! > > We didn't hit that issue, but it'd make sense to fix in this patch I guess. > > We could modify the hashing function to not include the action (not sure if > it does now..), > and also have a separate search function that ignores the action. > > On Mon, Jan 8, 2018 at 5:39 PM Ben Pfaff <blp@ovn.org> wrote: > > > I suspect that this doesn't go far enough, because it includes actions > > in the hash, so that it would fail to deduplicate two identical ACLs > > with different actions (e.g. "drop" versus "allow"). > > > > On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote: > > > When there are two ACLs in a Logical Switch with same direction, > > > priority, match and action fields, ovn-northd will generate the > > > exact same logical flow for them into SB database. This will make > > > ovn-controller log messages (INFO) saying that the duplicate flow > > > is going to be dropped. > > > > > > This patch avoids adding duplicate lflows into SB database so that > > > ovn-controller doesn't have to process them. > > > > > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > > > --- > > > > > > This patch is needed as part of the consistency work we're doing in the > > > OpenStack integration [0]. In our effort to ensure consistency across > > > objects in Neutron and OVN databases we find some special cases like > > > security group rules which match OVN ACLs but not in 1:1 relationship. > > > Until now, two identical security group rules beloning each to a > > > different security group would generate a single ACL in NB database. > > > With this behavior, there's no way to map the ACL in OVN to the > > > corresponding Neutron object. > > > > > > By implementing [0] we're trying to ensure this mapping so we make use > > > of the external_ids column of every table for this purpose. It may > happen > > > that we'll have two identical ACLs but each referencing a different > > > Neutron object in their external_ids field. However, this will make > > > ovn-northd to generate two duplicate lflows into SB database which will > > > make ovn-controller drop them when installing the actual flows. With > this > > > patch we'll avoid duplicate flows to be inserted in SB database in such > > > cases. > > > > > > [0] > > https://docs.openstack.org/networking-ovn/latest/ > contributor/design/database_consistency.html > > > > > > ovn/northd/ovn-northd.c | 11 +++++++++++ > > > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > > > 2 files changed, 35 insertions(+) > > > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > > index 7e6b1d9..cc64861 100644 > > > --- a/ovn/northd/ovn-northd.c > > > +++ b/ovn/northd/ovn-northd.c > > > @@ -428,6 +428,13 @@ struct macam_node { > > > struct eth_addr mac_addr; /* Allocated MAC address. */ > > > }; > > > > > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > > > + struct ovn_datapath *od, > > > + enum ovn_stage stage, > > > + uint16_t priority, > > > + const char *match, > > > + const char *actions); > > > + > > > static void > > > cleanup_macam(struct hmap *macam) > > > { > > > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct > > ovn_datapath *od, > > > const char *stage_hint, const char *where) > > > { > > > ovs_assert(ovn_stage_to_datapath_type(stage) == > > ovn_datapath_get_type(od)); > > > + > > > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, > actions)) > > { > > > + return; > > > + } > > > > > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > > > ovn_lflow_init(lflow, od, stage, priority, > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > index 954e259..ba96c81 100644 > > > --- a/tests/ovn-northd.at > > > +++ b/tests/ovn-northd.at > > > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > > > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > > > > > AT_CLEANUP > > > + > > > +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate > > lflows]) > > > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > > > +ovn_start > > > + > > > +ovn-nbctl ls-add S1 > > > + > > > +# Insert a duplicate ACL into NB database. > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl > > @acl > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl > > @acl > > > + > > > +# Check that there are two entries in ACL table in NB database. > > > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > > > +grep _uuid | wc -l], [0], [2 > > > +]) > > > + > > > +# Now make sure that only one logical flow is added to SB database. > > > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ > > > +grep _uuid | wc -l], [0], [1 > > > +]) > > > + > > > +AT_CLEANUP > > > -- > > > 1.8.3.1 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Let's step back and consider the options. Duplicate flow matches can happen, either because of bugs at any given level of the code, or because of user-provided data that can't practically be validated before it is passed down to the next level. Consider just the ovn-northd level. ovn-northd can do string comparisons to determine whether two flow matches are identical, but flow matches can be duplicates without being the same string (due to white space differences, order of clauses, different ways to express the same condition, implied versus explicit prerequisites, and so on). You quickly get into a question of the big-O to determine whether two Boolean expressions are the same. Furthermore, ovn-northd doesn't currently parse flow matches (and it's probably better if it doesn't). Worse, for correctness, it is not enough to know whether two flow matches are identical. Instead, you have to know whether they overlap. Consider "ip4" versus "ip4 && ip4.src == 1.2.3.4". These expressions overlap because they both match ipv4 packets with source address 1.2.3.4; if they have the same priority, then the treatment of the packet is ambiguous. Most people would say that, in this case, the more specific match should "win", but that's not always obvious (what if the matches were "ip4 && ip4.src == 1.2.3.4" and "ip4 && ip4.dst == 1.2.3.4"?), OpenFlow says the behavior is unspecified, and OVS doesn't have predictable behavior in this case. I believe that determining whether a group of matches overlap requires superlinear time. Some of this is a little easier at the ovn-controller level. ovn-controller converts flow matches into OpenFlow matches, and duplicates are more likely to be found out at that point. I say "more likely" because simple differences like white space, etc. will not matter. Some kinds of overlap will be found out too. So it might be worthwhile to think more about the particular bug, and determine whether whatever observed bad behavior can be better suppressed at a lower level. On Mon, Jan 08, 2018 at 09:43:08AM -0800, Han Zhou wrote: > If both ACLs have same priority, match, ..., but different actions, it is a > misconfiguration in NB. What could northd do here besides raising an error > log? > > Another point, would this type of check increase the difficulty of probably > future incremental-processing in northd? > > From my point of view, it might be better just keep northd simple, and let > clients handle the correctness, and let ovn-controller to do the final > check. In this case, could Neutron maintain multiple sg-rule ids in > external-ids of the same ACL entry? > > Thanks, > Han > > On Mon, Jan 8, 2018 at 8:52 AM, Miguel Angel Ajo Pelayo <majopela@redhat.com > > wrote: > > > Right! > > > > We didn't hit that issue, but it'd make sense to fix in this patch I guess. > > > > We could modify the hashing function to not include the action (not sure if > > it does now..), > > and also have a separate search function that ignores the action. > > > > On Mon, Jan 8, 2018 at 5:39 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > I suspect that this doesn't go far enough, because it includes actions > > > in the hash, so that it would fail to deduplicate two identical ACLs > > > with different actions (e.g. "drop" versus "allow"). > > > > > > On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote: > > > > When there are two ACLs in a Logical Switch with same direction, > > > > priority, match and action fields, ovn-northd will generate the > > > > exact same logical flow for them into SB database. This will make > > > > ovn-controller log messages (INFO) saying that the duplicate flow > > > > is going to be dropped. > > > > > > > > This patch avoids adding duplicate lflows into SB database so that > > > > ovn-controller doesn't have to process them. > > > > > > > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > > > > --- > > > > > > > > This patch is needed as part of the consistency work we're doing in the > > > > OpenStack integration [0]. In our effort to ensure consistency across > > > > objects in Neutron and OVN databases we find some special cases like > > > > security group rules which match OVN ACLs but not in 1:1 relationship. > > > > Until now, two identical security group rules beloning each to a > > > > different security group would generate a single ACL in NB database. > > > > With this behavior, there's no way to map the ACL in OVN to the > > > > corresponding Neutron object. > > > > > > > > By implementing [0] we're trying to ensure this mapping so we make use > > > > of the external_ids column of every table for this purpose. It may > > happen > > > > that we'll have two identical ACLs but each referencing a different > > > > Neutron object in their external_ids field. However, this will make > > > > ovn-northd to generate two duplicate lflows into SB database which will > > > > make ovn-controller drop them when installing the actual flows. With > > this > > > > patch we'll avoid duplicate flows to be inserted in SB database in such > > > > cases. > > > > > > > > [0] > > > https://docs.openstack.org/networking-ovn/latest/ > > contributor/design/database_consistency.html > > > > > > > > ovn/northd/ovn-northd.c | 11 +++++++++++ > > > > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > > > > 2 files changed, 35 insertions(+) > > > > > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > > > index 7e6b1d9..cc64861 100644 > > > > --- a/ovn/northd/ovn-northd.c > > > > +++ b/ovn/northd/ovn-northd.c > > > > @@ -428,6 +428,13 @@ struct macam_node { > > > > struct eth_addr mac_addr; /* Allocated MAC address. */ > > > > }; > > > > > > > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > > > > + struct ovn_datapath *od, > > > > + enum ovn_stage stage, > > > > + uint16_t priority, > > > > + const char *match, > > > > + const char *actions); > > > > + > > > > static void > > > > cleanup_macam(struct hmap *macam) > > > > { > > > > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct > > > ovn_datapath *od, > > > > const char *stage_hint, const char *where) > > > > { > > > > ovs_assert(ovn_stage_to_datapath_type(stage) == > > > ovn_datapath_get_type(od)); > > > > + > > > > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, > > actions)) > > > { > > > > + return; > > > > + } > > > > > > > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > > > > ovn_lflow_init(lflow, od, stage, priority, > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > > index 954e259..ba96c81 100644 > > > > --- a/tests/ovn-northd.at > > > > +++ b/tests/ovn-northd.at > > > > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 > > > > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > > > > > > > AT_CLEANUP > > > > + > > > > +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate > > > lflows]) > > > > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > > > > +ovn_start > > > > + > > > > +ovn-nbctl ls-add S1 > > > > + > > > > +# Insert a duplicate ACL into NB database. > > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl > > > @acl > > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ > > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl > > > @acl > > > > + > > > > +# Check that there are two entries in ACL table in NB database. > > > > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > > > > +grep _uuid | wc -l], [0], [2 > > > > +]) > > > > + > > > > +# Now make sure that only one logical flow is added to SB database. > > > > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ > > > > +grep _uuid | wc -l], [0], [1 > > > > +]) > > > > + > > > > +AT_CLEANUP > > > > -- > > > > 1.8.3.1 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
You're probably right, it's probably not worth increasing the complexity of ovn-northd when we can check this in the low level, and suppress the final OpenFlow duplicates, the code for that is already there, and working. An alternate an not very intrusive approach would be lowering the log level of the duplicate flow messages to DEBUG instead of INFO. Let me recap a bit. We need this because it's the best way we found on the db-consistency effort for networking-ovn, making the security group rules a 1:1 with ACLS for ports, which sometimes will mean that we have a duplicate ACL. It happens when you have a port attached to several security groups with equivalent rules: sg-web: ingress tcp port 80 ingress tcp port 22 sg-db: ingress tcp port 1234 ingress tcp port 22 port-A: security groups: sg-web + sg-db Until now, we were deduplicating the "ingress tcp 22" acl resulting of the two groups. But then tracking integrity/consistency of neutron_db vs ovn-nb explodes in complexity. (dalvarez can probably explain better.) On Mon, Jan 8, 2018 at 7:25 PM Ben Pfaff <blp@ovn.org> wrote: > Let's step back and consider the options. Duplicate flow matches can > happen, either because of bugs at any given level of the code, or > because of user-provided data that can't practically be validated before > it is passed down to the next level. > > Consider just the ovn-northd level. ovn-northd can do string > comparisons to determine whether two flow matches are identical, but > flow matches can be duplicates without being the same string (due to > white space differences, order of clauses, different ways to express the > same condition, implied versus explicit prerequisites, and so on). You > quickly get into a question of the big-O to determine whether two > Boolean expressions are the same. Furthermore, ovn-northd doesn't > currently parse flow matches (and it's probably better if it doesn't). > > Worse, for correctness, it is not enough to know whether two flow > matches are identical. Instead, you have to know whether they overlap. > Consider "ip4" versus "ip4 && ip4.src == 1.2.3.4". These expressions > overlap because they both match ipv4 packets with source address > 1.2.3.4; if they have the same priority, then the treatment of the > packet is ambiguous. Most people would say that, in this case, the more > specific match should "win", but that's not always obvious (what if the > matches were "ip4 && ip4.src == 1.2.3.4" and "ip4 && ip4.dst == > 1.2.3.4"?), OpenFlow says the behavior is unspecified, and OVS doesn't > have predictable behavior in this case. I believe that determining > whether a group of matches overlap requires superlinear time. > > Some of this is a little easier at the ovn-controller level. > ovn-controller converts flow matches into OpenFlow matches, and > duplicates are more likely to be found out at that point. I say "more > likely" because simple differences like white space, etc. will not > matter. Some kinds of overlap will be found out too. > > So it might be worthwhile to think more about the particular bug, and > determine whether whatever observed bad behavior can be better > suppressed at a lower level. > > On Mon, Jan 08, 2018 at 09:43:08AM -0800, Han Zhou wrote: > > If both ACLs have same priority, match, ..., but different actions, it > is a > > misconfiguration in NB. What could northd do here besides raising an > error > > log? > > > > Another point, would this type of check increase the difficulty of > probably > > future incremental-processing in northd? > > > > From my point of view, it might be better just keep northd simple, and > let > > clients handle the correctness, and let ovn-controller to do the final > > check. In this case, could Neutron maintain multiple sg-rule ids in > > external-ids of the same ACL entry? > > > > Thanks, > > Han > > > > On Mon, Jan 8, 2018 at 8:52 AM, Miguel Angel Ajo Pelayo < > majopela@redhat.com > > > wrote: > > > > > Right! > > > > > > We didn't hit that issue, but it'd make sense to fix in this patch I > guess. > > > > > > We could modify the hashing function to not include the action (not > sure if > > > it does now..), > > > and also have a separate search function that ignores the action. > > > > > > On Mon, Jan 8, 2018 at 5:39 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > I suspect that this doesn't go far enough, because it includes > actions > > > > in the hash, so that it would fail to deduplicate two identical ACLs > > > > with different actions (e.g. "drop" versus "allow"). > > > > > > > > On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote: > > > > > When there are two ACLs in a Logical Switch with same direction, > > > > > priority, match and action fields, ovn-northd will generate the > > > > > exact same logical flow for them into SB database. This will make > > > > > ovn-controller log messages (INFO) saying that the duplicate flow > > > > > is going to be dropped. > > > > > > > > > > This patch avoids adding duplicate lflows into SB database so that > > > > > ovn-controller doesn't have to process them. > > > > > > > > > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > > > > > --- > > > > > > > > > > This patch is needed as part of the consistency work we're doing > in the > > > > > OpenStack integration [0]. In our effort to ensure consistency > across > > > > > objects in Neutron and OVN databases we find some special cases > like > > > > > security group rules which match OVN ACLs but not in 1:1 > relationship. > > > > > Until now, two identical security group rules beloning each to a > > > > > different security group would generate a single ACL in NB > database. > > > > > With this behavior, there's no way to map the ACL in OVN to the > > > > > corresponding Neutron object. > > > > > > > > > > By implementing [0] we're trying to ensure this mapping so we make > use > > > > > of the external_ids column of every table for this purpose. It may > > > happen > > > > > that we'll have two identical ACLs but each referencing a different > > > > > Neutron object in their external_ids field. However, this will make > > > > > ovn-northd to generate two duplicate lflows into SB database which > will > > > > > make ovn-controller drop them when installing the actual flows. > With > > > this > > > > > patch we'll avoid duplicate flows to be inserted in SB database in > such > > > > > cases. > > > > > > > > > > [0] > > > > https://docs.openstack.org/networking-ovn/latest/ > > > contributor/design/database_consistency.html > > > > > > > > > > ovn/northd/ovn-northd.c | 11 +++++++++++ > > > > > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > > > > > 2 files changed, 35 insertions(+) > > > > > > > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > > > > index 7e6b1d9..cc64861 100644 > > > > > --- a/ovn/northd/ovn-northd.c > > > > > +++ b/ovn/northd/ovn-northd.c > > > > > @@ -428,6 +428,13 @@ struct macam_node { > > > > > struct eth_addr mac_addr; /* Allocated MAC address. */ > > > > > }; > > > > > > > > > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > > > > > + struct ovn_datapath *od, > > > > > + enum ovn_stage stage, > > > > > + uint16_t priority, > > > > > + const char *match, > > > > > + const char *actions); > > > > > + > > > > > static void > > > > > cleanup_macam(struct hmap *macam) > > > > > { > > > > > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, > struct > > > > ovn_datapath *od, > > > > > const char *stage_hint, const char *where) > > > > > { > > > > > ovs_assert(ovn_stage_to_datapath_type(stage) == > > > > ovn_datapath_get_type(od)); > > > > > + > > > > > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, > > > actions)) > > > > { > > > > > + return; > > > > > + } > > > > > > > > > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > > > > > ovn_lflow_init(lflow, od, stage, priority, > > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > > > index 954e259..ba96c81 100644 > > > > > --- a/tests/ovn-northd.at > > > > > +++ b/tests/ovn-northd.at > > > > > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 > router-port=R1-S1 > > > > > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > > > > > > > > > AT_CLEANUP > > > > > + > > > > > +AT_SETUP([ovn -- check that duplicate acls don't generate > duplicate > > > > lflows]) > > > > > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > > > > > +ovn_start > > > > > + > > > > > +ovn-nbctl ls-add S1 > > > > > + > > > > > +# Insert a duplicate ACL into NB database. > > > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport > priority=1000 \ > > > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 > acl > > > > @acl > > > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport > priority=1000 \ > > > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 > acl > > > > @acl > > > > > + > > > > > +# Check that there are two entries in ACL table in NB database. > > > > > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > > > > > +grep _uuid | wc -l], [0], [2 > > > > > +]) > > > > > + > > > > > +# Now make sure that only one logical flow is added to SB > database. > > > > > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ > > > > > +grep _uuid | wc -l], [0], [1 > > > > > +]) > > > > > + > > > > > +AT_CLEANUP > > > > > -- > > > > > 1.8.3.1 > > > > > > > > > > _______________________________________________ > > > > > dev mailing list > > > > > dev@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
Thanks Ben, Han, Miguel. On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo < majopela@redhat.com> wrote: > You're probably right, it's probably not worth increasing the complexity > of ovn-northd when we can check this in the low level, and suppress > the final OpenFlow duplicates, the code for that is already there, > and working. > > An alternate an not very intrusive approach would be lowering the > log level of the duplicate flow messages to DEBUG instead of INFO. > > Let me recap a bit. We need this because it's the best way > we found on the db-consistency effort for networking-ovn, making > the security group rules a 1:1 with ACLS for ports, which sometimes > will mean that we have a duplicate ACL. > > It happens when you have a port attached to several security groups > with equivalent rules: > > > sg-web: > ingress tcp port 80 > ingress tcp port 22 > > sg-db: > ingress tcp port 1234 > ingress tcp port 22 > > > port-A: > security groups: sg-web + sg-db > > > Until now, we were deduplicating the "ingress tcp 22" acl resulting > of the two groups. But then tracking integrity/consistency of neutron_db > vs ovn-nb explodes in complexity. (dalvarez can probably explain better.) > > Yes, your explanation is quite precise and I elaborated a bit more in the 'annotation' part in the patch. As Miguel says, we need some way to map OVN resources to Neutron ones so we're basically using the external_ids field to map an ACL to the corresponding OpenStack security group rule. As Han suggests, we could add multiple sg_rule_id's to the external_ids field but that would be race-condition prone and will add complexity to the networking-ovn side. Especially when we eventually want to move over to a model where we have ACLs per SG per rule and not per SG per rule per port. I'm fine with your suggestion of addressing it at a lower level in ovn-controller and maybe just change the log level from INFO to DEBUG. I'll post a patch in a while for this. Thanks again for your valuable comments :) > > > > On Mon, Jan 8, 2018 at 7:25 PM Ben Pfaff <blp@ovn.org> wrote: > > > Let's step back and consider the options. Duplicate flow matches can > > happen, either because of bugs at any given level of the code, or > > because of user-provided data that can't practically be validated before > > it is passed down to the next level. > > > > Consider just the ovn-northd level. ovn-northd can do string > > comparisons to determine whether two flow matches are identical, but > > flow matches can be duplicates without being the same string (due to > > white space differences, order of clauses, different ways to express the > > same condition, implied versus explicit prerequisites, and so on). You > > quickly get into a question of the big-O to determine whether two > > Boolean expressions are the same. Furthermore, ovn-northd doesn't > > currently parse flow matches (and it's probably better if it doesn't). > > > > Worse, for correctness, it is not enough to know whether two flow > > matches are identical. Instead, you have to know whether they overlap. > > Consider "ip4" versus "ip4 && ip4.src == 1.2.3.4". These expressions > > overlap because they both match ipv4 packets with source address > > 1.2.3.4; if they have the same priority, then the treatment of the > > packet is ambiguous. Most people would say that, in this case, the more > > specific match should "win", but that's not always obvious (what if the > > matches were "ip4 && ip4.src == 1.2.3.4" and "ip4 && ip4.dst == > > 1.2.3.4"?), OpenFlow says the behavior is unspecified, and OVS doesn't > > have predictable behavior in this case. I believe that determining > > whether a group of matches overlap requires superlinear time. > > > > Some of this is a little easier at the ovn-controller level. > > ovn-controller converts flow matches into OpenFlow matches, and > > duplicates are more likely to be found out at that point. I say "more > > likely" because simple differences like white space, etc. will not > > matter. Some kinds of overlap will be found out too. > > > > So it might be worthwhile to think more about the particular bug, and > > determine whether whatever observed bad behavior can be better > > suppressed at a lower level. > > > > On Mon, Jan 08, 2018 at 09:43:08AM -0800, Han Zhou wrote: > > > If both ACLs have same priority, match, ..., but different actions, it > > is a > > > misconfiguration in NB. What could northd do here besides raising an > > error > > > log? > > > > > > Another point, would this type of check increase the difficulty of > > probably > > > future incremental-processing in northd? > > > > > > From my point of view, it might be better just keep northd simple, and > > let > > > clients handle the correctness, and let ovn-controller to do the final > > > check. In this case, could Neutron maintain multiple sg-rule ids in > > > external-ids of the same ACL entry? > > > > > > Thanks, > > > Han > > > > > > On Mon, Jan 8, 2018 at 8:52 AM, Miguel Angel Ajo Pelayo < > > majopela@redhat.com > > > > wrote: > > > > > > > Right! > > > > > > > > We didn't hit that issue, but it'd make sense to fix in this patch I > > guess. > > > > > > > > We could modify the hashing function to not include the action (not > > sure if > > > > it does now..), > > > > and also have a separate search function that ignores the action. > > > > > > > > On Mon, Jan 8, 2018 at 5:39 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > I suspect that this doesn't go far enough, because it includes > > actions > > > > > in the hash, so that it would fail to deduplicate two identical > ACLs > > > > > with different actions (e.g. "drop" versus "allow"). > > > > > > > > > > On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote: > > > > > > When there are two ACLs in a Logical Switch with same direction, > > > > > > priority, match and action fields, ovn-northd will generate the > > > > > > exact same logical flow for them into SB database. This will make > > > > > > ovn-controller log messages (INFO) saying that the duplicate flow > > > > > > is going to be dropped. > > > > > > > > > > > > This patch avoids adding duplicate lflows into SB database so > that > > > > > > ovn-controller doesn't have to process them. > > > > > > > > > > > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > > > > > > --- > > > > > > > > > > > > This patch is needed as part of the consistency work we're doing > > in the > > > > > > OpenStack integration [0]. In our effort to ensure consistency > > across > > > > > > objects in Neutron and OVN databases we find some special cases > > like > > > > > > security group rules which match OVN ACLs but not in 1:1 > > relationship. > > > > > > Until now, two identical security group rules beloning each to a > > > > > > different security group would generate a single ACL in NB > > database. > > > > > > With this behavior, there's no way to map the ACL in OVN to the > > > > > > corresponding Neutron object. > > > > > > > > > > > > By implementing [0] we're trying to ensure this mapping so we > make > > use > > > > > > of the external_ids column of every table for this purpose. It > may > > > > happen > > > > > > that we'll have two identical ACLs but each referencing a > different > > > > > > Neutron object in their external_ids field. However, this will > make > > > > > > ovn-northd to generate two duplicate lflows into SB database > which > > will > > > > > > make ovn-controller drop them when installing the actual flows. > > With > > > > this > > > > > > patch we'll avoid duplicate flows to be inserted in SB database > in > > such > > > > > > cases. > > > > > > > > > > > > [0] > > > > > https://docs.openstack.org/networking-ovn/latest/ > > > > contributor/design/database_consistency.html > > > > > > > > > > > > ovn/northd/ovn-northd.c | 11 +++++++++++ > > > > > > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > > > > > > 2 files changed, 35 insertions(+) > > > > > > > > > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > > > > > index 7e6b1d9..cc64861 100644 > > > > > > --- a/ovn/northd/ovn-northd.c > > > > > > +++ b/ovn/northd/ovn-northd.c > > > > > > @@ -428,6 +428,13 @@ struct macam_node { > > > > > > struct eth_addr mac_addr; /* Allocated MAC address. */ > > > > > > }; > > > > > > > > > > > > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, > > > > > > + struct ovn_datapath *od, > > > > > > + enum ovn_stage stage, > > > > > > + uint16_t priority, > > > > > > + const char *match, > > > > > > + const char *actions); > > > > > > + > > > > > > static void > > > > > > cleanup_macam(struct hmap *macam) > > > > > > { > > > > > > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, > > struct > > > > > ovn_datapath *od, > > > > > > const char *stage_hint, const char *where) > > > > > > { > > > > > > ovs_assert(ovn_stage_to_datapath_type(stage) == > > > > > ovn_datapath_get_type(od)); > > > > > > + > > > > > > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, > > > > actions)) > > > > > { > > > > > > + return; > > > > > > + } > > > > > > > > > > > > struct ovn_lflow *lflow = xmalloc(sizeof *lflow); > > > > > > ovn_lflow_init(lflow, od, stage, priority, > > > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > > > > index 954e259..ba96c81 100644 > > > > > > --- a/tests/ovn-northd.at > > > > > > +++ b/tests/ovn-northd.at > > > > > > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 > > router-port=R1-S1 > > > > > > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > > > > > > > > > > > > AT_CLEANUP > > > > > > + > > > > > > +AT_SETUP([ovn -- check that duplicate acls don't generate > > duplicate > > > > > lflows]) > > > > > > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > > > > > > +ovn_start > > > > > > + > > > > > > +ovn-nbctl ls-add S1 > > > > > > + > > > > > > +# Insert a duplicate ACL into NB database. > > > > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport > > priority=1000 \ > > > > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 > > acl > > > > > @acl > > > > > > +ovn-nbctl -- --id=@acl create acl direction=to-lport > > priority=1000 \ > > > > > > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 > > acl > > > > > @acl > > > > > > + > > > > > > +# Check that there are two entries in ACL table in NB database. > > > > > > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ > > > > > > +grep _uuid | wc -l], [0], [2 > > > > > > +]) > > > > > > + > > > > > > +# Now make sure that only one logical flow is added to SB > > database. > > > > > > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | > \ > > > > > > +grep _uuid | wc -l], [0], [1 > > > > > > +]) > > > > > > + > > > > > > +AT_CLEANUP > > > > > > -- > > > > > > 1.8.3.1 > > > > > > > > > > > > _______________________________________________ > > > > > > dev mailing list > > > > > > dev@openvswitch.org > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > > > > dev mailing list > > > > > dev@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jan 9, 2018 at 3:12 AM, Daniel Alvarez Sanchez <dalvarez@redhat.com> wrote: > > Thanks Ben, Han, Miguel. > > > On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo < > majopela@redhat.com> wrote: > > > You're probably right, it's probably not worth increasing the complexity > > of ovn-northd when we can check this in the low level, and suppress > > the final OpenFlow duplicates, the code for that is already there, > > and working. > > > > An alternate an not very intrusive approach would be lowering the > > log level of the duplicate flow messages to DEBUG instead of INFO. > > > > Let me recap a bit. We need this because it's the best way > > we found on the db-consistency effort for networking-ovn, making > > the security group rules a 1:1 with ACLS for ports, which sometimes > > will mean that we have a duplicate ACL. > > > > It happens when you have a port attached to several security groups > > with equivalent rules: > > > > > > sg-web: > > ingress tcp port 80 > > ingress tcp port 22 > > > > sg-db: > > ingress tcp port 1234 > > ingress tcp port 22 > > > > > > port-A: > > security groups: sg-web + sg-db > > > > > > Until now, we were deduplicating the "ingress tcp 22" acl resulting > > of the two groups. But then tracking integrity/consistency of neutron_db > > vs ovn-nb explodes in complexity. (dalvarez can probably explain better.) > > > > Yes, your explanation is quite precise and I elaborated a bit > more in the 'annotation' part in the patch. > > As Miguel says, we need some way to map OVN resources to Neutron > ones so we're basically using the external_ids field to map an ACL to the > corresponding OpenStack security group rule. As Han suggests, we could > add multiple sg_rule_id's to the external_ids field but that would be > race-condition > prone and will add complexity to the networking-ovn side. Especially when we > eventually want to move over to a model where we have ACLs per SG per > rule and not per SG per rule per port. Agree. Simplicity is preferred. Thanks for the detailed explaining.
On Tue, Jan 09, 2018 at 12:12:14PM +0100, Daniel Alvarez Sanchez wrote: > Thanks Ben, Han, Miguel. > > > On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo < > majopela@redhat.com> wrote: > > > You're probably right, it's probably not worth increasing the complexity > > of ovn-northd when we can check this in the low level, and suppress > > the final OpenFlow duplicates, the code for that is already there, > > and working. > > > > An alternate an not very intrusive approach would be lowering the > > log level of the duplicate flow messages to DEBUG instead of INFO. > > > > Let me recap a bit. We need this because it's the best way > > we found on the db-consistency effort for networking-ovn, making > > the security group rules a 1:1 with ACLS for ports, which sometimes > > will mean that we have a duplicate ACL. > > > > It happens when you have a port attached to several security groups > > with equivalent rules: > > > > > > sg-web: > > ingress tcp port 80 > > ingress tcp port 22 > > > > sg-db: > > ingress tcp port 1234 > > ingress tcp port 22 > > > > > > port-A: > > security groups: sg-web + sg-db > > > > > > Until now, we were deduplicating the "ingress tcp 22" acl resulting > > of the two groups. But then tracking integrity/consistency of neutron_db > > vs ovn-nb explodes in complexity. (dalvarez can probably explain better.) > > > > Yes, your explanation is quite precise and I elaborated a bit > more in the 'annotation' part in the patch. > > As Miguel says, we need some way to map OVN resources to Neutron > ones so we're basically using the external_ids field to map an ACL to the > corresponding OpenStack security group rule. As Han suggests, we could > add multiple sg_rule_id's to the external_ids field but that would be > race-condition > prone and will add complexity to the networking-ovn side. Especially when we > eventually want to move over to a model where we have ACLs per SG per > rule and not per SG per rule per port. > > I'm fine with your suggestion of addressing it at a lower level in > ovn-controller > and maybe just change the log level from INFO to DEBUG. > I'll post a patch in a while for this. Thanks. Please add a link to this thread in the email archive to the commit message.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 7e6b1d9..cc64861 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -428,6 +428,13 @@ struct macam_node { struct eth_addr mac_addr; /* Allocated MAC address. */ }; +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows, + struct ovn_datapath *od, + enum ovn_stage stage, + uint16_t priority, + const char *match, + const char *actions); + static void cleanup_macam(struct hmap *macam) { @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, const char *stage_hint, const char *where) { ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od)); + + if (ovn_lflow_find(lflow_map, od, stage, priority, match, actions)) { + return; + } struct ovn_lflow *lflow = xmalloc(sizeof *lflow); ovn_lflow_init(lflow, od, stage, priority, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 954e259..ba96c81 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) AT_CLEANUP + +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate lflows]) +AT_SKIP_IF([test $HAVE_PYTHON = no]) +ovn_start + +ovn-nbctl ls-add S1 + +# Insert a duplicate ACL into NB database. +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl @acl +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \ + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl @acl + +# Check that there are two entries in ACL table in NB database. +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \ +grep _uuid | wc -l], [0], [2 +]) + +# Now make sure that only one logical flow is added to SB database. +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \ +grep _uuid | wc -l], [0], [1 +]) + +AT_CLEANUP
When there are two ACLs in a Logical Switch with same direction, priority, match and action fields, ovn-northd will generate the exact same logical flow for them into SB database. This will make ovn-controller log messages (INFO) saying that the duplicate flow is going to be dropped. This patch avoids adding duplicate lflows into SB database so that ovn-controller doesn't have to process them. Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> --- This patch is needed as part of the consistency work we're doing in the OpenStack integration [0]. In our effort to ensure consistency across objects in Neutron and OVN databases we find some special cases like security group rules which match OVN ACLs but not in 1:1 relationship. Until now, two identical security group rules beloning each to a different security group would generate a single ACL in NB database. With this behavior, there's no way to map the ACL in OVN to the corresponding Neutron object. By implementing [0] we're trying to ensure this mapping so we make use of the external_ids column of every table for this purpose. It may happen that we'll have two identical ACLs but each referencing a different Neutron object in their external_ids field. However, this will make ovn-northd to generate two duplicate lflows into SB database which will make ovn-controller drop them when installing the actual flows. With this patch we'll avoid duplicate flows to be inserted in SB database in such cases. [0] https://docs.openstack.org/networking-ovn/latest/contributor/design/database_consistency.html ovn/northd/ovn-northd.c | 11 +++++++++++ tests/ovn-northd.at | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+)