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.
>
>>> 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
>
> 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
