OCPBUGS-98465: add diagnostic logging to pull secret propagation test#8991
OCPBUGS-98465: add diagnostic logging to pull secret propagation test#8991jparrill wants to merge 1 commit into
Conversation
…n test This is NOT a fix — we don't have enough data to determine the root cause yet. The test currently swallows all errors silently, making it impossible to triage failures. Add stage-by-stage logging to the pull secret propagation Eventually loops so the next failure reveals WHERE the propagation stalls: - Stage 1: whether HO synced the Secret to the CP namespace - Stage 2: whether guestClient.Get() fails (KAS unreachable) or the Secret exists but lacks the expected entry (HCCO not reconciled) Also increase timeout from 150s to 5min defensively — the single observed failure (1/14 runs, Jul 9) showed KAS i/o timeouts on the guest cluster, suggesting the propagation may have succeeded but the test couldn't verify it due to an unreachable API server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
/test e2e-aws |
📝 WalkthroughWalkthroughThe end-to-end pull-secret propagation test now verifies reconciliation in three stages: the control-plane namespace, the guest Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/testing
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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
|
/hold until we find the underneath issue which is causing the flake. |
openshift-ci
Bot
added
the
do-not-merge/hold
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98465, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/util/util.go (2)
2083-2083: 🧹 Nitpick | 🔵 TrivialWorst-case runtime for this subtest jumps meaningfully.
Combined, the two sequential
Eventuallycalls can now take up to 5 minutes each (10 minutes total) versus the prior 150s-based timeouts, on top of the rest of the already-longEnsureGlobalPullSecretflow. Since this change is explicitly diagnostic-only (per PR objective) and not a fix for the underlying flake, worth confirming the team is comfortable with the added CI time budget this could add on every run of this e2e test, especially if it needs to hit the full timeout repeatedly across retries.Also applies to: 2094-2094
🤖 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 `@test/e2e/util/util.go` at line 2083, Reduce the timeouts for the sequential Eventually calls in EnsureGlobalPullSecret, including the assertion for openshift-config/pull-secret propagation, so their combined worst-case duration does not substantially increase the e2e test runtime. Preserve the diagnostic behavior and existing polling interval while restoring the prior overall timeout budget.
2061-2094: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated poll-and-check pattern into a helper.
The
Get+bytes.Contains(..., []byte("e2e-dummy.example.com"))pattern is repeated three times (CP namespace secret, guestopenshift-config/pull-secret, guestkube-system/original-pull-secret) with the dummy marker string duplicated as a literal in each spot. A small shared helper would reduce duplication and centralize the marker string, lowering the risk of a typo diverging one check from the others.♻️ Suggested helper extraction
const dummyPullSecretAuthKey = "e2e-dummy.example.com" func hasDummyEntry(data []byte) bool { return bytes.Contains(data, []byte(dummyPullSecretAuthKey)) }Then reuse
hasDummyEntry(...)at lines 2072, 2082, and 2094, and referencedummyPullSecretAuthKeyat line 2053.🤖 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 `@test/e2e/util/util.go` around lines 2061 - 2094, Extract the repeated dummy pull-secret check into a shared helper near the surrounding test utilities, centralizing the marker in a named constant. Update the CP namespace check and both guest secret checks in the existing Eventually callbacks to call the helper, and use the constant wherever the dummy marker is logged or referenced, including the setup at the earlier marker location.
🤖 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.
Nitpick comments:
In `@test/e2e/util/util.go`:
- Line 2083: Reduce the timeouts for the sequential Eventually calls in
EnsureGlobalPullSecret, including the assertion for openshift-config/pull-secret
propagation, so their combined worst-case duration does not substantially
increase the e2e test runtime. Preserve the diagnostic behavior and existing
polling interval while restoring the prior overall timeout budget.
- Around line 2061-2094: Extract the repeated dummy pull-secret check into a
shared helper near the surrounding test utilities, centralizing the marker in a
named constant. Update the CP namespace check and both guest secret checks in
the existing Eventually callbacks to call the helper, and use the constant
wherever the dummy marker is logged or referenced, including the setup at the
earlier marker location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 65f3f336-80b8-4a76-a1f5-e55328cd8c35
📒 Files selected for processing (1)
test/e2e/util/util.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8991 +/- ##
=======================================
Coverage 43.79% 43.79%
=======================================
Files 772 772
Lines 96037 96045 +8
=======================================
+ Hits 42061 42067 +6
- Misses 51061 51063 +2
Partials 2915 2915 see 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Test Resultse2e-aws
Failed TestsTotal failed tests: 1
|
|
/test e2e-aws |
|
Trying to reproduce the propagation flake issue... |
|
@jparrill: The following test 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. |
|
I have all the evidence. The analysis is clear. Here is the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe sole failing test Root CauseAWS EC2 API rate-limiting (RequestLimitExceeded) on the shared CI account. The HyperShift e2e suite runs ~20 tests in parallel, each creating its own HostedCluster with dedicated AWS infrastructure (VPCs, subnets, endpoints, etc.). At the moment The infrastructure creation sequence for This failure is not related to PR #8991. The PR modifies only Recommendations
Evidence
|
3 participants
Summary
Add diagnostic logging to the
EnsureGlobalPullSecretpull secret propagation test. This is NOT a fix — we don't have sufficient evidence to determine the root cause yet.Fixes
Why this PR exists
The pull secret propagation test fails ~1/14 runs but provides zero diagnostic information when it does. The
Eventuallyloops silently swallow all errors, making it impossible to distinguish between:guestClient.Get()fails because the guest API server is downEvidence from the single observed failure (build 2075336365863604224, Jul 9) shows KAS
i/o timeouterrors on the same guest cluster during the test window, suggesting scenario 3 — but we can't confirm without diagnostic data.What this PR changes
guestClient.Get()error instead of silently returningfalseopenshift-config/pull-secretandkube-system/original-pull-secretwaitsWhat happens next
Once this merges, we wait for the test to fail again. The logs will tell us exactly where the propagation stalls, and we'll file the real fix.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests