You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test: gate sample-app DNS wait on ingress readiness (ARO-29937) - #7670
Gate the sample-app DNS wait on two readiness checks: the ingress ClusterOperator reporting Available=True (waitForClusterOperatorCondition), and openshift-ingress/router-default reporting a load-balancer entry with an assigned external IP (waitForRouterDefaultExternalIP). Both gates poll through the package's shared pollUntilReady, so they get its delta logging, elapsed reporting, and observation preservation across the deadline race.
On gate failure, name the actual blocker instead of surfacing it as DNS: the operator condition (reason/message), the missing external IP on timeout, the caller's context error reported distinctly as a cancellation rather than a timeout, and the last API error preserved when no condition or Service state was ever observed. The router gate also dumps the router-default diagnostics (Service, events, ingress operator conditions) when it times out, via pollUntilReady's diagnose hook.
Classify every failed DNS poll in WaitForDNSResolution (not-found vs. timeout/transient vs. other) and include the tally in the timeout error, so a timeout distinguishes "record not published yet" from "resolver not answering".
Why
This PR is to improve DNS/ingress readiness to prevent the verifySimpleWebApp from starting DNS resolution before published ingress is available. As of 10/27 this issue is now in INT and Prod.
During a Control Plane and Node Pool install the guest cluster apps route DNS never resolves (10m timeout).
This is due to verifySimpleWebApp starts its DNS wait right after deploying the app, but the DNS record is only published once ingress is Available and router-default has a load balancer IP.
Additional details below:
The teste2e-parallel presubmits fail ~1.5% of DEV runs with DNS for route host ... did not resolve within 10m0s (lookup ... on 172.30.0.10:53: i/o timeout). verifySimpleWebApp starts its DNS wait right after deploying the app, but the DNS record is only published once ingress is Available and router-default has a load balancer IP — a forming ingress therefore burns the whole DNS budget and the failure points at DNS instead of ingress readiness. Under the subscription-level ARM throttling caused by synchronized cluster creations, the guest cloud-controller-manager can take ~20 minutes to ensure the router-default load balancer (measured 20m03s from Service creation to "Ensured load balancer" in run 2104551230880616448).
Each readiness gate uses ingressReadinessTimeout = 25m — 2.5× the 10m DNS budget, sized above the 20m03s throttled LB-ensure — and polls at verifiers.DefaultPollInterval with a per-request 10s client timeout and a nil-config guard, so a single hung GET cannot consume the whole gate. The three waits run sequentially, so the verifier's worst-case wall clock grows 10m → ~60m, only on runs already failing; on healthy runs both gates pass on the first poll.
Testing
Unit tests for both gates via httptest fake API servers — happy path with exact request-path assertions, unavailable operator, missing condition, persistent API errors, interrupted requests preserving the last observation, gate timeout collecting router-default diagnostics, the gate clients' always-allow rate limiter, nil config, invalid interval (timeout and cancellation-between-polls pairing are inherited from pollUntilReady and pinned by poll_test.go), and caller cancellation — and for the DNS poll-outcome tally including nonzero, fully-classified poll counts.
Unit tests
Integration tests — not applicable: no test-integration surface is touched.
E2E tests — no new spec; the changed verifier runs as part of the existing sample-app specs in e2e-parallel presubmits.
gofmt, go build ./..., go vet ./... (including -tags E2Etests), go test ./util/framework/ ./util/verifiers/... (including -race), and golangci-lint on both changed packages pass.
Special notes for your reviewer
Cluster-creation start pacing is deliberately out of scope and ships as a separate PR (per review feedback and CONTRIBUTING.md scope guidance): this PR no longer changes creation-start timing, and the default-on pacing policy gets its own CI validation and sign-off there.
The unreachable in-cluster DNS (possibly ARO-30017) is still under investigation — this PR improves failure attribution, not the root cause; a run where ingress is ready and the resolver is still unreachable will now say so.
Screenshots included (if dashboards or other UI changes) — not applicable, test-framework only
Self-reviewed the diff
CI/CD checks are passing (ignore Tide) — e2e-parallel pending on the split revision (the run on the pre-split head failed in environment provisioning before any test executed)
If new E2E use case is covered (via a new test or new check/verifier), demonstrate that the test is able to detect a defect/error and fail with proper error message and logs which communicates nature of the problem — the readiness gates are new verifier checks whose unit tests assert the failure surfaces (operator condition reason/message, missing external IP, cancellation abort, persistent API error)
I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.
Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
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 kubernetes-sigs/prow repository.
time.RFC3339 formatting drops fractional seconds, but the parser accepts subsecond windows. With 1/500ms, a start at 12:00:00.900 is stored as 12:00:00; an immediate second call treats it as expired and admits another start without waiting. Record timestamps with time.RFC3339Nano to preserve the configured window. The existing RFC3339 parser accepts fractional seconds. Add a regression case with a fractional-second start time.
Use absolute DNS name to prevent search-suffix expansion
test/util/framework/dns_test.go:182
Without a trailing dot, LookupHost can try configured DNS search suffixes. If a search domain has a wildcard record, aro-29937-reserved.invalid.<search-domain> can resolve, making this timeout test fail even though .invalid itself does not resolve. Use an absolute DNS name to prevent search-suffix expansion.
Pacing only this creation path leaves BeginCreateHCPCluster20240610 unpaced: it calls the SDK directly and is used for two starts in clusters_sharing_resgroup.go and another in admin_credential_lifecycle.go. Those starts are neither delayed nor recorded, so they can exceed the configured limit while the shared window is full. Apply pacing to the asynchronous helper too, and move admission ahead of any caller-created provisioning timeout so waiting does not consume that budget.
…efault interval (ARO-29937)
Review follow-ups on PR Azure#7670:
- Pace HCP cluster-creation starts in BeginCreateHCPCluster20240610, which
called the SDK directly and let three e2e starts bypass the pacer entirely;
admission moves ahead of caller-armed provisioning timeouts via the exported
framework.PaceHCPClusterCreateStart.
- Bound pacer lock acquisition (in-process mutex and cross-process flock) with
non-blocking try-locks retried on a 250ms cadence: a stalled lock holder can
neither stall a creation past maxClusterCreatePacingWait nor swallow context
cancellation, and the pacing deadline is now shared with the admission wait.
- Record pacing starts as RFC3339Nano so a start at a fractional second keeps
its precision; parsing already accepts fractional seconds, and stale pure
RFC3339 records stay readable.
- Drop stared entries in the process-global pacedClusterCreates memo via
t.Cleanup so repeated test execution (-count=2) does not bypass admission,
with a regression test that a fractional-second start is not truncated.
- Make the DNS timeout-test hostname absolute (*.invalid.) so a wildcard DNS
search suffix cannot satisfy the lookup.
- Poll the ingress readiness gates at verifiers.DefaultPollInterval per the
verifier polling convention instead of a hardcoded 30s interval.
The cache marks a cluster as paced before admission completes, so a concurrent call for the same name can start creation while the first call is still waiting. If the first wait is cancelled, a later retry also bypasses pacing because the failed admission remains cached. Track the in-flight result, make duplicate callers wait for it, and remove entries when admission returns an error.
Default pacing batches admissions instead of providing smooth refill
The default does not provide the advertised initial burst of 12 followed by roughly one start every 30 seconds. With 58 queued creations, this rule admits batches at 0, 6, 12, 18, and 24 minutes as timestamps expire together, retaining synchronized bring-up waves. Resolve the scheduling contract: if smooth pacing is intended, persist refill/next-admission state and add a backlog test with max > 1; otherwise, correct the PR description and default-setting comment to describe batching.
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes mainly for the pacer semantics and admission memo.
The ingress/DNS readiness direction looks right, but the error handling and edge-case tests should be tightened. Non-blocking note: the 20m gate is just below the 20m03s measured recovery cited in the comment; consider 25m or a combined readiness poll.
Local focused tests, build, vet, and gofmt pass. Relevant Prow jobs were still pending when reviewed.
… errors (ARO-29937)
Address PR Azure#7670 review feedback:
- Pacing refill smoothing: the sliding window alone re-admitted the next
batch of max starts when the initial burst aged out together, so the
documented "burst of 12, then one per 30s" contract did not hold. Persist
a NextAdmission timestamp that, once the window has filled, spaces
starts at least window/max apart, refilling freed capacity one start at
a time. Backlog test with max > 1 asserts the 30s spacing.
- Singleflight admission memo: pacedClusterCreates marked a name paced
before admission completed, so a concurrent same-name call started
unpaced beside the in-flight wait and a retry after cancellation
bypassed pacing entirely. Track the in-flight call, make duplicates
wait for its result (a completed result wins over a cancelled caller
context), and drop failed entries so retries re-pace. Concurrent
same-name, cancel-then-retry, and follower-cancellation tests included.
- Readiness gate error surfaces: both gates now report caller-side
cancellation as an abort (distinct from a gate timeout), preserve the
last API error when no condition or Service state was ever observed
(errors.Join with the wait error), and the operator gate gains the
per-request 10s timeout and nil-config guard the Service gate already
had. Cancellation, persistent-API-error, missing-condition, nil-config
and invalid-timing tests added for both gates, plus exact request-path
assertions; the DNS tally test now asserts a nonzero, fully classified
poll count.
- ingressReadinessTimeout 20m -> 25m: the measured throttled recovery was
20m03s, so a 20-minute gate would fail a replay of that exact run.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Review summary
Security scan: no blocking findings. No .claude//.vscode/, CI/pipeline changes, or new executable scripts. This is all test code; no production service code is modified.
The earlier feedback is mostly addressed: readiness checks require an assigned IP, limits and cancellation/error reporting are explicit, asynchronous create is paced, memoization uses in-flight results and cluster identity, lock acquisition is bounded, fractional timestamps and smoothing are handled, and there is broad regression coverage. The timeout/runtime description is now consistent with the 25m + 25m + 10m sequential maximum (~60m). I consider those earlier findings resolved.
Remaining feedback
Split scope (CONTRIBUTING.md:83-85). This PR combines ingress/DNS diagnosis (readiness gates and poll tally) with a separate, default-on, cross-process cluster-creation pacing mechanism. They address related symptoms, but can be reviewed and shipped independently. Please split the pacing into a separate PR, or explain why both must land together.
Verify pacing behavior in its target CI. The ci/prow/e2e-parallel check is still pending. Since pacing changes create-start timing and is enabled by default, it is important to get that representative run green before merge. Other reported Prow checks are green; Tide is not a CI check.
Clarify early-exit max-wait semantics. The README says waits exceeding 30m fail open, but the loop checks the deadline after tryRecordStart returns. If tryRecordStart records successfully after acquiring a contended lock beyond the deadline, it returns success before the fail-open check. Consider checking the deadline before committing the admission (or document that an admission that becomes available at the deadline can still proceed paced).
The default-on pacing policy and its estimated suite impact are described, but should get explicit CI-owner sign-off. The PR otherwise follows the title, rationale, tracking-ticket, and targeted-reviewer guidance in CONTRIBUTING.md. The remaining open review threads should be resolved or explicitly answered before merge.
I'm going to check the PR, but let me provide early feedback:
We have a PR template in the project, which this PR ignores. Having a single template helps us to synchronize on assumptions, and helps the review understand the scope and reasoning behind the PR. Could we rephrase the PR accordingly? Moreover the "why" section doesn't actually provide good reasoning for all the changes, but rather explain the issue.
I don't see any unit tests in the PR. Could we make sure we add unit tests and change the design so that the code unit testable as needed?
Per review feedback on Azure#7670 (CONTRIBUTING.md scope guidance), the
default-on, cross-process cluster-creation pacing mechanism is reviewable
and shippable independently of the ingress/DNS readiness gates it was
diagnosed alongside. It now lives in its own PR; this branch keeps only
the readiness gates and the DNS poll tally.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Polling can mask the caller’s cancellation or deadline error after an unavailable observation, preventing errors.Is from recognizing context termination.
net.DNSError can have IsTimeout=true with IsTemporary=false; its Temporary() method still returns true in that case. This branch counts such a timeout as other, so the diagnostic can report zero timeout/transient failures for a timed-out lookup. Check IsTimeout as well, and add a timeout-only case to TestDNSPollTallyRecord.
Security scan: no blocking findings. No .claude//.vscode/, CI/pipeline changes, or new executable scripts. This is all test code; no production service code is modified.
The earlier feedback is mostly addressed: readiness checks require an assigned IP, limits and cancellation/error reporting are explicit, asynchronous create is paced, memoization uses in-flight results and cluster identity, lock acquisition is bounded, fractional timestamps and smoothing are handled, and there is broad regression coverage. The timeout/runtime description is now consistent with the 25m + 25m + 10m sequential maximum (~60m). I consider those earlier findings resolved.
Remaining feedback
Split scope (CONTRIBUTING.md:83-85). This PR combines ingress/DNS diagnosis (readiness gates and poll tally) with a separate, default-on, cross-process cluster-creation pacing mechanism. They address related symptoms, but can be reviewed and shipped independently. Please split the pacing into a separate PR, or explain why both must land together.
Verify pacing behavior in its target CI. The ci/prow/e2e-parallel check is still pending. Since pacing changes create-start timing and is enabled by default, it is important to get that representative run green before merge. Other reported Prow checks are green; Tide is not a CI check.
Clarify early-exit max-wait semantics. The README says waits exceeding 30m fail open, but the loop checks the deadline after tryRecordStart returns. If tryRecordStart records successfully after acquiring a contended lock beyond the deadline, it returns success before the fail-open check. Consider checking the deadline before committing the admission (or document that an admission that becomes available at the deadline can still proceed paced).
The default-on pacing policy and its estimated suite impact are described, but should get explicit CI-owner sign-off. The PR otherwise follows the title, rationale, tracking-ticket, and targeted-reviewer guidance in CONTRIBUTING.md. The remaining open review threads should be resolved or explicitly answered before merge.
e2e-parallel presubmits start dozens of cluster-creating specs within
seconds of each other; each reaches its ARM-heavy phase roughly ten
minutes later, and the synchronized wave saturates the subscription-level
ARM read quota. Under that throttling the guest cloud-controller-manager
cannot ensure the router-default load balancer for ~20 minutes
(ARO-29937: measured 20m03s from Service creation to "Ensured load
balancer"), which the sample-app verifier then misreports as a DNS
failure.
Pace HCP cluster-creation starts with ARO_HCP_E2E_CLUSTER_CREATE_PACING
("12/6m" default): a shared sliding window in an ARTIFACT_DIR state file
limits starts to max per trailing window across every spec subprocess of
the run, and a persisted next-admission timestamp spaces starts at least
window/max apart once the window has filled, so the capacity the initial
burst frees refills one start at a time instead of re-forming the wave.
The trailing window stays the hard cap.
The pacer fails open — it is storm mitigation, never a correctness gate:
state-file trouble, a lock that does not become available within the
pacing deadline, or an admission wait exceeding maxClusterCreatePacingWait
(30m) all proceed unpaced, with the deadline checked before each
admission attempt so no start is ever recorded past the bound. Only
context cancellation while waiting aborts the creation. Pacer lock
acquisition (in-process mutex and cross-process flock) is non-blocking
and deadline-bounded, so a stalled holder can neither stall a creation
nor swallow cancellation.
Admission is memoized per resource group and cluster name
singleflight-style: concurrent paces of the same cluster wait for the
in-flight admission, and a failed admission is dropped so a retry
re-paces. Pacing covers the synchronous CreateHCPClusterFromParam and
CreateHCPClusterAndWait helpers and the asynchronous
BeginCreateHCPCluster helpers; framework.PaceHCPClusterCreateStart lets
callers admit a start before arming their own provisioning timeout.
Split out of Azure#7670 per review feedback (CONTRIBUTING.md scope guidance):
the ingress/DNS readiness gates remain there; this change is the
default-on pacing policy and should get its own CI validation and
sign-off.
Unit tests cover parsing, admission, pruning, smoothing, the fail-open
bound (including capacity freeing past the deadline), lock contention
(mutex and flock), cancellation, and the memo's singleflight semantics,
run with -count=2 -race.
ARO-29937
Co-Authored-By: Claude Code <noreply@anthropic.com>
This pull-request has been approved by: moisescassidy Once this PR has been reviewed and has the lgtm label, please ask for approval from mbukatov. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Details
Needs approval from an approver in each of these files:
e2e-parallel presubmits fail ~1.5% of DEV runs with "DNS for route host ...
did not resolve within 10m0s": verifySimpleWebApp starts its DNS wait right
after deploying the app, but the DNS record is only published once ingress
is Available and router-default has a load balancer IP. Under the
subscription-level ARM throttling caused by synchronized cluster creations
the guest cloud-controller-manager can take ~20 minutes to ensure the
router-default load balancer (measured 20m03s in ARO-29937), so a forming
ingress burns the whole DNS budget and the failure points at DNS instead of
ingress readiness.
- Gate the DNS wait on the ingress ClusterOperator reporting Available=True
and on openshift-ingress/router-default reporting a load-balancer entry
with an assigned external IP, so an ingress with entries but no IP yet
also keeps waiting. Both gates poll through the package's shared
pollUntilReady (delta logging, elapsed reporting, cancellation-vs-timeout
split, observation preservation across the deadline race), each with a 25m
ingressReadinessTimeout sized above the 20m03s throttled LB-ensure, the
default poll interval, a per-request 10s client timeout with a nil-config
guard, and an always-allow rate limiter so a deadline-adjacent request
cannot mask the last observation. On gate failure the verifier reports
the actual blocker - the operator condition (reason/message), the missing
external IP on timeout, the caller's context error reported as a
cancellation, and the last API error preserved via errors.Join when no
condition or Service state was ever observed - and a gate timeout dumps
the router-default diagnostics (Service, events, ingress operator
conditions) via pollUntilReady's diagnose hook.
- Tally DNS poll outcomes in WaitForDNSResolution (not-found vs.
timeout/transient vs. other) and include the tally in the timeout error,
so a timeout distinguishes "record not published yet" from "resolver not
answering".
- Keep the caller's cancellation identifiable when the context ends between
polls: an observation recorded while the context was still live is paired
with the context's error on the cancelled exit (the same pairing the
in-request interruption path uses), so errors.Is(err, ctx.Err()) keeps
holding, while an error observed after the context had ended is retained
alone so a genuine API failure is not re-classified as a cancellation.
- Unit tests via httptest fake API servers for both gates (happy path with
exact request-path assertions, unavailable operator, missing condition,
persistent API errors, nil config, invalid interval, interrupted requests
preserving the last observation, gate timeout collecting router-default
diagnostics, the gate clients' always-allow limiter, caller cancellation)
and for the DNS poll-outcome tally including nonzero, fully-classified
poll counts.
Cluster-creation start pacing is deliberately out of scope and ships as a
separate change (per review feedback and CONTRIBUTING.md scope guidance).
Link: https://redhat.atlassian.net/browse/ARO-29937
Co-Authored-By: Claude Code <noreply@anthropic.com>
Moises Cassidy (@moisescassidy): The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
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 kubernetes-sigs/prow repository. I understand the commands that are listed here.
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
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.
openshift-ci
Bot
added
area/test
Garrick Dabbs (garrickdabbs)
left a comment
Anthony Vollmer (avollmer-redhat)
left a comment
ARO-29937
What
ingressClusterOperator reportingAvailable=True(waitForClusterOperatorCondition), andopenshift-ingress/router-defaultreporting a load-balancer entry with an assigned external IP (waitForRouterDefaultExternalIP). Both gates poll through the package's sharedpollUntilReady, so they get its delta logging, elapsed reporting, and observation preservation across the deadline race.pollUntilReady's diagnose hook.WaitForDNSResolution(not-found vs. timeout/transient vs. other) and include the tally in the timeout error, so a timeout distinguishes "record not published yet" from "resolver not answering".Why
This PR is to improve DNS/ingress readiness to prevent the
verifySimpleWebAppfrom starting DNS resolution before published ingress is available. As of 10/27 this issue is now in INT and Prod.During a Control Plane and Node Pool install the guest cluster apps route DNS never resolves (10m timeout).
This is due to
verifySimpleWebAppstarts its DNS wait right after deploying the app, but the DNS record is only published once ingress isAvailableandrouter-defaulthas a load balancer IP.Additional details below:
The test
e2e-parallelpresubmits fail ~1.5% of DEV runs withDNS for route host ... did not resolve within 10m0s (lookup ... on 172.30.0.10:53: i/o timeout).verifySimpleWebAppstarts its DNS wait right after deploying the app, but the DNS record is only published once ingress isAvailableandrouter-defaulthas a load balancer IP — a forming ingress therefore burns the whole DNS budget and the failure points at DNS instead of ingress readiness. Under the subscription-level ARM throttling caused by synchronized cluster creations, the guest cloud-controller-manager can take ~20 minutes to ensure the router-default load balancer (measured 20m03s from Service creation to "Ensured load balancer" in run 2104551230880616448).Each readiness gate uses
ingressReadinessTimeout= 25m — 2.5× the 10m DNS budget, sized above the 20m03s throttled LB-ensure — and polls atverifiers.DefaultPollIntervalwith a per-request 10s client timeout and a nil-config guard, so a single hung GET cannot consume the whole gate. The three waits run sequentially, so the verifier's worst-case wall clock grows 10m → ~60m, only on runs already failing; on healthy runs both gates pass on the first poll.Testing
Unit tests for both gates via
httptestfake API servers — happy path with exact request-path assertions, unavailable operator, missing condition, persistent API errors, interrupted requests preserving the last observation, gate timeout collecting router-default diagnostics, the gate clients' always-allow rate limiter, nil config, invalid interval (timeout and cancellation-between-polls pairing are inherited frompollUntilReadyand pinned bypoll_test.go), and caller cancellation — and for the DNS poll-outcome tally including nonzero, fully-classified poll counts.test-integrationsurface is touched.gofmt,go build ./...,go vet ./...(including-tags E2Etests),go test ./util/framework/ ./util/verifiers/...(including-race), andgolangci-linton both changed packages pass.Special notes for your reviewer
PR Checklist
If E2E tests are included:
🤖 Generated with Claude Code