SPIRE-617, OCPBUGS-90556: fix gcInterval to 10s to prevent spire-controller-manager high CPU usage - #150
Conversation
…high CPU usage The GCInterval field in ControllerManagerConfig was not being set, defaulting to Go's zero value (0s). This caused the spire-controller-manager to run garbage collection continuously without pause, consuming ~2300m CPU at idle. Set GCInterval to 10 * time.Second (the intended default) and add unit test coverage for the field. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@sujkini: This pull request references SPIRE-617 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 task to target the "5.0.0" version, but no target version was set. This pull request references Jira Issue OCPBUGS-90556, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughThe controller-manager configuration now sets ChangesSPIRE controller-manager GC interval
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant ControllerManagerConfigMap
participant Operator
participant SPIREServer
E2ETest->>ControllerManagerConfigMap: patch gcInterval to 0
ControllerManagerConfigMap->>Operator: expose invalid configuration
Operator->>ControllerManagerConfigMap: restore gcInterval to 10 seconds
E2ETest->>ControllerManagerConfigMap: delete ConfigMap
Operator->>ControllerManagerConfigMap: recreate ConfigMap with gcInterval 10 seconds
E2ETest->>SPIREServer: verify Ready=True
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sujkini 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 |
Journey 1: Verify controller-manager ConfigMap contains gcInterval=10s and SpireServer reaches Ready=True. Journey 2: Validate operator restores gcInterval after ConfigMap drift (gcInterval set to 0) and full ConfigMap deletion. Generated by OpenSpec /opsx-e2e pipeline. Co-authored-by: Cursor <cursoragent@cursor.com>
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/gc_interval_fix_test.go`:
- Around line 80-85: Update the spec containing the drift injection to capture
the original ConfigMap data before modifying it and register DeferCleanup before
the drift update and subsequent deletion. Cleanup must restore the original
ConfigMap data when it still exists, or recreate the ConfigMap with its original
metadata and data when deleted, so failures cannot leak shared state to later
specs.
- Around line 38-40: Update the BeforeAll setup for testCtx to derive it with
context.WithTimeout using an appropriate test deadline, retain the cancel
function, and invoke that cancel function during cleanup so blocked Kubernetes
Get, Update, or Delete calls terminate promptly.
- Around line 98-101: Replace the direct SpireServer fetch assertion in the
drift-recovery verification with a call to utils.WaitForSpireServerConditions,
matching the existing readiness assertion after ConfigMap recreation. Ensure the
check validates the expected Ready and ControllerManagerConfigAvailable
conditions rather than only confirming the CR exists.
🪄 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: Pro Plus
Run ID: 6d02e89f-3b2b-4785-9d87-3097a952d67c
📒 Files selected for processing (2)
test/e2e/gc_interval_fix_test.gotest/e2e/utils/constants.go
| BeforeAll(func() { | ||
| testCtx = context.Background() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'gc_interval_fix_test.go' . | sed 's#^\./##'
echo "== file outline/size =="
file="$(fd 'gc_interval_fix_test.go' . | head -n1)"
wc -l "$file"
ast-grep outline "$file" --view expanded || true
echo "== relevant slices =="
sed -n '1,140p' "$file" | nl -ba
echo "== contexts/verbs/readiness mentions =="
rg -n "context\.Background|context\.WithTimeout|Eventually|Eventuallyf|Get\(|Update\(|Delete\(|Expect\(IsReady\(|Readiness|readiness|DefaultTimeout|AfterEach|BeforeAll|BeforeEach" "$file"Repository: openshift/zero-trust-workload-identity-manager
Length of output: 535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository-wide Ginkgo setup patterns for context timeouts =="
rg -n "WithTimeout|context\.Background\(\)|cancel\(\)|AfterEach|BeforeEach|utils\.DefaultTimeout" --glob '*_test.go' --glob '*.go' | head -n 200
echo "== inspect likely helper definitions/usages =="
rg -n "DefaultTimeout|SetContext|SetDeadline|context\.WithTimeout\(" -t go . | head -n 200Repository: openshift/zero-trust-workload-identity-manager
Length of output: 342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./test/e2e/gc_interval_fix_test.go"
echo "== full target file =="
awk '{printf "%3d\t%s\n", NR, $0}' "$file"
echo "== imports/lines with context/client calls =="
sed -n '1,120p' "$file"
echo "== helper definitions for api clients =="
rg -n "ClientFor|set up|kubernetesClient|spireclient|New.*Client|ForNamespace" test/e2e test -g '*.go' | head -n 120Repository: openshift/zero-trust-workload-identity-manager
Length of output: 12840
Use a deadline-bound test context.
context.Background() cannot cancel a blocked Kubernetes request, so a stuck Get, Update, or Delete inside Eventually can run past Ginkgo timeouts. Derive testCtx from context.WithTimeout(...) and cancel it in cleanup.
🤖 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/e2e/gc_interval_fix_test.go` around lines 38 - 40, Update the BeforeAll
setup for testCtx to derive it with context.WithTimeout using an appropriate
test deadline, retain the cancel function, and invoke that cancel function
during cleanup so blocked Kubernetes Get, Update, or Delete calls terminate
promptly.
Source: Path instructions
| By("Drift injection: patching ConfigMap to set gcInterval to 0") | ||
| driftedData := strings.Replace(configData, "gcInterval: 10000000000", "gcInterval: 0", 1) | ||
| Expect(driftedData).NotTo(Equal(configData), "drift patch should have changed the data") | ||
| cm.Data[utils.SpireControllerManagerConfigKey] = driftedData | ||
| _, err = clientset.CoreV1().ConfigMaps(utils.OperatorNamespace).Update(testCtx, cm, metav1.UpdateOptions{}) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to patch ConfigMap with drifted gcInterval") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching gc_interval_fix_test.go:"
fd -a 'gc_interval_fix_test.go' . || true
file=$(fd 'gc_interval_fix_test.go' . | head -n1)
if [ -n "${file:-}" ]; then
echo
echo "Outline for $file:"
ast-grep outline "$file" --view expanded || true
echo
echo "Relevant source lines:"
sed -n '1,150p' "$file" | nl -ba
fi
echo
echo "Search for ConfigMap setup/cleanup patterns around operator namespace/config key:"
rg -n "SpireControllerManagerConfigKey|OperatorNamespace|ConfigMaps\\(|DeferCleanup|BeforeEach|AfterEach|Drift injection|gcInterval" test/e2e -S || trueRepository: openshift/zero-trust-workload-identity-manager
Length of output: 545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/gc_interval_fix_test.go"
echo "Relevant source lines:"
awk '{printf "%4d %s\n", NR, $0}' "$file" | sed -n '1,140p'
echo
echo "Search for ConfigMap setup/cleanup patterns around operator namespace/config key:"
rg -n "SpireControllerManagerConfigKey|OperatorNamespace|ConfigMaps\\(|DeferCleanup|BeforeEach|AfterEach|Drift injection|gcInterval" test/e2e -S || trueRepository: openshift/zero-trust-workload-identity-manager
Length of output: 38424
Restore shared ConfigMap state even when the spec fails.
The drift update and ConfigMap deletion mutate shared state, and this spec has no DeferCleanup/AfterEach restoration. If either mutation or later assertions fail, later specs can run with gcInterval: 0 or without the operator ConfigMap. Save the original config before the drift update and register cleanup logic before both mutations to restore/recreate it.
🤖 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/e2e/gc_interval_fix_test.go` around lines 80 - 85, Update the spec
containing the drift injection to capture the original ConfigMap data before
modifying it and register DeferCleanup before the drift update and subsequent
deletion. Cleanup must restore the original ConfigMap data when it still exists,
or recreate the ConfigMap with its original metadata and data when deleted, so
failures cannot leak shared state to later specs.
Source: Coding guidelines
| By("Verifying SpireServer remains Ready after drift recovery") | ||
| spireServer := &operatorv1alpha1.SpireServer{} | ||
| err = k8sClient.Get(testCtx, client.ObjectKey{Name: "cluster"}, spireServer) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to get SpireServer CR") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'gc_interval_fix_test.go|.*spire.*' . | sed 's#^\./##' | head -100
echo
echo "== relevant lines in target file =="
if [ -f test/e2e/gc_interval_fix_test.go ]; then
nl -ba test/e2e/gc_interval_fix_test.go | sed -n '60,125p'
fi
echo
echo "== WaitForSpireServerConditions definitions/usages =="
rg -n "WaitForSpireServerConditions|ControllerManagerConfigAvailable|Ready" -S . --glob '**/*.go' | head -200Repository: openshift/zero-trust-workload-identity-manager
Length of output: 4531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== list files =="
find . -type f \( -name gc_interval_fix_test.go -o -path '*/e2e/*' \) 2>/dev/null | sed 's#^\./##' | head -120
echo
echo "== target file slice =="
python3 - <<'PY'
from pathlib import Path
p = Path('test/e2e/gc_interval_fix_test.go')
lines = p.read_text().splitlines()
for i in range(60, 130):
print(f"{i+1:4}: {lines[i]}")
PY
echo
echo "== WaitForSpireServerConditions definitions/usages =="
rg -n "WaitForSpireServerConditions|ControllerManagerConfigAvailable|Ready|Expect\\(err\\)\\.NotTo\\(HaveOccurred\\)" test/e2e -S --glob '**/*.go' || true
echo
echo "== candidate utility files mentioning SpireServer/wait helpers =="
rg -n "func WaitFor.*Spire|SpireServerConditions|k8sClient\\.Get\\(|Get\\(testCtx" -S --glob '**/*.go' | head -250Repository: openshift/zero-trust-workload-identity-manager
Length of output: 4781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WaitForSpireServerConditions implementation =="
python3 - <<'PY'
from pathlib import Path
p = Path('test/e2e/utils/utils.go')
txt = p.read_text()
idx = txt.find('func WaitForSpireServerConditions')
if idx == -1:
print('WaitForSpireServerConditions not found in test/e2e/utils/utils.go')
else:
start = txt.rfind('\nfunc ', 0, idx) + 1
end = txt.find('\n', txt.find('\n', txt.find('\n', start) + 1) + 1)
print(txt[start:end])
PY
echo
echo "== nearby usage after configmap recreation =="
python3 - <<'PY'
from pathlib import Path
p = Path('test/e2e/gc_interval_fix_test.go')
lines = p.read_text().splitlines()
for i in range(122, 128):
print(f"{i+1:4}: {lines[i]}")
PY
echo
echo "== imports for operatorv1alpha1 in target file =="
python3 - <<'PY'
from pathlib import Path
p = Path('test/e2e/gc_interval_fix_test.go')
lines = p.read_text().splitlines()
for i in range(1, 45):
print(f"{i:4}: {lines[i-1]}")
PYRepository: openshift/zero-trust-workload-identity-manager
Length of output: 2487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('test/e2e/utils/utils.go')
txt = p.read_text().splitlines()
for i, line in enumerate(txt, 1):
if 'func WaitForSpireServerConditions' in line or 'func checkConditionsMatch' in line:
for j in range(max(1, i-5), min(len(txt), i+70)+1):
print(f"{j:4}: {txt[j-1]}")
print("---")
PYRepository: openshift/zero-trust-workload-identity-manager
Length of output: 9042
Assert SpireServer readiness after drift recovery.
The lines at 98-101 only confirm the CR can be fetched; a recovered ConfigMap with Ready=False/missing ControllerManagerConfigAvailable still passes. Reuse utils.WaitForSpireServerConditions here, matching the assertion after ConfigMap recreation.
Proposed fix
- spireServer := &operatorv1alpha1.SpireServer{}
- err = k8sClient.Get(testCtx, client.ObjectKey{Name: "cluster"}, spireServer)
- Expect(err).NotTo(HaveOccurred(), "failed to get SpireServer CR")
+ utils.WaitForSpireServerConditions(testCtx, k8sClient, "cluster", map[string]metav1.ConditionStatus{
+ "ControllerManagerConfigAvailable": metav1.ConditionTrue,
+ "Ready": metav1.ConditionTrue,
+ }, utils.DefaultTimeout)📝 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.
| By("Verifying SpireServer remains Ready after drift recovery") | |
| spireServer := &operatorv1alpha1.SpireServer{} | |
| err = k8sClient.Get(testCtx, client.ObjectKey{Name: "cluster"}, spireServer) | |
| Expect(err).NotTo(HaveOccurred(), "failed to get SpireServer CR") | |
| By("Verifying SpireServer remains Ready after drift recovery") | |
| utils.WaitForSpireServerConditions(testCtx, k8sClient, "cluster", map[string]metav1.ConditionStatus{ | |
| "ControllerManagerConfigAvailable": metav1.ConditionTrue, | |
| "Ready": metav1.ConditionTrue, | |
| }, utils.DefaultTimeout) |
🤖 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/e2e/gc_interval_fix_test.go` around lines 98 - 101, Replace the direct
SpireServer fetch assertion in the drift-recovery verification with a call to
utils.WaitForSpireServerConditions, matching the existing readiness assertion
after ConfigMap recreation. Ensure the check validates the expected Ready and
ControllerManagerConfigAvailable conditions rather than only confirming the CR
exists.
Source: Coding guidelines
|
/retest |
The separate gc_interval_fix_test.go file created an independent Ordered Describe block that ran after the CreateOnlyMode tests, causing state pollution (CreateOnlyMode=True not yet cleared). Move both gcInterval journey tests into the main e2e_test.go Describe block as a new Context before CreateOnlyMode, where the cluster state is known to be healthy. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/e2e_test.go (1)
1850-1906: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit drift and deletion recovery into separate specs.
Line 1850 tests two destructive recovery mechanisms in one
It; a drift failure prevents deletion coverage and can leave shared suite state mutated. Isolate each scenario and register cleanup for an early-failure path.As per coding guidelines, “Single responsibility — each test should test one behavior; Setup and cleanup using BeforeEach/AfterEach.”
🤖 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/e2e/e2e_test.go` around lines 1850 - 1906, The spec currently combines ConfigMap drift recovery and deletion recovery, so one failure can skip the other scenario and leave mutated state. Split the test around the existing “Drift injection” and “Destructive action” flows into separate It specs, each independently verifying recovery; add appropriate cleanup registration (such as DeferCleanup/AfterEach) to restore or recreate the controller-manager ConfigMap when either spec exits early.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/e2e/e2e_test.go`:
- Around line 1850-1906: The spec currently combines ConfigMap drift recovery
and deletion recovery, so one failure can skip the other scenario and leave
mutated state. Split the test around the existing “Drift injection” and
“Destructive action” flows into separate It specs, each independently verifying
recovery; add appropriate cleanup registration (such as DeferCleanup/AfterEach)
to restore or recreate the controller-manager ConfigMap when either spec exits
early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58b0e7eb-2dbc-4786-84f1-e6b05453034e
📒 Files selected for processing (1)
test/e2e/e2e_test.go
|
@sujkini: 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
2 participants
Summary
GCIntervalwas not set ingenerateControllerManagerConfig(), defaulting to0s(Go zero value), causing continuous garbage collection and ~2300m CPU usage at idle.GCInterval: 10 * time.Secondin theControllerManagerConfigstruct literal inpkg/controller/spire-server/configmap.go.configmaps_test.goverifying the struct field value and YAML serialization.Test plan
go build ./pkg/controller/spire-server/...passesgo test ./pkg/controller/spire-server/...passes (all existing + new tests)TestGenerateControllerManagerConfig_GCIntervalvalidates struct value =10*time.Secondand YAML containsgcInterval: 10000000000TestGenerateSpireControllerManagerConfigYaml/Valid_confignow assertsgcIntervalpresenceFixes: SPIRE-617
Duplicate of: OCPBUGS-90556
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests