On 4/8/21 8:45 PM, 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
> Datapath_Group: DPG1 {dps=[DP1, DP2]}
s/Datapath_Group/Logical_DP_Group/g
Not a big deal, but just for consistency for future readers.
> Logical_Flow: LF {dp_group=DPG1}
>
> If an ovn-controller is conditionally monitoring DP1 and DP2 its
> monitor conditions look like this:
>
> Datapath_Group table when:
> - Datapath_Group.datapaths in {DP1._uuid, DP2._uuid}
'in' doesn't sound right as it's more an opposite relation.
Again, it's a nitpick, but maybe worth changing to something
like:
- 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
> Datapath_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
>
> Datapath_Group:
> - "delete" DPG1
> - "insert" DPG1-new {dps=[DP1, DP2, DP3]}
We can clarify here that this new group received because it has
DP1 and DP2 and we have this condition set.
>
> Logical_Flow:
> - "delete" LF
>
> 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:
>
> Datapath_Group table when Datapath_Group.uuid in {DPG1-new._uuid}
This should look like this (we're not monitoring groups by group uuid):
- 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 <[email protected]>
> Signed-off-by: Dumitru Ceara <[email protected]>
> ---
Beside some small nitpicks above, LGTM.
Acked-by: Ilya Maximets <[email protected]>
> 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:;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
