On 12/15/25 2:48 PM, Dumitru Ceara via dev wrote:
> Hi Frode,
>
> On 12/15/25 2:45 PM, Frode Nordahl wrote:
>> On 12/15/25 11:44, Dumitru Ceara via dev wrote:
>>> On 12/14/25 12:32 AM, Frode Nordahl wrote:
>>>> The tc command from iproute2 changed its rounding behavior in commit
>>>> d947f365602b ("tc: Fix rounding in tc_calc_xmittime and
>>>> tc_calc_xmitsize.").
>>>> This caused the ovn-egress-qos tests to fail because they were matching
>>>> exact burst and cburst values in tc output.
>>>>
>>>> The rounding fix means that burst and cburst values may differ slightly
>>>> from previous versions. For example, values that were previously 750000
>>>> might now be 749999 or similar variations.
>>>>
>>>> To maintain compatibility with both old and new versions of tc, the test
>>>> assertions now use pattern matching that:
>>>> - Matches the most significant digit(s) of the value
>>>> - Ensures all characters are numeric
>>>> - Maintains the correct total number of digits
>>>> - Preserves the unit suffix (e.g., 'b' for bytes)
>>>>
>>>> For example, '375000b' now matches '3[0-9][0-9][0-9][0-9][0-9]b' which
>>>> accepts any 6-digit value starting with 3, allowing for rounding
>>>> differences while still validating the general magnitude is correct.
>>>
>>> Hi Frode,
>>>
>>> Thanks for v2!
>>
>> Thank you for taking the time to review!
>>
>
> No problem!
>
>>>>
>>>> Reported-at: https://launchpad.net/bugs/2129005
>>>> Signed-off-by: GitHub Copilot <[email protected]>
>>>
>>> Actually, according to our guidelines [0] this should probably be
>>> Assisted-by: ... . I don't think it's correct to include a sign off
>>> from an AI assistant. That's because the sign off should certify
>>> that the author of the specific code agrees with the DCO. I'm not
>>> sure an AI assistant can reliably sign off.
>>>
>>> So maybe something like you did in the OVS counterpart patch [1]?
>>>
>>> Assisted-by: GitHub Copilot <[email protected]>
>>>
>>> [0] https://github.com/ovn-org/ovn/
>>> blob/5220faeb7bf7aa0c9ee4cb9ad91a7010b12284b2/Documentation/internals/
>>> contributing/submitting-patches.rst#ai-assisted-contributions
>>> [1] https://patchwork.ozlabs.org/project/openvswitch/
>>> patch/[email protected]/
>>
>> Indeed, the OVS robot feedback on the signed-off-by line spurred me to
>> read that piece of documentation, and as you noticed I did it the right
>> way for the next patch. Apologies for the oversight, as the responsible
>> human I should have read that part before submitting the first patch.
I would suggest to also remove the email from the tag. The purpose of
the email is to be able to contact the person mentioned in the commit.
And, first of all, noone will contact copilot, and secondly this is
a "noreply" email, so doesn't make sense.
>>
>>>> Signed-off-by: Frode Nordahl <[email protected]>
>>>> ---
>>>> v2:
>>>> - use regular expression to match on digits instead of any character.
>>>> - replace all exact matches, not just the ones currently failing.
>>>> - use github copilot cli to make the changes.
>>>> v1: https://mail.openvswitch.org/pipermail/ovs-dev/2025-
>>>> October/427356.html
>>>>
>>>> tests/system-ovn.at | 38 +++++++++++++++++++-------------------
>>>> 1 file changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> github copilot cli prompt BEGIN:
>>>> Context
>>>> =======
>>>>
>>>> The current working directory contains a bootstrapped and built copy
>>>> of the
>>>> Open Virtual Network (OVN) project, which is also publicly available
>>>> from
>>>> https://github.com/ovn-org/ovn.git.
>>>>
>>>> There is a test that is currently failing due to a change in behavior
>>>> of a
>>>> system tool named ``tc``.
>>>>
>>>> The ``tc`` command is part of the iproute2 project
>>>> which is publicly available from https://github.com/iproute2/
>>>> iproute2.git.
>>>> For further information and reference on the change of behavior, you
>>>> can look
>>>> at the following commit:
>>>> d947f365602b ("tc: Fix rounding in tc_calc_xmittime and
>>>> tc_calc_xmitsize.")
>>>>
>>>> The tests assertions for the failing tests can be found in the file
>>>> named
>>>> tests/system-ovn.at. The reason for the failure is likely that the test
>>>> assertions match on exact values labeled ``burst`` and ``cburst`` in the
>>>> output from the ``tc`` command.
>>>>
>>>> Note that the tests are expected to pass on systems with a version of
>>>> ``tc``
>>>> from both before and after the change of behavior, so the assertions
>>>> must
>>>> be written in such a way so that they are compatible with both.
>>>>
>>>> The copy of OVN in the current working directory can be built using
>>>> the ``make``
>>>> command.
>>>>
>>>> The command to run the test:
>>>>
>>>> sudo make check-kernel TESTSUITEFLAGS="-j1 -k ovn-egress-qos
>>>>
>>>> Assignment
>>>> ==========
>>>>
>>>> 1. Locate tests with the ``ovn-egress-qos`` keyword.
>>>> 2. Change the ``grep`` command used to match on output from the
>>>> ``tc`` command
>>>> containing ``burst`` or ``cburst``, so that it instead of exact
>>>> match uses a
>>>> partial match that includes:
>>>> - most significant part of number.
>>>> - characters must be numbers.
>>>> - the number of characters must match.
>>>> - the last alphabetical character(s), denoting the unit, must match.
>>>> 3. Do one change per iteration and ensure tests pass for each iteration.
>>>> 4. Commit your proposal to the repository and write a commit message
>>>> that
>>>> explains why the change is necessary, with reference to the
>>>> change of
>>>> behavior in ``tc``.
>>>> END
>>>> github copilot cli transcript: https://gist.github.com/fnordahl/
>>>> ec57e9d6444e39ece45f619740724261
>>>>
>>>
>>> This was an interesting read. :)
>>
>> I found it to be a fun experiment, and why not share how it came to be,
>> delighted that you found joy in reading it too.
>>
>>>>
>>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>>>> index 303b10894..46bf1bc61 100644
>>>> --- a/tests/system-ovn.at
>>>> +++ b/tests/system-ovn.at
>>>> @@ -6718,11 +6718,11 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port
>>>> ext options:qos_burst=6000000])
>>>>
>>>> OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
>>>> OVS_WAIT_UNTIL([tc class show dev ovs-public | \
>>>> - grep -q 'class htb .* rate 200Kbit ceil 300Kbit
>>>> burst 375000b cburst 375000b'])
>>>> + grep -q 'class htb .* rate 200Kbit ceil 300Kbit
>>>> burst 3[[0-9]][[0-9]][[0-9]][[0-9]][[0-9]]b cburst 3[[0-9]][[0-9]]
>>>> [[0-9]][[0-9]][[0-9]]b'])
>>>
>>> I know Ales suggested matching on numbers here:
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2025-October/427357.html
>>>
>>> For fun I asked another code assistant (gemini-cli with
>>> gemini-3-pro-preview) to review your v2 and take into account
>>> the suggestion Ales had, of explicitly matching on "[0-9]{4}" with
>>> an extended regex. It concluded it's not nice :)
>>>
>>> "
>>> > OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
>>> > OVS_WAIT_UNTIL([tc class show dev ovs-public | \
>>> > - grep -q 'class htb .* rate 200Kbit ceil 300Kbit
>>> burst 375000b cburst 375000b'])
>>> > + grep -q 'class htb .* rate 200Kbit ceil 300Kbit
>>> burst 3[0-9][0-9][0-9][0-9][0-9]b cburst 3[0-9][0-9][0-9][0-9][0-9]b'])
>>>
>>> This regex is definitely safe and portable.
>>>
>>> I know there was a suggestion to use quantifiers (like `[0-9]{4}`
>>> or `[0-9]{5}`). However, `grep` in `tests/system-ovn.at` is currently
>>> used without `-E` (Extended Regex), so to use curly braces we'd either
>>> need to switch to `grep -E` or use the escaped BRE syntax `[0-9]\
>>> \{5\\}`.
>>> Given that this file avoids `grep -E` so far, keeping this verbose
>>> version seems consistent and fine to me.
>>> "
>>>
>>> But as a human, I kind of disagree, to me "grep -q 'class htb .* rate
>>> 12Kbit ceil 34359Mbit burst 375000b cburst 37....b'" seemed way easier
>>> to read and and probably good enough for our tests.
>>>
>>> I won't ask for a v3, I can also adjust the tests myself if you agree
>>> with me doing that, to avoid another back-and-forth on this patch.
>>> The changes I'm thinking of are:
>>>
>>> https://github.com/dceara/ovn/commit/55fb6a9
FWIW, we have a few 'grep -E' calls with the length quantifiers in OVS
testsuite, so these should be perfectly fine to use as well. At least
in OVS version of the patch.
Best regards, Ilya Maximets.
>>
>> I definitively agree that it looks nicer, so we're basically back to v1
>> approach, but with targeting of all burst / cburst assertions.
>>
>> Taking into consideration that the commit message contains details of
>> the implementation, I would not want to burden you with that.
>>
>> Happy to take on addressing the review feedback and submit a v3, (either
>> myself or as another experiment ot see what copilot would do with your
>> feedback).
>>
>
> Ack, thank you! I'll mark v2 as "Changes Requested" in patchwork and
> will be looking forward to the v3.
>
> Regards,
> Dumitru
>
>
> _______________________________________________
> 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
