WIP: etcd backups#1657
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAutomated backups now use ChangesAutomated backup migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PeriodicBackupController
participant EtcdBackupPolicy
participant EtcdBackup
participant BackupController
participant BackupJob
participant BackupPod
PeriodicBackupController->>EtcdBackupPolicy: read scheduled policy
PeriodicBackupController->>EtcdBackup: create EtcdBackup per selected node
BackupController->>EtcdBackup: validate and update backup
BackupController->>BackupJob: create backup Job
BackupJob->>BackupPod: run snapshot and write termination log
BackupPod-->>BackupController: return path and size metadata
BackupController->>EtcdBackup: record job and file status
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings, 1 inconclusive)
✅ Passed checks (9 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: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@go.mod`:
- Line 37: Update the google.golang.org/grpc dependency in go.mod from v1.79.3
to a patched version such as v1.82.1, or remove it if no longer required
transitively; regenerate the corresponding go.sum entries and rerun
vulnerability scanning to confirm the vulnerable version is absent.
- Line 138: Upgrade the go.opentelemetry.io/otel/sdk dependency from v1.40.0 to
v1.43.0 or later, then refresh go.mod, go.sum, and vendor/modules.txt so all
OpenTelemetry module versions remain consistent.
In `@pkg/backuphelpers/backupvars_test.go`:
- Around line 38-46: The retention expectations in TestBackupConfig_ToArgs and
the matching TestBackupConfig_ToArgList cases must be removed because ArgList()
and ArgString() no longer emit retention arguments. In
pkg/backuphelpers/backupvars_test.go lines 38-46, remove the RetentionNumber and
RetentionSize cases; remove the corresponding cases at lines 99-119. At lines
154-159, remove withRetentionSizeOneGB since it is no longer used and constructs
an invalid quantity.
In `@pkg/operator/backupcontroller/backupcontroller.go`:
- Around line 383-403: In the successful-job handling around the pod loop,
update the termination message extraction to use
pod.Status.ContainerStatuses[0].State.Terminated rather than
LastTerminationState.Terminated. Add nil checks for the container status, State,
and Terminated before reading Message, while preserving the existing JSON
parsing and FilePath/FileSize updates.
In `@pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go`:
- Around line 128-130: In the re-fetch flow for EtcdBackupPolicies, add a
require.NoError assertion immediately after the Get call and before accessing
backupPolicy.Status. Keep the existing LastScheduleTime and LastScheduleNodes
assertions unchanged.
In `@pkg/operator/periodicbackupcontroller/periodicbackupcontroller.go`:
- Line 65: Update NewPeriodicBackupController so the Node informer is obtained
from the cluster-scoped informer factory, such as the existing clusterInformers,
rather than calling InformersFor("") on kubeInformersForNamespaces. Ensure the
factory used for Nodes is initialized before the .Core().V1().Nodes() chain
executes.
- Around line 237-239: Update the masterNodes truncation logic in the periodic
backup controller so the slice upper bound never exceeds len(masterNodes). When
backupPolicy.Spec.NodeCount is positive, cap it at the available master-node
count before slicing, while preserving the existing behavior when the requested
count is within bounds.
- Around line 216-222: Update the default selector in the periodic backup
controller’s fallback branch to parse the canonical OpenShift master-node label
key, node-role.kubernetes.io/master, instead of node-role.kubernetes.master.io.
Preserve the existing custom NodeSelector handling and error propagation.
In `@test/e2e/backup.go`:
- Around line 186-190: Update the MaxSize value in the
EtcdBackupPolicyRetentionRule to represent approximately 5GB in bytes, using
resource.NewScaledQuantity with the appropriate giga suffix or an explicitly
scaled byte quantity; do not use resource.NewQuantity(5, "GB"), which stores
only 5 bytes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| go.uber.org/zap v1.27.1 | ||
| golang.org/x/sys v0.45.0 | ||
| google.golang.org/grpc v1.72.2 | ||
| google.golang.org/grpc v1.79.3 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -sS https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' \
--data '{"package":{"name":"google.golang.org/grpc","ecosystem":"Go"},"version":"v1.79.3"}' |
jq .Repository: openshift/cluster-etcd-operator
Length of output: 6584
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== go.mod grpc line =="
grep -n 'google.golang.org/grpc' go.mod || true
echo
echo "== go.sum grpc entries =="
grep -n 'google.golang.org/grpc' go.sum || true
echo
echo "== gRPC usage / RBAC/xDS import context =="
rg -n 'google\.golang\.org/grpc|grpc/xds|xds|rbac|Rbac|RequestedServerName|Metadata|SourcedMetadata' -S . --glob '!vendor/**' --glob '!_out/**' | head -200Repository: openshift/cluster-etcd-operator
Length of output: 4622
Remove or update the vulnerable gRPC dependency.
google.golang.org/grpc v1.79.3 is affected by GHSA-hrxh-6v49-42gf. This module is only referenced as an indirect/transitive dependency now; remove the stale go.mod/go.sum entry or update it to a patched version such as v1.82.1, then regenerate go.sum and rerun vulnerability scanning.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 37-37: google.golang.org/grpc 1.79.3: gRPC-Go: xDS RBAC and HTTP/2 Vulnerabilities
🤖 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 `@go.mod` at line 37, Update the google.golang.org/grpc dependency in go.mod
from v1.79.3 to a patched version such as v1.82.1, or remove it if no longer
required transitively; regenerate the corresponding go.sum entries and rerun
vulnerability scanning to confirm the vulnerable version is absent.
Source: Linters/SAST tools
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.40.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.40.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.41.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.40.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -sS https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' \
--data '{"package":{"name":"go.opentelemetry.io/otel/sdk","ecosystem":"Go"},"version":"v1.40.0"}' |
jq .Repository: openshift/cluster-etcd-operator
Length of output: 3970
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "go.mod entries for OpenTelemetry packages:"
grep -n 'go.opentelemetry.io/otel' go.mod || true
echo
echo "Dependency metadata for go.opentelemetry.io/otel/sdk@v1.40.0:"
go list -m -json go.opentelemetry.io/otel/sdk@v1.40.0 2>&1 || true
echo
echo "Dependency metadata for patched/compatible versions:"
for v in v1.43.0 v1.43.1 v1.44.0; do
echo "--- go.opentelemetry.io/otel/sdk@${v}"
go list -m -json "go.opentelemetry.io/otel/sdk@${v}" 2>&1 || true
doneRepository: openshift/cluster-etcd-operator
Length of output: 1437
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Module metadata from go.sum:"
rg 'go.opentelemetry.io/otel/sdk v1\.4[0-9]\.[0-9]+|go.opentelemetry.io/otel/sdk v1\.43\.0|go.opentelemetry.io/otel/sdk v1\.43\.1|go.opentelemetry.io/otel/sdk v1\.44\.0' go.sum || true
echo
echo "Current OpenTelemetry versions in go.mod:"
python3 - <<'PY'
from pathlib import Path
for line in Path("go.mod").read_text().splitlines():
if "opentelemetry.io/otel" in line:
print(line.strip())
PY
echo
echo "Vendor manifest module version for SDK if present:"
python3 - <<'PY'
from pathlib import Path
import re
p = Path("vendor/modules.txt")
if p.exists():
for m in re.findall(r'^## go\.opentelemetry\.io/otel/sdk v[0-9][^\n]*', p.read_text().splitlines()[0:100] + p.read_text().splitlines()):
pass
for line in p.read_text().splitlines():
if line.startswith("## go.opentelemetry.io/otel/sdk "):
print(line)
else:
print("vendor/modules.txt not found")
PYRepository: openshift/cluster-etcd-operator
Length of output: 1124
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("vendor/modules.txt")
if p.exists():
for line in p.read_text().splitlines():
if line.startswith("## go.opentelemetry.io/otel/sdk "):
print(line)
else:
print("vendor/modules.txt not found")
PYRepository: openshift/cluster-etcd-operator
Length of output: 169
Upgrade go.opentelemetry.io/otel/sdk to v1.43.0 or later.
v1.40.0 is affected by GHSA-hfvc-g4fc-pqhx / CVE-2026-39883, a high-severity PATH-hijacking issue in the BSD/Solaris resource host ID path. Run go get -u go.opentelemetry.io/otel/sdk@v1.43.0 and refresh go.mod, go.sum, and vendor/modules.txt to keep the OpenTelemetry modules consistent.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 138-138: go.opentelemetry.io/otel/sdk 1.40.0: opentelemetry-go: BSD kenv command not using absolute path enables PATH hijacking
🤖 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 `@go.mod` at line 138, Upgrade the go.opentelemetry.io/otel/sdk dependency from
v1.40.0 to v1.43.0 or later, then refresh go.mod, go.sum, and vendor/modules.txt
so all OpenTelemetry module versions remain consistent.
Source: Linters/SAST tools
| { | ||
| "backup spec with timezone and schedule and retention number", | ||
| withRetentionNumberThreeBackups(createEtcdBackupSpec(timezone, schedule)), | ||
| withRetentionNumberThreeBackups(createEtcdBackupPolicySpec(timezone, schedule)), | ||
| " args:\n - --enabled=true\n - --timezone=GMT\n - --schedule=0 */2 * * *\n - --type=RetentionNumber\n - --maxNumberOfBackups=3", | ||
| }, | ||
| { | ||
| "backup spec with timezone and schedule and retention size", | ||
| withRetentionSizeOneGB(createEtcdBackupSpec(timezone, schedule)), | ||
| withRetentionSizeOneGB(createEtcdBackupPolicySpec(timezone, schedule)), | ||
| " args:\n - --enabled=true\n - --timezone=GMT\n - --schedule=0 */2 * * *\n - --type=RetentionSize\n - --maxSizeOfBackupsGb=1", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retention test fixtures/expectations are stale after retention args were removed from ArgList()/ArgString(). The production methods no longer emit --type/--maxNumberOfBackups/--maxSizeOfBackupsGb, so the retention cases fail; the retention helpers also build an incorrect quantity.
pkg/backuphelpers/backupvars_test.go#L38-L46: remove the--type=RetentionNumber/--maxNumberOfBackups=3andRetentionSizeretention assertions fromTestBackupConfig_ToArgs(and the matching cases inTestBackupConfig_ToArgList, L99-L119).pkg/backuphelpers/backupvars_test.go#L154-L159: drop/repairwithRetentionSizeOneGB—resource.NewQuantity(10, "GB")uses value10and a non-standardFormat"GB", inconsistent with theOneGB/=1intent; remove it if retention args are no longer asserted.
📍 Affects 1 file
pkg/backuphelpers/backupvars_test.go#L38-L46(this comment)pkg/backuphelpers/backupvars_test.go#L154-L159
🤖 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 `@pkg/backuphelpers/backupvars_test.go` around lines 38 - 46, The retention
expectations in TestBackupConfig_ToArgs and the matching
TestBackupConfig_ToArgList cases must be removed because ArgList() and
ArgString() no longer emit retention arguments. In
pkg/backuphelpers/backupvars_test.go lines 38-46, remove the RetentionNumber and
RetentionSize cases; remove the corresponding cases at lines 99-119. At lines
154-159, remove withRetentionSizeOneGB since it is no longer used and constructs
an invalid quantity.
| backupPolicy, err = operatorClient.OperatorV1alpha1().EtcdBackupPolicies().Get(context.TODO(), backupPolicy.Name, v1.GetOptions{}) | ||
| require.NotNil(t, backupPolicy.Status.LastScheduleTime) | ||
| require.Equal(t, backupPolicy.Status.LastScheduleNodes, []string{"master-0"}) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing error assertion on the re-fetched policy.
backupPolicy, err = ...Get(...) is followed by assertions on backupPolicy.Status without a require.NoError(t, err); a failed Get would produce a confusing nil-deref instead of a clear failure.
🤖 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 `@pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go`
around lines 128 - 130, In the re-fetch flow for EtcdBackupPolicies, add a
require.NoError assertion immediately after the Get call and before accessing
backupPolicy.Status. Keep the existing LastScheduleTime and LastScheduleNodes
assertions unchanged.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@go.mod`:
- Around line 5-7: Remove the personal-fork replace directives for
github.com/openshift/api and github.com/openshift/client-go from go.mod. Restore
dependencies to reviewed upstream module versions or an auditable
organization-owned fork, ensuring production builds no longer resolve through
github.com/bhperry.
In `@pkg/backuphelpers/backupmessage.go`:
- Around line 20-26: Update the file lifecycle in the function containing the
termination-log Encode call: defer closing f immediately after opening it so
encoding failures release the descriptor, while preserving explicit close-error
propagation when encoding succeeds. Ensure a prior Encode error remains the
returned error and is not replaced by the deferred close result.
In `@pkg/operator/backupcontroller/backupcontroller.go`:
- Line 471: Update the logging statement in the backup job assignment flow to
stop including backup.Spec.NodeName, which may expose an internal hostname.
Retain only the job identifier in the klog message.
- Around line 383-403: The job-completion handling must not finalize the backup
when no succeeded Pod with parseable termination metadata has been observed. In
the jobState == batchv1.JobComplete path, require finding and successfully
parsing a succeeded Pod’s termination message before allowing the surrounding
flow to persist processed state and finalize the Job; otherwise defer or retry
processing so later Pod cache updates can populate FilePath and FileSize.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| replace github.com/openshift/api => github.com/bhperry/openshift-api v0.0.0-20260724171108-0e2863a63be5 | ||
|
|
||
| replace github.com/openshift/client-go => github.com/bhperry/openshift-client-go v0.0.0-20260724172246-3b02de06ce22 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod relevant lines =="
sed -n '1,30p' go.mod 2>/dev/null || true
echo
echo "== repo occurrences of bhperry and openshift replacements =="
rg -n "github.com/bhperry|github.com/openshift/api|github.com/openshift/client-go" -S . || true
echo
echo "== README/dependency mentions =="
rg -n "openshift|dependency|bhperry|supply|SBOM|sigstore|cosign" -S --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' . || trueRepository: openshift/cluster-etcd-operator
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate go-related files =="
git ls-files | rg '(^go\.mod$|^go\.sum$|^Dockerfile$|\.github/workflows/|helm/|Makefile$|vendor/|pkg/|cmd/|openshift|openshift-client-go|openshift-api)' | head -200
echo
echo "== module references in code =="
rg -n '"github.com/(openshift|bhperry)/(api|client-go)|bhperry|openshift-client-go|openshift-api' --glob '*.go' --glob '!vendor/**' . || trueRepository: openshift/cluster-etcd-operator
Length of output: 19739
Remove the personal-fork replace directives before merge.
These module replacements make the build depend on github.com/bhperry/openshift-api and github.com/bhperry/openshift-client-go, so production builds can include unreviewed fork content. Keep the fork only as temporary draft wiring; use reviewed upstream commits or an auditable organization-owned fork before merging.
🤖 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 `@go.mod` around lines 5 - 7, Remove the personal-fork replace directives for
github.com/openshift/api and github.com/openshift/client-go from go.mod. Restore
dependencies to reviewed upstream module versions or an auditable
organization-owned fork, ensuring production builds no longer resolve through
github.com/bhperry.
| if err := json.NewEncoder(f).Encode(BackupTerminationLog{Path: snapshotFile, Size: size}); err != nil { | ||
| return fmt.Errorf("Error writing termination log: %w", err) | ||
| } | ||
|
|
||
| if err := f.Close(); err != nil { | ||
| return fmt.Errorf("Error closing termination log: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the file on encoding failures.
If Encode returns an I/O error, this function returns without closing f, leaking the descriptor and potentially leaving a truncated termination log. Defer cleanup immediately after opening the file, while still propagating a close error when no earlier error occurred.
Proposed fix
-func WriteTerminationLog(terminationLog, snapshotFile string, size int64) error {
+func WriteTerminationLog(terminationLog, snapshotFile string, size int64) (retErr error) {
f, err := os.OpenFile(terminationLog, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
if err != nil {
return fmt.Errorf("Error opening termination log: %w", err)
}
+ defer func() {
+ if closeErr := f.Close(); closeErr != nil && retErr == nil {
+ retErr = fmt.Errorf("Error closing termination log: %w", closeErr)
+ }
+ }()
if err := json.NewEncoder(f).Encode(BackupTerminationLog{Path: snapshotFile, Size: size}); err != nil {
return fmt.Errorf("Error writing termination log: %w", err)
}
-
- if err := f.Close(); err != nil {
- return fmt.Errorf("Error closing termination log: %w", err)
- }
return nil
}📝 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.
| if err := json.NewEncoder(f).Encode(BackupTerminationLog{Path: snapshotFile, Size: size}); err != nil { | |
| return fmt.Errorf("Error writing termination log: %w", err) | |
| } | |
| if err := f.Close(); err != nil { | |
| return fmt.Errorf("Error closing termination log: %w", err) | |
| } | |
| func WriteTerminationLog(terminationLog, snapshotFile string, size int64) (retErr error) { | |
| f, err := os.OpenFile(terminationLog, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) | |
| if err != nil { | |
| return fmt.Errorf("Error opening termination log: %w", err) | |
| } | |
| defer func() { | |
| if closeErr := f.Close(); closeErr != nil && retErr == nil { | |
| retErr = fmt.Errorf("Error closing termination log: %w", closeErr) | |
| } | |
| }() | |
| if err := json.NewEncoder(f).Encode(BackupTerminationLog{Path: snapshotFile, Size: size}); err != nil { | |
| return fmt.Errorf("Error writing termination log: %w", err) | |
| } | |
| return 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 `@pkg/backuphelpers/backupmessage.go` around lines 20 - 26, Update the file
lifecycle in the function containing the termination-log Encode call: defer
closing f immediately after opening it so encoding failures release the
descriptor, while preserving explicit close-error propagation when encoding
succeeds. Ensure a prior Encode error remains the returned error and is not
replaced by the deferred close result.
| if jobState == batchv1.JobComplete { | ||
| pods, err := podLister.List(labels.SelectorFromSet(labels.Set{"batch.kubernetes.io/job-name": job.Name})) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, pod := range pods { | ||
| if pod.Status.Phase == corev1.PodSucceeded && len(pod.Status.ContainerStatuses) > 0 && pod.Status.ContainerStatuses[0].State.Terminated != nil { | ||
| terminationMessage := pod.Status.ContainerStatuses[0].State.Terminated.Message | ||
| if terminationMessage != "" { | ||
| data := backuphelpers.BackupTerminationLog{} | ||
| if err := json.Unmarshal([]byte(terminationMessage), &data); err != nil { | ||
| klog.V(4).Infof("error reading termination log: %s", err) | ||
| } else { | ||
| bp.Status.FilePath = data.Path | ||
| bp.Status.FileSize = *resource.NewQuantity(data.Size, resource.BinarySI) | ||
| } | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not process the backup before its termination metadata is available.
The Job and Pod caches update independently. If the completed Job arrives first, pods can be empty; this code then persists state=processed and finalizes the Job. Later Pod updates cannot populate FilePath or FileSize because both resources are excluded by the state!=processed selectors. Delay marking the backup/job processed, or retry, until the succeeded Pod has been observed and parsed.
🤖 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 `@pkg/operator/backupcontroller/backupcontroller.go` around lines 383 - 403,
The job-completion handling must not finalize the backup when no succeeded Pod
with parseable termination metadata has been observed. In the jobState ==
batchv1.JobComplete path, require finding and successfully parsing a succeeded
Pod’s termination message before allowing the surrounding flow to persist
processed state and finalize the Job; otherwise defer or retry processing so
later Pod cache updates can populate FilePath and FileSize.
| job.Spec.Template.Spec.NodeName = backup.Spec.NodeName | ||
| // Remove nodeSelector to avoid conflicts | ||
| delete(job.Spec.Template.Spec.NodeSelector, "node-role.kubernetes.io/master") | ||
| klog.V(4).Infof("BackupController assigned job [%s] to node [%s]", job.Name, backup.Spec.NodeName) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid logging the target node name.
backup.Spec.NodeName may be an internal hostname. Omit it from this log entry or log only the Job identifier. As per coding guidelines, flag logging that may expose “internal hostnames.”
🤖 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 `@pkg/operator/backupcontroller/backupcontroller.go` at line 471, Update the
logging statement in the backup job assignment flow to stop including
backup.Spec.NodeName, which may expose an internal hostname. Retain only the job
identifier in the klog message.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/operator/backupcontroller/backupcontroller.go`:
- Line 473: Update the TTLSecondsAfterFinished assignment in the backup job
setup to use ptr.To(ttlSecondsAfterFinished) instead of
new(ttlSecondsAfterFinished), preserving the intended pointer value and the
existing ptr import.
🪄 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: 4a83fcfe-d2dc-4904-993e-aa1ba088733a
📒 Files selected for processing (14)
bindata/etcd/cluster-backup-job.yamlpkg/backuphelpers/backupmessage.gopkg/backuphelpers/backupvars.gopkg/backuphelpers/backupvars_test.gopkg/cmd/backuprestore/backuprestore.gopkg/cmd/backuprestore/backuputils.gopkg/cmd/backuprestore/etcdclientutils.gopkg/operator/backupcontroller/backupcontroller.gopkg/operator/backupcontroller/backupcontroller_test.gopkg/operator/periodicbackupcontroller/periodicbackupcontroller.gopkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.gopkg/operator/starter.gopkg/tnf/internal/testutil/fakeinformer.gotest/e2e/backup.go
💤 Files with no reviewable changes (1)
- pkg/backuphelpers/backupvars.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/cmd/backuprestore/backuputils.go
- pkg/tnf/internal/testutil/fakeinformer.go
- pkg/cmd/backuprestore/backuprestore.go
- pkg/backuphelpers/backupmessage.go
- pkg/cmd/backuprestore/etcdclientutils.go
- bindata/etcd/cluster-backup-job.yaml
- pkg/backuphelpers/backupvars_test.go
- pkg/operator/starter.go
- pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go
- pkg/operator/periodicbackupcontroller/periodicbackupcontroller.go
- pkg/operator/backupcontroller/backupcontroller_test.go
- test/e2e/backup.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@pkg/cmd/backuprestore/etcdclientutils.go`:
- Around line 59-61: Update backup() and saveSnapshot() to accept and propagate
the running command context, replacing context.Background() in the cli.Snapshot
call with a derived context that has an appropriate snapshot timeout. Ensure the
timeout context is cancelled and preserve existing error wrapping while allowing
command cancellation to interrupt snapshot creation or streaming.
- Around line 50-70: Update the snapshot-writing function around cli.Snapshot
and io.Copy to always close rd, including when copying fails, while preserving
the existing copy error. Capture and return any f.Close failure as currently
done, and check the deferred os.RemoveAll cleanup result so removal failures
after successful output are propagated.
In `@pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go`:
- Around line 83-88: Update newFakeSyncContext to accept *testing.T, create the
rate-limiting queue as a local variable, and register t.Cleanup to call
queue.ShutDown before returning the context. Update all three newFakeSyncContext
callers to pass their testing handle while preserving the existing context
setup.
- Around line 40-57: Update createTestController to accept *testing.T, then
check the error returned by each nodeIndexer.Add(node) call and fail the test
via the testing handle when insertion fails. Update all callers of
createTestController to pass their test handle.
🪄 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: 81b96d58-2373-4796-a102-b8ec8f5362cc
📒 Files selected for processing (14)
bindata/etcd/cluster-backup-job.yamlpkg/backuphelpers/backupmessage.gopkg/backuphelpers/backupvars.gopkg/backuphelpers/backupvars_test.gopkg/cmd/backuprestore/backuprestore.gopkg/cmd/backuprestore/backuputils.gopkg/cmd/backuprestore/etcdclientutils.gopkg/operator/backupcontroller/backupcontroller.gopkg/operator/backupcontroller/backupcontroller_test.gopkg/operator/periodicbackupcontroller/periodicbackupcontroller.gopkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.gopkg/operator/starter.gopkg/tnf/internal/testutil/fakeinformer.gotest/e2e/backup.go
💤 Files with no reviewable changes (1)
- pkg/backuphelpers/backupvars.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/backuphelpers/backupmessage.go
- pkg/tnf/internal/testutil/fakeinformer.go
- pkg/cmd/backuprestore/backuprestore.go
- bindata/etcd/cluster-backup-job.yaml
- pkg/cmd/backuprestore/backuputils.go
- pkg/backuphelpers/backupvars_test.go
- pkg/operator/periodicbackupcontroller/periodicbackupcontroller.go
- pkg/operator/starter.go
- pkg/operator/backupcontroller/backupcontroller_test.go
- test/e2e/backup.go
| defer os.RemoveAll(partpath) | ||
|
|
||
| f, err := os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) | ||
| if err != nil { | ||
| return fmt.Errorf("could not open %s (%w)", partpath, err) | ||
| return -1, fmt.Errorf("could not open %s (%w)", partpath, err) | ||
| } | ||
|
|
||
| opBegin := time.Now() | ||
| var rd io.ReadCloser | ||
| rd, err = cli.Snapshot(context.Background()) | ||
| if err != nil { | ||
| return fmt.Errorf("saveSnapshot failed: %w", err) | ||
| return -1, fmt.Errorf("saveSnapshot failed: %w", err) | ||
| } | ||
|
|
||
| if _, err := io.Copy(f, rd); err != nil { | ||
| return fmt.Errorf("saveSnapshot failed: %w", err) | ||
| var size int64 | ||
| if size, err = io.Copy(f, rd); err != nil { | ||
| return -1, fmt.Errorf("saveSnapshot failed: %w", err) | ||
| } | ||
| if err := f.Close(); err != nil { | ||
| return fmt.Errorf("saveSnapshot failed: %w", err) | ||
| return -1, fmt.Errorf("saveSnapshot failed: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/cmd/backuprestore/etcdclientutils.go --items all
rg -n -C3 'os\.RemoveAll|cli\.Snapshot|io\.Copy|\.Close\(' pkg/cmd/backuprestore/etcdclientutils.goRepository: openshift/cluster-etcd-operator
Length of output: 1344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file content =="
cat -n pkg/cmd/backuprestore/etcdclientutils.go
echo
echo "== package/module refs for etcd client =="
rg -n 'go\.etcd\.io/etcd/client/v3|require|module' -g '*.go' -g 'go.mod' -g 'go.sum' .
echo
echo "== snapshot/readcloser close calls in surrounding code =="
rg -n -C2 'Snapshot\(|ReadCloser|\.Close\(' pkg/cmd/backuprestore/etcdclientutils.go pkg/cmd/backuprestoreRepository: openshift/cluster-etcd-operator
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== saveSnapshot location =="
cat -n pkg/cmd/backuprestore/etcdclientutils.go | sed -n '1,120p'
echo
echo "== saveSnapshot usages =="
rg -n -C3 'saveSnapshot\(' pkg/cmd/backuprestore
echo
echo "== etcd client/v3 dependency =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('go.mod'):
if 'vendor' in p.parts:
continue
text=p.read_text()
if 'go.etcd.io/etcd/client/v3' in text:
print(p)
for line in text.splitlines():
if 'go.etcd.io/etcd/client/v3' in line or 'require' in line or 'module ' in line:
print(line)
PY
echo
echo "== go module cached source if present =="
modpath="$(go env GOPATH 2>/dev/null)/pkg/mod"; if [ -n "$modpath" ] && [ -d "$modpath" ]; then
find "$modpath" -path '*etcd*client*v3*client.go' -o -path '*etcd*client*v3*client*snapshot*' 2>/dev/null | head -50
if [ -f "$modpath/go.etcd.io/etcd/client/v3@*/client.go" ]; then
grep -n "func .*Snapshot" "$modpath/go.etcd.io/etcd/client/v3@*/client.go"
fi
else
echo "no GOPATH module cache"
fiRepository: openshift/cluster-etcd-operator
Length of output: 4353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cache="$(go env GOPATH)/pkg/mod/go.etcd.io/etcd/client/v3@v3.6.8"
echo "== client.go outline around Snapshot =="
grep -n "func .*Snapshot\|Snapshot\|ReadCloser\|Close\|grpc.*snapshot" "$cache/client.go"
echo
sed -n '330,430p' "$cache/client.go"
echo
echo "== source search snapshot response handling =="
rg -n -C4 'func .*Snapshot|type .*SnapshotResponse|SnapshotResponse|Close\(\) error|snapshotpb\.Snapshot' "$cache" vendor/go.etcd.io 2>/dev/null | head -200
echo
echo "== local os.RemoveAll callers in file to show return ignored =="
sed -n '80,140p' pkg/cmd/backuprestore/etcdclientutils.goRepository: openshift/cluster-etcd-operator
Length of output: 21520
Close the snapshot stream and return cleanup file failures.
cli.Snapshot(...) returns a ReaderCloser; io.Copy reads it but does not close it, so the snapshot stream remains open until the file is closed. Also, f.Close() errors are returned, but rd is left open on any io.Copy failure, and the immediate cleanup return from os.RemoveAll is ignored on the success path. Wrap the cleanup return so removal failures after successful output are surfaced.
🤖 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 `@pkg/cmd/backuprestore/etcdclientutils.go` around lines 50 - 70, Update the
snapshot-writing function around cli.Snapshot and io.Copy to always close rd,
including when copying fails, while preserving the existing copy error. Capture
and return any f.Close failure as currently done, and check the deferred
os.RemoveAll cleanup result so removal failures after successful output are
propagated.
Source: Path instructions
| rd, err = cli.Snapshot(context.Background()) | ||
| if err != nil { | ||
| return fmt.Errorf("saveSnapshot failed: %w", err) | ||
| return -1, fmt.Errorf("saveSnapshot failed: %w", err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/cmd/backuprestore --items all
rg -n -C3 '\bsaveSnapshot\s*\(|\.Snapshot\s*\(|context\.Background\(\)' pkg/cmd/backuprestoreRepository: openshift/cluster-etcd-operator
Length of output: 5068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- etcdclientutils.go ---\n'
sed -n '1,120p' pkg/cmd/backuprestore/etcdclientutils.go
printf '\n--- backuputils.go saveSnapshot call site ---\n'
sed -n '1,100p' pkg/cmd/backuprestore/backuputils.go
printf '\n--- backupOptions/flags and command context usage ---\n'
sed -n '1,220p' pkg/cmd/backuprestore/backuprestore.goRepository: openshift/cluster-etcd-operator
Length of output: 8045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- backupserver.go timeout policy ---'
sed -n '1,240p' pkg/cmd/backuprestore/backupserver.goRepository: openshift/cluster-etcd-operator
Length of output: 3659
🌐 Web query:
etcd client/v3 Client Snapshot context.Context timeout cancellation default documentation
💡 Result:
In the etcd v3 Go client, snapshot operations—specifically via the Maintenance interface methods Snapshot and SnapshotWithVersion—do not have a default client-side timeout applied automatically by the etcd library [1]. The context.Context passed to these methods is used to manage the lifecycle and cancellation of the streaming RPC [1]. If the provided context is canceled or reaches its deadline, the etcd client will immediately abort the operation, typically resulting in a context.Canceled or context.DeadlineExceeded error [2][1]. Because there is no internal default timeout, it is standard practice to wrap the context using context.WithTimeout to ensure your application can recover from stalled network requests or unresponsive server nodes [3][4]. Key takeaways: 1. No Default Timeout: The etcd client library does not impose a default timeout on Snapshot calls [1]. If you pass context.Background or any context without a deadline, the operation will wait indefinitely for the server to respond [3][4]. 2. Context Responsibility: You are responsible for providing a context with an appropriate timeout or deadline to prevent resource leaks or long-hanging requests [3][4]. 3. Cancellation Behavior: If the context is canceled or times out, the snapshot stream will error out, and the client will stop processing data [2][1]. 4. Recommended Pattern: Use context.WithTimeout when initiating snapshot requests to ensure deterministic behavior [4]. Example Usage: ctx, cancel:= context.WithTimeout(context.Background, 10 * time.Minute) defer cancel resp, err:= cli.SnapshotWithVersion(ctx) if err!= nil { // Handle context.DeadlineExceeded or context.Canceled }
Citations:
- 1: https://github.com/etcd-io/etcd/blob/8ff746c2/client/v3/maintenance.go
- 2: https://github.com/etcd-io/etcd/blob/8ff746c2/client/v3/snapshot/v3_snapshot.go
- 3: https://github.com/etcd-io/etcd/blob/client/v3.6.10/client/v3/doc.go
- 4: https://github.com/etcd-io/etcd/blob/main/client/v3/README.md
Use a cancellable, bounded context for the etcd snapshot.
cli.Snapshot(context.Background()) has no command-backed cancellation or timeout here, and the etcd client does not apply its own snapshot deadline. This can leave the backup job stuck if snapshot creation/streaming hangs. Thread the running context into backup()/saveSnapshot() and apply an appropriate snapshot timeout before calling cli.Snapshot().
🤖 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 `@pkg/cmd/backuprestore/etcdclientutils.go` around lines 59 - 61, Update
backup() and saveSnapshot() to accept and propagate the running command context,
replacing context.Background() in the cli.Snapshot call with a derived context
that has an appropriate snapshot timeout. Ensure the timeout context is
cancelled and preserve existing error wrapping while allowing command
cancellation to interrupt snapshot creation or streaming.
Source: Path instructions
| func createTestController(kubeClient *k8sfakeclient.Clientset, operatorClient *operatorfake.Clientset, fakeOperatorClient v1helpers.StaticPodOperatorClient, nodes []*corev1.Node) *PeriodicBackupController { | ||
| fakeClock := testingclock.NewFakeClock(time.Now()) | ||
| eventRecorder := events.NewInMemoryRecorder("test", fakeClock) | ||
|
|
||
| if nodes == nil { | ||
| nodes = []*corev1.Node{ | ||
| { | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "master-0", | ||
| Labels: map[string]string{"node-role.kubernetes.io/master": ""}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| } | ||
| nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) | ||
| for _, node := range nodes { | ||
| nodeIndexer.Add(node) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go --items all
rg -n -C2 'createTestController\(|nodeIndexer\.Add\(' pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.goRepository: openshift/cluster-etcd-operator
Length of output: 4240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant helper and call sites around createTestController.
sed -n '35,65p' pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go
printf '\n--- createTestController call sites ---\n'
rg -n -C2 'createTestController\(' pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go
# Check whether the test file already imports require and whether Node creation is custom or generated.
printf '\n--- imports ---\n'
sed -n '1,35p' pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.goRepository: openshift/cluster-etcd-operator
Length of output: 4213
🌐 Web query:
k8s.io/client-go tools/cache Indexer Add signature and duplicate key behavior
💡 Result:
In the k8s.io/client-go/tools/cache package, the Add method on the Indexer (and Store) interface is defined with the signature: Add(obj interface{}) error [1][2][3]. The behavior of the Add method is to insert an object into the cache [1][2]. It uses a KeyFunc—a function provided when the Indexer is initialized—to determine the storage key for the object [1][3]. If the KeyFunc returns an error, the Add operation fails and returns that error [1][2]. If a transformer is configured for the store, the object is transformed before being stored [1]. Regarding duplicate keys, the underlying storage mechanism (ThreadSafeStore) manages the accumulation of objects [1][3]. When an object is added, if an item with the same key already exists, the new object effectively replaces or updates the existing entry within the accumulator for that key, maintaining a consistent state for the informer's cache [3][4]. The Store interface ensures that operations are performed against the accumulator associated with the object's key [3][4].
Citations:
- 1: https://github.com/kubernetes/client-go/blob/master/tools/cache/store.go
- 2: https://github.com/kubernetes/client-go/blob/787fa2adce7cc0b0780acdefda3255c8adf8765c/tools/cache/store.go
- 3: https://github.com/kubernetes/client-go/blob/release-1.28/tools/cache/store.go
- 4: https://github.com/kubernetes/client-go/blob/master/tools/cache/reflector.go
Assert indexer insertion succeeds.
cache.Indexer.Add can return an error, so ignoring nodeIndexer.Add(node) can silently drop test node setup failures. Pass t into the helper and assert the error.
Proposed fix
-func createTestController(kubeClient *k8sfakeclient.Clientset, operatorClient *operatorfake.Clientset, fakeOperatorClient v1helpers.StaticPodOperatorClient, nodes []*corev1.Node) *PeriodicBackupController {
+func createTestController(t *testing.T, kubeClient *k8sfakeclient.Clientset, operatorClient *operatorfake.Clientset, fakeOperatorClient v1helpers.StaticPodOperatorClient, nodes []*corev1.Node) *PeriodicBackupController {
+ t.Helper()
...
for _, node := range nodes {
- nodeIndexer.Add(node)
+ require.NoError(t, nodeIndexer.Add(node))
}📝 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.
| func createTestController(kubeClient *k8sfakeclient.Clientset, operatorClient *operatorfake.Clientset, fakeOperatorClient v1helpers.StaticPodOperatorClient, nodes []*corev1.Node) *PeriodicBackupController { | |
| fakeClock := testingclock.NewFakeClock(time.Now()) | |
| eventRecorder := events.NewInMemoryRecorder("test", fakeClock) | |
| if nodes == nil { | |
| nodes = []*corev1.Node{ | |
| { | |
| ObjectMeta: v1.ObjectMeta{ | |
| Name: "master-0", | |
| Labels: map[string]string{"node-role.kubernetes.io/master": ""}, | |
| }, | |
| }, | |
| } | |
| } | |
| nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) | |
| for _, node := range nodes { | |
| nodeIndexer.Add(node) | |
| func createTestController(t *testing.T, kubeClient *k8sfakeclient.Clientset, operatorClient *operatorfake.Clientset, fakeOperatorClient v1helpers.StaticPodOperatorClient, nodes []*corev1.Node) *PeriodicBackupController { | |
| t.Helper() | |
| fakeClock := testingclock.NewFakeClock(time.Now()) | |
| eventRecorder := events.NewInMemoryRecorder("test", fakeClock) | |
| if nodes == nil { | |
| nodes = []*corev1.Node{ | |
| { | |
| ObjectMeta: v1.ObjectMeta{ | |
| Name: "master-0", | |
| Labels: map[string]string{"node-role.kubernetes.io/master": ""}, | |
| }, | |
| }, | |
| } | |
| } | |
| nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) | |
| for _, node := range nodes { | |
| require.NoError(t, nodeIndexer.Add(node)) | |
| } |
🤖 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 `@pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go`
around lines 40 - 57, Update createTestController to accept *testing.T, then
check the error returned by each nodeIndexer.Add(node) call and fail the test
via the testing handle when insertion fails. Update all callers of
createTestController to pass their test handle.
Source: Path instructions
| func newFakeSyncContext(key string) *fakeSyncContext { | ||
| return &fakeSyncContext{ | ||
| key: key, | ||
| recorder: events.NewInMemoryRecorder("test", clock.RealClock{}), | ||
| queue: workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()), | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go --items all
rg -n -C2 'newFakeSyncContext\(|NewRateLimitingQueue|\.ShutDown\(' pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.goRepository: openshift/cluster-etcd-operator
Length of output: 3766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Go module versions and relevant imports to decide whether workqueue.ShutDown exists/needs import path.
printf '== go.mod deps around k8s/client-go/workqueue ==\n'
rg -n 'k8s\.io/(client-go|apimachinery)|workqueue' go.mod go.sum || true
printf '\n== test helper and callers context ==\n'
sed -n '1,270p' pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go
printf '\n== workqueue ShutDown references in repo ==\n'
rg -n 'ShutDown|NewRateLimitingQueue' pkg -g '*.go' || trueRepository: openshift/cluster-etcd-operator
Length of output: 10812
🌐 Web query:
k8s.io/client-go v0.36.2 util/workqueue RateLimitingInterface ShutDown
💡 Result:
In k8s.io/client-go v0.36.2, the ShutDown method on a RateLimitingInterface (which implements the base workqueue.Interface) is used to gracefully signal that the queue should stop accepting new items [1][2][3]. When ShutDown is called: 1. The queue ignores all new items added to it [1][2][3]. 2. Any worker goroutines currently blocked on Get will receive a shutdown signal (shutdown=true), allowing them to exit their processing loops [2][4][3]. 3. If workers are still processing items, they will continue to process them until the queue is empty before receiving the signal, depending on the implementation details and how Get is handled [1][2][3]. Common Usage In Kubernetes controller patterns, ShutDown is typically invoked using the defer statement at the beginning of the Run method to ensure the queue closes when the controller stops [5][6][4]. Example pattern: func (c *Controller) Run(stopCh <-chan struct{}) { defer c.queue.ShutDown //... controller logic } Alternative: ShutDownWithDrain If you require that the queue finishes processing all items currently in the queue before the shutdown signal is fully realized, you should use ShutDownWithDrain instead [1][2][3]. Note that for ShutDownWithDrain to function correctly, workers must call Done on each item after processing; otherwise, the drain operation may block indefinitely [2][3]. Important Version Note As of v0.36.2, RateLimitingInterface is deprecated in favor of TypedRateLimitingInterface[T] [7][1]. It is recommended to migrate to the typed interface to take advantage of Go generics and improved type safety [7][1].
Citations:
- 1: https://godocs.io/k8s.io/client-go/util/workqueue
- 2: https://github.com/kubernetes/client-go/blob/v0.35.3/util/workqueue/queue.go
- 3: https://github.com/kubernetes/client-go/blob/master/util/workqueue/queue.go
- 4: https://svalle.ru/posts/kubernetes/client-go-patterns/
- 5: https://raw.githubusercontent.com/kubernetes/client-go/v8.0.0/examples/workqueue/main.go
- 6: https://chromium.googlesource.com/external/github.com/kubernetes/client-go/+/refs/tags/upstream/v0.30.8/examples/workqueue/main.go
- 7: https://github.com/kubernetes/client-go/blob/master/util/workqueue/rate_limiting_queue.go
Shut down test workqueues.
Each NewRateLimitingQueue creates delayed-queue goroutines, and the helper owns the queue via fakeSyncContext.Queue(). Accept t *testing.T, register t.Cleanup(queue.ShutDown), and update the three newFakeSyncContext(...) callers.
🤖 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 `@pkg/operator/periodicbackupcontroller/periodicbackupcontroller_test.go`
around lines 83 - 88, Update newFakeSyncContext to accept *testing.T, create the
rate-limiting queue as a local variable, and register t.Cleanup to call
queue.ShutDown before returning the context. Update all three newFakeSyncContext
callers to pass their testing handle while preserving the existing context
setup.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests