| Message ID | 20241007104536.23938-1-vlodintsov@k2.cloud |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [ovs-dev,branch-23.03] ovs: Bump submodule to tip of OVS branch-3.2. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Bleep bloop. Greetings Vladislav Odintsov, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <numans@ovn.org>, Vladislav Odintsov <vlodintsov@k2.cloud> Lines checked: 31, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 10/7/24 12:45, Vladislav Odintsov wrote: > From: Dumitru Ceara <dceara@redhat.com> > > This picks up the following relevant commit: > cd8ffc956c3c ovs-atomic: Fix inclusion of Clang header by GCC 14. > > Without this builds on Fedora 40 (rawhide) are broken due to failing to > compile the submodule. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > Acked-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > (cherry picked from commit f224c6e5f69c099ddb008f99dba2e19a902a612f) > Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud> > --- > Without this patch there are errors building OVN on a modern systems. > > I kindly request for this patch to be backported down to 22.03 LTS > including already officially unsupported branches 23.03, 22.09 and 22.06, > since we internally still need to base on 22.09 branch in development. > > Thanks in advance if it is possible to make an exception and ignore > backport rules for non-LTS releases and patch them too. > --- Hi Vladislav, I see this patch was marked as "Changes Requested" in patchwork but I'm not sure who did that (either one of the other maintainers or yourself). In any case, I assume the request still applies, right? > ovs | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ovs b/ovs > index 8fd5f77cd..49e64f13b 160000 > --- a/ovs > +++ b/ovs > @@ -1 +1 @@ > -Subproject commit 8fd5f77cd84ea04667f987c7b84181604dc99f60 > +Subproject commit 49e64f13b2c965f5b53a65eeab70ac2e3f0bf69a Shouldn't we bump to 3ff343a96b4c instead? AFAICT that's the tip of branch-3.2 in OVS today. While we're at it we should probably bump branch-23.09 to the same version too. What do you think? Regards, Dumitru
Hi Dumitru, On 11.10.2024 14:00, Dumitru Ceara wrote: On 10/7/24 12:45, Vladislav Odintsov wrote: From: Dumitru Ceara <dceara@redhat.com><mailto:dceara@redhat.com> This picks up the following relevant commit: cd8ffc956c3c ovs-atomic: Fix inclusion of Clang header by GCC 14. Without this builds on Fedora 40 (rawhide) are broken due to failing to compile the submodule. Signed-off-by: Dumitru Ceara <dceara@redhat.com><mailto:dceara@redhat.com> Acked-by: Numan Siddique <numans@ovn.org><mailto:numans@ovn.org> Signed-off-by: Numan Siddique <numans@ovn.org><mailto:numans@ovn.org> (cherry picked from commit f224c6e5f69c099ddb008f99dba2e19a902a612f) Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud><mailto:vlodintsov@k2.cloud> --- Without this patch there are errors building OVN on a modern systems. I kindly request for this patch to be backported down to 22.03 LTS including already officially unsupported branches 23.03, 22.09 and 22.06, since we internally still need to base on 22.09 branch in development. Thanks in advance if it is possible to make an exception and ignore backport rules for non-LTS releases and patch them too. --- Hi Vladislav, I see this patch was marked as "Changes Requested" in patchwork but I'm not sure who did that (either one of the other maintainers or yourself). I did it by myself. The are compilation errors due to the patch, which I haven't solved yet. As soon as I solve them and test more carefully, I'll re-send v2. Sorry for the noise. In any case, I assume the request still applies, right? ovs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovs b/ovs index 8fd5f77cd..49e64f13b 160000 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 8fd5f77cd84ea04667f987c7b84181604dc99f60 +Subproject commit 49e64f13b2c965f5b53a65eeab70ac2e3f0bf69a Shouldn't we bump to 3ff343a96b4c instead? AFAICT that's the tip of branch-3.2 in OVS today. While we're at it we should probably bump branch-23.09 to the same version too. What do you think? Regards, Dumitru -- --- Regards, Vladislav Odintsov
On 11.10.2024 14:00, Dumitru Ceara wrote: On 10/7/24 12:45, Vladislav Odintsov wrote: From: Dumitru Ceara <dceara@redhat.com><mailto:dceara@redhat.com> This picks up the following relevant commit: cd8ffc956c3c ovs-atomic: Fix inclusion of Clang header by GCC 14. Without this builds on Fedora 40 (rawhide) are broken due to failing to compile the submodule. Signed-off-by: Dumitru Ceara <dceara@redhat.com><mailto:dceara@redhat.com> Acked-by: Numan Siddique <numans@ovn.org><mailto:numans@ovn.org> Signed-off-by: Numan Siddique <numans@ovn.org><mailto:numans@ovn.org> (cherry picked from commit f224c6e5f69c099ddb008f99dba2e19a902a612f) Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud><mailto:vlodintsov@k2.cloud> --- Without this patch there are errors building OVN on a modern systems. I kindly request for this patch to be backported down to 22.03 LTS including already officially unsupported branches 23.03, 22.09 and 22.06, since we internally still need to base on 22.09 branch in development. Thanks in advance if it is possible to make an exception and ignore backport rules for non-LTS releases and patch them too. --- Hi Vladislav, I see this patch was marked as "Changes Requested" in patchwork but I'm not sure who did that (either one of the other maintainers or yourself). In any case, I assume the request still applies, right? ovs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovs b/ovs index 8fd5f77cd..49e64f13b 160000 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 8fd5f77cd84ea04667f987c7b84181604dc99f60 +Subproject commit 49e64f13b2c965f5b53a65eeab70ac2e3f0bf69a Shouldn't we bump to 3ff343a96b4c instead? AFAICT that's the tip of branch-3.2 in OVS today. While we're at it we should probably bump branch-23.09 to the same version too. What do you think? Am I understanding you correctly that it's worth to bump OVN branch-23.03's OVS submodule to 120050c26 (which is the backport commit of proposed by you 3ff343a96b4c, but from branch-3.1)? I'm not sure if it is good idea to bump minor OVS version between (possible) patch/hotfix OVN versions. If yes, I've got a question: shouldn't we bump to f59f19bf6 (ovsdb-idl: Fix IDL memory leak.) instead? Regards, Dumitru -- Regards, Vladislav Odintsov
On 10/11/24 21:20, Odintsov Vladislav wrote: > On 11.10.2024 14:00, Dumitru Ceara wrote: > >> On 10/7/24 12:45, Vladislav Odintsov wrote: >>> From: Dumitru Ceara <dceara@redhat.com> >>> >>> This picks up the following relevant commit: >>> cd8ffc956c3c ovs-atomic: Fix inclusion of Clang header by GCC 14. >>> >>> Without this builds on Fedora 40 (rawhide) are broken due to failing to >>> compile the submodule. >>> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> Acked-by: Numan Siddique <numans@ovn.org> >>> Signed-off-by: Numan Siddique <numans@ovn.org> >>> (cherry picked from commit f224c6e5f69c099ddb008f99dba2e19a902a612f) >>> Signed-off-by: Vladislav Odintsov <vlodintsov@k2.cloud> >>> --- >>> Without this patch there are errors building OVN on a modern systems. >>> >>> I kindly request for this patch to be backported down to 22.03 LTS >>> including already officially unsupported branches 23.03, 22.09 and 22.06, >>> since we internally still need to base on 22.09 branch in development. >>> >>> Thanks in advance if it is possible to make an exception and ignore >>> backport rules for non-LTS releases and patch them too. >>> --- >> Hi Vladislav, >> >> I see this patch was marked as "Changes Requested" in patchwork but I'm >> not sure who did that (either one of the other maintainers or yourself). >> >> In any case, I assume the request still applies, right? >> >>> ovs | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/ovs b/ovs >>> index 8fd5f77cd..49e64f13b 160000 >>> --- a/ovs >>> +++ b/ovs >>> @@ -1 +1 @@ >>> -Subproject commit 8fd5f77cd84ea04667f987c7b84181604dc99f60 >>> +Subproject commit 49e64f13b2c965f5b53a65eeab70ac2e3f0bf69a >> Shouldn't we bump to 3ff343a96b4c instead? AFAICT that's the tip of >> branch-3.2 in OVS today. While we're at it we should probably bump >> branch-23.09 to the same version too. >> >> What do you think? > > Am I understanding you correctly that it's worth to bump OVN > branch-23.03's OVS submodule to 120050c26 (which is the backport commit > of proposed by you 3ff343a96b4c, but from branch-3.1)? I'm not sure if > it is good idea to bump minor OVS version between (possible) > patch/hotfix OVN versions. > Sorry for the confusion. I was trying to suggest to bump the submodule on all OVN stable branches to the current tip of a stable OVS release branch. That's what I think our documentation suggests: https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases Right now we have this: 24.09: c598c05c85b2 ("Set release date for 3.4.0.") 24.03: f19448b86189 ("github: Update python to 3.12.") - branch-3.3 23.09: c88a35fc29f0 ("github: Update python to 3.12.") - branch-3.2 23.03: 8fd5f77cd84e ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.1 22.12: 8fd5f77cd84e ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.1 22.09: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0 22.06: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0 22.03: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0 We could bump the submodule on all these branches to the tip of the OVS branch they're already tracking and I think we should be fine. OVS commit 335a5deac3ff ("ovs-atomic: Fix inclusion of Clang header by GCC 14.") was backported to all branches down to branch-3.0. > If yes, I've got a question: shouldn't we bump to f59f19bf6 (ovsdb-idl: > Fix IDL memory leak.) instead? > If we do what I suggest above we'll also get this fix. Our documentation also says: " For choice 1, the decision of whether to update the submodule commit to OVS branch-Z is based on several factors. Is OVN release X still being supported? Is there any known benefit to updating the submodule? E.g., are there performance improvements we could take advantage of by updating the submodule? Is there risk in updating the submodule? " While I understand it's riskier to bump to the tip of the stable OVS branch (potentially across minor OVS versions) the alternative of bumping strictly to the commit that fixes the compilation error also carries some risks: we might be missing follow up fixes that went in afterwards. However, I didn't do a thorough review yet of what other commits we'd be picking up on each branch. I'll try to do that when the patches are posted. What do you think? Thanks, Dumitru
On 11.10.2024 22:59, Dumitru Ceara wrote:
Sorry for the confusion. I was trying to suggest to bump the
submodule on all OVN stable branches to the current tip of a
stable OVS release branch. That's what I think our documentation
suggests:
https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases
Right now we have this:
24.09: c598c05c85b2 ("Set release date for 3.4.0.")
24.03: f19448b86189 ("github: Update python to 3.12.") - branch-3.3
23.09: c88a35fc29f0 ("github: Update python to 3.12.") - branch-3.2
23.03: 8fd5f77cd84e ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.1
22.12: 8fd5f77cd84e ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.1
22.09: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0
22.06: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0
22.03: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0
We could bump the submodule on all these branches to the tip of the
OVS branch they're already tracking and I think we should be fine.
OVS commit 335a5deac3ff ("ovs-atomic: Fix inclusion of Clang header by
GCC 14.") was backported to all branches down to branch-3.0.
If yes, I've got a question: shouldn't we bump to f59f19bf6 (ovsdb-idl:
Fix IDL memory leak.) instead?
If we do what I suggest above we'll also get this fix.
Our documentation also says:
"
For choice 1, the decision of whether to update the submodule commit to OVS branch-Z is based on several factors.
Is OVN release X still being supported?
Is there any known benefit to updating the submodule? E.g., are there performance improvements we could take advantage of by updating the submodule?
Is there risk in updating the submodule?
"
While I understand it's riskier to bump to the tip of the stable OVS
branch (potentially across minor OVS versions) the alternative of
bumping strictly to the commit that fixes the compilation error also
carries some risks: we might be missing follow up fixes that went in
afterwards.
However, I didn't do a thorough review yet of what other commits we'd
be picking up on each branch. I'll try to do that when the patches
are posted.
What do you think?
Thanks for the detailed description!
I've read through the ovs_submodule document and your suggestion is almost what I wanted to do. One note is that I'd propose to bump branch-3.4, branch-3.3, branch-3.2 and branch-3.1 -based OVN branches to commit "ofproto-dpif: Improve load balancing in dp_hash select groups." as it is the tip of these branches. And branch-3.0 -based OVN branches to bump to a9fb87867 "selinux: Update policy file." commit.
After all autotests pass, I can submit patches.
Do you see such approach reasonable?
--
Regards,
Vladislav Odintsov
Hi Dumitru, I went ahead and sent out a bunch of patches to bump OVS submodule to latest commit in appropriate OVS branch. I did this for all OVN branches from 24.09 down to 22.03: 24.09: OVS branch-3.4 - https://patchwork.ozlabs.org/project/ovn/patch/20241013081912.75095-1-vlodintsov@k2.cloud/ 24.03: OVS branch-3.3 - https://patchwork.ozlabs.org/project/ovn/patch/20241013082856.78348-1-vlodintsov@k2.cloud/ 23.09: OVS branch-3.2 - https://patchwork.ozlabs.org/project/ovn/patch/20241013085510.84940-1-vlodintsov@k2.cloud/ 23.06: OVS branch-3.1 - https://patchwork.ozlabs.org/project/ovn/patch/20241013090534.87896-1-vlodintsov@k2.cloud/ 23.03: OVS branch-3.1 - https://patchwork.ozlabs.org/project/ovn/patch/20241013091013.89283-1-vlodintsov@k2.cloud/ 22.12: OVS branch-3.0 - https://patchwork.ozlabs.org/project/ovn/patch/20241013092313.92774-1-vlodintsov@k2.cloud/ 22.09: OVS branch-3.0 - https://patchwork.ozlabs.org/project/ovn/patch/20241013092739.94076-1-vlodintsov@k2.cloud/ 22.06: OVS branch-3.0 - https://patchwork.ozlabs.org/project/ovn/patch/20241013093119.95171-1-vlodintsov@k2.cloud/ 22.03: OVS branch-3.0 - https://patchwork.ozlabs.org/project/ovn/patch/20241013093329.95874-1-vlodintsov@k2.cloud/ On 12.10.2024 00:57, Odintsov Vladislav wrote: > On 11.10.2024 22:59, Dumitru Ceara wrote: > > Sorry for the confusion. I was trying to suggest to bump the > submodule on all OVN stable branches to the current tip of a > stable OVS release branch. That's what I think our documentation > suggests: > > https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases > > Right now we have this: > 24.09: c598c05c85b2 ("Set release date for 3.4.0.") > 24.03: f19448b86189 ("github: Update python to 3.12.") - branch-3.3 > 23.09: c88a35fc29f0 ("github: Update python to 3.12.") - branch-3.2 > 23.03: 8fd5f77cd84e ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.1 > 22.12: 8fd5f77cd84e ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.1 > 22.09: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0 > 22.06: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0 > 22.03: 94191b7a4926 ("ovsdb-idl: Preserve change_seqno when deleting rows.") - branch-3.0 > > We could bump the submodule on all these branches to the tip of the > OVS branch they're already tracking and I think we should be fine. > > OVS commit 335a5deac3ff ("ovs-atomic: Fix inclusion of Clang header by > GCC 14.") was backported to all branches down to branch-3.0. > > > > If yes, I've got a question: shouldn't we bump to f59f19bf6 (ovsdb-idl: > Fix IDL memory leak.) instead? > > > > If we do what I suggest above we'll also get this fix. > > Our documentation also says: > " > For choice 1, the decision of whether to update the submodule commit to OVS branch-Z is based on several factors. > Is OVN release X still being supported? > Is there any known benefit to updating the submodule? E.g., are there performance improvements we could take advantage of by updating the submodule? > Is there risk in updating the submodule? > " > > While I understand it's riskier to bump to the tip of the stable OVS > branch (potentially across minor OVS versions) the alternative of > bumping strictly to the commit that fixes the compilation error also > carries some risks: we might be missing follow up fixes that went in > afterwards. > > However, I didn't do a thorough review yet of what other commits we'd > be picking up on each branch. I'll try to do that when the patches > are posted. > > What do you think? > > Thanks for the detailed description! > > I've read through the ovs_submodule document and your suggestion is almost what I wanted to do. One note is that I'd propose to bump branch-3.4, branch-3.3, branch-3.2 and branch-3.1 -based OVN branches to commit "ofproto-dpif: Improve load balancing in dp_hash select groups." as it is the tip of these branches. And branch-3.0 -based OVN branches to bump to a9fb87867 "selinux: Update policy file." commit. > > After all autotests pass, I can submit patches. > > Do you see such approach reasonable? > > -- > Regards, > Vladislav Odintsov > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ovs b/ovs index 8fd5f77cd..49e64f13b 160000 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 8fd5f77cd84ea04667f987c7b84181604dc99f60 +Subproject commit 49e64f13b2c965f5b53a65eeab70ac2e3f0bf69a