On Tue, May 17, 2022 at 9:55 AM Mark Michelson <[email protected]> wrote:
> OK thanks for the explanations, Xavier and Mary. This is pretty > important since we expect that ovn-controller should be upgraded before > ovn-northd. > > I'm acking the patch because the content is correct. I suggest that when > this is merged, the person that merges should expand on the commit > message a bit to explain why the commit is necessary. If nothing else, > make it clear that 20.09 is the cutoff between an "older" and "newer" > version of ovn-northd in this case. > > Acked-by: Mark Michelson > Thanks Mark for the review. @Mary - would you mind updating just the commit message here ? Thanks Numan > > On 5/12/22 11:59, Mary Manohar wrote: > > Thanks Xavier for clarifying. > > > > Hi Mark, > > > > Xavier’s explanation is bang on. The ‘up’ field is introduced in the > > Port Binding table post 20.09. > > > > So, when the southbound is at 20.09 version, the set operation fails in > > ovn-controller. > > > > Thanks > > > > Mary > > > > *From: *Xavier Simonart <[email protected]> > > *Date: *Wednesday, May 11, 2022 at 8:10 AM > > *To: *Mark Michelson <[email protected]> > > *Cc: *Mary Manohar <[email protected]>, [email protected] > > <[email protected]> > > *Subject: *Re: [ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the > > port-binding table if ovn-northd is at an older version. > > > > Hi Mark > > > > This is fixing a backward compatibility issue when using an older > > ovn-northd. > > > > ovn-controller should only set Port_Binding.up field if the Southbound > > DB is aware of this field (or transaction would fail). > > > > We already do this when setting pb up through notification (if-status > > module). > > > > More explanations on commit 8b45fc9b2 from Dumitru. > > > > Thanks > > > > Xavier > > > > On Tue, May 10, 2022 at 8:12 PM Mark Michelson <[email protected] > > <mailto:[email protected]>> wrote: > > > > Hi Mary, > > > > I'm confused by this change. The summary mentions ovn-northd being > > at an > > older version, but there's no version check being performed in the > > patch. Also, what constitutes an "older" version of ovn-northd? Why > > shouldn't we set the port up in this case? What problem is being > solved? > > > > On 5/9/22 15:28, [email protected] > > <mailto:[email protected]> wrote: > > > From: Mary Manohar <[email protected] <mailto: > [email protected]>> > > > > > > Signed-off-by: Mary Manohar <[email protected] <mailto: > [email protected]>> > > > --- > > > controller/binding.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index e284704..e7dc537 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -908,7 +908,9 @@ claimed_lport_set_up(const struct > sbrec_port_binding *pb, > > > if (!notify_up) { > > > bool up = true; > > > if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) > { > > > - sbrec_port_binding_set_up(pb, &up, 1); > > > + if (pb->n_up) { > > > + sbrec_port_binding_set_up(pb, &up, 1); > > > + } > > > } > > > return; > > > } > > > > > > > _______________________________________________ > > dev mailing list > > [email protected] <mailto:[email protected]> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > [mail.openvswitch.org] > > < > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=PrJpP14VwAwIfuISqmhQn4UbXr0029-Ifb7y0YpWR_A&m=4M1akZ2SPpiYOkwcXOUTtZYo6SeEU7e0T0dg9b9E3E1ypfMSyt_QPOvkYzq20Pkr&s=go8izusrxnEFuOBTOkGitblsby6P-0arnWEq0ZkNJ2w&e= > > > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
