CNTRLPLANE-2589: Migrate test/e2e-encryption-perf to OpenShift Tests Extension framework#926
CNTRLPLANE-2589: Migrate test/e2e-encryption-perf to OpenShift Tests Extension framework#926ropatil010 wants to merge 2 commits into
Conversation
WalkthroughAdds a serial encryption performance suite, shared helpers for database population and migration polling, and encryption-type scenario helpers for provider selection and on/off transitions. ChangesEncryption performance tests
Sequence Diagram(s)sequenceDiagram
participant TestPerfEncryption
participant populateDatabase
participant EncryptionMigrationControllerProgressing
participant runTestEncryptionPerf
TestPerfEncryption->>populateDatabase: populate access-token data
TestPerfEncryption->>EncryptionMigrationControllerProgressing: start polling for migration start
TestPerfEncryption->>runTestEncryptionPerf: run encryption scenario and capture assertion time
runTestEncryptionPerf-->>TestPerfEncryption: assertion timestamp
EncryptionMigrationControllerProgressing-->>TestPerfEncryption: migration start timestamp
TestPerfEncryption->>TestPerfEncryption: compute migration duration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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.
Inline comments:
In `@test/e2e-encryption-perf/encryption_perf.go`:
- Around line 56-67: The AssertDBPopulatedFunc closure is not validating the
errorStore parameter, allowing the test to pass even when DB loader workers
report failures. Add an assertion early in the AssertDBPopulatedFunc that checks
if errorStore contains any entries, and if it does, fail the test using
require.Fail or similar with a message that includes the collected errors so
loader failures are caught instead of silently ignored.
In `@test/library/encryption/scenarios.go`:
- Around line 46-57: The hardcoded 30 second timeout in the time.After() call
within the select statement can cause premature failures on slow clusters even
when the system is functioning correctly. Replace the fixed duration of 30 *
time.Second with a reference to the migration watcher's configured timeout value
to ensure the channel wait window is properly aligned with the watcher's actual
timeout configuration instead of using an arbitrary fixed duration.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1bbedebe-122c-4fce-a0aa-369d62586ef2
📒 Files selected for processing (6)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.gotest/library/encryption_wrappers.go
✅ Action performedFull review finished. |
This commit migrates the encryption performance tests to support both traditional go test and the OpenShift Tests Extension (OTE) framework, following the dual-mode pattern established in PR openshift#859. Changes: - Add encryption_perf.go with Ginkgo test registration - Refactor encryption_perf_test.go to call shared test function - Add test/library/encryption_wrappers.go for testing.TB compatibility - Add test/library/encryption/scenarios.go with core test implementations - Add test/library/encryption/perf_helpers.go for performance test utilities - Register encryption-perf suite in OTE main.go with ClusterStability: Disruptive The tests now use testing.TB interface for framework compatibility and context.Background() instead of t.Context() to avoid Ginkgo panics. Code review fixes: - Add errorStore assertion to fail test when DB loader workers report errors - Use waitPollTimeout instead of hardcoded 30s for migration start wait to align with the watcher timeout and prevent premature failures Suite details: - Name: openshift/cluster-authentication-operator/operator-encryption-perf/serial - Parallelism: 1 (serial execution) - ClusterStability: Disruptive (triggers API server rollouts) Test: [sig-auth] authentication operator [Encryption][Serial] TestPerfEncryptionTypeAESCBC Co-Authored-By: Rohit Patil <ropatil@redhat.com>
|
/test e2e-agnostic-upgrade |
ropatil010
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ropatil010: This pull request references CNTRLPLANE-2589 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 epic to target the "5.0.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. |
|
/assign @liouk
PASS logs for migrated case |
|
/retest |
liouk
left a comment
liouk
left a comment
There was a problem hiding this comment.
Overall, it looks like we're copying and adapting quite a lot of functions from library-go mainly because we're changing *testing.T to testing.TB. Is there a plan to eventually update these funcs in library-go as well, so that we don't have to copy code over for this change? If we're doing a global OTE migration, this should definitely be part of it (I'd even argue that we should have started there). This way we can reduce code duplication just for plumbing needs.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This commit addresses all reviewer suggestions for PR openshift#926: 1. Context Injection and Propagation: - Add context.Context parameter to Ginkgo tests - Use parent context instead of context.Background() - Ginkgo now injects context: g.It(..., func(ctx context.Context)) - Standard tests use tt.Context() - Context flows properly: Ginkgo -> test -> library functions 2. Migration Watcher Check: - Remove overly-restrictive cond.Reason == "Migrating" check - Now matches library-go behavior (only checks Status) - Prevents false failures if controller uses different reason strings 3. Remove Unnecessary Wrapper File: - Delete test/library/encryption_wrappers.go (39 lines) - Update caller to use local implementation directly - No functionality lost - wrappers were just pass-throughs 4. Improved Assertion Pattern: - Replace error creation + require.NoError() with direct assertions - Use require.True() and require.GreaterOrEqual() - More readable and follows Go testing best practices - Remove unused errors import 5. Dual-Mode Support: - Both standard Go test and Ginkgo modes work correctly - Standard: TestPerfEncryptionTypeAESCBC(tt.Context(), tt) - Ginkgo: g.It(..., func(ctx context.Context)) - Shared implementation uses testing.TB interface Changes: - Modified: 4 files - Deleted: 1 file - Net: -47 lines (simplified code) All changes verified: - Compiles in both test modes - No go vet issues - Proper context propagation - Consistent application of patterns Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/library/encryption/perf_helpers.go (1)
43-47: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the test-process clock for the value sent on
migrationStartedCh.Line 47 sends the operator condition timestamp, but
scenarios.gosubtracts it from a locally captured end timestamp. If the operator/test clocks differ, the perf duration can be negative or inflated. Send a local observation timestamp here, or compute both endpoints from the same clock source.Suggested direction
if cond.Type == "EncryptionMigrationControllerProgressing" && cond.Status == operatorv1.ConditionTrue && cond.LastTransitionTime.Time.After(testStartTime) { - t.Logf("EncryptionMigrationControllerProgressing condition observed at %v with reason %q", cond.LastTransitionTime, cond.Reason) - migrationStartedCh <- cond.LastTransitionTime.Time + observedAt := time.Now() + t.Logf("EncryptionMigrationControllerProgressing condition observed at %v with reason %q; local observation time %v", cond.LastTransitionTime, cond.Reason, observedAt) + migrationStartedCh <- observedAt return true, nil }🤖 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/library/encryption/perf_helpers.go` around lines 43 - 47, The timestamp sent on migrationStartedCh in the condition-watching helper should come from the test-process clock, not from cond.LastTransitionTime, because scenarios.go later compares it to a locally captured end time. Update the logic around the EncryptionMigrationControllerProgressing check so it logs the operator timestamp for diagnostics if needed, but sends a local observation time from the test clock on migrationStartedCh to keep both endpoints on the same clock source.
🤖 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.
Outside diff comments:
In `@test/library/encryption/perf_helpers.go`:
- Around line 43-47: The timestamp sent on migrationStartedCh in the
condition-watching helper should come from the test-process clock, not from
cond.LastTransitionTime, because scenarios.go later compares it to a locally
captured end time. Update the logic around the
EncryptionMigrationControllerProgressing check so it logs the operator timestamp
for diagnostics if needed, but sends a local observation time from the test
clock on migrationStartedCh to keep both endpoints on the same clock source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e3e9b695-94e4-499f-9a3b-dacbd0879ece
📒 Files selected for processing (4)
test/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e-encryption-perf/encryption_perf.go
- test/e2e-encryption-perf/encryption_perf_test.go
Thanks for the feedback, how ever i have copied the same contents of PR: #859 in this PR without any modifications. Initiated discussion here: https://redhat-internal.slack.com/archives/C07RDCVEYJG/p1782475409270789 |
|
@ropatil010: 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. |
3 participants
Hi Team,
Splitting the original PR: #859 into smaller once to better debug and merge the PR's easily.
PR about:
This commit migrates the encryption performance tests to support both traditional go test and the OpenShift Tests Extension (OTE) framework, following the dual-mode pattern established in PR #859.
Changes:
The tests now use testing.TB interface for framework compatibility and context.Background() instead of t.Context() to avoid Ginkgo panics.
Suite details:
Test: [sig-auth] authentication operator [Encryption][Serial] TestPerfEncryptionTypeAESCBC
Summary by CodeRabbit