| Message ID | 20210412124009.20468-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] controller: Monitor all logical flows that refer to datapath groups. | expand |
Thanks for the explanation Dumitru. It made good sense out of what had to be a difficult thing to debug. I'm assuming you went with unconditional monitoring since this will work with any change to a DP group, and not just the addition new datapaths to the group. Acked-by: Mark Michelson <mmichels@redhat.com> On 4/12/21 8:40 AM, Dumitru Ceara wrote: > Considering two logical datapaths (DP1 and DP2) and a logical flow that > is shared between the two, ovn-northd will create the following SB > records: > > Datapaths: DP1, DP2 > Logical_DP_Group: DPG1 {dps=[DP1, DP2]} > Logical_Flow: LF {dp_group=DPG1} > > If an ovn-controller is conditionally monitoring DP1 and DP2 its > monitor conditions look like this: > > Logical_DP_Group table when: > - Logical_DP_Group.datapaths includes [DP1._uuid or DP2._uuid] > Logical_Flow table when: > - Logical_Flow.logical_datapath in {DP1._uuid, DP2._uuid} > - Logical_Flow.logical_dp_group in {DPG1._uuid} > > If at this point a new logical datapath DP3 is created (e.g., a LS is > added) and the logical flow LF is shared with this third datapath, then > ovn-northd populates the SB with the following contents, replacing > old DPG1 with DPG1-new: > > Datapaths: DP1, DP2, DP3 > Logical_DP_Group: DPG1-new {dps=[DP1, DP2, DP3]} > Logical_Flow: LF {dp_group=DPG1-new} > > At this point, the SB ovsdb-server will send the following updates to > ovn-controller, to match the current monitor condition it requested: > > Datapaths: > - "insert" DP3 > > Logical_DP_Group: > - "delete" DPG1 > - "insert" DPG1-new {dps=[DP1, DP2, DP3]} > > Logical_Flow: > - "delete" LF > > DPG1-new is inserted in the client's view of the database because its > datapaths include DP1 and DP2 which are part of the client's monitor > condition for Logical_DP_Group. However, the logical flow is deleted > from the client's view of the database because the monitor condition > for Logical_Flow records doesn't include DPG1-new._uuid. > > Now ovn-controller will: > - delete all openflows corresponding to LF, including the ones generated > for datapaths DP1 and DP2. > - compute new set of monitor conditions and send the monitor_cond_change > request to the SB: > > Logical_DP_Group.datapaths includes > [DP1._uuid or DP2._uuid or DP3._uuid] > Logical_Flow table when: > - Logical_Flow.logical_datapath in {DP1._uuid, DP2._uuid, DP3._uuid} > - Logical_Flow.logical_dp_group in {DPG1-new._uuid} > > Upon processing this new set of conditions, the SB sends the following > update: > Logical_Flow: > - "insert" LF <--- now LF.logical_dp_group matches the condition. > > Finally, ovn-controller will add all openflows that correspond to LF, on > all datapaths, DP1, DP2, DP3. > > There is therefore a window in which traffic on DP1 and DP2 might be > dropped because openflows corresponding to LF1 on DP1 and DP2 have been > removed but not yet reinstalled. > > To avoid this we now unconditionally monitor all logical flows that > refer to datapath groups. > > Fixes: 4b946b366c4c ("controller: Add support for Logical Datapath Groups.") > Reported-at: https://bugzilla.redhat.com/1947056 > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Acked-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > V2: > - Rephrase commit log addressing Ilya's comments. > - Add Ilya's ack. > --- > controller/ovn-controller.c | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 16c8ecb21..6f7c9ea61 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -259,23 +259,15 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > uuid); > } > > - /* Updating conditions to receive logical flows that references > - * datapath groups containing local datapaths. */ > - const struct sbrec_logical_dp_group *group; > - SBREC_LOGICAL_DP_GROUP_FOR_EACH (group, ovnsb_idl) { > - struct uuid *uuid = CONST_CAST(struct uuid *, > - &group->header_.uuid); > - size_t i; > - > - for (i = 0; i < group->n_datapaths; i++) { > - if (get_local_datapath(local_datapaths, > - group->datapaths[i]->tunnel_key)) { > - sbrec_logical_flow_add_clause_logical_dp_group( > - &lf, OVSDB_F_EQ, uuid); > - break; > - } > - } > - } > + /* Datapath groups are immutable, which means a new group record is > + * created when a datapath is added to a group. The logical flows > + * referencing a datapath group are also updated in such cases but the > + * new group UUID is not known by ovn-controller until the SB update > + * is received. To avoid unnecessarily removing and adding lflows > + * that reference datapath groups, set the monitor condition to always > + * request all of them. > + */ > + sbrec_logical_flow_add_clause_logical_dp_group(&lf, OVSDB_F_NE, NULL); > } > > out:; >
On 4/16/21 7:16 PM, Mark Michelson wrote: > Thanks for the explanation Dumitru. It made good sense out of what had > to be a difficult thing to debug. I'm assuming you went with > unconditional monitoring since this will work with any change to a DP > group, and not just the addition new datapaths to the group. There's also a case I didn't mention in the commit log (but I can add it if you think it's worth adding): If a logical flow LF1 is not shared between datapaths, so it's only applied to DP1, when DP2 is added that needs an identical flow, ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, and create a new LF1' applied on datapath_group=DPG. Without unconditional monitoring for datapath groups there's no easy way to get the notification about DPG and LF1' being added in the same jsonrpc update from SB with the delete for LF. Given that the number of datapath groups is expected to be relatively low compared to the total number of logical flows, unconditional monitoring seems like the best solution. > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks!
On Mon, Apr 19, 2021 at 3:27 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 4/16/21 7:16 PM, Mark Michelson wrote: > > Thanks for the explanation Dumitru. It made good sense out of what had > > to be a difficult thing to debug. I'm assuming you went with > > unconditional monitoring since this will work with any change to a DP > > group, and not just the addition new datapaths to the group. > > There's also a case I didn't mention in the commit log (but I can add it > if you think it's worth adding): > > If a logical flow LF1 is not shared between datapaths, so it's only > applied to DP1, when DP2 is added that needs an identical flow, > ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, and > create a new LF1' applied on datapath_group=DPG. > > Without unconditional monitoring for datapath groups there's no easy way > to get the notification about DPG and LF1' being added in the same > jsonrpc update from SB with the delete for LF. > > Given that the number of datapath groups is expected to be relatively > low compared to the total number of logical flows, unconditional > monitoring seems like the best solution. > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks Dumitru. I applied this patch to the main branch. I checked your above comment after pushing the patch. Numan > > Thanks! > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Apr 19, 2021 at 12:27 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 4/16/21 7:16 PM, Mark Michelson wrote: > > Thanks for the explanation Dumitru. It made good sense out of what had > > to be a difficult thing to debug. I'm assuming you went with > > unconditional monitoring since this will work with any change to a DP > > group, and not just the addition new datapaths to the group. > > There's also a case I didn't mention in the commit log (but I can add it > if you think it's worth adding): > > If a logical flow LF1 is not shared between datapaths, so it's only > applied to DP1, when DP2 is added that needs an identical flow, > ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, and > create a new LF1' applied on datapath_group=DPG. > > Without unconditional monitoring for datapath groups there's no easy way > to get the notification about DPG and LF1' being added in the same > jsonrpc update from SB with the delete for LF. > > Given that the number of datapath groups is expected to be relatively > low compared to the total number of logical flows, unconditional > monitoring seems like the best solution. > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks! > +1 (although late) I am catching up the DPG implementation, and here are some thoughts on the use cases. For my understanding, except for the DPG that contains all DPs, most DPGs are mapping to port-groups, which usually map to tenants/namespaces. The conditional monitoring feature (when ovn-monitor-all is disabled) is useful mainly when there are a large number of small tenants and they don't need to talk to each other, so their workloads may reside on different HVs and DPs, so conditional monitoring can result in a smaller number of lflows on each HV in this scenario. However, when DPG related flows are unconditionally monitored, it makes conditional monitoring not as useful as it should be when there are a lot of ACLs, because ACLs are the major user of the port-group based DPGs, which may contribute to a significant portion of lflows. In such scenarios (when there are many small tenants), maybe it is better to disable DPG (and disable ovn-monitor-all) to benefit more from the conditional monitoring feature. In the contrary use cases when there are few but large tenants, each of them maps to a large port-group, crossing a large amount of datapaths, then enabling DPG may be a better choice, and at the same time enabling ovn-monitor-all may be recommended. Is the above understanding correct?
On 4/19/21 7:24 PM, Han Zhou wrote: > On Mon, Apr 19, 2021 at 12:27 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 4/16/21 7:16 PM, Mark Michelson wrote: >>> Thanks for the explanation Dumitru. It made good sense out of what had >>> to be a difficult thing to debug. I'm assuming you went with >>> unconditional monitoring since this will work with any change to a DP >>> group, and not just the addition new datapaths to the group. >> >> There's also a case I didn't mention in the commit log (but I can add it >> if you think it's worth adding): >> >> If a logical flow LF1 is not shared between datapaths, so it's only >> applied to DP1, when DP2 is added that needs an identical flow, >> ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, and >> create a new LF1' applied on datapath_group=DPG. >> >> Without unconditional monitoring for datapath groups there's no easy way >> to get the notification about DPG and LF1' being added in the same >> jsonrpc update from SB with the delete for LF. >> >> Given that the number of datapath groups is expected to be relatively >> low compared to the total number of logical flows, unconditional >> monitoring seems like the best solution. >> >>> >>> Acked-by: Mark Michelson <mmichels@redhat.com> >> >> Thanks! >> > > +1 (although late) > > I am catching up the DPG implementation, and here are some thoughts on the > use cases. For my understanding, except for the DPG that contains all DPs, > most DPGs are mapping to port-groups, which usually map to > tenants/namespaces. The conditional monitoring feature (when > ovn-monitor-all is disabled) is useful mainly when there are a large number > of small tenants and they don't need to talk to each other, so their > workloads may reside on different HVs and DPs, so conditional monitoring > can result in a smaller number of lflows on each HV in this scenario. > However, when DPG related flows are unconditionally monitored, it makes > conditional monitoring not as useful as it should be when there are a lot > of ACLs, because ACLs are the major user of the port-group based DPGs, > which may contribute to a significant portion of lflows. In such scenarios > (when there are many small tenants), maybe it is better to disable DPG (and > disable ovn-monitor-all) to benefit more from the conditional monitoring I think DPGs may be useful in this case too (many small tenants) with conditional monitoring enabled. There's a reasonable number of "trivial" logical flows, e.g., the flows that advance to next table by default, that are shared between all logical datapaths and DPGs will optimize those. For example, on one such large downstream OpenStack deployment we saw IIRC a reduction of ~20% w.r.t. number of logical flows and SB size when enabling DPG. It's not a huge factor but it's still a relevant reduction in number of objects in the SB that need to be sent to all ovn-controllers. > feature. In the contrary use cases when there are few but large tenants, > each of them maps to a large port-group, crossing a large amount of > datapaths, then enabling DPG may be a better choice, and at the same time > enabling ovn-monitor-all may be recommended. > > Is the above understanding correct? > I think so. Maybe it makes sense to try to add some guidelines about when to enable the datapath groups feature and how to combine it with conditional monitoring, maybe as part of the OVN documentation? What do you think? Thanks, Dumitru
On 4/19/21 6:34 PM, Numan Siddique wrote: > On Mon, Apr 19, 2021 at 3:27 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 4/16/21 7:16 PM, Mark Michelson wrote: >>> Thanks for the explanation Dumitru. It made good sense out of what had >>> to be a difficult thing to debug. I'm assuming you went with >>> unconditional monitoring since this will work with any change to a DP >>> group, and not just the addition new datapaths to the group. >> >> There's also a case I didn't mention in the commit log (but I can add it >> if you think it's worth adding): >> >> If a logical flow LF1 is not shared between datapaths, so it's only >> applied to DP1, when DP2 is added that needs an identical flow, >> ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, and >> create a new LF1' applied on datapath_group=DPG. >> >> Without unconditional monitoring for datapath groups there's no easy way >> to get the notification about DPG and LF1' being added in the same >> jsonrpc update from SB with the delete for LF. >> >> Given that the number of datapath groups is expected to be relatively >> low compared to the total number of logical flows, unconditional >> monitoring seems like the best solution. >> >>> >>> Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks Dumitru. I applied this patch to the main branch. > I checked your above comment after pushing the patch. > > Numan > Thanks!
On Tue, Apr 20, 2021 at 12:43 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 4/19/21 7:24 PM, Han Zhou wrote: > > On Mon, Apr 19, 2021 at 12:27 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On 4/16/21 7:16 PM, Mark Michelson wrote: > >>> Thanks for the explanation Dumitru. It made good sense out of what had > >>> to be a difficult thing to debug. I'm assuming you went with > >>> unconditional monitoring since this will work with any change to a DP > >>> group, and not just the addition new datapaths to the group. > >> > >> There's also a case I didn't mention in the commit log (but I can add it > >> if you think it's worth adding): > >> > >> If a logical flow LF1 is not shared between datapaths, so it's only > >> applied to DP1, when DP2 is added that needs an identical flow, > >> ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, and > >> create a new LF1' applied on datapath_group=DPG. > >> > >> Without unconditional monitoring for datapath groups there's no easy way > >> to get the notification about DPG and LF1' being added in the same > >> jsonrpc update from SB with the delete for LF. > >> > >> Given that the number of datapath groups is expected to be relatively > >> low compared to the total number of logical flows, unconditional > >> monitoring seems like the best solution. > >> > >>> > >>> Acked-by: Mark Michelson <mmichels@redhat.com> > >> > >> Thanks! > >> > > > > +1 (although late) > > > > I am catching up the DPG implementation, and here are some thoughts on the > > use cases. For my understanding, except for the DPG that contains all DPs, > > most DPGs are mapping to port-groups, which usually map to > > tenants/namespaces. The conditional monitoring feature (when > > ovn-monitor-all is disabled) is useful mainly when there are a large number > > of small tenants and they don't need to talk to each other, so their > > workloads may reside on different HVs and DPs, so conditional monitoring > > can result in a smaller number of lflows on each HV in this scenario. > > However, when DPG related flows are unconditionally monitored, it makes > > conditional monitoring not as useful as it should be when there are a lot > > of ACLs, because ACLs are the major user of the port-group based DPGs, > > which may contribute to a significant portion of lflows. In such scenarios > > (when there are many small tenants), maybe it is better to disable DPG (and > > disable ovn-monitor-all) to benefit more from the conditional monitoring > > I think DPGs may be useful in this case too (many small tenants) with > conditional monitoring enabled. > > There's a reasonable number of "trivial" logical flows, e.g., the flows > that advance to next table by default, that are shared between all > logical datapaths and DPGs will optimize those. > > For example, on one such large downstream OpenStack deployment we saw > IIRC a reduction of ~20% w.r.t. number of logical flows and SB size when > enabling DPG. It's not a huge factor but it's still a relevant > reduction in number of objects in the SB that need to be sent to all > ovn-controllers. It is true that DPG will still reduce SB size on the central nodes, but my above point is about the lflows being conditionally monitored by each individual HV, because usually central DB size is not a concern if ovn-monitor-all is disabled. In the particular scenario of large amount of small tenants, without DPG, a HV may be interested in only a small amount of DPs which has a small amount of flows in total. With DPG enabled, although sharing the common flows between the DPs would reduce some redundant flows, but since the number of interested DPs is small for each HV, this savings may be small. On the other hand, since all PG related ACL flows for all tenants (all DPs) are now monitored, each HV could have a huge increase of unnecessary ACL flows from unrelated tenants. That's why I think in such a scenario DPG should be disabled. > > > feature. In the contrary use cases when there are few but large tenants, > > each of them maps to a large port-group, crossing a large amount of > > datapaths, then enabling DPG may be a better choice, and at the same time > > enabling ovn-monitor-all may be recommended. > > > > Is the above understanding correct? > > > > I think so. > > Maybe it makes sense to try to add some guidelines about when to enable > the datapath groups feature and how to combine it with conditional > monitoring, maybe as part of the OVN documentation? What do you think? > Yes, I agree we should document the guidelines. > Thanks, > Dumitru > >
On 4/20/21 8:54 PM, Han Zhou wrote: > On Tue, Apr 20, 2021 at 12:43 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 4/19/21 7:24 PM, Han Zhou wrote: >>> On Mon, Apr 19, 2021 at 12:27 AM Dumitru Ceara <dceara@redhat.com> > wrote: >>>> >>>> On 4/16/21 7:16 PM, Mark Michelson wrote: >>>>> Thanks for the explanation Dumitru. It made good sense out of what had >>>>> to be a difficult thing to debug. I'm assuming you went with >>>>> unconditional monitoring since this will work with any change to a DP >>>>> group, and not just the addition new datapaths to the group. >>>> >>>> There's also a case I didn't mention in the commit log (but I can add > it >>>> if you think it's worth adding): >>>> >>>> If a logical flow LF1 is not shared between datapaths, so it's only >>>> applied to DP1, when DP2 is added that needs an identical flow, >>>> ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, > and >>>> create a new LF1' applied on datapath_group=DPG. >>>> >>>> Without unconditional monitoring for datapath groups there's no easy > way >>>> to get the notification about DPG and LF1' being added in the same >>>> jsonrpc update from SB with the delete for LF. >>>> >>>> Given that the number of datapath groups is expected to be relatively >>>> low compared to the total number of logical flows, unconditional >>>> monitoring seems like the best solution. >>>> >>>>> >>>>> Acked-by: Mark Michelson <mmichels@redhat.com> >>>> >>>> Thanks! >>>> >>> >>> +1 (although late) >>> >>> I am catching up the DPG implementation, and here are some thoughts on > the >>> use cases. For my understanding, except for the DPG that contains all > DPs, >>> most DPGs are mapping to port-groups, which usually map to >>> tenants/namespaces. The conditional monitoring feature (when >>> ovn-monitor-all is disabled) is useful mainly when there are a large > number >>> of small tenants and they don't need to talk to each other, so their >>> workloads may reside on different HVs and DPs, so conditional monitoring >>> can result in a smaller number of lflows on each HV in this scenario. >>> However, when DPG related flows are unconditionally monitored, it makes >>> conditional monitoring not as useful as it should be when there are a > lot >>> of ACLs, because ACLs are the major user of the port-group based DPGs, >>> which may contribute to a significant portion of lflows. In such > scenarios >>> (when there are many small tenants), maybe it is better to disable DPG > (and >>> disable ovn-monitor-all) to benefit more from the conditional monitoring >> >> I think DPGs may be useful in this case too (many small tenants) with >> conditional monitoring enabled. >> >> There's a reasonable number of "trivial" logical flows, e.g., the flows >> that advance to next table by default, that are shared between all >> logical datapaths and DPGs will optimize those. >> >> For example, on one such large downstream OpenStack deployment we saw >> IIRC a reduction of ~20% w.r.t. number of logical flows and SB size when >> enabling DPG. It's not a huge factor but it's still a relevant >> reduction in number of objects in the SB that need to be sent to all >> ovn-controllers. > > It is true that DPG will still reduce SB size on the central nodes, but my > above point is about the lflows being conditionally monitored by each > individual HV, because usually central DB size is not a concern if > ovn-monitor-all is disabled. In the particular scenario of large amount of > small tenants, without DPG, a HV may be interested in only a small amount > of DPs which has a small amount of flows in total. With DPG enabled, > although sharing the common flows between the DPs would reduce some > redundant flows, but since the number of interested DPs is small for each > HV, this savings may be small. On the other hand, since all PG related ACL > flows for all tenants (all DPs) are now monitored, each HV could have a > huge increase of unnecessary ACL flows from unrelated tenants. That's why I > think in such a scenario DPG should be disabled. I see, you're right, makes sense. Another way to make this work in all scenarios is to add two-level deep conditional monitoring to the IDL and ovsdb-server, that is, allow ovn-controller to conditionally monitor "all logical flows that refer to DPG that contain datapaths equal to a specific value". > >> >>> feature. In the contrary use cases when there are few but large tenants, >>> each of them maps to a large port-group, crossing a large amount of >>> datapaths, then enabling DPG may be a better choice, and at the same > time >>> enabling ovn-monitor-all may be recommended. >>> >>> Is the above understanding correct? >>> >> >> I think so. >> >> Maybe it makes sense to try to add some guidelines about when to enable >> the datapath groups feature and how to combine it with conditional >> monitoring, maybe as part of the OVN documentation? What do you think? >> > Yes, I agree we should document the guidelines. > >> Thanks, >> Dumitru >> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 4/21/21 9:40 AM, Dumitru Ceara wrote: > On 4/20/21 8:54 PM, Han Zhou wrote: >> On Tue, Apr 20, 2021 at 12:43 AM Dumitru Ceara <dceara@redhat.com> wrote: >>> >>> On 4/19/21 7:24 PM, Han Zhou wrote: >>>> On Mon, Apr 19, 2021 at 12:27 AM Dumitru Ceara <dceara@redhat.com> >> wrote: >>>>> >>>>> On 4/16/21 7:16 PM, Mark Michelson wrote: >>>>>> Thanks for the explanation Dumitru. It made good sense out of what had >>>>>> to be a difficult thing to debug. I'm assuming you went with >>>>>> unconditional monitoring since this will work with any change to a DP >>>>>> group, and not just the addition new datapaths to the group. >>>>> >>>>> There's also a case I didn't mention in the commit log (but I can add >> it >>>>> if you think it's worth adding): >>>>> >>>>> If a logical flow LF1 is not shared between datapaths, so it's only >>>>> applied to DP1, when DP2 is added that needs an identical flow, >>>>> ovn-northd might generate a fresh new DPG = {.datapaths=[DP1, DP2]}, >> and >>>>> create a new LF1' applied on datapath_group=DPG. >>>>> >>>>> Without unconditional monitoring for datapath groups there's no easy >> way >>>>> to get the notification about DPG and LF1' being added in the same >>>>> jsonrpc update from SB with the delete for LF. >>>>> >>>>> Given that the number of datapath groups is expected to be relatively >>>>> low compared to the total number of logical flows, unconditional >>>>> monitoring seems like the best solution. >>>>> >>>>>> >>>>>> Acked-by: Mark Michelson <mmichels@redhat.com> >>>>> >>>>> Thanks! >>>>> >>>> >>>> +1 (although late) >>>> >>>> I am catching up the DPG implementation, and here are some thoughts on >> the >>>> use cases. For my understanding, except for the DPG that contains all >> DPs, >>>> most DPGs are mapping to port-groups, which usually map to >>>> tenants/namespaces. The conditional monitoring feature (when >>>> ovn-monitor-all is disabled) is useful mainly when there are a large >> number >>>> of small tenants and they don't need to talk to each other, so their >>>> workloads may reside on different HVs and DPs, so conditional monitoring >>>> can result in a smaller number of lflows on each HV in this scenario. >>>> However, when DPG related flows are unconditionally monitored, it makes >>>> conditional monitoring not as useful as it should be when there are a >> lot >>>> of ACLs, because ACLs are the major user of the port-group based DPGs, >>>> which may contribute to a significant portion of lflows. In such >> scenarios >>>> (when there are many small tenants), maybe it is better to disable DPG >> (and >>>> disable ovn-monitor-all) to benefit more from the conditional monitoring >>> >>> I think DPGs may be useful in this case too (many small tenants) with >>> conditional monitoring enabled. >>> >>> There's a reasonable number of "trivial" logical flows, e.g., the flows >>> that advance to next table by default, that are shared between all >>> logical datapaths and DPGs will optimize those. >>> >>> For example, on one such large downstream OpenStack deployment we saw >>> IIRC a reduction of ~20% w.r.t. number of logical flows and SB size when >>> enabling DPG. It's not a huge factor but it's still a relevant >>> reduction in number of objects in the SB that need to be sent to all >>> ovn-controllers. >> >> It is true that DPG will still reduce SB size on the central nodes, but my >> above point is about the lflows being conditionally monitored by each >> individual HV, because usually central DB size is not a concern if >> ovn-monitor-all is disabled. In the particular scenario of large amount of >> small tenants, without DPG, a HV may be interested in only a small amount >> of DPs which has a small amount of flows in total. With DPG enabled, >> although sharing the common flows between the DPs would reduce some >> redundant flows, but since the number of interested DPs is small for each >> HV, this savings may be small. On the other hand, since all PG related ACL >> flows for all tenants (all DPs) are now monitored, each HV could have a >> huge increase of unnecessary ACL flows from unrelated tenants. That's why I >> think in such a scenario DPG should be disabled. > > I see, you're right, makes sense. Another way to make this work in all > scenarios is to add two-level deep conditional monitoring to the IDL and > ovsdb-server, that is, allow ovn-controller to conditionally monitor > "all logical flows that refer to DPG that contain datapaths equal to a > specific value". Another (probably, easier in implementation?) option is to avoid flow updates if we have in-flight condition changes, i.e. if we do not have a full view over logical flows. > >> >>> >>>> feature. In the contrary use cases when there are few but large tenants, >>>> each of them maps to a large port-group, crossing a large amount of >>>> datapaths, then enabling DPG may be a better choice, and at the same >> time >>>> enabling ovn-monitor-all may be recommended. >>>> >>>> Is the above understanding correct? >>>> >>> >>> I think so. >>> >>> Maybe it makes sense to try to add some guidelines about when to enable >>> the datapath groups feature and how to combine it with conditional >>> monitoring, maybe as part of the OVN documentation? What do you think? >>> >> Yes, I agree we should document the guidelines. >> >>> Thanks, >>> Dumitru >>> >>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 16c8ecb21..6f7c9ea61 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -259,23 +259,15 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, uuid); } - /* Updating conditions to receive logical flows that references - * datapath groups containing local datapaths. */ - const struct sbrec_logical_dp_group *group; - SBREC_LOGICAL_DP_GROUP_FOR_EACH (group, ovnsb_idl) { - struct uuid *uuid = CONST_CAST(struct uuid *, - &group->header_.uuid); - size_t i; - - for (i = 0; i < group->n_datapaths; i++) { - if (get_local_datapath(local_datapaths, - group->datapaths[i]->tunnel_key)) { - sbrec_logical_flow_add_clause_logical_dp_group( - &lf, OVSDB_F_EQ, uuid); - break; - } - } - } + /* Datapath groups are immutable, which means a new group record is + * created when a datapath is added to a group. The logical flows + * referencing a datapath group are also updated in such cases but the + * new group UUID is not known by ovn-controller until the SB update + * is received. To avoid unnecessarily removing and adding lflows + * that reference datapath groups, set the monitor condition to always + * request all of them. + */ + sbrec_logical_flow_add_clause_logical_dp_group(&lf, OVSDB_F_NE, NULL); } out:;
Considering two logical datapaths (DP1 and DP2) and a logical flow that is shared between the two, ovn-northd will create the following SB records: Datapaths: DP1, DP2 Logical_DP_Group: DPG1 {dps=[DP1, DP2]} Logical_Flow: LF {dp_group=DPG1} If an ovn-controller is conditionally monitoring DP1 and DP2 its monitor conditions look like this: Logical_DP_Group table when: - Logical_DP_Group.datapaths includes [DP1._uuid or DP2._uuid] Logical_Flow table when: - Logical_Flow.logical_datapath in {DP1._uuid, DP2._uuid} - Logical_Flow.logical_dp_group in {DPG1._uuid} If at this point a new logical datapath DP3 is created (e.g., a LS is added) and the logical flow LF is shared with this third datapath, then ovn-northd populates the SB with the following contents, replacing old DPG1 with DPG1-new: Datapaths: DP1, DP2, DP3 Logical_DP_Group: DPG1-new {dps=[DP1, DP2, DP3]} Logical_Flow: LF {dp_group=DPG1-new} At this point, the SB ovsdb-server will send the following updates to ovn-controller, to match the current monitor condition it requested: Datapaths: - "insert" DP3 Logical_DP_Group: - "delete" DPG1 - "insert" DPG1-new {dps=[DP1, DP2, DP3]} Logical_Flow: - "delete" LF DPG1-new is inserted in the client's view of the database because its datapaths include DP1 and DP2 which are part of the client's monitor condition for Logical_DP_Group. However, the logical flow is deleted from the client's view of the database because the monitor condition for Logical_Flow records doesn't include DPG1-new._uuid. Now ovn-controller will: - delete all openflows corresponding to LF, including the ones generated for datapaths DP1 and DP2. - compute new set of monitor conditions and send the monitor_cond_change request to the SB: Logical_DP_Group.datapaths includes [DP1._uuid or DP2._uuid or DP3._uuid] Logical_Flow table when: - Logical_Flow.logical_datapath in {DP1._uuid, DP2._uuid, DP3._uuid} - Logical_Flow.logical_dp_group in {DPG1-new._uuid} Upon processing this new set of conditions, the SB sends the following update: Logical_Flow: - "insert" LF <--- now LF.logical_dp_group matches the condition. Finally, ovn-controller will add all openflows that correspond to LF, on all datapaths, DP1, DP2, DP3. There is therefore a window in which traffic on DP1 and DP2 might be dropped because openflows corresponding to LF1 on DP1 and DP2 have been removed but not yet reinstalled. To avoid this we now unconditionally monitor all logical flows that refer to datapath groups. Fixes: 4b946b366c4c ("controller: Add support for Logical Datapath Groups.") Reported-at: https://bugzilla.redhat.com/1947056 Suggested-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- V2: - Rephrase commit log addressing Ilya's comments. - Add Ilya's ack. --- controller/ovn-controller.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)