| Message ID | 20240122141443.2074919-1-mheib@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,1/2] ovs: Bump submodule to include igmp protocol version. | expand |
On 1/22/24 15:14, Mohammad Heib wrote: > Specifically the following commit: > 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.") > > Also fix a small compilation error due to prototype change. > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- Hi Mohammad, Thanks for the patch! > controller/pinctrl.c | 6 +++++- > ovs | 2 +- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 4992eab08..77bf67e58 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn, > switch (ntohs(ip_flow->tp_src)) { > case IGMP_HOST_MEMBERSHIP_REPORT: > case IGMPV2_HOST_MEMBERSHIP_REPORT: > + mcast_group_proto grp_proto = > + (ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT) > + ? MCAST_GROUP_IGMPV1 > + : MCAST_GROUP_IGMPV2; > group_change = > mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN, > - port_key_data); > + port_key_data, grp_proto); > break; > case IGMP_HOST_LEAVE_MESSAGE: > group_change = > diff --git a/ovs b/ovs > index 4102674b3..b222593bc 160000 > --- a/ovs > +++ b/ovs > @@ -1 +1 @@ > -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6 > +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3 However, it's probably desirable to bump the submodule to the tip of the latest stable branch, i.e. branch-3.3: https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases That would also fix our scheduled CI runs: https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531 However, there's a crash in OVS on branch-3.3 that needs to be fixed first: https://issues.redhat.com/browse/FDP-300 I'd wait with bumping the submodule until then. Regards, Dumitru
Hi Dumitru, Thank you for the review, yes sure will bump the submodule to the last stable once they fix the issue. Thanks On Mon, Jan 22, 2024 at 5:01 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 1/22/24 15:14, Mohammad Heib wrote: > > Specifically the following commit: > > 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.") > > > > Also fix a small compilation error due to prototype change. > > > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > > --- > > Hi Mohammad, > > Thanks for the patch! > > > controller/pinctrl.c | 6 +++++- > > ovs | 2 +- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 4992eab08..77bf67e58 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn, > > switch (ntohs(ip_flow->tp_src)) { > > case IGMP_HOST_MEMBERSHIP_REPORT: > > case IGMPV2_HOST_MEMBERSHIP_REPORT: > > + mcast_group_proto grp_proto = > > + (ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT) > > + ? MCAST_GROUP_IGMPV1 > > + : MCAST_GROUP_IGMPV2; > > group_change = > > mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN, > > - port_key_data); > > + port_key_data, grp_proto); > > break; > > case IGMP_HOST_LEAVE_MESSAGE: > > group_change = > > diff --git a/ovs b/ovs > > index 4102674b3..b222593bc 160000 > > --- a/ovs > > +++ b/ovs > > @@ -1 +1 @@ > > -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6 > > +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3 > > However, it's probably desirable to bump the submodule to the tip of the > latest stable branch, i.e. branch-3.3: > > > https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases > > That would also fix our scheduled CI runs: > > https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531 > > However, there's a crash in OVS on branch-3.3 that needs to be fixed first: > https://issues.redhat.com/browse/FDP-300 > > I'd wait with bumping the submodule until then. > > Regards, > Dumitru > >
On 1/23/24 14:53, Mohammad Heib wrote: > Hi Dumitru, > > Thank you for the review, > yes sure will bump the submodule to the last stable once they fix the issue. I applied the BFD fix and also applied the typedef fix for the mcast_group_proto enum. So, make sure to include both for the update. There are a few other things though. This patch needs to upgrae the DPDk version, since OVS 3.3 will be using DPDK 23.11 and branch-3.3 is supposed to be used with this version. 22.11 is not supported on that branch. So, you need to port CI changes from: https://github.com/openvswitch/ovs/commit/8893e24d9d0921aaf934f263a06ba223ef0db369 See one more comment inline. > Thanks > > On Mon, Jan 22, 2024 at 5:01 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> On 1/22/24 15:14, Mohammad Heib wrote: >>> Specifically the following commit: >>> 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.") >>> >>> Also fix a small compilation error due to prototype change. >>> >>> Signed-off-by: Mohammad Heib <mheib@redhat.com> >>> --- >> >> Hi Mohammad, >> >> Thanks for the patch! >> >>> controller/pinctrl.c | 6 +++++- >>> ovs | 2 +- >>> 2 files changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>> index 4992eab08..77bf67e58 100644 >>> --- a/controller/pinctrl.c >>> +++ b/controller/pinctrl.c >>> @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn, >>> switch (ntohs(ip_flow->tp_src)) { >>> case IGMP_HOST_MEMBERSHIP_REPORT: >>> case IGMPV2_HOST_MEMBERSHIP_REPORT: >>> + mcast_group_proto grp_proto = You're defining a variable inside the case, the whole case should be braced for this to work. It may work with some compilers, but it is generally incorrect and CI fails to build because of this. Alternatively, move the declaration outside of the switch statement. >>> + (ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT) >>> + ? MCAST_GROUP_IGMPV1 >>> + : MCAST_GROUP_IGMPV2; >>> group_change = >>> mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN, >>> - port_key_data); >>> + port_key_data, grp_proto); >>> break; >>> case IGMP_HOST_LEAVE_MESSAGE: >>> group_change = >>> diff --git a/ovs b/ovs >>> index 4102674b3..b222593bc 160000 >>> --- a/ovs >>> +++ b/ovs >>> @@ -1 +1 @@ >>> -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6 >>> +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3 >> >> However, it's probably desirable to bump the submodule to the tip of the >> latest stable branch, i.e. branch-3.3: >> >> >> https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases >> >> That would also fix our scheduled CI runs: >> >> https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531 >> >> However, there's a crash in OVS on branch-3.3 that needs to be fixed first: >> https://issues.redhat.com/browse/FDP-300 >> >> I'd wait with bumping the submodule until then. >> >> Regards, >> Dumitru
On 1/29/24 16:30, Ilya Maximets wrote: > On 1/23/24 14:53, Mohammad Heib wrote: >> Hi Dumitru, >> >> Thank you for the review, >> yes sure will bump the submodule to the last stable once they fix the issue. > > I applied the BFD fix and also applied the typedef fix for the mcast_group_proto > enum. So, make sure to include both for the update. > > There are a few other things though. > > This patch needs to upgrae the DPDk version, since OVS 3.3 will be using DPDK 23.11 > and branch-3.3 is supposed to be used with this version. 22.11 is not supported on > that branch. So, you need to port CI changes from: > https://github.com/openvswitch/ovs/commit/8893e24d9d0921aaf934f263a06ba223ef0db369 > It turns out there's another issue at hand, this time on Fedora 40 (rawhide) where OVN builds fail completely without picking up latest branch-3.3. That triggered me to go ahead and post a patch to bump OVN submodule, take care of the DPDK update and pick up all other fixes: https://patchwork.ozlabs.org/project/ovn/patch/20240129233652.123111-1-dceara@redhat.com/ I hope that's fine. Regards, Dumitru > See one more comment inline. > >> Thanks >> >> On Mon, Jan 22, 2024 at 5:01 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >>> On 1/22/24 15:14, Mohammad Heib wrote: >>>> Specifically the following commit: >>>> 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.") >>>> >>>> Also fix a small compilation error due to prototype change. >>>> >>>> Signed-off-by: Mohammad Heib <mheib@redhat.com> >>>> --- >>> >>> Hi Mohammad, >>> >>> Thanks for the patch! >>> >>>> controller/pinctrl.c | 6 +++++- >>>> ovs | 2 +- >>>> 2 files changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>>> index 4992eab08..77bf67e58 100644 >>>> --- a/controller/pinctrl.c >>>> +++ b/controller/pinctrl.c >>>> @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn, >>>> switch (ntohs(ip_flow->tp_src)) { >>>> case IGMP_HOST_MEMBERSHIP_REPORT: >>>> case IGMPV2_HOST_MEMBERSHIP_REPORT: >>>> + mcast_group_proto grp_proto = > > You're defining a variable inside the case, the whole case should be > braced for this to work. It may work with some compilers, but it is > generally incorrect and CI fails to build because of this. > Alternatively, move the declaration outside of the switch statement. > >>>> + (ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT) >>>> + ? MCAST_GROUP_IGMPV1 >>>> + : MCAST_GROUP_IGMPV2; >>>> group_change = >>>> mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN, >>>> - port_key_data); >>>> + port_key_data, grp_proto); >>>> break; >>>> case IGMP_HOST_LEAVE_MESSAGE: >>>> group_change = >>>> diff --git a/ovs b/ovs >>>> index 4102674b3..b222593bc 160000 >>>> --- a/ovs >>>> +++ b/ovs >>>> @@ -1 +1 @@ >>>> -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6 >>>> +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3 >>> >>> However, it's probably desirable to bump the submodule to the tip of the >>> latest stable branch, i.e. branch-3.3: >>> >>> >>> https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases >>> >>> That would also fix our scheduled CI runs: >>> >>> https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531 >>> >>> However, there's a crash in OVS on branch-3.3 that needs to be fixed first: >>> https://issues.redhat.com/browse/FDP-300 >>> >>> I'd wait with bumping the submodule until then. >>> >>> Regards, >>> Dumitru >
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 4992eab08..77bf67e58 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn, switch (ntohs(ip_flow->tp_src)) { case IGMP_HOST_MEMBERSHIP_REPORT: case IGMPV2_HOST_MEMBERSHIP_REPORT: + mcast_group_proto grp_proto = + (ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT) + ? MCAST_GROUP_IGMPV1 + : MCAST_GROUP_IGMPV2; group_change = mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN, - port_key_data); + port_key_data, grp_proto); break; case IGMP_HOST_LEAVE_MESSAGE: group_change = diff --git a/ovs b/ovs index 4102674b3..b222593bc 160000 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6 +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3
Specifically the following commit: 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.") Also fix a small compilation error due to prototype change. Signed-off-by: Mohammad Heib <mheib@redhat.com> --- controller/pinctrl.c | 6 +++++- ovs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-)