USHIFT-6870: Add e2e test for router service IP connectivity#6515
USHIFT-6870: Add e2e test for router service IP connectivity#6515pacevedom wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-important
|
@pacevedom: This pull request references Jira Issue OCPBUGS-45192, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdded a new test case "Router Service IP Connectivity" to verify dual stack IPv4 route connectivity through router LoadBalancer IPs, along with two supporting keywords for retrieving router service IPs and validating SSH+cURL access from the host. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![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
🧹 Nitpick comments (1)
test/suites/standard1/networking-smoke.robot (1)
79-90: Prefer reusing existing shared router-IP keyword to avoid drift.This duplicates logic already provided in
test/resources/microshift-network.resource(Get All IPs From Router Default LB, snippet lines 1-100). Reusing the shared keyword keeps LB IP parsing behavior consistent across suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/networking-smoke.robot` around lines 79 - 90, The keyword Get Router Service IPs duplicates parsing logic already provided by the shared keyword Get All IPs From Router Default LB; replace the body of Get Router Service IPs with a single call to Get All IPs From Router Default LB and return its result (e.g., @{ips}= Get All IPs From Router Default LB RETURN @{ips}), ensuring you keep the same return variable name (@{ips}) so callers remain 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 `@test/suites/standard1/networking-smoke.robot`:
- Around line 46-50: The test currently calls Get Router Service IPs once into
@{ips} immediately after router restart which can return empty/partial results;
wrap that discovery in a retry (e.g., a loop using Wait Until Keyword Succeeds
or Retry Keyword) so Get Router Service IPs is retried until non-empty before
iterating FOR ${ip} IN @{ips}; keep the existing inner Wait Until Keyword
Succeeds + Access Hello Microshift Success logic (using ${HTTP_PORT} and
ushift_ip=${ip}) but ensure @{ips} is reobtained/revalidated by Get Router
Service IPs on each retry attempt to avoid flakes.
---
Nitpick comments:
In `@test/suites/standard1/networking-smoke.robot`:
- Around line 79-90: The keyword Get Router Service IPs duplicates parsing logic
already provided by the shared keyword Get All IPs From Router Default LB;
replace the body of Get Router Service IPs with a single call to Get All IPs
From Router Default LB and return its result (e.g., @{ips}= Get All IPs From
Router Default LB RETURN @{ips}), ensuring you keep the same return
variable name (@{ips}) so callers remain 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9aafb157-3429-43e2-a8ba-9a87aa1f97f3
📒 Files selected for processing (1)
test/suites/standard1/networking-smoke.robot
| @{ips}= Get Router Service IPs | ||
| FOR ${ip} IN @{ips} | ||
| Wait Until Keyword Succeeds 10x 6s | ||
| ... Access Hello Microshift Success ${HTTP_PORT} ushift_ip=${ip} | ||
| END |
There was a problem hiding this comment.
Add retry around IP discovery to prevent post-restart flakes.
Line 46 fetches ingress IPs only once right after router restart. If LB status isn’t populated yet, the test fails before Line 48 retries can help.
Proposed fix
- @{ips}= Get Router Service IPs
+ @{ips}= Wait Until Keyword Succeeds 10x 6s Get Router Service IPs
FOR ${ip} IN @{ips}
Wait Until Keyword Succeeds 10x 6s
... Access Hello Microshift Success ${HTTP_PORT} ushift_ip=${ip}
END🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/suites/standard1/networking-smoke.robot` around lines 46 - 50, The test
currently calls Get Router Service IPs once into @{ips} immediately after router
restart which can return empty/partial results; wrap that discovery in a retry
(e.g., a loop using Wait Until Keyword Succeeds or Retry Keyword) so Get Router
Service IPs is retried until non-empty before iterating FOR ${ip} IN @{ips};
keep the existing inner Wait Until Keyword Succeeds + Access Hello Microshift
Success logic (using ${HTTP_PORT} and ushift_ip=${ip}) but ensure @{ips} is
reobtained/revalidated by Get Router Service IPs on each retry attempt to avoid
flakes.
pacevedom
force-pushed
the
OCPBUGS-45192
branch
from
4ef04e7 to
13d5056
Compare
pacevedom
force-pushed
the
OCPBUGS-45192
branch
from
13d5056 to
c4a39ab
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/suites/ipv6/dualstack.robot (3)
91-111: Consider droppingRestart Routerfrom setup or justifying its need.The two sibling tests restart the router to resync routes after apiserver churn. This new test follows the same pattern, which is fine — but since it runs after them in the same suite, the restart here mostly duplicates work. Not a blocker; leave it if you prefer symmetry/robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/ipv6/dualstack.robot` around lines 91 - 111, The test "Router Service IP Connectivity" re-runs "Restart Router" in its [Setup] which duplicates the restart performed by sibling tests; remove the "Restart Router" step from the test's [Setup] to avoid redundant work or alternatively add an explicit justification comment in the test documentation (the [Documentation] block) explaining why the restart is necessary for this test; update the Setup block that currently contains "Restart Router" (and any related sequence like "Create Hello MicroShift Pod", "Expose Hello MicroShift Service Via Route IPv4") to either omit the "Restart Router" line or document why it must remain.
171-181:-kis a no-op forhttp://andLog Manyon failure loses stderr.Two small nits:
curl -konly affects TLS; the URL here is plainhttp://. Safe to drop.- On non-zero
rc,Should Be Equal As Integersfails beforeLog Manyruns, so stderr never gets logged. Consider logging before assertions to aid debugging of the retry loop.♻️ Proposed tweak
- ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command - ... curl -k -i --connect-to "${hostname}::${ip}:" "http://${hostname}:${port}" - ... sudo=False return_rc=True return_stderr=True return_stdout=True - Log Many ${stdout} ${stderr} - Should Be Equal As Integers 0 ${rc} + ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command + ... curl -i --connect-to "${hostname}::${ip}:" "http://${hostname}:${port}" + ... sudo=False return_rc=True return_stderr=True return_stdout=True + Log Many rc=${rc} stdout=${stdout} stderr=${stderr} + Should Be Equal As Integers ${rc} 0 Should Match Regexp ${stdout} HTTP.*200 Should Contain ${stdout} Hello MicroShift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/ipv6/dualstack.robot` around lines 171 - 181, In the Access Hello MicroShift From Host test, remove the redundant curl -k flag (curl is using http://) inside the SSHLibrary.Execute Command call, and move the Log Many ${stdout} ${stderr} line to immediately after the Execute Command so stderr is logged before any assertions; reference the Add Brackets If Ipv6 preprocessing and the SSHLibrary.Execute Command invocation and keep the subsequent assertions (Should Be Equal As Integers, Should Match Regexp, Should Contain) unchanged.
159-169: LGTM — but worth asserting both families are present.Since the suite name advertises dual stack and the PR’s intent is to validate connectivity across all router IPs, consider asserting the returned list contains at least one IPv4 and one IPv6 address. That way, a regression where the LB only advertises one family is caught here rather than masquerading as a passing test that iterated a single IP.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/ipv6/dualstack.robot` around lines 159 - 169, Update the "Get Router Service IPs" keyword to assert that the returned @{ips} list contains at least one IPv4 and one IPv6 address: after calling "Split String ${output}" and before "RETURN @{ips}", add two assertions (e.g., using Robot's regex match/contain keywords) that check for an IPv4 pattern and an IPv6 pattern across the list entries returned by "Oc Get JsonPath" (use ${output} or @{ips} for the checks) so the test fails if only one address family is advertised by the load balancer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/ipv6/dualstack.robot`:
- Around line 91-111: The test "Router Service IP Connectivity" re-runs "Restart
Router" in its [Setup] which duplicates the restart performed by sibling tests;
remove the "Restart Router" step from the test's [Setup] to avoid redundant work
or alternatively add an explicit justification comment in the test documentation
(the [Documentation] block) explaining why the restart is necessary for this
test; update the Setup block that currently contains "Restart Router" (and any
related sequence like "Create Hello MicroShift Pod", "Expose Hello MicroShift
Service Via Route IPv4") to either omit the "Restart Router" line or document
why it must remain.
- Around line 171-181: In the Access Hello MicroShift From Host test, remove the
redundant curl -k flag (curl is using http://) inside the SSHLibrary.Execute
Command call, and move the Log Many ${stdout} ${stderr} line to immediately
after the Execute Command so stderr is logged before any assertions; reference
the Add Brackets If Ipv6 preprocessing and the SSHLibrary.Execute Command
invocation and keep the subsequent assertions (Should Be Equal As Integers,
Should Match Regexp, Should Contain) unchanged.
- Around line 159-169: Update the "Get Router Service IPs" keyword to assert
that the returned @{ips} list contains at least one IPv4 and one IPv6 address:
after calling "Split String ${output}" and before "RETURN @{ips}", add two
assertions (e.g., using Robot's regex match/contain keywords) that check for an
IPv4 pattern and an IPv6 pattern across the list entries returned by "Oc Get
JsonPath" (use ${output} or @{ips} for the checks) so the test fails if only one
address family is advertised by the load balancer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c4f45c14-f47d-441e-8d6c-dc95c43b0ce5
📒 Files selected for processing (1)
test/suites/ipv6/dualstack.robot
|
@pacevedom: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
pacevedom
changed the title
openshift-ci-robot
removed
jira/severity-important
|
@pacevedom: This pull request references USHIFT-6870 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@pacevedom: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
2 participants
Summary by CodeRabbit
Tests