Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jcmoraisjr: This pull request references NE-906 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 302e553
|
|
/assign @davidesalerno |
|
/assign |
|
@jcmoraisjr: This pull request references NE-906 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
jcmoraisjr
force-pushed
the
NE-906-add-dcm-e2e-tests
branch
from
302e553 to
0712389
Compare
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded router concurrency tuning, extensive test fixture expansion, new helper functions, and replaced the single HAProxy router test with multiple It cases exercising dynamic route capacity, replica scaling, overlapping-path precedence, and sequential route operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
@jcmoraisjr: This pull request references NE-906 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
![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
♻️ Duplicate comments (1)
test/extended/router/config_manager.go (1)
820-827:⚠️ Potential issue | 🟡 MinorNormalize
readURLoutput before returning.
RunHostCmdoutput may include trailing whitespace/newlines, which can make exact hostname comparisons flaky. Trim before returning.Proposed fix
func readURL(ns, execPodName, host, abspath, ipaddr string) (string, error) { host = exutil.IPUrl(host) proto := "http" port := 80 uri := fmt.Sprintf("%s://%s:%d%s", proto, host, port, abspath) cmd := fmt.Sprintf("curl -ksfL -m 5 --resolve %s:%d:%s %q", host, port, ipaddr, uri) - return e2eoutput.RunHostCmd(ns, execPodName, cmd) + output, err := e2eoutput.RunHostCmd(ns, execPodName, cmd) + if err != nil { + return "", err + } + return strings.TrimSpace(output), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/config_manager.go` around lines 820 - 827, The readURL function returns raw output from e2eoutput.RunHostCmd which can include trailing whitespace/newlines; update readURL (function name: readURL) to trim the returned string (use strings.TrimSpace) before returning to avoid flaky hostname comparisons, and add the strings import if missing; keep the same signature and error handling but return the trimmed output and the existing error.
🤖 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/extended/router/config_manager.go`:
- Around line 721-724: The error returned by createRoute(oc, routeTypeInsecure,
name, "insecure-service", hostName, "/") is being overwritten by the subsequent
call to waitForRouteToRespond; immediately check/assert the err from createRoute
(the variable err) right after createRoute returns (using the same test
assertion mechanism o.Expect(err).NotTo(o.HaveOccurred())), and only then call
waitForRouteToRespond, assigning its error to err for the second assertion; this
ensures failures in createRoute (referenced by createRoute, err, oc, name,
hostName) are not masked.
---
Duplicate comments:
In `@test/extended/router/config_manager.go`:
- Around line 820-827: The readURL function returns raw output from
e2eoutput.RunHostCmd which can include trailing whitespace/newlines; update
readURL (function name: readURL) to trim the returned string (use
strings.TrimSpace) before returning to avoid flaky hostname comparisons, and add
the strings import if missing; keep the same signature and error handling but
return the trimmed output and the existing error.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/router/config_manager.go
jcmoraisjr
changed the title
|
@jcmoraisjr: This pull request references NE-2498 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
@jcmoraisjr: This pull request references NE-2498 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 0712389
New tests seen in this PR at sha: 0712389
|
|
/cc @Thealisyed |
gcs278
left a comment
gcs278
left a comment
There was a problem hiding this comment.
Overall looks great. I'm actually surprised that we don't have more DCM E2E coverage, so I think this is a great improvement to our existing.
No major issues, mostly some ideas and questions. Generally LGTM.
jcmoraisjr
force-pushed
the
NE-906-add-dcm-e2e-tests
branch
from
0712389 to
c35a43a
Compare
openshift-ci
bot
added
the
approved
|
Tests should be failing with the current codebase, it needs either openshift/router#738 or openshift/router#739 as part of the router image to succeed. They both are already merged, so I'll try a retest in a while, or as soon as I observed any of them being part of the nightly build. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/extended/router/config_manager.go`:
- Around line 691-726: The test currently only verifies exact paths, which can
miss prefix-order bugs; update the test in the "should not conflict overlapping
route configurations" case to also probe a descendant path (e.g.
"/api/v1/subcommand") after creating the "/api/v1" route by calling readURL for
that descendant and asserting it routes to the "/api/v1" endpointName
(endpointName for i where path == "/api/v1"); place this extra check inside the
same Eventually block (or directly after the existing check for "/api/v1") so
failures surface the prefix-matching bug while still using createRoute and
readURL helpers.
- Around line 662-687: The test currently only checks for N distinct hostnames
(using allBackendServers.Len() == expectedBackendServersCount) which can pass if
DCM returns stale or wrong pods; update the logic to build the expected pod name
set from NUM_CONCURRENT_REPLICAS (use the same podName format
"insecure-concurrent-endpoint-replicas-%d") and in the wait callback call
readURL and assert each returned hostname is a member of that expected set
before inserting into allBackendServers; finish only when allBackendServers
equals the expected pod set (not just cardinality) so functions/vars to touch
are NUM_CONCURRENT_REPLICAS, podName generation, readURL call, allBackendServers
and expectedBackendServersCount.
- Around line 623-645: The test currently only verifies HTTP 200 via
waitForRouteToRespond so it can miss routes wired to the wrong backend; update
the check to assert the actual backend by validating the response body contains
the expected backend identifier (e.g. the serve-hostname output or the
serviceName/hostName). Either extend waitForRouteToRespond to accept an expected
body string or add a new helper (e.g. waitForRouteToRespondWithBody) and call it
in both the creation loop and the verification loop using the per-iteration
serviceName/hostName (from createRoute) so each route's response is asserted to
come from its dedicated backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7c894dc-cfd3-425e-8120-c4714ac6f600
📒 Files selected for processing (1)
test/extended/router/config_manager.go
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: c35a43a
|
jcmoraisjr
force-pushed
the
NE-906-add-dcm-e2e-tests
branch
from
c35a43a to
b917a75
Compare
|
Scheduling required tests: |
|
/test e2e-aws-ovn-fips |
|
/test e2e-aws-ovn-serial-2of2 |
|
/verified by ci/prow |
openshift-ci-robot
added
the
verified
|
@ShudiLi: 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. |
jcmoraisjr
force-pushed
the
NE-906-add-dcm-e2e-tests
branch
from
b917a75 to
5b03840
Compare
openshift-ci-robot
removed
the
verified
|
Scheduling required tests: |
|
/retest |
|
/verified by ci/prow |
openshift-ci-robot
added
the
verified
|
@ShudiLi: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 5b03840
|
Adding new tests exploring user experience when DCM is enabled. They were planned based on misbehavior found during manual exploratory tests. The only test before this update is preserved, parts of it were extracted for reuse, which explains the somewhat big change. A number of testing pods and related services are being added representing two distinct sort of test: * several pods under the same service are used for scale in/out tests * several pods and their own 1:1 service are used for overlapping route tests https://redhat.atlassian.net/browse/NE-2498
jcmoraisjr
force-pushed
the
NE-906-add-dcm-e2e-tests
branch
from
5b03840 to
0cde7d0
Compare
openshift-ci-robot
removed
the
verified
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/router/config_manager.go (1)
784-790: Consider aligning with the establishedCreateExecPodOrFailusage pattern.Other router tests (
scoped.go,weighted.go,headers.go) callexutil.CreateExecPodOrFaildirectly without the wrapper. While the cleanup function pattern is convenient, the inconsistency may cause confusion when maintaining the router test suite.This is a minor consistency observation — the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/config_manager.go` around lines 784 - 790, The helper createExecPod wraps exutil.CreateExecPodOrFail and returns a cleanup func, which is inconsistent with other router tests (scoped.go, weighted.go, headers.go) that call exutil.CreateExecPodOrFail directly; remove this wrapper and update callers to call exutil.CreateExecPodOrFail(oc.AdminKubeClient(), ns, "execpod") inline (using the same ns retrieval as in createExecPod) and perform cleanup using the established pattern those tests use (i.e., delete the pod directly where needed) so usage is consistent across the router test suite.
🤖 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/extended/router/config_manager.go`:
- Around line 784-790: The helper createExecPod wraps exutil.CreateExecPodOrFail
and returns a cleanup func, which is inconsistent with other router tests
(scoped.go, weighted.go, headers.go) that call exutil.CreateExecPodOrFail
directly; remove this wrapper and update callers to call
exutil.CreateExecPodOrFail(oc.AdminKubeClient(), ns, "execpod") inline (using
the same ns retrieval as in createExecPod) and perform cleanup using the
established pattern those tests use (i.e., delete the pod directly where needed)
so usage is consistent across the router test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb197884-2842-4459-b42e-78f902bd1309
📒 Files selected for processing (1)
test/extended/router/config_manager.go
|
Scheduling required tests: |
|
I think this is a great start - I know we have a lot more to cover for dynamic config manager (especially with openshift/api#2757), but I think this is the right direction to add coverage to our existing tests. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278, jcmoraisjr 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 |
|
@jcmoraisjr: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 0cde7d0
|
6 participants
Improves DCM coverage by adding new e2e tests.
Jira: https://issues.redhat.com/browse/NE-2498