test: improve Eventually() polling diagnostics in cluster TLS endpoint test by aklymenk · Pull Request #5999 · Azure/ARO-HCP

aklymenk · 2026-07-09T16:32:09Z

What

Improves logging across all three Eventually() polling loops in cluster_tls_endpoints.go:

Ingress certificate check: tracks the last-seen issuer and error, logging only on first observation or when the value changes, plus a guaranteed final-state summary via defer (as suggested here).
API certificate check: adds the same delta-only logging (previously completely silent while polling, only logged once, on success) plus a final-state summary via defer — unlike the ingress one, this only fires when there's an actual error to report, since success is already marked by the existing interior log.
Console URL wait: converts the polled closure from bool-returning to error-returning so Gomega's own timeout message is descriptive (Expected success, but got an error: ...) instead of the generic Expected <bool>: false to be true; adds the same delta-only logging plus the same conditional final-state summary via defer; removes a dead Expect(...).NotTo(BeNil()) check that could never fail given the preceding guard clause.
All three loops now accept ctx context.Context and call .WithContext(ctx). This lets Gomega's own polling loop react to external context cancellation with a clean "Context was cancelled" failure instead of grinding through its full timeout budget regardless.

Why

Ingress-cert loop loggs the same issuer line on every poll regardless of whether anything changed (example of 'bad' output can be found here. Investigating further surfaced that the other two loops had the opposite problem: they were silent while polling, giving zero visibility into what was blocking them until either success or the final timeout. A second round of review also pointed out these loops weren't context-aware, meaning they couldn't respond to external cancellation cleanly. This PR addresses all three issues so the test's diagnostics accurately reflect what's happening during a 10-15 minute wait.

Testing

This PR modifies logging/diagnostics/cancellation behavior in an existing E2E test rather than adding new test coverage, so.

PR Checklist

openshift-ci · 2026-07-09T16:32:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aklymenk
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. 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:

test/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

This PR improves diagnostics in the cluster_tls_endpoints E2E test by adding delta-only logging during Eventually() polling and by switching a console URL wait from a bool predicate to an error predicate so timeouts are more descriptive.

Changes:

Added delta-only logging for API certificate polling so stuck polling surfaces the current blocking error.
Converted the console URL wait to Eventually(func() error) with delta-only logging and a clearer Gomega timeout message.
Reduced ingress certificate polling log spam by logging issuer changes only, and added a deferred final-state summary log.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

aklymenk · 2026-07-09T21:35:21Z

/test ?

aklymenk · 2026-07-09T21:36:10Z

/test integration-e2e-parallel

…t test The ingress-cert Eventually() block logged the same issuer line on every poll regardless of whether anything changed. The API-cert and console-URL Eventually() blocks had the opposite problem: completely silent while polling, giving zero visibility if they got stuck. - Ingress cert check: track last-seen issuer and error, log only on first observation or change, plus a guaranteed final-state summary via defer. - API cert check: same delta-only logging, added for visibility that didn't exist before. - Console URL wait: same delta-only logging, plus converted from a bool-returning closure to error-returning so Gomega's own timeout message is now descriptive instead of "Expected <bool>: false to be true".

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


 			By("examining the server certificate returned by the default ingress when routing the console URL")
 			// Wait for the certificate to be loaded after console starts
 			consoleUrlWithPort := fmt.Sprintf("%s:%d", consoleURL, 443)


aklymenk · 2026-07-10T21:18:32Z

/test integration-e2e-parallel

openshift-ci · 2026-07-10T23:36:18Z

@aklymenk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/integration-e2e-parallel	`e81a8a9`	link	false	`/test integration-e2e-parallel`

Full PR test history. Your PR dashboard.

Details

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.

mgahagan73 · 2026-07-13T18:59:11Z

-			Eventually(func() error {
+			var lastIngressIssuer, lastIngressErr string
+			defer func() {
+				GinkgoLogr.Info("Ingress certificate final state", "issuer", lastIngressIssuer, "error", lastIngressErr)


Add a check to see if lastIngressErr is not empty here, otherwise we would be logging an empty error message we don't need. See line 166. Maybe something like:
defer func() {
kv := []any{"issuer", lastIngressIssuer}
if lastIngressErr != "" {
kv = append(kv, "error", lastIngressErr)
}
GinkgoLogr.Info("Ingress certificate final state", kv...)
}()

Copilot AI review requested due to automatic review settings July 9, 2026 16:32

openshift-ci Bot requested review from deads2k and mgahagan73 July 9, 2026 16:32

Copilot started reviewing on behalf of aklymenk July 9, 2026 16:32 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread test/e2e/cluster_tls_endpoints.go Outdated

Copilot AI review requested due to automatic review settings July 9, 2026 16:58

aklymenk force-pushed the test/tls-endpoint-eventually-logging branch from 3433754 to bd20d03 Compare July 9, 2026 16:58

Copilot started reviewing on behalf of aklymenk July 9, 2026 16:58 View session

aklymenk changed the title ~~test: improve Eventually() polling diagnostics in cluster TLS endpoin…~~ test: improve Eventually() polling diagnostics in cluster TLS endpoint test Jul 9, 2026

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread test/e2e/cluster_tls_endpoints.go Outdated

Comment thread test/e2e/cluster_tls_endpoints.go Outdated

Comment thread test/e2e/cluster_tls_endpoints.go Outdated

Comment thread test/e2e/cluster_tls_endpoints.go Outdated

Comment thread test/e2e/cluster_tls_endpoints.go

Copilot AI review requested due to automatic review settings July 9, 2026 17:19

aklymenk force-pushed the test/tls-endpoint-eventually-logging branch from bd20d03 to a4d8ef0 Compare July 9, 2026 17:19

Copilot started reviewing on behalf of aklymenk July 9, 2026 17:19 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

aklymenk marked this pull request as draft July 10, 2026 05:54

openshift-ci Bot added the do-not-merge/work-in-progress label Jul 10, 2026

aklymenk force-pushed the test/tls-endpoint-eventually-logging branch from a4d8ef0 to e81a8a9 Compare July 10, 2026 16:24

aklymenk marked this pull request as ready for review July 10, 2026 16:27

Copilot AI review requested due to automatic review settings July 10, 2026 16:27

openshift-ci Bot removed the do-not-merge/work-in-progress label Jul 10, 2026

openshift-ci Bot requested review from bennerv and miquelsi July 10, 2026 16:27

Copilot started reviewing on behalf of aklymenk July 10, 2026 16:28 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread test/e2e/cluster_tls_endpoints.go

By("examining the server certificate returned by the default ingress when routing the console URL")

// Wait for the certificate to be loaded after console starts

consoleUrlWithPort := fmt.Sprintf("%s:%d", consoleURL, 443)

mgahagan73 reviewed Jul 13, 2026

View reviewed changes

Uh oh!

Conversation

aklymenk commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Testing

PR Checklist

Uh oh!

openshift-ci Bot commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

aklymenk commented Jul 9, 2026

Uh oh!

aklymenk commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

aklymenk commented Jul 10, 2026

Uh oh!

openshift-ci Bot commented Jul 10, 2026

Uh oh!

mgahagan73 Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aklymenk commented Jul 9, 2026 •

edited

Loading

mgahagan73 Jul 13, 2026 •

edited

Loading