DNM/SPLAT: e2e-ccm-aws: investigate #tmp-ocpbugs-86789-early-requests - #499
Draft
mtulio wants to merge 15 commits into
Draft
DNM/SPLAT: e2e-ccm-aws: investigate #tmp-ocpbugs-86789-early-requests#499mtulio wants to merge 15 commits into
mtulio wants to merge 15 commits into
Conversation
Extend the health observer to capture full TG state on every poll (TargetSnapshot with healthy/unhealthy/initial/draining counts and per-target state map), matching the SPLAT-307 CSV format for consistent cross-run comparison. Add DescribeTGAttributes() to fetch TG configuration (connection termination, draining interval, etc.) for inclusion in test reports. Add TGAttribute type to the observer package so callers can read the TG config without importing the AWS SDK directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Major enhancement of the NLB health transition e2e tests based on feedback from first test runs and alignment with SPLAT-307 research. Graceful shutdown simulation: - Signal readyz→503 via K8s API server pod proxy before deleting pod - Configurable shutdown-delay (192s, matching KAS shutdown-delay-duration) - Pod keeps serving with X-Server-State: draining during shutdown window Scenario 5.2 (SPLAT-307 revalidation): - Shutdown propagation measurement without pod restart - Signals readyz→503, observes propagation, signals readyz→200, observes recovery. Measures T_route_stop and T_route_start independently. Consistent timing model (t5→t10): - Every test reports the same set of timers regardless of scenario - New restart-phase timers t7.1 (pod deleted), t7.3 (new TCP up), t7.4 (pre-readyz request = BUG) - Computed metrics map directly to SPLAT-307 data table rows Report improvements: - Single-block output (all lines in one framework.Logf call, no per-line logger timestamps) - Service annotations and TG attributes/health check config in report - Unified chronological timeline merging test milestones with TG events, full RFC3339 timestamps Bug fixes from first 3 test runs: - knownServers built from pod list, not client records (which may miss pods due to NLB routing distribution during 30s steady state) - t8 timezone: apply .Local() after parsing UTC X-First-Readyz-Time - t9 anchored to t7.1 (pod deletion), not t8 (which could be stale) - t6 filter requires healthy→unhealthy (excludes initial→unhealthy from nodes without local pods) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Schedule healthserver pods on control-plane nodes to match KAS topology: - nodeSelector for node-role.kubernetes.io/master - tolerations for master and control-plane taints - target-node-labels annotation filters NLB targets to master nodes only (eliminates initial→unhealthy noise from worker nodes without pods) Enable cross-zone load balancing for HA parity with production NLBs via the aws-load-balancer-cross-zone-load-balancing-enabled annotation. Wait for ALL TG targets to report healthy (zero unhealthy/initial) before starting the test, instead of just waiting for N=replicas. This ensures Hyperplane has fully converged before measurements begin. Add CAPA variant test (Scenario 5.5-CAPA) that applies TG attributes via ModifyTargetGroupAttributes after TG creation: target_health_state.unhealthy.connection_termination.enabled=false target_health_state.unhealthy.draining_interval_seconds=300 Re-fetches TG config after modification so the report reflects the actual TG state during the test. Add initial registration timers (t0-t4) to transitionTimeline: t0: deployment created t1: pods running t2: NLB provisioned t3: all TG targets healthy t4: first client request These appear in both the TIMING TABLE and the chronological TIMELINE. Increase client request rate to 200ms (5 req/s) for better data density. Increase steady state baseline to 2min and post-restart observation to 5min for more reliable measurements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace fixed-duration observation windows with event-driven waits: - After initial setup: wait for ALL TG targets healthy (already done), then observe for 90s to confirm stable routing. - After pod restart: wait for restarted target to become healthy via waitForAllTGTargetsHealthy(), then observe 90s post-recovery. Previously used a fixed startup-delay+5min which was either too short (missed late propagation) or too long (wasted time). Add ENVIRONMENT section to the report with platform, region, and topology (HighlyAvailable vs External/HyperShift) from the cluster's Infrastructure resource. Gives full visibility of the test setup alongside the SERVICE CONFIGURATION and TARGET GROUP CONFIGURATION. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix waitForAllTGTargetsHealthy getting stuck indefinitely: The function read from observer.Snapshots() which requires the observer's background polling loop to be running. During setup the observer is not started yet, so snapshots were always empty and the function spun until the 10min context deadline. Fix by adding Observer.PollOnce() — a single DescribeTargetHealth call that works independently of the background loop. The wait function now calls PollOnce directly with per-target state logging every 10s so the operator can see convergence progress. Switch nodeSelector from deprecated node-role.kubernetes.io/master to node-role.kubernetes.io/control-plane (OCP 5.x). Keep tolerations for both labels for backward compatibility. Update target-node-labels annotation accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add explicit log lines when the client and observer start, so the operator can confirm that request generation begins after all TG targets are healthy and see the exact timestamp in the test output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comprehensive reference for humans and agents covering: - Problem statement (OCPBUGS-86789, SPLAT-307) - Architecture and file layout - Complete timing model (t0-t10) with SPLAT-307 correspondence - All three test scenarios (5.5, 5.5-CAPA, 5.2) - Component documentation (healthserver, observer, client) - Infrastructure config (control-plane scheduling, NLB annotations) - How to build, run, and interpret the report output - Related issues and next steps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add two new report sections for full traffic visibility: REQUEST STATISTICS: total request count with breakdown by HTTP status class (2xx, 4xx, 5xx) and connection errors. REQUEST BREAKDOWN BY PHASE: per-phase request counts (total, 2xx, errors, pre-readyz) across the test lifecycle phases: Warmup (t3→t5): all targets healthy, steady-state baseline Shutdown (t5→t7.1): readyz→503, target still serving Restart (t7.1→t9): pod deleted → new target healthy Recovery (t9→end): new target healthy, traffic flowing For Scenario 5.2: Shutdown (t5→t8), Recovery (t8→end). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Show how long each phase lasted alongside the request counts. Open-ended phases (Recovery→end) use the last recorded request timestamp to compute duration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Client throughput fix:
Run 4 parallel request goroutines instead of 1 sequential worker.
With ~1.8s RTT (e.g., South America → us-east-1), a single worker
achieves only ~0.5 req/s regardless of ticker interval. 4 parallel
workers each fire independently on their own 200ms ticker, giving
~2 req/s even on high-latency links.
NewClient() now takes a numWorkers parameter. Each worker creates
new TCP connections (DisableKeepAlives) independently. The shared
records slice is protected by the existing mutex.
CAPA state matching fix:
When connection_termination.enabled=false (CAPA fix), the NLB TG
transitions through "unhealthy.draining" instead of "unhealthy".
Add isUnhealthyState() helper using strings.HasPrefix("unhealthy")
to match both states. Applied to all state comparisons in timeline
computation. This also confirms v5 open question Q5: unhealthy.draining
occurs during HC-driven transitions with conn_term=false.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document parallel client workers (4 goroutines × 200ms ticker), request statistics and per-phase breakdown in report output, and the confirmed unhealthy.draining state finding for CAPA config. Update Scenario 5.5 flow to reflect current observation windows (90s post-healthy, event-driven) and client config (4 workers). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a lifecycle-aware health server, concurrent request client, AWS target-health observer, and AWS NLB transition tests. The tests cover readiness changes, shutdown propagation, target-group attributes, replacement pods, timeline metrics, and reports. ChangesAWS NLB health transition testing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Transition test
participant K8s as Kubernetes API
participant Server as Healthserver
participant NLB as AWS NLB
participant Client as Health client
participant Observer as Target-health observer
participant ELB as AWS ELB API
Test->>K8s: Create Deployment and LoadBalancer Service
K8s->>Server: Start configured healthserver pod
Test->>Observer: Discover target group and start polling
Test->>Client: Start concurrent HTTP checks
Test->>Server: Change readiness or request shutdown
Server-->>NLB: Return readiness and lifecycle state
NLB->>ELB: Update target health
Observer->>ELB: Poll target health and record events
Client->>NLB: Send requests and record responses
Test->>K8s: Restart pod or restore readiness
Test->>Test: Compute timeline and emit report
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 6 warnings)
✅ Passed checks (7 passed)
✨ 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: 13
🧹 Nitpick comments (9)
openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go (7)
599-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the draining count in the wait log.
totalat line 590 includessnap.DrainingCount, but the log prints only healthy, unhealthy, and initial. The printed counts then do not sum tototal, and a stalled draining target looks unexplained.♻️ Proposed change
- framework.Logf("[tg-wait] healthy=%d unhealthy=%d initial=%d total=%d | %s", - snap.HealthyCount, snap.UnhealthyCount, snap.InitialCount, total, + framework.Logf("[tg-wait] healthy=%d unhealthy=%d initial=%d draining=%d total=%d | %s", + snap.HealthyCount, snap.UnhealthyCount, snap.InitialCount, snap.DrainingCount, total, strings.Join(details, ", "))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 599 - 601, Update the wait log in the tg-wait reporting code to include snap.DrainingCount alongside the healthy, unhealthy, and initial counts, and add the corresponding value to the format arguments while preserving the existing total and details output.
931-934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort the annotation keys before printing.
Go map iteration order is random. The
SERVICE CONFIGURATIONblock then differs between runs, which blocks the cross-run report comparison described in the README. Collect the keys, sort them, then print.♻️ Proposed change
- for k, v := range cfg.ServiceAnnotations { - short := strings.TrimPrefix(k, "service.beta.kubernetes.io/aws-load-balancer-") - w(" svc/%s: %s", short, v) - } + annKeys := make([]string, 0, len(cfg.ServiceAnnotations)) + for k := range cfg.ServiceAnnotations { + annKeys = append(annKeys, k) + } + sort.Strings(annKeys) + for _, k := range annKeys { + short := strings.TrimPrefix(k, "service.beta.kubernetes.io/aws-load-balancer-") + w(" svc/%s: %s", short, cfg.ServiceAnnotations[k]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 931 - 934, Update the ServiceAnnotations printing logic in the configuration-report function to collect and sort the annotation keys before iterating. Use the sorted keys to retrieve values from cfg.ServiceAnnotations and preserve the existing trimmed-key output format.
714-724: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSelect t7 by the maximum timestamp, not by iteration order.
The client runs 4 parallel workers, so appended records can be slightly out of timestamp order. Line 721 keeps the last record reached by iteration, which is not guaranteed to be the latest request. t7 is the primary SPLAT-307 measurement, so make the selection explicit.
The same pattern exists in
computeTimeline52at lines 811-819.♻️ Proposed change
if r.ServerID == oldPod { - tl.T7 = r.Timestamp + if r.Timestamp.After(tl.T7) { + tl.T7 = r.Timestamp + } tl.UnhealthyReqCount++ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 714 - 724, Update the t7 selection in the current timeline computation and in computeTimeline52 to choose the oldPod request with the greatest Timestamp, rather than overwriting it based on record iteration order; increment UnhealthyReqCount for every qualifying request while only replacing tl.T7 when the timestamp is later.
147-150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a context-aware wait instead of
time.Sleep.
time.Sleepignoresctx. If Ginkgo cancels the spec through the node timeout, the spec still blocks for the full duration. The scenario sleeps for 90s, 192s, and 90s, so cancellation is delayed by more than 6 minutes.Replace each sleep with a select on
ctx.Done()and a timer.♻️ Proposed helper
// observeFor waits for d or until ctx is cancelled. func observeFor(ctx context.Context, d time.Duration) { t := time.NewTimer(d) defer t.Stop() select { case <-ctx.Done(): case <-t.C: } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 147 - 150, Replace every time.Sleep call in the health-transition scenario with a context-aware wait using ctx.Done() and a timer, including the steady-state wait near the By call and the other 192s/90s waits. Add or reuse an observeFor helper that stops its timer and returns when either the duration elapses or ctx is cancelled.
277-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the discarded
DescribeTGAttributeserror.Lines 278-281 drop the error. The same pattern exists at lines 557-560 and in
fetchTGHealthCheckConfigat lines 616-624. The report then silently omits target-group configuration, and the reader cannot tell whether the attributes are absent or the API call failed. Log the error withframework.Logfin each case.As per path instructions: "Never ignore error returns".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 277 - 282, Log every discarded error from DescribeTGAttributes and fetchTGHealthCheckConfig using framework.Logf, including the existing error branches near the report update and within fetchTGHealthCheckConfig. Preserve successful assignments and configuration-fetch behavior while ensuring each failed API call is clearly reported.Source: Path instructions
119-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared scenario body.
Lines 119-238 and lines 247-369 repeat about 110 lines of identical orchestration. The only differences are the CAPA target-group attributes and the report label. Extract one helper, for example
runPreReadyzScenario(ctx, cs, ns, scenarioName string, tgAttrs map[string]string), and call it from bothItblocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 119 - 238, Extract the duplicated orchestration from both pre-readyz `It` blocks into a shared `runPreReadyzScenario` helper accepting `ctx`, `cs`, `ns`, a scenario/report name, and target-group attributes. Move setup, traffic observation, pod transition, timeline construction, and report generation into the helper, then have each `It` block provide only its scenario-specific CAPA attributes and report label.
436-442: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse
allRecordsinstead of callingclient.Records()again.Line 437 calls
client.Records(), which copies the full record slice a second time.allRecordsat line 428 holds the same data. Iterate overallRecords.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines 436 - 442, Update the t4 successful-request loop to iterate over the existing allRecords collection instead of calling client.Records() again, preserving the current filtering and timestamp assignment behavior.openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go (2)
143-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain the last API error instead of discarding it.
Line 149 drops the
DescribeTargetHealtherror. If the call fails for the whole timeout, the caller sees only a context-deadline error and cannot see the cause. Keep the last error and wrap it in the returned error.As per path instructions: "Never ignore error returns".♻️ Proposed change
func (o *Observer) WaitForAllHealthy(ctx context.Context, minHealthy int, timeout time.Duration) error { - return wait.PollUntilContextTimeout(ctx, o.interval, timeout, true, func(ctx context.Context) (bool, error) { + var lastErr error + err := wait.PollUntilContextTimeout(ctx, o.interval, timeout, true, func(ctx context.Context) (bool, error) { output, err := o.elbClient.DescribeTargetHealth(ctx, &elbv2.DescribeTargetHealthInput{ TargetGroupArn: aws.String(o.tgARN), }) if err != nil { + lastErr = err return false, nil } healthy := 0 for _, d := range output.TargetHealthDescriptions { if d.TargetHealth.State == elbv2types.TargetHealthStateEnumHealthy { healthy++ } } return healthy >= minHealthy, nil }) + if err != nil && lastErr != nil { + return fmt.Errorf("%w (last API error: %v)", err, lastErr) + } + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go` around lines 143 - 159, Update Observer.WaitForAllHealthy to retain the most recent error returned by DescribeTargetHealth instead of returning false, nil. After polling ends, wrap and return that retained API error when applicable, while preserving the existing healthy-count polling behavior and context error when no API error occurred.Source: Path instructions
205-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared snapshot builder and log the poll error.
Lines 218-239 duplicate the snapshot construction and state counting in
PollOnce(lines 74-92). Extract one helper that convertsoutput.TargetHealthDescriptionsinto aTargetSnapshot, then call it from both paths. The nil-pointer guard then applies in one place.Line 210 also returns without any record of the failure. Store the last poll error on the
Observerso the report can show polling gaps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go` around lines 205 - 256, The snapshot construction and state-counting logic in Observer.pollOnce should be extracted into a shared helper that converts target health descriptions into a TargetSnapshot, then reused by both PollOnce paths with the nil-pointer guard centralized there. In pollOnce, persist DescribeTargetHealth errors on the Observer instead of returning silently, and expose that stored last poll error through the existing report flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile`:
- Around line 8-10: Update the final scratch image definition near ENTRYPOINT to
include a HEALTHCHECK using exec form against /readyz. Build a small statically
linked probe binary in the builder stage, copy it into the final image alongside
/healthserver, and configure the health check to invoke that probe without
relying on shell or HTTP client binaries.
- Around line 8-10: Update the final scratch image in the Dockerfile to add USER
65532:65532 before ENTRYPOINT, so the /healthserver process runs as a non-root
user while preserving the existing port 8080 behavior.
- Line 1: Update the Dockerfile’s builder-stage FROM instruction to pin the
golang:1.22-alpine image by its reviewed, platform-appropriate immutable digest,
retaining the existing builder stage name.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/main.go`:
- Line 9: Update the health server startup flow around the server creation and
existing tcpUp recording: explicitly create the listener with net.Listen, handle
a bind error before recording readiness, record tcpUp only after Listen
succeeds, then serve using srv.Serve(listener) instead of srv.ListenAndServe.
- Line 90: Handle every discarded error in the health server and client: in
openshift-tests/ccm-aws-tests/cmd/healthserver/main.go (90, 120, 130, 133, 149,
154, 178, and 215), check srv.Shutdown, all response writes, and lifecycle JSON
encoding, logging shutdown failures or stopping/recording request failures as
appropriate; in openshift-tests/ccm-aws-tests/e2e/aws/health/client.go
(126-127), capture response body read and close errors in RequestRecord. No
affected site requires no direct change.
- Around line 59-63: Update the http.Server initialization in the health server
to set bounded ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout
values appropriate for short health requests, while preserving the existing Addr
and Handler configuration.
In `@openshift-tests/ccm-aws-tests/e2e/aws/health/client.go`:
- Around line 40-55: Update NewClient to reject non-positive interval values or
replace them with a documented positive default before assigning the interval
field. Ensure the value stored in Client is always valid for time.NewTicker,
while preserving the existing worker-count normalization.
- Around line 129-134: Update the non-ready classification in the
response-recording logic around rec.IsNonReadyReq to recognize every non-ready
server state returned by /readyz: pre-readyz, draining, and shutdown. Keep ready
responses classified as ready while ensuring all three non-ready states are
included in the non-ready request statistics.
In `@openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go`:
- Around line 78-92: Guard the pointer fields in PollOnce, WaitForAllHealthy,
and background pollOnce before accessing Target.Id or TargetHealth.State. Handle
nil Target or TargetHealth safely without panicking, including inside the
polling goroutine, while preserving the existing target-state recording and
health-count behavior for valid descriptions.
In `@openshift-tests/ccm-aws-tests/e2e/aws/health/README.md`:
- Line 80: Update the metric name in the README table row for the full initial
registration measurement from T_tg_initial to T_tg_initial_healthy, matching the
report output produced by lb_health_transition.go.
In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go`:
- Around line 490-501: Add a pre-test gate in the setup flow after determining
topology and before deployment, using the existing topology and schedulable
control-plane node information. Skip the spec with an explicit message when
topology is External (HyperShift) or when the schedulable control-plane count is
less than the scenario’s replicas, including SNO/TNF/TNA cases. Keep the
existing report population and control-plane targeting unchanged.
- Around line 405-410: Before accessing pods.Items[0] in the pod lookup flow,
validate that the returned pod list is non-empty and fail with a clear framework
assertion if it is empty. Apply the same guard pattern used by the scenario
flows around targetPod and targetNode, while preserving the existing error check
and indexing behavior for non-empty lists.
- Around line 655-678: Update waitForNewPod to identify the replacement rather
than any running replica: pass the knownServers set into the function and skip
pods already present there, or select the eligible pod with the newest
CreationTimestamp. Preserve excluding oldPodName and terminating pods, and
ensure the returned name is the newly created pod used by tl.NewPod.
---
Nitpick comments:
In `@openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go`:
- Around line 143-159: Update Observer.WaitForAllHealthy to retain the most
recent error returned by DescribeTargetHealth instead of returning false, nil.
After polling ends, wrap and return that retained API error when applicable,
while preserving the existing healthy-count polling behavior and context error
when no API error occurred.
- Around line 205-256: The snapshot construction and state-counting logic in
Observer.pollOnce should be extracted into a shared helper that converts target
health descriptions into a TargetSnapshot, then reused by both PollOnce paths
with the nil-pointer guard centralized there. In pollOnce, persist
DescribeTargetHealth errors on the Observer instead of returning silently, and
expose that stored last poll error through the existing report flow.
In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go`:
- Around line 599-601: Update the wait log in the tg-wait reporting code to
include snap.DrainingCount alongside the healthy, unhealthy, and initial counts,
and add the corresponding value to the format arguments while preserving the
existing total and details output.
- Around line 931-934: Update the ServiceAnnotations printing logic in the
configuration-report function to collect and sort the annotation keys before
iterating. Use the sorted keys to retrieve values from cfg.ServiceAnnotations
and preserve the existing trimmed-key output format.
- Around line 714-724: Update the t7 selection in the current timeline
computation and in computeTimeline52 to choose the oldPod request with the
greatest Timestamp, rather than overwriting it based on record iteration order;
increment UnhealthyReqCount for every qualifying request while only replacing
tl.T7 when the timestamp is later.
- Around line 147-150: Replace every time.Sleep call in the health-transition
scenario with a context-aware wait using ctx.Done() and a timer, including the
steady-state wait near the By call and the other 192s/90s waits. Add or reuse an
observeFor helper that stops its timer and returns when either the duration
elapses or ctx is cancelled.
- Around line 277-282: Log every discarded error from DescribeTGAttributes and
fetchTGHealthCheckConfig using framework.Logf, including the existing error
branches near the report update and within fetchTGHealthCheckConfig. Preserve
successful assignments and configuration-fetch behavior while ensuring each
failed API call is clearly reported.
- Around line 119-238: Extract the duplicated orchestration from both pre-readyz
`It` blocks into a shared `runPreReadyzScenario` helper accepting `ctx`, `cs`,
`ns`, a scenario/report name, and target-group attributes. Move setup, traffic
observation, pod transition, timeline construction, and report generation into
the helper, then have each `It` block provide only its scenario-specific CAPA
attributes and report label.
- Around line 436-442: Update the t4 successful-request loop to iterate over the
existing allRecords collection instead of calling client.Records() again,
preserving the current filtering and timestamp assignment behavior.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0db7e5c1-5984-4889-833c-a32516522305
📒 Files selected for processing (7)
openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfileopenshift-tests/ccm-aws-tests/cmd/healthserver/main.goopenshift-tests/ccm-aws-tests/e2e/aws/health/README.mdopenshift-tests/ccm-aws-tests/e2e/aws/health/client.goopenshift-tests/ccm-aws-tests/e2e/aws/health/observer.goopenshift-tests/ccm-aws-tests/e2e/aws/health/types.goopenshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go
| @@ -0,0 +1,10 @@ | |||
| FROM golang:1.22-alpine AS builder | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
file='openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile'
printf '%s\n' '--- Dockerfile ---'
cat -n "$file"
printf '%s\n' '--- image references ---'
rg -n '^\s*(FROM|USER|HEALTHCHECK)\b' "$file"Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 643
Security Misconfiguration (CWE-494): Download of Code Without Integrity Check
Reachability: Internal
Pin the builder image by digest.
golang:1.22-alpine is a mutable non-Red-Hat tag. Use a reviewed, platform-appropriate digest.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile` at line 1, Update
the Dockerfile’s builder-stage FROM instruction to pin the golang:1.22-alpine
image by its reviewed, platform-appropriate immutable digest, retaining the
existing builder stage name.
Source: Path instructions
Comment on lines
+8
to
+10
| FROM scratch | ||
| COPY --from=builder /build/healthserver /healthserver | ||
| ENTRYPOINT ["/healthserver"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a functional image health check.
The final image has no HEALTHCHECK. Because scratch has no shell or HTTP client, build and copy a small static probe binary, then use an exec-form health check against /readyz.
As per path instructions: “HEALTHCHECK defined.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile` around lines 8 -
10, Update the final scratch image definition near ENTRYPOINT to include a
HEALTHCHECK using exec form against /readyz. Build a small statically linked
probe binary in the builder stage, copy it into the final image alongside
/healthserver, and configure the health check to invoke that probe without
relying on shell or HTTP client binaries.
Source: Path instructions
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
file='openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile'
printf '%s\n' '--- Dockerfile ---'
cat -n "$file"
printf '%s\n' '--- healthserver references ---'
rg -n --glob '!vendor' --glob '!node_modules' '8080|ListenAndServe|USER|healthserver' \
openshift-tests/ccm-aws-tests/cmd/healthserver \
openshift-tests/ccm-aws-tests 2>/dev/null | head -200
printf '%s\n' '--- final-stage instruction probe ---'
python3 - <<'PY'
from pathlib import Path
p = Path("openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile")
lines = p.read_text().splitlines()
from_idx = [i for i, line in enumerate(lines) if line.lstrip().upper().startswith("FROM ")]
last = from_idx[-1]
stage = lines[last:]
print("final_stage_start_line:", last + 1)
print("final_stage_has_user:", any(line.lstrip().upper().startswith("USER ") for line in stage))
print("final_stage_user_lines:", [
(last + i + 1, line) for i, line in enumerate(stage)
if line.lstrip().upper().startswith("USER ")
])
print("final_stage_instructions:")
for i, line in enumerate(stage, last + 1):
stripped = line.strip()
if stripped and not stripped.startswith("#"):
print(f"{i}: {stripped}")
PYRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 6360
Security Misconfiguration (CWE-250)
Reachability: Internal
Run /healthserver as a non-root user.
The final scratch image has no USER instruction and runs as UID 0 by default. Add USER 65532:65532 before ENTRYPOINT; port 8080 does not require root.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/Dockerfile` around lines 8 -
10, Update the final scratch image in the Dockerfile to add USER 65532:65532
before ENTRYPOINT, so the /healthserver process runs as a non-root user while
preserving the existing port 8080 behavior.
Sources: Path instructions, Linters/SAST tools
| "flag" | ||
| "fmt" | ||
| "log" | ||
| "net/http" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Record tcpUp after the listener binds.
Line 94 records t_tcp_up before ListenAndServe calls net.Listen. A bind failure or delay can make the lifecycle report claim that TCP was available when no listener existed. Create the listener explicitly, record tcpUp after net.Listen succeeds, then call srv.Serve(listener).
Proposed fix
+ "net"
...
- s.mu.Lock()
- s.tcpUp = time.Now()
- s.mu.Unlock()
+ listener, err := net.Listen("tcp", srv.Addr)
+ if err != nil {
+ log.Fatalf("listen: %v", err)
+ }
+ s.mu.Lock()
+ s.tcpUp = time.Now()
+ s.mu.Unlock()
log.Printf("healthserver %s listening on :%d (startup-delay=%s)", id, *port, *startupDelay)
- if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ if err := srv.Serve(listener); err != nil && err != http.ErrServerClosed {
log.Fatalf("listen: %v", err)
}Also applies to: 93-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/main.go` at line 9, Update the
health server startup flow around the server creation and existing tcpUp
recording: explicitly create the listener with net.Listen, handle a bind error
before recording readiness, record tcpUp only after Listen succeeds, then serve
using srv.Serve(listener) instead of srv.ListenAndServe.
Comment on lines
+59
to
+63
|
|
||
| srv := &http.Server{ | ||
| Addr: fmt.Sprintf(":%d", *port), | ||
| Handler: mux, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- healthserver source ---'
sed -n '1,180p' openshift-tests/ccm-aws-tests/cmd/healthserver/main.go
printf '%s\n' '--- healthserver references and deployment/service definitions ---'
rg -n -C 4 'healthserver|ReadHeaderTimeout|ReadTimeout|WriteTimeout|IdleTimeout|ListenAndServe|targetPort|port:|LoadBalancer|healthCheck' \
openshift-tests/ccm-aws-tests/cmd openshift-tests/ccm-aws-tests/e2e/awsRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 50409
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining handlers ---'
sed -n '160,280p' openshift-tests/ccm-aws-tests/cmd/healthserver/main.go
printf '%s\n' '--- deployment and service builders ---'
rg -n -C 35 'func buildHealthserverDeployment|func buildHealthTransitionService' \
openshift-tests/ccm-aws-tests/e2e/aws
printf '%s\n' '--- exact health-transition annotations and ports ---'
python3 - <<'PY'
from pathlib import Path
p = Path("openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go")
lines = p.read_text().splitlines()
for i, line in enumerate(lines):
if "func buildHealthserverDeployment" in line or "func buildHealthTransitionService" in line:
lo = max(0, i - 2)
hi = min(len(lines), i + 130)
print(f"--- lines {lo+1}-{hi} ---")
for n in range(lo, hi):
print(f"{n+1}:{lines[n]}")
PYRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 22100
Denial of Service (CWE-400): Uncontrolled Resource Consumption
Reachability: External · Exploitability: Moderate
Set bounded HTTP server timeouts. The health server is exposed through an external NLB. Without read deadlines, a client can keep connections open by sending request data slowly. Set ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout to bounded values for these short requests.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 59-62: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{
Addr: fmt.Sprintf(":%d", *port),
Handler: mux,
}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/main.go` around lines 59 - 63,
Update the http.Server initialization in the health server to set bounded
ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout values appropriate
for short health requests, while preserving the existing Addr and Handler
configuration.
Source: Linters/SAST tools
| s.mu.Unlock() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| srv.Shutdown(ctx) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle all returned errors.
The affected calls discard errors from shutdown, response writes, JSON encoding, body reads, and body close operations. Check each error and either stop the handler, record the request failure, or log the shutdown failure.
openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L90-L90: handle the error fromsrv.Shutdown.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L120-L120: handle the main response write error.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L130-L130: handle the ready response write error.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L133-L133: handle the non-ready response write error.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L149-L149: handle the admin-ready response write error.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L154-L154: handle the admin-draining response write error.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L178-L178: handle the shutdown response write error.openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L215-L215: handle the lifecycle JSON encoding error.openshift-tests/ccm-aws-tests/e2e/aws/health/client.go#L126-L127: record response body read and close errors inRequestRecord.
As per path instructions: “Never ignore error returns.”
📍 Affects 2 files
openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L90-L90(this comment)openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L120-L120openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L130-L130openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L133-L133openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L149-L149openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L154-L154openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L178-L178openshift-tests/ccm-aws-tests/cmd/healthserver/main.go#L215-L215openshift-tests/ccm-aws-tests/e2e/aws/health/client.go#L126-L127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/cmd/healthserver/main.go` at line 90, Handle
every discarded error in the health server and client: in
openshift-tests/ccm-aws-tests/cmd/healthserver/main.go (90, 120, 130, 133, 149,
154, 178, and 215), check srv.Shutdown, all response writes, and lifecycle JSON
encoding, logging shutdown failures or stopping/recording request failures as
appropriate; in openshift-tests/ccm-aws-tests/e2e/aws/health/client.go
(126-127), capture response body read and close errors in RequestRecord. No
affected site requires no direct change.
Source: Path instructions
Comment on lines
+78
to
+92
| for _, d := range output.TargetHealthDescriptions { | ||
| id := aws.ToString(d.Target.Id) | ||
| state := string(d.TargetHealth.State) | ||
| snap.Targets[id] = state | ||
| switch d.TargetHealth.State { | ||
| case elbv2types.TargetHealthStateEnumHealthy: | ||
| snap.HealthyCount++ | ||
| case elbv2types.TargetHealthStateEnumUnhealthy, elbv2types.TargetHealthStateEnumUnhealthyDraining: | ||
| snap.UnhealthyCount++ | ||
| case elbv2types.TargetHealthStateEnumInitial: | ||
| snap.InitialCount++ | ||
| case elbv2types.TargetHealthStateEnumDraining: | ||
| snap.DrainingCount++ | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm TargetHealthDescription field pointer-ness in the vendored SDK.
fd -t f 'types.go' | rg -l 'TargetHealthDescription' | while IFS= read -r f; do
rg -n -A 20 'type TargetHealthDescription struct' "$f"
doneRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -t f | rg '(^|/)(observer\.go|go\.mod|go\.sum|vendor/.*elasticloadbalancingv2.*|types\.go)$' || true
printf '%s\n' '--- observer references ---'
rg -n -C 4 'TargetHealthDescription|TargetHealthDescriptions|func .*Poll|pollOnce|PollOnce' . --glob '*.go' || true
printf '%s\n' '--- AWS SDK declarations ---'
rg -n -C 3 'elasticloadbalancingv2|aws-sdk-go-v2' . --glob 'go.mod' --glob 'go.sum' --glob '*.go' || trueRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 50408
🏁 Script executed:
#!/bin/bash
set -eu
observer="$(fd -t f 'observer.go' | rg 'openshift-tests/ccm-aws-tests/e2e/aws/health/observer\.go$' | head -n 1)"
printf '%s\n' "--- $observer: relevant methods ---"
rg -n -C 12 'TargetHealthDescriptions|func .*Poll|pollOnce|go func' "$observer"
sdk_dir='openshift-tests/ccm-aws-tests/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2'
printf '%s\n' "--- vendored TargetHealthDescription ---"
rg -n -A 18 -B 2 'type TargetHealthDescription struct' "$sdk_dir"
printf '%s\n' '--- vendored module version ---'
rg -n -C 2 'aws-sdk-go-v2/service/elasticloadbalancingv2' openshift-tests/ccm-aws-tests/go.mod openshift-tests/ccm-aws-tests/go.sum 2>/dev/null || trueRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 8649
🏁 Script executed:
#!/bin/bash
set -eu
observer='openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go'
sdk='openshift-tests/ccm-aws-tests/vendor/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types/types.go'
printf '%s\n' '--- observer.go lines 140-160 ---'
sed -n '140,160p' "$observer"
printf '%s\n' '--- observer.go lines 218-250 ---'
sed -n '218,250p' "$observer"
printf '%s\n' '--- TargetHealthDescription fields ---'
sed -n '1575,1605p' "$sdk"
printf '%s\n' '--- read-only structural verifier ---'
python3 - "$observer" "$sdk" <<'PY'
from pathlib import Path
import re
import sys
observer = Path(sys.argv[1]).read_text()
sdk = Path(sys.argv[2]).read_text()
m = re.search(
r"type TargetHealthDescription struct \{(?P<body>.*?)\n\}",
sdk,
re.S,
)
if not m:
raise SystemExit("TargetHealthDescription definition not found")
body = m.group("body")
for field in ("Target", "TargetHealth"):
match = re.search(rf"^\s*{field}\s+\*([A-Za-z0-9_]+)", body, re.M)
print(f"{field}: {match.group(0).strip() if match else 'not a pointer field'}")
for expression in (
"d.Target.Id",
"d.Target.Port",
"d.TargetHealth.State",
"d.TargetHealth.Reason",
):
print(f"{expression}: {observer.count(expression)} occurrence(s)")
PYRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 2997
Guard TargetHealthDescription pointer fields before dereferencing them.
Target and TargetHealth are pointers. Add nil checks in PollOnce, WaitForAllHealthy, and background pollOnce; otherwise a nil field can panic the test process, including the polling goroutine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/e2e/aws/health/observer.go` around lines 78 -
92, Guard the pointer fields in PollOnce, WaitForAllHealthy, and background
pollOnce before accessing Target.Id or TargetHealth.State. Handle nil Target or
TargetHealth safely without panicking, including inside the polling goroutine,
while preserving the existing target-state recording and health-count behavior
for valid descriptions.
| |------------------|-------------|-----------------|--------------------------------------| | ||
| | T_deploy_ready | t1 - t0 | | Pod scheduling + startup | | ||
| | T_nlb_provision | t2 - t0 | | NLB creation in AWS | | ||
| | T_tg_initial | t3 - t0 | | Full initial registration | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the metric name with the report output.
The report prints T_tg_initial_healthy (lb_health_transition.go line 955). The table lists T_tg_initial. Use the same name in both places.
📝 Proposed change
-| T_tg_initial | t3 - t0 | | Full initial registration |
+| T_tg_initial_healthy | t3 - t0 | | Full initial registration |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | T_tg_initial | t3 - t0 | | Full initial registration | | |
| | T_tg_initial_healthy | t3 - t0 | | Full initial registration | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/e2e/aws/health/README.md` at line 80, Update
the metric name in the README table row for the full initial registration
measurement from T_tg_initial to T_tg_initial_healthy, matching the report
output produced by lb_health_transition.go.
Comment on lines
+405
to
+410
| pods, err := cs.CoreV1().Pods(ns.Name).List(ctx, metav1.ListOptions{ | ||
| LabelSelector: fmt.Sprintf("app=%s", deployName), | ||
| }) | ||
| framework.ExpectNoError(err) | ||
| targetPod := pods.Items[0].Name | ||
| targetNode := pods.Items[0].Spec.NodeName |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert the pod list is not empty before indexing.
Line 409 indexes pods.Items[0] with no length check. If the list is empty, the spec panics with an index-out-of-range error instead of failing with a clear message. Scenarios 5.5 and 5.5-CAPA guard this at lines 166 and 307.
🛡️ Proposed guard
framework.ExpectNoError(err)
+ Expect(len(pods.Items)).To(BeNumerically(">=", int(replicas)))
targetPod := pods.Items[0].Name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pods, err := cs.CoreV1().Pods(ns.Name).List(ctx, metav1.ListOptions{ | |
| LabelSelector: fmt.Sprintf("app=%s", deployName), | |
| }) | |
| framework.ExpectNoError(err) | |
| targetPod := pods.Items[0].Name | |
| targetNode := pods.Items[0].Spec.NodeName | |
| pods, err := cs.CoreV1().Pods(ns.Name).List(ctx, metav1.ListOptions{ | |
| LabelSelector: fmt.Sprintf("app=%s", deployName), | |
| }) | |
| framework.ExpectNoError(err) | |
| Expect(len(pods.Items)).To(BeNumerically(">=", int(replicas))) | |
| targetPod := pods.Items[0].Name | |
| targetNode := pods.Items[0].Spec.NodeName |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines
405 - 410, Before accessing pods.Items[0] in the pod lookup flow, validate that
the returned pod list is non-empty and fail with a clear framework assertion if
it is empty. Apply the same guard pattern used by the scenario flows around
targetPod and targetNode, while preserving the existing error check and indexing
behavior for non-empty lists.
Comment on lines
+490
to
+501
| // Populate environment summary from the cluster's Infrastructure resource | ||
| cfg.Platform = "AWS" | ||
| if region, rErr := common.GetRegionFromInfrastructure(ctx); rErr == nil { | ||
| cfg.Region = region | ||
| } | ||
| if isExternal, tErr := common.IsExternalTopology(ctx); tErr == nil { | ||
| if isExternal { | ||
| cfg.Topology = "External (HyperShift)" | ||
| } else { | ||
| cfg.Topology = "HighlyAvailable" | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Skip the test on topologies without schedulable control-plane nodes.
The setup reads IsExternalTopology only to fill a report string. The deployment then pins pods to control-plane nodes (buildHealthserverDeployment, lines 1141-1148) and the Service targets only control-plane nodes (line 1188).
On External (HyperShift) topology the guest cluster has no control-plane nodes. The pods stay Pending and the rollout wait at lines 513-521 fails after 5 minutes with an unclear error. On single-node and arbiter topologies the control-plane-only target set does not provide the multiple healthy targets that the scenarios require.
If the topology is External, or the count of schedulable control-plane nodes is below replicas, skip the spec with an explicit message.
🛡️ Proposed gate
if isExternal, tErr := common.IsExternalTopology(ctx); tErr == nil {
if isExternal {
- cfg.Topology = "External (HyperShift)"
+ cfg.Topology = "External (HyperShift)"
+ Skip("control-plane-targeted NLB scenario requires control-plane nodes in the cluster")
} else {
cfg.Topology = "HighlyAvailable"
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines
490 - 501, Add a pre-test gate in the setup flow after determining topology and
before deployment, using the existing topology and schedulable control-plane
node information. Skip the spec with an explicit message when topology is
External (HyperShift) or when the schedulable control-plane count is less than
the scenario’s replicas, including SNO/TNF/TNA cases. Keep the existing report
population and control-plane targeting unchanged.
Source: Coding guidelines
Comment on lines
+655
to
+678
| func waitForNewPod(ctx context.Context, cs clientset.Interface, namespace, deployName, oldPodName string) string { | ||
| var newPod string | ||
| err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) { | ||
| pods, err := cs.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{ | ||
| LabelSelector: fmt.Sprintf("app=%s", deployName), | ||
| }) | ||
| if err != nil { | ||
| return false, nil | ||
| } | ||
| for i := range pods.Items { | ||
| p := &pods.Items[i] | ||
| if p.Name == oldPodName || p.DeletionTimestamp != nil { | ||
| continue | ||
| } | ||
| if p.Status.Phase == v1.PodRunning { | ||
| newPod = p.Name | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| }) | ||
| framework.ExpectNoError(err, "wait for replacement pod") | ||
| return newPod | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
waitForNewPod returns a surviving replica, not the replacement pod.
The deployment runs 3 replicas. After the target pod is deleted, the other two replicas are still Running and their names differ from oldPodName. On the first poll, the loop at lines 664-673 returns one of those existing pods.
The returned name is then assigned to tl.NewPod at line 225 and line 356. That assignment runs after computeTimeline, so it overwrites the correctly identified new pod and the report shows the wrong pod name.
Pass the knownServers set, or select the pod with the newest CreationTimestamp.
🐛 Proposed fix
-func waitForNewPod(ctx context.Context, cs clientset.Interface, namespace, deployName, oldPodName string) string {
+func waitForNewPod(ctx context.Context, cs clientset.Interface, namespace, deployName string, knownPods map[string]bool) string {
var newPod string
err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) {
pods, err := cs.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
LabelSelector: fmt.Sprintf("app=%s", deployName),
})
if err != nil {
return false, nil
}
for i := range pods.Items {
p := &pods.Items[i]
- if p.Name == oldPodName || p.DeletionTimestamp != nil {
+ if knownPods[p.Name] || p.DeletionTimestamp != nil {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func waitForNewPod(ctx context.Context, cs clientset.Interface, namespace, deployName, oldPodName string) string { | |
| var newPod string | |
| err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) { | |
| pods, err := cs.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{ | |
| LabelSelector: fmt.Sprintf("app=%s", deployName), | |
| }) | |
| if err != nil { | |
| return false, nil | |
| } | |
| for i := range pods.Items { | |
| p := &pods.Items[i] | |
| if p.Name == oldPodName || p.DeletionTimestamp != nil { | |
| continue | |
| } | |
| if p.Status.Phase == v1.PodRunning { | |
| newPod = p.Name | |
| return true, nil | |
| } | |
| } | |
| return false, nil | |
| }) | |
| framework.ExpectNoError(err, "wait for replacement pod") | |
| return newPod | |
| } | |
| func waitForNewPod(ctx context.Context, cs clientset.Interface, namespace, deployName string, knownPods map[string]bool) string { | |
| var newPod string | |
| err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) { | |
| pods, err := cs.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{ | |
| LabelSelector: fmt.Sprintf("app=%s", deployName), | |
| }) | |
| if err != nil { | |
| return false, nil | |
| } | |
| for i := range pods.Items { | |
| p := &pods.Items[i] | |
| if knownPods[p.Name] || p.DeletionTimestamp != nil { | |
| continue | |
| } | |
| if p.Status.Phase == v1.PodRunning { | |
| newPod = p.Name | |
| return true, nil | |
| } | |
| } | |
| return false, nil | |
| }) | |
| framework.ExpectNoError(err, "wait for replacement pod") | |
| return newPod | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/e2e/aws/lb_health_transition.go` around lines
655 - 678, Update waitForNewPod to identify the replacement rather than any
running replica: pass the knownServers set into the function and skip pods
already present there, or select the eligible pod with the newest
CreationTimestamp. Preserve excluding oldPodName and terminating pods, and
ensure the returned name is the newly created pod used by tl.NewPod.
Add PER-SERVER REQUEST DISTRIBUTION BY PHASE section to the report,
showing how many requests each backend (pod/ServerID) received in
each phase. Servers are annotated with their role (← TARGET for the
pod being rolled out, ← NEW for the replacement pod). This reveals
whether the NLB is correctly routing away from the unhealthy target
during Shutdown and Restart phases.
Restructure the verdict into dedicated buildVerdict55/buildVerdict52
functions that check both client-side and server-side metrics:
Scenario 5.5 verdict checks:
- [BUG] Pre-readyz requests (X-Server-State header)
- [SHUTDOWN] Requests to target pod after readyz→503
- [RESTART] Unhealthy/pre-readyz requests on target node during
Restart phase (t7.1→t9)
- [OK] No pre-readyz routing when all checks pass
Scenario 5.2 verdict shows:
- Unhealthy request count and T_route_stop
- T_route_start for recovery measurement
The per-server view makes it immediately clear whether the 2xx
requests during Shutdown/Restart phases went to healthy backends
(expected) or to the target pod (the NLB bug).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document the PER-SERVER REQUEST DISTRIBUTION BY PHASE report section and the multi-signal verdict logic (BUG/SHUTDOWN/RESTART/OK checks). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
NLB health transition E2E test framework for investigating and reproducing
OCPBUGS-86789 — NLB
routing new TCP connections to a KAS target before
/readyzreturns 200,despite other healthy targets being available.
Revalidates SPLAT-307
shutdown propagation measurements with current AWS infrastructure.
What it does
Health-controllable server (
cmd/healthserver/) — standalone Go HTTPserver with
/readyzcontrol,X-Server-Stateheaders, and admin API.Deployed as pods on control-plane nodes to match KAS topology.
Extractable health package (
e2e/aws/health/) — TG health observerwith per-poll snapshots, HTTP client with httptrace hooks and parallel
workers, TG attribute read/modify. Zero parent-path imports.
Three test scenarios (
e2e/aws/lb_health_transition.go):graceful shutdown (readyz→503 → 192s delay → pod delete → observe)
conn_term=false, draining=300sTG attributesapplied via SDK after TG creation
Timing model
Full t0–t10 timing model aligned with SPLAT-307 state machine, extended
with restart-phase timers (t7.1–t7.4). Every test reports the same metrics
for cross-scenario/cross-region comparison:
Report output
Single-block consolidated report with: environment, target, test parameters,
service/TG config dump, timing table, request statistics (2xx/4xx/5xx/errors),
per-phase breakdown (Warmup/Shutdown/Restart/Recovery with duration + counts),
chronological timeline merging test milestones with TG health events, and
TG snapshot summary.
Key findings from initial runs (us-east-1, 2026-08-06)
setup). May require different target type or higher load conditions.
conn_term=false) causes TG to useunhealthy.drainingstate instead of
unhealthyduring HC-driven transitions — confirmsv5 plan Q5 (not limited to deregistration).
detection, ~28s route start after recovery.
Infrastructure
nodeSelector+ tolerations)target-node-labelsannotation)/readyzhealth check (10s interval, threshold=2)externalTrafficPolicy: LocalTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores