NE-2032: e2e: Signal service deprovisioning issues during Gateway DNS test#1220
NE-2032: e2e: Signal service deprovisioning issues during Gateway DNS test#1220alebedev87 wants to merge 1 commit intoopenshift:masterfrom
Conversation
alebedev87
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
alebedev87
force-pushed
the
dnsrecord-listener-update
branch
from
301e0bc to
a079a5a
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
alebedev87
force-pushed
the
dnsrecord-listener-update
branch
from
a079a5a to
849cdc8
Compare
alebedev87
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
alebedev87
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@alebedev87: This pull request references NE-2032 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.20.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. |
|
/assign @grzpiotrowski |
|
/retest |
1 similar comment
|
/retest |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
|
|
||
| if err := kclient.Delete(context.TODO(), gateway); err != nil { | ||
| t.Errorf("failed to delete gateway %q: %v", gateway.Name, err) | ||
| t.Fatalf("Failed to delete gateway %q: %v", gateway.Name, err) |
There was a problem hiding this comment.
do we care about the same problems of networking here? eg.: do you want to retry the delete also?
Additionally, if you do add this delete to the retry function below, it is worth ignoring the error if the object does not exist, as previous loop may have deleted it.
One more comment, out of this change but above on line 646: I would add a RetryOnConflict for that update/patch, as you may have other controllers (GatewayAPI/OSSM) changing it, the Update may fail with a conflict. It would be good to ignore and retry the update
Edit: OTOH it may lead to a false negative if you try to get a service name that doesn't match the gateway name, as it will be not found
There was a problem hiding this comment.
Fixed with a rebase from master.
|
|
||
| // The load balancer deprovisioning can take some time. | ||
| // Signal a long deprovisioning to help distinguish it from DNS management problems. | ||
| gtwSvcName := types.NamespacedName{Namespace: "openshift-ingress", Name: "test-gateway-update-openshift-default"} |
There was a problem hiding this comment.
IIRC the service name is derived from the Gateway name, so instead of calling it "test-gateway-update-openshift-default" do you want to compose the name from the gateway name? This way in case of some change on some logic/naming it will not break the test
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
1 similar comment
|
PR needs rebase. 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
Bot
removed
the
lifecycle/rotten
alebedev87
force-pushed
the
dnsrecord-listener-update
branch
from
849cdc8 to
f424fcc
Compare
📝 WalkthroughWalkthroughIn Changes
Estimated code review effort✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
alebedev87
force-pushed
the
dnsrecord-listener-update
branch
from
f424fcc to
36c7332
Compare
This adds a check for the service associated with a deleted gateway. The test now waits for the service to be fully removed and logs a warning if it persists beyond a specified timeout. This helps identify delays in deprovisioning of cloud load balancers backing gateway services.
alebedev87
force-pushed
the
dnsrecord-listener-update
branch
from
36c7332 to
6352f79
Compare
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/gateway_api_test.go (1)
1159-1166:⚠️ Potential issue | 🟠 MajorUse the controller naming helper to prevent false-positive service-deletion checks.
On Line 1159, manually composing the Service name can drift from controller logic. Since
NotFoundis treated as success, a naming mismatch would make this poll pass immediately and hide the deprovisioning issue this test is meant to detect.Proposed fix
- gtwSvcName := types.NamespacedName{Namespace: operatorcontroller.DefaultOperandNamespace, Name: gateway.Name + "-" + gatewayClass.Name} + gtwSvcName := operatorcontroller.LoadBalancerServiceNameFromGatewayName(gateway.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/gateway_api_test.go` around lines 1159 - 1166, The test builds gtwSvcName by concatenating gateway.Name + "-" + gatewayClass.Name which can diverge from the controller's naming scheme and cause false-positive deletion success in the wait.PollUntilContextTimeout check; replace the manual concatenation with the controller's canonical naming helper (the function used by the controller to compute the operand Service name) to construct gtwSvcName so the poll checks the real service the controller creates (update the code that sets gtwSvcName and keep the rest of the polling logic unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/gateway_api_test.go`:
- Around line 1159-1166: The test builds gtwSvcName by concatenating
gateway.Name + "-" + gatewayClass.Name which can diverge from the controller's
naming scheme and cause false-positive deletion success in the
wait.PollUntilContextTimeout check; replace the manual concatenation with the
controller's canonical naming helper (the function used by the controller to
compute the operand Service name) to construct gtwSvcName so the poll checks the
real service the controller creates (update the code that sets gtwSvcName and
keep the rest of the polling logic unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: de36da67-963a-4141-8054-1bdcf9774b8c
📒 Files selected for processing (1)
test/e2e/gateway_api_test.go
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grzpiotrowski 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
|
Failures in |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@alebedev87: 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. |
|
@alebedev87: The following tests failed, say
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. |
|
/hold Revision 6352f79 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
7 participants
This PR adds a check for the service associated with a deleted gateway. The test now waits for the service to be fully removed and logs a warning if it persists beyond a specified timeout. This helps identify delays in deprovisioning of cloud load balancers backing gateway services.