SPIRE-450: Fix e2e test nodeSelector/tolerations for OpenShift CI#95
SPIRE-450: Fix e2e test nodeSelector/tolerations for OpenShift CI#95sayak-redhat wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sayak-redhat: This pull request references SPIRE-450 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. |
WalkthroughAdded a control-plane toleration to the SPIRE Server pod spec in the end-to-end test and increased the test default timeout from 5 to 8 minutes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sayak-redhat 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 |
|
@sayak-redhat: This pull request references SPIRE-450 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. |
|
@sayak-redhat: This pull request references SPIRE-450 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: 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/e2e_test.go`:
- Line 527: Create a new context with an 8-minute timeout and pass that to
utils.WaitForStatefulSetReady instead of the existing testCtx (which uses
DefaultTimeout of 5 minutes); specifically, before calling
WaitForStatefulSetReady(create a context via context.WithTimeout or equivalent)
and use that newCtx when invoking utils.WaitForStatefulSetReady(testCtx ->
newCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace,
8*time.Minute), ensuring you cancel the context when done.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/e2e/e2e_test.go
test/e2e/e2e_test.go
Outdated
|
|
||
| By("Waiting for SPIRE Server StatefulSet to become Ready") | ||
| utils.WaitForStatefulSetReady(testCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace, utils.DefaultTimeout) | ||
| utils.WaitForStatefulSetReady(testCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace, 8*time.Minute) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check how testCtx is created in e2e test (around Line 75) ==="
sed -n '70,85p' test/e2e/e2e_test.go
echo
echo "=== 2) Find DefaultTimeout definition ==="
rg -n 'DefaultTimeout\s*=' test/e2e/
echo
echo "=== 3) Inspect WaitForStatefulSetReady implementation ==="
rg -n -A20 'func WaitForStatefulSetReady\(' test/e2e/utils/utils.go
echo
echo "=== 4) Check Line 527 and surrounding context ==="
sed -n '520,535p' test/e2e/e2e_test.goRepository: openshift/zero-trust-workload-identity-manager
Length of output: 3048
Create a dedicated context with 8-minute timeout for WaitForStatefulSetReady at Line 527.
The testCtx has a 5-minute deadline (DefaultTimeout), but this call requests an 8-minute wait. Ginkgo's Eventually respects the context's deadline, so the operation will fail at 5 minutes instead of waiting the requested 8 minutes. Create a separate context with the full 8-minute timeout:
- utils.WaitForStatefulSetReady(testCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace, 8*time.Minute)
+ waitCtx, cancel := context.WithTimeout(context.Background(), 8*time.Minute)
+ DeferCleanup(cancel)
+ utils.WaitForStatefulSetReady(waitCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace, 8*time.Minute)📝 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.
| utils.WaitForStatefulSetReady(testCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace, 8*time.Minute) | |
| waitCtx, cancel := context.WithTimeout(context.Background(), 8*time.Minute) | |
| DeferCleanup(cancel) | |
| utils.WaitForStatefulSetReady(waitCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace, 8*time.Minute) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/e2e_test.go` at line 527, Create a new context with an 8-minute
timeout and pass that to utils.WaitForStatefulSetReady instead of the existing
testCtx (which uses DefaultTimeout of 5 minutes); specifically, before calling
WaitForStatefulSetReady(create a context via context.WithTimeout or equivalent)
and use that newCtx when invoking utils.WaitForStatefulSetReady(testCtx ->
newCtx, clientset, utils.SpireServerStatefulSetName, utils.OperatorNamespace,
8*time.Minute), ensuring you cancel the context when done.
|
@sayak-redhat: This pull request references SPIRE-450 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. |
sayak-redhat
force-pushed
the
SPIRE-450
branch
from
8e9b4ef to
bee344f
Compare
|
@sayak-redhat: This pull request references SPIRE-450 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. |
|
@sayak-redhat: This pull request references SPIRE-450 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/utils/constants.go`:
- Line 46: DefaultTimeout was broadened to 8m and impacts many e2e waits; revert
DefaultTimeout back to its original value (5 * time.Minute) in constants.go, add
a new targeted exported constant (e.g., SpireServerStatefulSetReadyTimeout = 8 *
time.Minute) and replace the specific call site that needs the longer wait by
passing SpireServerStatefulSetReadyTimeout to WaitForStatefulSetReady in the e2e
test (the call referencing WaitForStatefulSetReady for the SPIRE Server
StatefulSet in e2e_test.go). Ensure no other call sites use the new constant so
only that single StatefulSet wait is extended.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/e2e/utils/constants.go
| DefaultInterval = 10 * time.Second | ||
| ShortInterval = 5 * time.Second | ||
| DefaultTimeout = 5 * time.Minute | ||
| DefaultTimeout = 8 * time.Minute |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type go 'DefaultTimeout' test/e2e -C 2Repository: openshift/zero-trust-workload-identity-manager
Length of output: 14952
Global DefaultTimeout change affects 40+ call sites across the e2e test suite, contradicting the PR's stated targeted intent.
The PR description indicates this timeout increase is scoped to a single WaitForStatefulSetReady call for the SPIRE Server StatefulSet. However, DefaultTimeout is an exported package-level constant used throughout the entire e2e test suite—affecting context timeouts, condition waits, StatefulSet readiness checks, DaemonSet availability checks, Deployment updates, and rolling update waits. This 60% timeout increase (5 → 8 minutes) globally extends waits for all e2e tests, potentially masking slow/hanging tests and inflating total CI wall-clock time.
Introduce a dedicated constant instead and use it only at the specific call site:
♻️ Proposed fix: introduce a targeted constant
DefaultInterval = 10 * time.Second
ShortInterval = 5 * time.Second
- DefaultTimeout = 8 * time.Minute
+ DefaultTimeout = 5 * time.Minute
+ SpireServerReadyTimeout = 8 * time.Minute
ShortTimeout = 2 * time.MinuteThen update the specific call site in test/e2e/e2e_test.go:
-WaitForStatefulSetReady(..., utils.DefaultTimeout)
+WaitForStatefulSetReady(..., utils.SpireServerReadyTimeout)📝 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.
| DefaultTimeout = 8 * time.Minute | |
| DefaultInterval = 10 * time.Second | |
| ShortInterval = 5 * time.Second | |
| DefaultTimeout = 5 * time.Minute | |
| SpireServerReadyTimeout = 8 * time.Minute | |
| ShortTimeout = 2 * time.Minute |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/utils/constants.go` at line 46, DefaultTimeout was broadened to 8m
and impacts many e2e waits; revert DefaultTimeout back to its original value (5
* time.Minute) in constants.go, add a new targeted exported constant (e.g.,
SpireServerStatefulSetReadyTimeout = 8 * time.Minute) and replace the specific
call site that needs the longer wait by passing
SpireServerStatefulSetReadyTimeout to WaitForStatefulSetReady in the e2e test
(the call referencing WaitForStatefulSetReady for the SPIRE Server StatefulSet
in e2e_test.go). Ensure no other call sites use the new constant so only that
single StatefulSet wait is extended.
|
@sayak-redhat: all tests passed! 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. |
|
/retest |
3 participants
Summary by CodeRabbit