On 4/4/24 19:17, Ihar Hrachyshka wrote: > I tried to revert the util change and the test case passed just fine. >
I had done that before pushing the patch but.. I got tricked by the fact that northd was spinning and using cpu 100% while the switches were added. My bad. > I think the scenario that may get the hint out of bounds is 1) start with > no vxlan chassis; 2) create 4097 DPs; 3) add a vxlan chassis - this makes > northd downgrade its max key to 4096. Now when we create a DP, it will spin > in circles. Posted this here: > https://patchwork.ozlabs.org/project/ovn/patch/20240404171358.54678-1-ihrac...@redhat.com/ > > (We can probably discuss in this context whether it's a good idea for a > cluster to change the max tun id value as chassis come and go. Perhaps it > should be initialized once and for all?) > > What I also notice is that with the new patch, *hint is always overridden > at the start of the function, so it's always bumped by 1. I am not sure it > was intended. Comments? > But the actual change in behavior for '*hint' is only for the case in which we run out of IDs, or am I missing something? It didn't seem that big of a deal to me. > This is all probably relevant to the question of whether any backports > should happen for this patch. > > Ihar > Regards, Dumitru > > On Thu, Apr 4, 2024 at 12:46 PM Dumitru Ceara <dce...@redhat.com> wrote: > >> On 4/4/24 17:52, Vladislav Odintsov wrote: >>> Thanks Dumitru! >>> I’m totally fine with your change. >>> Should I send backport patches with resolved conflicts for remaining >> branches at least till 22.03, which is an LTS? >>> >> >> Well, 24.03 is the most recent LTS. We don't really backport patches to >> 22.03 unless they fix critical issues. I'm not completely convinced >> that this is such a critical issue though. You need 4K logical >> datapaths with vxlan enabled before this gets hit. In any case, Mark, >> what do you think? >> >> Regards, >> Dumitru >> >>>> On 4 Apr 2024, at 18:26, Dumitru Ceara <dce...@redhat.com> wrote: >>>> >>>> On 4/1/24 16:27, Mark Michelson wrote: >>>>> Thanks Vladislav, >>>>> >>>>> Acked-by: Mark Michelson <mmich...@redhat.com <mailto: >> mmich...@redhat.com>> >>>>> >>>> >>>> Thanks, Vladislav and Mark! Applied to main and backported down to >>>> 23.06 with a minor test change, please see below. >>>> >>>>> On 4/1/24 08:15, Vladislav Odintsov wrote: >>>>>> In case if all tunnel ids are exhausted, ovn_allocate_tnlid() function >>>>>> iterates over tnlids indefinitely when *hint is outside of [min, max]. >>>>>> This is because when tnlid reaches max, next tnlid is min and for-loop >>>>>> never reaches exit condition for tnlid != *hint. >>>>>> >>>>>> This patch fixes mentioned issue and adds a testcase. >>>>>> >>>>>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >>>>>> --- >>>>>> lib/ovn-util.c | 10 +++++++--- >>>>>> tests/ovn-northd.at | 26 ++++++++++++++++++++++++++ >>>>>> 2 files changed, 33 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >>>>>> index ee5cbcdc3..9f97ae2ca 100644 >>>>>> --- a/lib/ovn-util.c >>>>>> +++ b/lib/ovn-util.c >>>>>> @@ -693,13 +693,17 @@ uint32_t >>>>>> ovn_allocate_tnlid(struct hmap *set, const char *name, uint32_t min, >>>>>> uint32_t max, uint32_t *hint) >>>>>> { >>>>>> - for (uint32_t tnlid = next_tnlid(*hint, min, max); tnlid != >> *hint; >>>>>> - tnlid = next_tnlid(tnlid, min, max)) { >>>>>> + /* Normalize hint, because it can be outside of [min, max]. */ >>>>>> + *hint = next_tnlid(*hint, min, max); >>>>>> + >>>>>> + uint32_t tnlid = *hint; >>>>>> + do { >>>>>> if (ovn_add_tnlid(set, tnlid)) { >>>>>> *hint = tnlid; >>>>>> return tnlid; >>>>>> } >>>>>> - } >>>>>> + tnlid = next_tnlid(tnlid, min, max); >>>>>> + } while (tnlid != *hint); >>>>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); >>>>>> VLOG_WARN_RL(&rl, "all %s tunnel ids exhausted", name); >>>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>>>>> index cd53755b2..174dbacda 100644 >>>>>> --- a/tests/ovn-northd.at >>>>>> +++ b/tests/ovn-northd.at >>>>>> @@ -2822,6 +2822,32 @@ AT_CHECK([test $lsp02 = 3 && test $ls1 = 123]) >>>>>> AT_CLEANUP >>>>>> ]) >>>>>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>>>>> +AT_SETUP([check tunnel ids exhaustion]) >>>>>> +ovn_start >>>>>> + >>>>>> +# Create a fake chassis with vxlan encap to lower MAX DP tunnel key >>>>>> to 2^12 >>>>>> +ovn-sbctl \ >>>>>> + --id=@e create encap chassis_name=hv1 ip="192.168.0.1" >>>>>> type="vxlan" \ >>>>>> + -- --id=@c create chassis name=hv1 encaps=@e >>>>>> + >>>>>> +cmd="ovn-nbctl --wait=sb" >>>>>> + >>>>>> +for i in {1..4097..1}; do >>>> >>>> This can be changed to: >>>> >>>> for i in {1..4097}; do >>>> >>>>>> + cmd="${cmd} -- ls-add lsw-${i}" >>>>>> +done >>>>>> + >>>>>> +eval $cmd >>>>>> + >>>>>> +check_row_count nb:Logical_Switch 4097 >>>>>> +wait_row_count sb:Datapath_Binding 4095 >>>>>> + >>>>>> +OVS_WAIT_UNTIL([grep "all datapath tunnel ids exhausted" >>>>>> northd/ovn-northd.log]) >>>>>> + >>>>>> +AT_CLEANUP >>>>>> +]) >>>>>> + >>>>>> + >>>>>> OVN_FOR_EACH_NORTHD_NO_HV([ >>>>>> AT_SETUP([Logical Flow Datapath Groups]) >>>>>> ovn_start >>>> >>>> Regards, >>>> Dumitru >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> d...@openvswitch.org <mailto:d...@openvswitch.org> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >>> Regards, >>> Vladislav Odintsov >>> >>> >> >> _______________________________________________ >> 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
