PROOF: certrotation refactor (structural only)#2089
Closed
sanchezl wants to merge 2 commits into
Closed
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
WalkthroughThis PR updates Go module dependencies, refactors feature gate handling in certificate rotation controllers to pass feature gate snapshots instead of accessor objects, restructures certificate rotation controller configuration using new config types, and simplifies e2e test control flow for pod stability validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sanchezl 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
Closed
4 tasks
sanchezl
force-pushed
the
certrotation-refactor-proof-2151
branch
2 times, most recently
from
April 3, 2026 14:48
71def72 to
9c024ca
Compare
sanchezl
force-pushed
the
certrotation-refactor-proof-2151
branch
from
April 3, 2026 20:51
9c024ca to
15520e3
Compare
sanchezl
marked this pull request as ready for review
April 7, 2026 16:15
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/hold for openshift/library-go#2151 |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 139-140: Remove the temporary replace directive that points
github.com/openshift/library-go to the personal fork (the replace line in
go.mod); update the module to reference the official upstream
(github.com/openshift/library-go) and ensure any required changes are merged
into that upstream, then run go mod tidy and re-vendor modules (e.g., go mod
vendor or your repo's vendoring step) so the repository no longer depends on
github.com/sanchezl/library-go before merging.
In `@test/e2e/bound_sa_token.go`:
- Around line 182-183: The call to
testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision currently ignores
its error return; change the call to capture the error (e.g., err :=
testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t,
kubeClient.Pods(operatorclient.TargetNamespace))) and fail the test on error
(e.g., t.Fatalf or t.Fatalff with a clear message and the error) so the test
stops if stabilization times out or fails.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22fa1a8f-28fe-458f-809a-0e6fa936d382
⛔ Files ignored due to path filters (108)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.coderabbit.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/apiserverspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/prefixedclaimmapping.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimmapping.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/update.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/usernameclaimmapping.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/additionalalertmanagerconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/alertmanagercustomconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/authorizationconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/basicauth.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/certificateconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicyspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicystatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/custompkipolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/defaultcertificateconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/dropequalactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/ecdsakeyconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/hashmodactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyfulciocawithrekorrootoftrust.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicypkirootoftrust.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicypublickeyrootoftrust.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicystatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagesigstoreverificationpolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/keepequalactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/keyconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/label.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/labelmapactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/lowercaseactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/metadataconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/metadataconfigcustom.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/oauth2.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/oauth2endpointparam.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/openshiftstatemetricsconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/pki.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/pkicertificatemanagement.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/pkicertificatesubject.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/pkiprofile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/pkispec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyfulciosubject.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyidentity.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policymatchexactrepository.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policymatchremapidentity.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyrootoftrust.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusremotewriteheader.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/queueconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/relabelactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/relabelconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/remotewriteauthorization.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/remotewritespec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/replaceactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/retention.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/rsakeyconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/secretkeyselector.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/sigv4.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/tlsconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/uppercaseactionconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/clusterimagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/generated_expansion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/imagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/pki.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/clusterimagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/interface.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/pki.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/clusterimagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/expansion_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/imagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/pki.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/operatorcontrolplane/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/security/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/cert_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/keygen.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/cabundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/client_cert_rotation_controller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/signer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/target.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/v1helpers/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
go.modpkg/cmd/certregenerationcontroller/cmd.gopkg/operator/certrotationcontroller/certrotationcontroller.gopkg/operator/starter.gotest/e2e/bound_sa_token.go
Comment on lines
+139
to
+140
|
|
||
| replace github.com/openshift/library-go => github.com/sanchezl/library-go v0.0.0-20260403202735-a957edb574ab |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the upstream library-go PR is merged
gh pr view 2151 --repo openshift/library-go --json state,mergedAt,titleRepository: openshift/cluster-kube-apiserver-operator
Length of output: 183
🏁 Script executed:
#!/bin/bash
# First, let's verify the go.mod file and the replace directive
echo "=== Reading go.mod around lines 139-140 ==="
sed -n '130,145p' go.mod
echo ""
echo "=== All replace directives in go.mod ==="
grep -n "^replace " go.modRepository: openshift/cluster-kube-apiserver-operator
Length of output: 965
Remove replace directive pointing to personal fork before merging.
The replace directive for github.com/openshift/library-go points to a personal fork (github.com/sanchezl/library-go) instead of the official repository. While acceptable for a work-in-progress PR, this must be removed before the code can be merged to main. Ensure the upstream changes are merged to openshift/library-go and properly vendored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 139 - 140, Remove the temporary replace directive that
points github.com/openshift/library-go to the personal fork (the replace line in
go.mod); update the module to reference the official upstream
(github.com/openshift/library-go) and ensure any required changes are merged
into that upstream, then run go mod tidy and re-vendor modules (e.g., go mod
vendor or your repo's vendoring step) so the repository no longer depends on
github.com/sanchezl/library-go before merging.
Comment on lines
+182
to
+183
| // deletion triggers a roll-out - wait until a new version has been rolled out | ||
| testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== helper signature =="
sed -n '/func WaitForAPIServerToStabilizeOnTheSameRevision/,/}/p' \
vendor/github.com/openshift/library-go/test/library/apiserver/apiserver.go
echo
echo "== local call site =="
rg -n -C2 'WaitForAPIServerToStabilizeOnTheSameRevision\(' test/e2e/bound_sa_token.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 756
Handle the API server stabilization error.
Line 183 ignores the error return value from WaitForAPIServerToStabilizeOnTheSameRevision. If the stabilization wait times out, the test continues to run assertions against an unstabilized rollout, producing misleading and flaky test failures.
Capture and assert on the error:
Fix
- testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace))
+ err = testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace))
+ require.NoError(t, err)📝 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
| // deletion triggers a roll-out - wait until a new version has been rolled out | |
| testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) | |
| // deletion triggers a roll-out - wait until a new version has been rolled out | |
| err = testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/bound_sa_token.go` around lines 182 - 183, The call to
testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision currently ignores
its error return; change the call to capture the error (e.g., err :=
testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t,
kubeClient.Pods(operatorclient.TargetNamespace))) and fail the test on error
(e.g., t.Fatalf or t.Fatalff with a clear message and the error) so the test
stops if stabilization times out or fails.
Comment on lines
+192
to
208
| // Use a higher success threshold (15) with a longer interval (3 min) to | ||
| // ensure pods stay stable for 45 minutes (15 × 3 min). This accounts for | ||
| // delayed rollouts triggered by configmap deletion, where the operator's | ||
| // propagation delay can cause the stability check to falsely pass on the | ||
| // old revision. KAS rollouts can take 15-20 minutes each. | ||
| const ( | ||
| extendedSuccessThreshold = 15 | ||
| successInterval = 3 * time.Minute | ||
| pollInterval = 30 * time.Second | ||
| timeout = 45 * time.Minute | ||
| ) | ||
| err = libgotest.WaitForPodsToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace), "apiserver=true", extendedSuccessThreshold, successInterval, pollInterval, timeout) | ||
| require.NoError(t, err) | ||
|
|
||
| t.Log("bouncing crash-looping pods to refresh SA tokens after cleanup") | ||
| bounceCrashLoopingPods(ctx, t, kubeClient) | ||
| waitForBouncedPodsHealthy(ctx, t, kubeClient) | ||
|
|
||
| t.Log("waiting for all ClusterOperators to stabilize after cleanup rollout") | ||
| err = waitForAllClusterOperatorsStable(t, configClient, clusterStabilityTimeout) | ||
| t.Log("waiting for all ClusterOperators to stabilize") | ||
| err = waitForAllClusterOperatorsStable(t, configClient, timeout) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Keep the deferred recovery inside one timeout budget.
Lines 197-208 can already spend 90 minutes in the defer path by themselves (45m + 45m). Because this test does more work before reaching the defer, the outer [Timeout:90m] can fire before recovery finishes.
Proposed fix
const (
extendedSuccessThreshold = 15
successInterval = 3 * time.Minute
pollInterval = 30 * time.Second
timeout = 45 * time.Minute
)
+ cleanupStart := time.Now()
err = libgotest.WaitForPodsToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace), "apiserver=true", extendedSuccessThreshold, successInterval, pollInterval, timeout)
require.NoError(t, err)
t.Log("waiting for all ClusterOperators to stabilize")
- err = waitForAllClusterOperatorsStable(t, configClient, timeout)
+ remaining := timeout - time.Since(cleanupStart)
+ if remaining < 0 {
+ remaining = 0
+ }
+ err = waitForAllClusterOperatorsStable(t, configClient, remaining)
require.NoError(t, err)📝 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
| // Use a higher success threshold (15) with a longer interval (3 min) to | |
| // ensure pods stay stable for 45 minutes (15 × 3 min). This accounts for | |
| // delayed rollouts triggered by configmap deletion, where the operator's | |
| // propagation delay can cause the stability check to falsely pass on the | |
| // old revision. KAS rollouts can take 15-20 minutes each. | |
| const ( | |
| extendedSuccessThreshold = 15 | |
| successInterval = 3 * time.Minute | |
| pollInterval = 30 * time.Second | |
| timeout = 45 * time.Minute | |
| ) | |
| err = libgotest.WaitForPodsToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace), "apiserver=true", extendedSuccessThreshold, successInterval, pollInterval, timeout) | |
| require.NoError(t, err) | |
| t.Log("bouncing crash-looping pods to refresh SA tokens after cleanup") | |
| bounceCrashLoopingPods(ctx, t, kubeClient) | |
| waitForBouncedPodsHealthy(ctx, t, kubeClient) | |
| t.Log("waiting for all ClusterOperators to stabilize after cleanup rollout") | |
| err = waitForAllClusterOperatorsStable(t, configClient, clusterStabilityTimeout) | |
| t.Log("waiting for all ClusterOperators to stabilize") | |
| err = waitForAllClusterOperatorsStable(t, configClient, timeout) | |
| require.NoError(t, err) | |
| // Use a higher success threshold (15) with a longer interval (3 min) to | |
| // ensure pods stay stable for 45 minutes (15 × 3 min). This accounts for | |
| // delayed rollouts triggered by configmap deletion, where the operator's | |
| // propagation delay can cause the stability check to falsely pass on the | |
| // old revision. KAS rollouts can take 15-20 minutes each. | |
| const ( | |
| extendedSuccessThreshold = 15 | |
| successInterval = 3 * time.Minute | |
| pollInterval = 30 * time.Second | |
| timeout = 45 * time.Minute | |
| ) | |
| cleanupStart := time.Now() | |
| err = libgotest.WaitForPodsToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace), "apiserver=true", extendedSuccessThreshold, successInterval, pollInterval, timeout) | |
| require.NoError(t, err) | |
| t.Log("waiting for all ClusterOperators to stabilize") | |
| remaining := timeout - time.Since(cleanupStart) | |
| if remaining < 0 { | |
| remaining = 0 | |
| } | |
| err = waitForAllClusterOperatorsStable(t, configClient, remaining) | |
| require.NoError(t, err) |
Contributor
|
/retest-required |
Contributor
|
/test e2e-gcp-operator-serial-ote |
1 similar comment
Contributor
|
/test e2e-gcp-operator-serial-ote |
Contributor
|
@sanchezl: 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. |
Contributor
Author
|
Closing — proof for library-go#2151 (structural refactor). Went with library-go#2145 + CKAO#2051 instead due to how CEO uses the certrotation package. |
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
2 participants
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
For the full PKI integration proof, see #2087 (vendors openshift/library-go#2152).
Test plan
go build ./...passes