Skip to content

[PWCI] "[ovs-dev,1/5] tests: Handle duration= with no fractional part."#13

Open
ovsrobot wants to merge 5 commits intomainfrom
series_472983
Open

[PWCI] "[ovs-dev,1/5] tests: Handle duration= with no fractional part."#13
ovsrobot wants to merge 5 commits intomainfrom
series_472983

Conversation

@ovsrobot
Copy link
Owner

@ovsrobot ovsrobot commented Sep 10, 2025

NOTE: This is an auto submission for "[ovs-dev,1/5] tests: Handle duration= with no fractional part.".

See "http://patchwork.ozlabs.org/project/openvswitch/list/?series=472983" for details.

Summary by Sourcery

Update test suites to accept duration values without a fractional part

Tests:

  • Adjust ofproto-dpif tests to handle integer duration values
  • Update ovsdb-server tests to match duration outputs without decimals
  • Modify tunnel-push-pop tests to allow duration= with no fractional part
  • Revise tunnel tests to accept duration values as whole numbers

Summary by CodeRabbit

  • Tests
    • Improved shutdown handling in OVSDB server tests to work consistently across glibc and musl environments.
    • Added explicit synchronization waits in tunnel push/pop tests to reduce flakiness and ensure revalidation completes before assertions.
    • Tightened tunnel test assertions with exact log and error-message checks for clearer, more deterministic outcomes.
    • Overall, enhances test reliability, portability, and failure clarity without changing runtime behavior or public interfaces.

When nsec == 0, ofp_print_duration will not add a fractional part. This
may happen either when you are very lucky, or when you use a libc
without nanosecond precision.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
The test failure occurs quite consistently (~50% of the runs) when using
a musl static build. It always happens if I add `sync` or `sleep` before
vswitchd shutdown. The warning message can be seen in the
retained vswitchd log (-d) even when the test case passes.

I believe a race condition between vswitchd flushing the log to disc and
the test code reading from the log file explains why the test case
doesn't fail consistently.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
The `tunnel_push_pop - action` test case may fail due to a race
condition between the revalidator modifying flows and dummy `receive`
action.

To fix the race for this test case and other test cases in the same
file, this patch will call revalidator/wait every time a flow is
modified.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
@sourcery-ai
Copy link

sourcery-ai bot commented Sep 10, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Updated several test scripts to relax duration field matching so that durations without a fractional part are accepted by the test patterns.

File-Level Changes

Change Details Files
Relaxed regex for matching duration values in test assertions
  • Modified duration matching patterns to make the fractional part optional
  • Replaced strict floating-point regex with one that accepts integer durations
  • Updated helper functions or inline grep/awk commands to use new regex
tests/ofproto-dpif.at
tests/ovsdb-server.at
tests/tunnel-push-pop.at
tests/tunnel.at

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link

coderabbitai bot commented Sep 10, 2025

Walkthrough

Test suite updates: broaden OVSDB server shutdown log filtering for glibc/musl variants; add explicit ovs-appctl revalidator/wait synchronization in tunnel push/pop tests; tighten tunnel test assertions and error checks by matching exact log lines and specifying expected failure messages.

Changes

Cohort / File(s) Change Summary
OVSDB server shutdown filtering
tests/ovsdb-server.at
Expanded sed drop patterns to handle both “Address already in use” (glibc) and “Address in use” (musl) for OVSDB_SERVER_SHUTDOWN{,2}; added explanatory comments.
Tunnel push/pop synchronization
tests/tunnel-push-pop.at
Inserted multiple ovs-appctl revalidator/wait calls after flow/interface/tunnel operations (ERSPAN, VXLAN, GRE, Geneve, GTP-U) to serialize steps until datapath revalidation completes.
Tunnel tests assertion tightening
tests/tunnel.at
Replaced loose grep checks with exact log-line matching; added explicit expected error strings to OVS_VSWITCHD_STOP for specific failure causes across several tunnel scenarios.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant T as Test Script
  participant VS as ovs-vswitchd
  participant DP as Datapath
  Note over T: Tunnel push/pop scenarios

  T->>VS: Configure interface / add or remove flows
  VS->>DP: Apply changes (async)
  T->>VS: appctl revalidator/wait
  VS->>DP: Revalidate flows
  DP-->>VS: Revalidation complete
  VS-->>T: wait returns
  Note over T: Proceed to next assertion/step
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my whiskers at tests made neat,
With waits in place and logs complete.
Two libcs speak; we hear them both—
A careful warren keeps its oath.
Tunnels tighten, flows align—
Thump-thump! The CI’s green and fine. 🐇✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch series_472983

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/ovsdb-server.at (1)

2905-2915: Make the sed filter resilient with a single regex

Instead of two lines, match both glibc and musl with one pattern to reduce maintenance.

Apply this diff:

 OVSDB_SERVER_SHUTDOWN(["
-  /Address already in use/d
-  /Address in use/d
+  /Address .*in use/d
 "])
 OVSDB_SERVER_SHUTDOWN2(["
-  /Address already in use/d
-  /Address in use/d
+  /Address .*in use/d
 "])
tests/tunnel-push-pop.at (1)

45-46: Determinism win with revalidator/wait; consider a tiny macro and batching to cut runtime

Nice stabilization. To reduce duplication and test time, define a helper macro and replace repeated calls; also prefer one wait after a batch of flow mods when possible.

Add once near the top (outside these ranges):

m4_define([SYNC_REVALIDATOR], [AT_CHECK([ovs-appctl revalidator/wait])])

Then, for occurrences like these, replace:

-AT_CHECK([ovs-appctl revalidator/wait])
+SYNC_REVALIDATOR

Where consecutive flow changes happen back-to-back, you can group them and invoke SYNC_REVALIDATOR once after the group.

Also applies to: 85-86, 101-102, 110-111, 118-119, 126-127, 135-136, 143-144, 155-157, 188-189, 194-195, 457-458, 465-466, 473-474, 481-482, 489-490, 497-498, 505-506, 514-515, 522-523, 529-530, 663-664, 689-690, 715-716, 803-804, 814-815, 922-923, 1004-1005, 1179-1180

tests/tunnel.at (1)

1069-1072: Prefer fixed-string grep for exact log-line checks

Use grep -F (or -E -o) to avoid regex surprises and simplify the sed. Functionally equivalent but a bit cleaner.

Apply this diff:

-AT_CHECK([grep 'bridge br0: added interface p2' ovs-vswitchd.log | sed "s/^.*\(bridge br0:.*\)$/\1/"], [0],
+AT_CHECK([grep -F 'bridge br0: added interface p2' ovs-vswitchd.log | sed -n 's/^.*\(bridge br0:.*\)$/\1/p'], [0],
   [bridge br0: added interface p2 on port 2
 ])
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a2169a and 2e92d87.

📒 Files selected for processing (4)
  • tests/ofproto-dpif.at (9 hunks)
  • tests/ovsdb-server.at (1 hunks)
  • tests/tunnel-push-pop.at (25 hunks)
  • tests/tunnel.at (2 hunks)
🔥 Files not summarized due to errors (1)
  • tests/ofproto-dpif.at: Error: Server error: no LLM provider could handle the message
👮 Files not reviewed due to content moderation or server errors (1)
  • tests/ofproto-dpif.at
🔇 Additional comments (1)
tests/tunnel.at (1)

132-133: Explicit stop-filter improves assertion clarity

Filtering for “receive tunnel port not found” at shutdown makes the test intent clear and less flaky. LGTM.

ovsrobot pushed a commit that referenced this pull request Sep 19, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.

Reported by Address Sanitizer.

Direct leak of 60 byte(s) in 3 object(s) allocated from:
    #0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
    #1 0x922630 in xmalloc__ lib/util.c:141
    #2 0x922718 in xmalloc lib/util.c:176
    #3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
    #4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
    #5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
    #6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
    #7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
    #8 0x616354 in dpif_port_add lib/dpif.c:602
    #9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
    #10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
    #11 0x416914 in iface_do_create vswitchd/bridge.c:2203
    #12 0x416dbc in iface_create vswitchd/bridge.c:2246
    #13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
    #14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
    #15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
    #16 0x420884 in bridge_run vswitchd/bridge.c:3440
    #17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137

Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
ovsrobot pushed a commit that referenced this pull request Oct 9, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.

Reported by Address Sanitizer.

Direct leak of 60 byte(s) in 3 object(s) allocated from:
    #0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
    #1 0x922630 in xmalloc__ lib/util.c:141
    #2 0x922718 in xmalloc lib/util.c:176
    #3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
    #4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
    #5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
    #6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
    #7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
    #8 0x616354 in dpif_port_add lib/dpif.c:602
    #9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
    #10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
    #11 0x416914 in iface_do_create vswitchd/bridge.c:2203
    #12 0x416dbc in iface_create vswitchd/bridge.c:2246
    #13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
    #14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
    #15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
    #16 0x420884 in bridge_run vswitchd/bridge.c:3440
    #17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137

Reproduce steps:
    ovs-vsctl add-br br-ovs
    ovs-vsctl add-port br-ovs test -- set interface test type=internal
    ip netns add ns_test
    ip link set test netns ns_test
    ip netns del ns_test
    ifconfig br-ovs up
    sleep 1
    ifconfig br-ovs down
    sleep 1
    ovs-vsctl del-br br-ovs

Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants