Stabilize Amphora serial LB tests: per-spec teardown and sourceRanges wait fix - #320
Conversation
94a3bbf to
bd44dc7
Compare
tusharjadhav3302
changed the title
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
I agree that we should try to cleanup resources to avoid leaving behind something. I left a few suggestions inline, but I believe we should try to follow the code pattern -- use more methods from the ginkgo framework when logging and making assertions. These applies throughout all the changes.
| jig.Labels = labels | ||
| // Amphora LBs are costly (slots / max-shared-lb). Register before create so a | ||
| // timed-out ensure still tears down any partial Octavia LB for the next serial [lb] case. | ||
| registerAmphoraLoadBalancerTeardown(loadBalancerClient, clientSet, oc.Namespace(), svcName, lbProviderUnderTest) |
There was a problem hiding this comment.
Wouldn't be better to register the function within BeforeEach since it's conditionally cleans up when the driver is amphora?
There was a problem hiding this comment.
The cleanup stays registered inside the three Its (create, ETP:Local+monitors, and UDP sourceRanges), and it is registered before CreateLoadBalancerService so a timed-out ensure still deletes a partial load balancer. BeforeEach would run for every spec in the provider/protocol loop, including OVN and the Amphora cases we are not changing. The service name is also different in each It. The Amphora check already lives in registerAmphoraLoadBalancerTeardown.
| if delErr := clientSet.CoreV1().Services(namespace).Delete(ctx, svcName, metav1.DeleteOptions{}); delErr != nil && !apierrors.IsNotFound(delErr) { | ||
| e2e.Logf("Teardown: error deleting service %s/%s: %v", namespace, svcName, delErr) | ||
| } | ||
| } else if !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
I would guard for whatever error this function would return. So I think you can use
o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Teardown: error getting service %s/%s: %v", namespace, svcName, err))This would give us a better error message. This should apply for other error references.
There was a problem hiding this comment.
These errors stay as logs. DeferCleanup runs after the spec, so an Expect on Get or Delete would fail a passing spec on a transient API error, or cover the original timeout when the spec already failed. NotFound is expected when the Service never got a load balancer. The UDP diagnostic dump is log-only for the same reason: it runs after the connectivity assert has already failed, and an Expect there would stop the snapshot before sourceRanges_diag_verdict is printed.
| deadline := time.Now().Add(3 * time.Minute) | ||
| for time.Now().Before(deadline) { |
There was a problem hiding this comment.
for a retry mechanism, I think you can use the Eventually method. There are some examples in this file.
bd44dc7 to
bb9ebef
Compare
|
/test test |
ekuris-redhat
left a comment
ekuris-redhat
left a comment
There was a problem hiding this comment.
one comment regarding IPv6 support.
| // Wait until allowed_cidrs in the LB listener is updated to 0.0.0.0/0 (all traffic allowed) | ||
| if lbProviderUnderTest == "amphora" { // it only makes sense with Amphora | ||
| if isAmphoraProvider { | ||
| allowAllAllowedCidrs := []string{"0.0.0.0/0"} |
There was a problem hiding this comment.
Could we derive allowAllAllowedCidrs from the service/VIP address family? For IPv6-primary dual-stack and single-stack IPv6 clusters, allowed_sourcerange is IPv6, but this assertion always waits for 0.0.0.0/0. That will cause the Amphora test to time out after clearing LoadBalancerSourceRanges please handle the IPv6 (and, if applicable, dual-stack) expected CIDRs here.
There was a problem hiding this comment.
Good catch. Updated the Amphora allow-all wait to derive the expected CIDR from the service VIP family: 0.0.0.0/0 for IPv4 and ::/0 for IPv6. The sourceRanges failure diagnostic uses the same rule when classifying allow-all. Squashed into tip 43b125a.
… fix Register Amphora-only per-spec Service/Octavia cleanup for create, ETP:Local+monitors, and UDP sourceRanges. Fix provider case mismatch so Amphora waits for allowed_cidrs allow-all, and dump diagnostics on UDP connectivity failure. Co-authored-by: Cursor <cursoragent@cursor.com>
bb9ebef to
43b125a
Compare
|
/lgtm |
|
Looks legit to me. I think its ready to be merged after all gates are passed |
|
@tusharjadhav3302: 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. |
|
/approve tests are passing and it looks like the teardown is also working. well done @tusharjadhav3302! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: winiciusallan 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
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@winiciusallan: 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. |
4 participants
Summary
[lb]specs can leave Octavia LBs behind between Its, which undermax-shared-lb/ amphora capacity pressure contributes to ensure-LB timeouts inosp_verification.DeferCleanupfor the create, ETP:Local+monitors, and UDP sourceRanges Its (delete Service + cascade-delete Octavia LB if it remains). Registered beforeCreateLoadBalancerServiceso a timed-out ensure still cleans up. OVN / other[lb]specs unchanged.lbProviderUnderTest == "amphora"case mismatch (EqualFold) so Amphora actually waits for listenerallowed_cidrs→0.0.0.0/0after clearingloadBalancerSourceRanges(previously took the OVN 30s sleep path). On post-open UDP failure, dump listener ACL / LB / pool / member state and asourceRanges_diag_verdict.Test plan
Related