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 Numan
Sure, I have sent out a v3 with the updated commit message.
Thanks
Mary
From: Numan Siddique
Date: Wednesday, May 18, 2022 at 7:37 AM
To: Mark Michelson
Cc: Mary Manohar , Xavier Simonart
, ovs-dev@openvswitch.org
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.
On Tue, May 17, 2022 at 9:55 AM Mark Michelson
mailto:mmich...@redhat.com>> 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 mailto:xsimo...@redhat.com>>
> *Date: *Wednesday, May 11, 2022 at 8:10 AM
> *To: *Mark Michelson mailto:mmich...@redhat.com>>
> *Cc: *Mary Manohar
> mailto:mary.mano...@nutanix.com>>,
> ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org>
> mailto:ovs-dev@openvswitch.org>>
> *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
> mailto:mmich...@redhat.com>
> <mailto:mmich...@redhat.com<mailto:mmich...@redhat.com>>> 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, mary.mano...@nutanix.com<mailto:mary.mano...@nutanix.com>
> <mailto:mary.mano...@nutanix.com<mailto:mary.mano...@nutanix.com>> wrote:
> > From: Mary Manohar
> mailto:mary.mano...@nutanix.com>
> <mailto:mary.mano...@nutanix.com<mailto:mary.mano...@nutanix.com>>>
> >
> > Signed-off-by: Mary Manohar
> mailto:mary.mano...@nutanix.com>
> <mailto:mary.mano...@nutanix.com<mailto:mary.mano...@nutanix.com>>>
> > ---
> > 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
> d...@openvswitch.org<mailto:d...@openvswitch.org>
> <mailto:d...@openvswitch.org<mailto:d...@openvswitch.org>>
> 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=KmOoW5dfxKzbFrROo_PaGE36ZvSWHlCdMXuBsimXSH7E92h_RChtC
Re: [ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the port-binding table if ovn-northd is at an older version.
On Tue, May 17, 2022 at 9:55 AM Mark Michelson 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
> > *Date: *Wednesday, May 11, 2022 at 8:10 AM
> > *To: *Mark Michelson
> > *Cc: *Mary Manohar , ovs-dev@openvswitch.org
> >
> > *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 > <mailto:mmich...@redhat.com>> 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, mary.mano...@nutanix.com
> > <mailto:mary.mano...@nutanix.com> wrote:
> > > From: Mary Manohar mary.mano...@nutanix.com>>
> > >
> > > Signed-off-by: Mary Manohar mary.mano...@nutanix.com>>
> > > ---
> > > 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
> > d...@openvswitch.org <mailto:d...@openvswitch.org>
> > 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the port-binding table if ovn-northd is at an older version.
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
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
*Date: *Wednesday, May 11, 2022 at 8:10 AM
*To: *Mark Michelson
*Cc: *Mary Manohar , ovs-dev@openvswitch.org
*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 <mailto:mmich...@redhat.com>> 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, mary.mano...@nutanix.com
<mailto:mary.mano...@nutanix.com> wrote:
> From: Mary Manohar mailto:mary.mano...@nutanix.com>>
>
> Signed-off-by: Mary Manohar mailto:mary.mano...@nutanix.com>>
> ---
> 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
d...@openvswitch.org <mailto:d...@openvswitch.org>
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the port-binding table if ovn-northd is at an older version.
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
Date: Wednesday, May 11, 2022 at 8:10 AM
To: Mark Michelson
Cc: Mary Manohar , ovs-dev@openvswitch.org
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
mailto:mmich...@redhat.com>> 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, mary.mano...@nutanix.com<mailto:mary.mano...@nutanix.com>
wrote:
> From: Mary Manohar mailto:mary.mano...@nutanix.com>>
>
> Signed-off-by: Mary Manohar
> mailto:mary.mano...@nutanix.com>>
> ---
> 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
d...@openvswitch.org<mailto:d...@openvswitch.org>
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 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, mary.mano...@nutanix.com wrote:
> > From: Mary Manohar
> >
> > Signed-off-by: Mary Manohar
> > ---
> > 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 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, mary.mano...@nutanix.com wrote:
From: Mary Manohar
Signed-off-by: Mary Manohar
---
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the port-binding table if ovn-northd is at an older version.
From: Mary Manohar
Signed-off-by: Mary Manohar
---
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;
}
--
2.9.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
