| Message ID | 1603185512-8070-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-controller: Monitor chassis_private by chassis name. | expand |
On 20/10/2020 10:18, Dumitru Ceara wrote: > Remove the use of sbrec_chassis_is_new() for uncommitted records. This > is not the way IDL *_is_new() functions are supposed to be used. > > Note: With this change if the system-id changes there will be a > transient error in ovn-controller due to ovn-controller trying to insert > a new chassis_private record. This is due to the fact that the view of > the chassis_private table changes and only chassis_private records > matching the new chassis name are sent to ovn-controller. This gets > corrected though in the next iteration of the ovn-controller processing > loop. > > Suggested-by: Han Zhou <hzhou@ovn.org> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/376339.html > Fixes: dce1af31b550 ("chassis: Fix chassis_private record updates when the system-id changes.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/ovn-controller.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 066e31f..a06cae3 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -181,7 +181,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > * chassis */ > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "chassisredirect"); > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "external"); > - if (chassis && !sbrec_chassis_is_new(chassis)) { > + if (chassis) { > /* This should be mostly redundant with the other clauses for port > * bindings, but it allows us to catch any ports that are assigned to > * us but should not be. That way, we can clear their chassis > @@ -205,8 +205,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > &chassis->header_.uuid); > > /* Monitors Chassis_Private record for current chassis only. */ > - sbrec_chassis_private_add_clause_chassis(&chprv, OVSDB_F_EQ, > - &chassis->header_.uuid); > + sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ, > + chassis->name); > } else { > /* During initialization, we monitor all records in Chassis_Private so > * that we don't try to recreate existing ones. */ > I tested this against my v1 patch [1] and the OVN tests passed as expected. Acked-by: Mark Gray <mark.d.gray@redhat.com> [1] v1: https://patchwork.ozlabs.org/project/openvswitch/patch/20200929183433.925570-1-mark.d.gray@redhat.com/
On Tue, Oct 20, 2020 at 3:58 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > On 20/10/2020 10:18, Dumitru Ceara wrote: > > Remove the use of sbrec_chassis_is_new() for uncommitted records. This > > is not the way IDL *_is_new() functions are supposed to be used. > > > > Note: With this change if the system-id changes there will be a > > transient error in ovn-controller due to ovn-controller trying to insert > > a new chassis_private record. This is due to the fact that the view of > > the chassis_private table changes and only chassis_private records > > matching the new chassis name are sent to ovn-controller. This gets > > corrected though in the next iteration of the ovn-controller processing > > loop. > > > > Suggested-by: Han Zhou <hzhou@ovn.org> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/376339.html > > Fixes: dce1af31b550 ("chassis: Fix chassis_private record updates when the system-id changes.") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > controller/ovn-controller.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 066e31f..a06cae3 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -181,7 +181,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > * chassis */ > > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "chassisredirect"); > > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "external"); > > - if (chassis && !sbrec_chassis_is_new(chassis)) { > > + if (chassis) { > > /* This should be mostly redundant with the other clauses for port > > * bindings, but it allows us to catch any ports that are assigned to > > * us but should not be. That way, we can clear their chassis > > @@ -205,8 +205,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > &chassis->header_.uuid); > > > > /* Monitors Chassis_Private record for current chassis only. */ > > - sbrec_chassis_private_add_clause_chassis(&chprv, OVSDB_F_EQ, > > - &chassis->header_.uuid); > > + sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ, > > + chassis->name); > > } else { > > /* During initialization, we monitor all records in Chassis_Private so > > * that we don't try to recreate existing ones. */ > > > > I tested this against my v1 patch [1] and the OVN tests passed as expected. > > Acked-by: Mark Gray <mark.d.gray@redhat.com> > > [1] v1: > https://patchwork.ozlabs.org/project/openvswitch/patch/20200929183433.925570-1-mark.d.gray@redhat.com/ > Thanks Dumitru and Mark. I applied this to master. Han
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 066e31f..a06cae3 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -181,7 +181,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, * chassis */ sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "chassisredirect"); sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "external"); - if (chassis && !sbrec_chassis_is_new(chassis)) { + if (chassis) { /* This should be mostly redundant with the other clauses for port * bindings, but it allows us to catch any ports that are assigned to * us but should not be. That way, we can clear their chassis @@ -205,8 +205,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, &chassis->header_.uuid); /* Monitors Chassis_Private record for current chassis only. */ - sbrec_chassis_private_add_clause_chassis(&chprv, OVSDB_F_EQ, - &chassis->header_.uuid); + sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ, + chassis->name); } else { /* During initialization, we monitor all records in Chassis_Private so * that we don't try to recreate existing ones. */
Remove the use of sbrec_chassis_is_new() for uncommitted records. This is not the way IDL *_is_new() functions are supposed to be used. Note: With this change if the system-id changes there will be a transient error in ovn-controller due to ovn-controller trying to insert a new chassis_private record. This is due to the fact that the view of the chassis_private table changes and only chassis_private records matching the new chassis name are sent to ovn-controller. This gets corrected though in the next iteration of the ovn-controller processing loop. Suggested-by: Han Zhou <hzhou@ovn.org> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/376339.html Fixes: dce1af31b550 ("chassis: Fix chassis_private record updates when the system-id changes.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/ovn-controller.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)