CNTRLPLANE-2888: Add guest cluster client to v2 TestContext#7877
CNTRLPLANE-2888: Add guest cluster client to v2 TestContext#7877csrwng wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@csrwng: This pull request references CNTRLPLANE-2888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
Skipping CI for Draft Pull Request. |
WalkthroughThe changes add Kubernetes client initialization helpers for end-to-end testing. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant TC as TestContext
participant Secret as Kubernetes API
participant Config as REST Config Builder
participant DNS as DNS Check
participant Client as Client Initializer
Test->>TC: GetGuestClient()
activate TC
rect rgba(100, 200, 255, 0.5)
Note over TC: Lazy Init (sync.Once)
end
TC->>Secret: Retrieve HostedCluster kubeconfig secret
activate Secret
Secret-->>TC: kubeconfig data
deactivate Secret
TC->>Config: Build REST config from kubeconfig
activate Config
Config-->>TC: *rest.Config
deactivate Config
rect rgba(255, 200, 100, 0.5)
Note over TC,DNS: Retry loop with polling
end
TC->>DNS: Check DNS readiness
activate DNS
DNS-->>TC: ready/not ready
deactivate DNS
TC->>Client: Create controller-runtime client
activate Client
Client-->>TC: crclient.Client
deactivate Client
TC-->>Test: Return guest cluster client
deactivate TC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
area/testing
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng 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
|
@csrwng: This pull request references CNTRLPLANE-2888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/v2/internal/test_context.go (1)
95-99: Usetc.Contextfor secret retrieval instead ofcontext.Background().Line 95 should honor suite cancellation/timeouts consistently with the rest of
GetGuestClient.Proposed change
- err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{ + err := tc.MgmtClient.Get(tc.Context, crclient.ObjectKey{ Namespace: hc.Namespace, Name: hc.Status.KubeConfig.Name, }, &secret)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/v2/internal/test_context.go` around lines 95 - 99, Replace the use of context.Background() when calling tc.MgmtClient.Get for the secret with the test context tc.Context so the secret retrieval honors suite cancellation/timeouts; update the call inside GetGuestClient (the tc.MgmtClient.Get invocation that populates the local `secret` variable using hc.Namespace and hc.Status.KubeConfig.Name) to pass tc.Context instead of context.Background().test/e2e/v2/tests/guest_cluster_test.go (1)
51-52: Use test context instead ofcontext.Background()for the guest-cluster List call.Line 51 bypasses suite cancellation/deadline propagation. Using
testCtx.Contextmakes the call terminate with the spec lifecycle.Proposed change
- err := guestClient.List(context.Background(), namespaceList, &crclient.ListOptions{}) + err := guestClient.List(testCtx.Context, namespaceList, &crclient.ListOptions{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/v2/tests/guest_cluster_test.go` around lines 51 - 52, Replace the use of context.Background() in the guest cluster list call with the test suite context so the request observes spec cancellation/deadlines: update the call to guestClient.List(context.Background(), namespaceList, &crclient.ListOptions{}) to use testCtx.Context (i.e., guestClient.List(testCtx.Context, namespaceList, &crclient.ListOptions{}) or testCtx.Context() depending on the test helper) so the List invocation uses the suite's context and terminates with the spec lifecycle; ensure you reference the same namespaceList and crclient.ListOptions arguments unchanged and run tests to confirm proper cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/util/client.go`:
- Around line 42-47: Add an explicit nil check for the incoming config at the
top of GetClientFromConfig: if config == nil return a clear error (e.g.,
fmt.Errorf("config is nil")) before calling crclient.New so callers get an
immediate, descriptive failure instead of a deeper construction error; update
error message to mention GetClientFromConfig and reference the config parameter
to aid debugging.
In `@test/e2e/v2/internal/test_context.go`:
- Around line 116-128: The retry loop using wait.PollUntilContextTimeout
currently swallows concrete errors from GetClientFromConfig and ServerVersion;
modify the anonymous polling func (the closure passed to
wait.PollUntilContextTimeout) to record the last non-nil error into a
surrounding variable (e.g., lastErr) whenever err or apiErr is non-nil, return
false with the error (or at least record it) so it’s preserved, and after
PollUntilContextTimeout returns include that lastErr in the panic message
(replace the current panic(fmt.Sprintf("failed to connect to guest cluster: %v",
err)) with one that prints the timeout plus lastErr). Ensure you reference the
existing symbols: wait.PollUntilContextTimeout, GetClientFromConfig,
discovery.NewDiscoveryClientForConfigOrDie(...).ServerVersion, guestClient, and
err/lastErr.
---
Nitpick comments:
In `@test/e2e/v2/internal/test_context.go`:
- Around line 95-99: Replace the use of context.Background() when calling
tc.MgmtClient.Get for the secret with the test context tc.Context so the secret
retrieval honors suite cancellation/timeouts; update the call inside
GetGuestClient (the tc.MgmtClient.Get invocation that populates the local
`secret` variable using hc.Namespace and hc.Status.KubeConfig.Name) to pass
tc.Context instead of context.Background().
In `@test/e2e/v2/tests/guest_cluster_test.go`:
- Around line 51-52: Replace the use of context.Background() in the guest
cluster list call with the test suite context so the request observes spec
cancellation/deadlines: update the call to
guestClient.List(context.Background(), namespaceList, &crclient.ListOptions{})
to use testCtx.Context (i.e., guestClient.List(testCtx.Context, namespaceList,
&crclient.ListOptions{}) or testCtx.Context() depending on the test helper) so
the List invocation uses the suite's context and terminates with the spec
lifecycle; ensure you reference the same namespaceList and crclient.ListOptions
arguments unchanged and run tests to confirm proper cancellation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9b80c80-c824-491d-9552-80795ac9a0ec
📒 Files selected for processing (3)
test/e2e/util/client.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/tests/guest_cluster_test.go
|
@csrwng: This pull request references CNTRLPLANE-2888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/v2/internal/test_context.go (2)
95-99: Use the test context instead ofcontext.Background()for secret fetch.At Line 95, using
context.Background()bypasses cancellation/deadline from the suite context and can prolong stuck API calls. SinceTestContextembedscontext.Context, usetcdirectly instead.Suggested patch
- err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{ + err := tc.MgmtClient.Get(tc, crclient.ObjectKey{ Namespace: hc.Namespace, Name: hc.Status.KubeConfig.Name, }, &secret)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/v2/internal/test_context.go` around lines 95 - 99, The call to tc.MgmtClient.Get uses context.Background(), which ignores the test suite's cancellation/deadline; replace context.Background() with the test context (tc) so the call uses the suite's embedded context and will honour cancellation/timeouts — update the invocation of MgmtClient.Get (where the secret is fetched) to pass tc instead of context.Background().
123-126: AvoidOrDiein the retry path.At line 123,
NewDiscoveryClientForConfigOrDiecan panic and escape the retry loop; useNewDiscoveryClientForConfiginstead to properly report the error vialastErrand continue retrying.Suggested patch
- _, apiErr := discovery.NewDiscoveryClientForConfigOrDie(restConfig).ServerVersion() + discoveryClient, discErr := discovery.NewDiscoveryClientForConfig(restConfig) + if discErr != nil { + lastErr = fmt.Errorf("build discovery client: %w", discErr) + return false, nil + } + _, apiErr := discoveryClient.ServerVersion() if apiErr != nil { lastErr = fmt.Errorf("discover guest API server version: %w", apiErr) return false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/v2/internal/test_context.go` around lines 123 - 126, The retry loop uses discovery.NewDiscoveryClientForConfigOrDie which can panic and break retries; replace that call with discovery.NewDiscoveryClientForConfig(restConfig), check its returned (client, err) and on error assign lastErr (e.g. fmt.Errorf("create discovery client: %w", err)) and return false, nil so the loop continues; if client creation succeeds, call client.ServerVersion() and keep the existing apiErr handling (assign lastErr and return false, nil) so all errors are reported via lastErr instead of aborting with a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/v2/internal/test_context.go`:
- Around line 95-99: The call to tc.MgmtClient.Get uses context.Background(),
which ignores the test suite's cancellation/deadline; replace
context.Background() with the test context (tc) so the call uses the suite's
embedded context and will honour cancellation/timeouts — update the invocation
of MgmtClient.Get (where the secret is fetched) to pass tc instead of
context.Background().
- Around line 123-126: The retry loop uses
discovery.NewDiscoveryClientForConfigOrDie which can panic and break retries;
replace that call with discovery.NewDiscoveryClientForConfig(restConfig), check
its returned (client, err) and on error assign lastErr (e.g. fmt.Errorf("create
discovery client: %w", err)) and return false, nil so the loop continues; if
client creation succeeds, call client.ServerVersion() and keep the existing
apiErr handling (assign lastErr and return false, nil) so all errors are
reported via lastErr instead of aborting with a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e73aae54-824e-4dc6-8d18-87eea3a87cbb
📒 Files selected for processing (2)
test/e2e/util/client.gotest/e2e/v2/internal/test_context.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/util/client.go
Add GetGuestClient() to TestContext with sync.Once lazy-loading, kubeconfig retrieval from HostedCluster status, and retry-based connectivity checks. Add GetClientFromConfig() helper to e2eutil and refactor GetClient() to use it. Fixes: CNTRLPLANE-2888 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
csrwng
force-pushed
the
guest-cluster-client
branch
from
a3cac11 to
55b04dc
Compare
|
@csrwng: This pull request references CNTRLPLANE-2888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
@csrwng: This pull request references CNTRLPLANE-2888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/v2/internal/test_context.go`:
- Around line 90-98: The precondition check in test_context.go conflates a nil
KubeConfig with a missing secret name; update the guard around hc and
hc.Status.KubeConfig (in the code surrounding variable hc and the call to
tc.MgmtClient.Get) to explicitly check and fail fast with distinct messages:
first panic if hc == nil, then panic if hc.Status.KubeConfig == nil, then panic
if hc.Status.KubeConfig.Name == "" (empty string) before attempting the secret
lookup via tc.MgmtClient.Get; ensure the panic/log messages clearly mention
which condition failed (nil HostedCluster, nil KubeConfig, or empty
KubeConfig.Name) to make triage easier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e459a2ef-0167-4da2-bae7-ea1cf726ca52
📒 Files selected for processing (2)
test/e2e/util/client.gotest/e2e/v2/internal/test_context.go
| if hc == nil || hc.Status.KubeConfig == nil { | ||
| panic("HostedCluster has no kubeconfig in status") | ||
| } | ||
|
|
||
| var secret corev1.Secret | ||
| err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{ | ||
| Namespace: hc.Namespace, | ||
| Name: hc.Status.KubeConfig.Name, | ||
| }, &secret) |
There was a problem hiding this comment.
Tighten precondition checks before secret lookup.
The current guard conflates different failures and does not validate an empty kubeconfig secret name, which makes triage harder when this fails.
Suggested change
hc := tc.GetHostedCluster()
- if hc == nil || hc.Status.KubeConfig == nil {
- panic("HostedCluster has no kubeconfig in status")
+ if hc == nil {
+ panic("HostedCluster is not available in test context")
+ }
+ if hc.Status.KubeConfig == nil || hc.Status.KubeConfig.Name == "" {
+ panic("HostedCluster status kubeconfig secret reference is missing")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/v2/internal/test_context.go` around lines 90 - 98, The precondition
check in test_context.go conflates a nil KubeConfig with a missing secret name;
update the guard around hc and hc.Status.KubeConfig (in the code surrounding
variable hc and the call to tc.MgmtClient.Get) to explicitly check and fail fast
with distinct messages: first panic if hc == nil, then panic if
hc.Status.KubeConfig == nil, then panic if hc.Status.KubeConfig.Name == ""
(empty string) before attempting the secret lookup via tc.MgmtClient.Get; ensure
the panic/log messages clearly mention which condition failed (nil
HostedCluster, nil KubeConfig, or empty KubeConfig.Name) to make triage easier.
2 participants
What this PR does / why we need it:
Adds a lazy-loaded guest cluster client (
GetGuestClient()) to the v2TestContextso that tests requiring access to the guest (hosted) cluster API can obtain acrclient.Clientwithout each test reimplementing the kubeconfig retrieval logic.Also adds a
GetClientFromConfig()helper toe2eutilfor creating a client from an explicit REST config, and refactorsGetClient()to use it.Key design decisions:
sync.Oncelazy-loading: Follows the existingGetHostedCluster()patternQPS: -1, Burst: -1: Matches v1 pattern — we are the only client in test, so no throttling neededGetHostedCluster()pattern — a test cannot proceed without a guest clientServerVersion()call verifies the API server is reachableWhich issue(s) this PR fixes:
Fixes CNTRLPLANE-2888
Special notes for your reviewer:
GetClientFromConfig()creates a client from a REST config while reusing the standard test scheme, with nil-config validationGetClient()was refactored to delegate toGetClientFromConfig()to reduce duplicationChecklist:
Summary by CodeRabbit