CI | Adding eventually for some KMS test to avoid timing failures #1710
Conversation
WalkthroughTest suites now construct Ginkgo suite and reporter configurations, enable FailFast, and pass them to RunSpecs. Additionally, the rotate tests add a pre-test step that creates and immediately deletes a Secret derived from the NooBaa instance before running the rotation schedule tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test Runner (go test)
participant G as Ginkgo
participant S as Suite Tests
T->>G: Build suiteConfig, reporterConfig (FailFast=true)
T->>G: RunSpecs(t, "KMS ... Suite", suiteConfig, reporterConfig)
G->>S: Execute specs
alt First spec fails (FailFast)
G-->>T: Abort remaining specs
else All pass or continue
G-->>T: Complete suite
end
Note right of G: FailFast behavior is newly enabled
sequenceDiagram
autonumber
actor T as Rotate Test
participant K as Kubernetes API
participant R as Rotation Logic
T->>K: Create Secret (derived from NooBaa)
T->>K: Delete Secret (pre-cleanup)
Note over T,K: New precondition ensures clean state
T->>R: Start rotate schedule test
R-->>T: Assert rotation outcomes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (1)
pkg/util/kms/test/rotate/kms_rotate_test.go (1)
91-92: Avoid hard-coded sleep before checking KMSInitA fixed
time.Sleep(10 * time.Second)trades one flake for another: it still fails if the init takes >10 s and always slows the test even when the condition is ready immediately. Ginkgo/Gomega already give usEventually, so we can poll until the condition stabilizes while keeping a sane upper bound.- time.Sleep(10 * time.Second) - Expect(util.NooBaaCondStatus(noobaa, nbv1.ConditionKMSInit)).To(BeTrue()) + Eventually(func() bool { + return util.NooBaaCondStatus(noobaa, nbv1.ConditionKMSInit) + }).WithTimeout(2 * time.Minute).WithPolling(5 * time.Second).Should(BeTrue())
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/kms/test/rotate/kms_rotate_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: golangci-lint
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-dev-test
liranmauda
force-pushed
the
liran-add-sleep-kms-rotate-test
branch
from
c629de4 to
596fc7c
Compare
liranmauda
changed the title
liranmauda
force-pushed
the
liran-add-sleep-kms-rotate-test
branch
from
596fc7c to
3fcd17f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/util/kms/test/rotate/kms_rotate_suite_test.go (1)
25-26: Fail‑fast in suite: good for CI; consider gating for local runsEnabling fail‑fast will shorten CI feedback. Optionally gate it behind an env var (e.g., GINKGO_FAIL_FAST) so local runs can opt out.
Would you like me to add an env‑guarded toggle and unify default Eventually timeouts across suites?
pkg/util/kms/test/kmip/kms_kmip_suite_test.go (1)
25-26: Consistent fail‑fast across KMS suitesMatches rotate suite; helps reduce wasted CI time after first failure. Same optional env‑guard suggestion applies.
pkg/util/kms/test/kmip/kms_kmip_test.go (1)
77-80: Deduplicate timeout/interval configYou repeat
2*time.Minute, 5*time.Secondin all polls. Consider setting Gomega defaults once in the suite.Add this in the suite’s
BeforeSuite(or just after env check) and remove per‑call durations:// in a suite *_suite_test.go SetDefaultEventuallyTimeout(2 * time.Minute) SetDefaultEventuallyPollingInterval(5 * time.Second)Also applies to: 97-100, 115-118, 125-128, 148-151, 162-165, 167-170
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/util/kms/test/kmip/kms_kmip_suite_test.go(1 hunks)pkg/util/kms/test/kmip/kms_kmip_test.go(6 hunks)pkg/util/kms/test/rotate/kms_rotate_suite_test.go(1 hunks)pkg/util/kms/test/rotate/kms_rotate_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/util/kms/test/rotate/kms_rotate_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/util/kms/test/kmip/kms_kmip_test.go (4)
pkg/util/util.go (2)
NooBaaCondStatus(1842-1844)KubeCreateFailExisting(388-390)pkg/apis/noobaa/v1alpha1/noobaa_types.go (4)
ConditionKMSInit(527-527)ConditionKMSSync(530-530)ConditionKMSInvalid(536-536)ConditionKMSKeyRotate(533-533)pkg/admission/test/integ/admission_integ_test.go (1)
KubeDelete(329-337)pkg/util/kms/kms_kmip.go (1)
KMIPEndpoint(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-operator-tests
- GitHub Check: run-admission-test
- GitHub Check: run-cli-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
🔇 Additional comments (8)
pkg/util/kms/test/kmip/kms_kmip_test.go (8)
5-5: Import of time is appropriateNeeded for the new Eventually timeouts/intervals.
97-100: Same re‑fetch recommendation as above (Sync status)Apply the same refresh pattern before checking
ConditionKMSSync.
115-118: Same re‑fetch recommendation as above (Invalid status, bad token)Apply the same refresh pattern before checking
ConditionKMSInvalid.
125-128: Same re‑fetch recommendation as above (Invalid status, missing endpoint)Apply the same refresh pattern before checking
ConditionKMSInvalid.
148-151: Same re‑fetch recommendation as above (Init status in rotate flow)Apply the same refresh pattern before checking
ConditionKMSInit.
162-165: Same re‑fetch recommendation as above (Sync status in rotate flow)Apply the same refresh pattern before checking
ConditionKMSSync.
167-170: Same re‑fetch recommendation as above (KeyRotate status)Apply the same refresh pattern before checking
ConditionKMSKeyRotate.
77-80: No change required: util.NooBaaCondStatus already refreshes the CR each poll
NooBaaCondStatus calls NooBaaCondition, which invokes KubeGet(noobaa) on every loop iteration, fetching fresh state.Likely an incorrect or invalid review comment.
liranmauda
force-pushed
the
liran-add-sleep-kms-rotate-test
branch
from
3fcd17f to
1f0884a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/util/kms/test/kmip/kms_kmip_suite_test.go(1 hunks)pkg/util/kms/test/kmip/kms_kmip_test.go(6 hunks)pkg/util/kms/test/rotate/kms_rotate_suite_test.go(1 hunks)pkg/util/kms/test/rotate/kms_rotate_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/util/kms/test/kmip/kms_kmip_test.go (4)
pkg/util/util.go (2)
NooBaaCondStatus(1842-1844)KubeCreateFailExisting(388-390)pkg/apis/noobaa/v1alpha1/noobaa_types.go (4)
ConditionKMSInit(527-527)ConditionKMSSync(530-530)ConditionKMSInvalid(536-536)ConditionKMSKeyRotate(533-533)pkg/admission/test/integ/admission_integ_test.go (1)
KubeDelete(329-337)pkg/util/kms/kms_kmip.go (1)
KMIPEndpoint(14-14)
pkg/util/kms/test/rotate/kms_rotate_test.go (2)
pkg/util/util.go (1)
NooBaaCondStatus(1842-1844)pkg/apis/noobaa/v1alpha1/noobaa_types.go (3)
ConditionKMSInit(527-527)ConditionKMSSync(530-530)ConditionKMSKeyRotate(533-533)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-hac-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
liranmauda
force-pushed
the
liran-add-sleep-kms-rotate-test
branch
2 times, most recently
from
890de99 to
b63e24f
Compare
liranmauda
changed the title
Increasing the timeout of `NooBaaCondition` Failing some KMS tests on the first fail Signed-off-by: liranmauda <[email protected]>
liranmauda
force-pushed
the
liran-add-sleep-kms-rotate-test
branch
from
b63e24f to
868b2f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/kms/test/rotate/kms_rotate_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/util/kms/test/rotate/kms_rotate_test.go (1)
pkg/util/util.go (1)
KubeDelete(444-497)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-hac-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-token-test
| secret := &corev1.Secret{} | ||
| secret.Name = noobaa.Name + "-root-master-key-backend" | ||
| secret.Namespace = noobaa.Namespace | ||
| _ = util.KubeDelete(secret) |
There was a problem hiding this comment.
Fail fast on backend-secret cleanup
util.KubeDelete returns false when the API delete fails for reasons other than NotFound. By discarding that result we can sail past a real failure and still enter the rotation specs with a stale backend key, reintroducing the very flake we’re trying to eliminate. Please move this cleanup into a BeforeEach/JustBeforeEach block and assert on the outcome so the suite stops immediately when the delete cannot be confirmed. For example:
- secret := &corev1.Secret{}
- secret.Name = noobaa.Name + "-root-master-key-backend"
- secret.Namespace = noobaa.Namespace
- _ = util.KubeDelete(secret)
+ BeforeEach(func() {
+ backendSecret := &corev1.Secret{}
+ backendSecret.Name = noobaa.Name + "-root-master-key-backend"
+ backendSecret.Namespace = noobaa.Namespace
+ Expect(util.KubeDelete(backendSecret)).To(
+ BeTrue(),
+ "cleanup stale backend key secret before running rotation checks",
+ )
+ })📝 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.
| secret := &corev1.Secret{} | |
| secret.Name = noobaa.Name + "-root-master-key-backend" | |
| secret.Namespace = noobaa.Namespace | |
| _ = util.KubeDelete(secret) | |
| BeforeEach(func() { | |
| backendSecret := &corev1.Secret{} | |
| backendSecret.Name = noobaa.Name + "-root-master-key-backend" | |
| backendSecret.Namespace = noobaa.Namespace | |
| Expect(util.KubeDelete(backendSecret)).To( | |
| BeTrue(), | |
| "cleanup stale backend key secret before running rotation checks", | |
| ) | |
| }) |
🤖 Prompt for AI Agents
In pkg/util/kms/test/rotate/kms_rotate_test.go around lines 84-87, the call to
util.KubeDelete on the backend secret ignores its boolean result so a delete
failure (other than NotFound) is swallowed; move this cleanup into a BeforeEach
or JustBeforeEach block and assert the delete succeeded (e.g.,
require.True/Expect on util.KubeDelete returning true or fail the test with a
clear message) so the suite fails fast if the secret cannot be removed,
preventing tests from running against a stale backend key.
|
Not relevant, Closing |
1 participant
Explain the changes
running every 5 seconds for 2 minutes
Summary by CodeRabbit