sriov: Enhance network accessibility check with static IP fallback#6853
Open
liang-cong-red-hat wants to merge 1 commit intoautotest:masterfrom
Open
sriov: Enhance network accessibility check with static IP fallback#6853liang-cong-red-hat wants to merge 1 commit intoautotest:masterfrom
liang-cong-red-hat wants to merge 1 commit intoautotest:masterfrom
Conversation
WalkthroughThe change adds optional parameters Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/sriov/sriov_base.py`:
- Line 101: The regex string in the return statement uses an invalid escape
sequence ("\\d+$"); change the pattern to a raw string literal (r"\d+$") in the
re.sub call so the backslash is preserved, e.g. update the return line that
calls re.sub(...) to use r"\d+$" instead of "\d+$".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a043da7-9f05-4868-a91b-5284092aa041
📒 Files selected for processing (3)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.pyprovider/sriov/check_points.pyprovider/sriov/sriov_base.py
liang-cong-red-hat
force-pushed
the
sriov_add_self_ping
branch
from
aa053c2 to
1d79ad1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
provider/sriov/sriov_base.py (1)
101-101:⚠️ Potential issue | 🟡 MinorUse a raw regex literal in
re.subto avoid invalid escape warnings.
'\d+$'should ber"\d+$"; this warning was already identified earlier and is still present.Proposed fix
- return re.sub('\d+$', '1', output.strip().splitlines()[-1]) + return re.sub(r'\d+$', '1', output.strip().splitlines()[-1])In Python, does using "\d" inside a normal string literal for regex trigger invalid escape warnings (e.g., W605), and is r"\d+$" the recommended form?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` at line 101, The regex string in the re.sub call is using a normal string literal ('\d+$') which can trigger invalid escape warnings; update the re.sub invocation in sriov_base.py (the line returning re.sub(...)) to use a raw string literal r"\d+$" instead, so change the pattern argument to r"\d+$" while leaving the rest of the expression (output.strip().splitlines()[-1]) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/sriov/sriov_base.py`:
- Around line 108-111: The current immediate read after set_guest_ip_addr is
race-prone and uses a brittle index check; replace the direct one-shot check
that uses utils_net.get_net_if_addrs(...).get("ipv4") and ip_list[0] == self_ip
with a short retry loop (with timeout) that re-reads the interface addresses
until the expected IP appears, and test membership (self_ip in ip_list) rather
than comparing ip_list[0]; ensure you refresh the command output used by
vm_session (so the loop invokes whatever method populates vm_session.cmd_output
or re-runs the underlying command) and fail after the timeout with a clear error
if the IP never appears.
---
Duplicate comments:
In `@provider/sriov/sriov_base.py`:
- Line 101: The regex string in the re.sub call is using a normal string literal
('\d+$') which can trigger invalid escape warnings; update the re.sub invocation
in sriov_base.py (the line returning re.sub(...)) to use a raw string literal
r"\d+$" instead, so change the pattern argument to r"\d+$" while leaving the
rest of the expression (output.strip().splitlines()[-1]) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f55b51f-b387-41cf-b6ac-841d68bc5ac3
📒 Files selected for processing (3)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.pyprovider/sriov/check_points.pyprovider/sriov/sriov_base.py
🚧 Files skipped from review as they are similar to previous changes (2)
- libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.py
- provider/sriov/check_points.py
liang-cong-red-hat
force-pushed
the
sriov_add_self_ping
branch
from
1d79ad1 to
7ba00ce
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
provider/sriov/sriov_base.py (1)
108-113:⚠️ Potential issue | 🟡 MinorHandle
Nonereturn from.get("ipv4")to avoidTypeError.If
get_net_if_addrs(...).get("ipv4")returnsNone, the expressionself_ip in NoneraisesTypeError: argument of type 'NoneType' is not iterable. Addor []fallback.Proposed fix
if utils_misc.wait_for( lambda: self_ip in (utils_net.get_net_if_addrs( iface_name, vm_session.cmd_output, "-c=never" - ).get("ipv4")), + ).get("ipv4") or []), timeout=10 ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` around lines 108 - 113, The lambda passed to utils_misc.wait_for can raise TypeError when utils_net.get_net_if_addrs(...).get("ipv4") returns None; update the lambda used in the wait_for call so the membership test uses a safe fallback (e.g. replace .get("ipv4") with .get("ipv4") or [] or otherwise coerce None to an empty iterable) so the expression "self_ip in ..." never tries to iterate over None; modify the lambda in the wait_for invocation that references self_ip, iface_name, and vm_session.cmd_output to use this fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@provider/sriov/sriov_base.py`:
- Around line 108-113: The lambda passed to utils_misc.wait_for can raise
TypeError when utils_net.get_net_if_addrs(...).get("ipv4") returns None; update
the lambda used in the wait_for call so the membership test uses a safe fallback
(e.g. replace .get("ipv4") with .get("ipv4") or [] or otherwise coerce None to
an empty iterable) so the expression "self_ip in ..." never tries to iterate
over None; modify the lambda in the wait_for invocation that references self_ip,
iface_name, and vm_session.cmd_output to use this fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10fe58ab-05e1-4819-a302-be5168e4e2b8
📒 Files selected for processing (3)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.pyprovider/sriov/check_points.pyprovider/sriov/sriov_base.py
🚧 Files skipped from review as they are similar to previous changes (2)
- libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.py
- provider/sriov/check_points.py
This commit updates the SR-IOV network verification logic to handle cases where the VM interface fails to acquire a DHCP address. If the gateway IP cannot be determined from the route table, it attempts to assign a static IP address as the ping destination. Signed-off-by: Liang Cong <lcong@redhat.com>
liang-cong-red-hat
force-pushed
the
sriov_add_self_ping
branch
from
7ba00ce to
04dfd3e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
provider/sriov/sriov_base.py (2)
104-104:⚠️ Potential issue | 🟡 MinorUse a raw regex literal at Line 104.
'\d+$'triggers an invalid escape warning; user'\d+$'to preserve the regex token.Proposed fix
- ip_addr = re.sub('\d+$', '1', output.strip().splitlines()[-1]) + ip_addr = re.sub(r'\d+$', '1', output.strip().splitlines()[-1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` at line 104, Change the regular expression literal used in the re.sub call that computes ip_addr to a raw string to avoid escape-sequence warnings: update the pattern argument in the re.sub invocation that assigns ip_addr (the line using re.sub('\d+$', '1', output.strip().splitlines()[-1])) to use r'\d+$' instead of '\d+$'.
117-120:⚠️ Potential issue | 🟠 MajorGuard line 117 against
Nonebefore membership check.
self_ip in ...get("ipv4")raisesTypeErrorif"ipv4"isNone. The code immediately above (line 113-115) checksif mac_list:after calling.get("mac")on the same function, demonstrating that.get()can return falsy values. This inconsistency in the "ipv4" check breaks the wait_for retry mechanism.Proposed fix
if utils_misc.wait_for( lambda: self_ip in (utils_net.get_net_if_addrs( iface_name, vm_session.cmd_output, "-c=never" - ).get("ipv4")), + ).get("ipv4") or []), timeout=10 ): return self_ip🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` around lines 117 - 120, The membership test in the lambda passed to wait_for uses self_ip in utils_net.get_net_if_addrs(...).get("ipv4") which can be None and will raise TypeError; update the lambda in sriov_base.py (the call that uses utils_net.get_net_if_addrs(iface_name, vm_session.cmd_output, "-c=never") and checks for "ipv4") to guard against None before testing membership (e.g., retrieve ipv4 = ...get("ipv4") and use a safe check like ipv4 and self_ip in ipv4 or default to an empty list) so it behaves consistently with the earlier mac_list check and preserves the wait_for retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@provider/sriov/sriov_base.py`:
- Line 104: Change the regular expression literal used in the re.sub call that
computes ip_addr to a raw string to avoid escape-sequence warnings: update the
pattern argument in the re.sub invocation that assigns ip_addr (the line using
re.sub('\d+$', '1', output.strip().splitlines()[-1])) to use r'\d+$' instead of
'\d+$'.
- Around line 117-120: The membership test in the lambda passed to wait_for uses
self_ip in utils_net.get_net_if_addrs(...).get("ipv4") which can be None and
will raise TypeError; update the lambda in sriov_base.py (the call that uses
utils_net.get_net_if_addrs(iface_name, vm_session.cmd_output, "-c=never") and
checks for "ipv4") to guard against None before testing membership (e.g.,
retrieve ipv4 = ...get("ipv4") and use a safe check like ipv4 and self_ip in
ipv4 or default to an empty list) so it behaves consistently with the earlier
mac_list check and preserves the wait_for retry behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d22a9fa7-fcc0-4883-b03a-539fc9b1b1ec
📒 Files selected for processing (3)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.pyprovider/sriov/check_points.pyprovider/sriov/sriov_base.py
🚧 Files skipped from review as they are similar to previous changes (2)
- libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device.py
- provider/sriov/check_points.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit updates the SR-IOV network verification logic to handle cases where the VM interface fails to acquire a DHCP address. If the gateway IP cannot be determined from the route table, it attempts to assign a static IP address as the ping destination.
Test results:
(01/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.managed_yes: STARTED
(01/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.managed_yes: PASS (104.75 s)
(02/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.managed_no: STARTED
(02/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.managed_no: PASS (107.39 s)
(03/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.without_managed: STARTED
(03/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.without_managed: PASS (106.99 s)
(04/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.vlan: STARTED
(04/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_interface.vf_address.vlan: PASS (71.58 s)
(05/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.vf_address.managed_yes: STARTED
(05/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.vf_address.managed_yes: PASS (105.00 s)
(06/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.vf_address.managed_no: STARTED
(06/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.vf_address.managed_no: PASS (165.89 s)
(07/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.pf_address.managed_yes: STARTED
(07/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.pf_address.managed_yes: PASS (104.42 s)
(08/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.pf_address.managed_no: STARTED
(08/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.hostdev_device.pf_address.managed_no: PASS (106.03 s)
(09/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.pf_name.managed_no: STARTED
(09/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.pf_name.managed_no: PASS (105.55 s)
(10/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.pf_name.managed_yes: STARTED
(10/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.pf_name.managed_yes: PASS (105.64 s)
(11/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.pf_name.without_managed: STARTED
(11/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.pf_name.without_managed: PASS (107.26 s)
(12/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.vf_address.managed_yes: STARTED
(12/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.vf_address.managed_yes: PASS (105.65 s)
(13/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.vf_address.without_managed: STARTED
(13/13) type_specific.io-github-autotest-libvirt.sriov.plug_unplug.attach_detach_device.network_interface.network.vf_address.without_managed: PASS (105.92 s)
Summary by CodeRabbit
Bug Fixes
Tests