NO-JIRA: Backup refactor

[dusk125]

and a bit of modernization

Summary by CodeRabbit

• New Features

  • Added a templated EtcdBackup resource manifest for backup requests.

• Chores

  • Removed the backup-server CLI subcommand and related backup helper code.
  • Switched request-backup to use the embedded YAML template when creating backups.
  • Modernized retention/prune logic to use standardized retention types and improved folder sorting.
  • Exposed an Enqueue() method on the target config controller; various internal simplifications.

• Tests

  • Removed obsolete backup-related unit tests and updated tests to reflect sorting/validation changes.
  • Test fixtures now include an unsupported-arch env var on specific architectures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 10a1c0e2-d9ab-487f-9bde-bd76bfed79fa

📥 Commits

Reviewing files that changed from the base of the PR and between b164d1a and d9e8927.

⛔ Files ignored due to path filters (14)

• go.sum is excluded by !**/*.sum
• vendor/github.com/robfig/cron/v3/.gitignore is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/.travis.yml is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/LICENSE is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/README.md is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/chain.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/constantdelay.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/cron.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/doc.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/logger.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/option.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/parser.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/robfig/cron/v3/spec.go is excluded by !**/vendor/**, !vendor/**
• vendor/modules.txt is excluded by !**/vendor/**, !vendor/**

📒 Files selected for processing (2)

• go.mod
• pkg/cmd/request-backup/requestbackup.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/cmd/request-backup/requestbackup.go

---

Walkthrough

Removes the scheduled backup server and its helpers/tests, switches retention handling to the v1alpha1 API enum, replaces hardcoded EtcdBackup CR construction with a YAML template decoder, and modernizes sorting and small controller utilities to use the slices package.

Changes

Backup server removal

Layer / File(s)	Summary
Backup server command removed pkg/cmd/backuprestore/backupserver.go, pkg/cmd/backuprestore/backupserver_test.go	Deletes the NewBackupServer cobra command and its unit tests that validated schedule parsing, validation, and scheduled-run behavior.
Backup helpers removed pkg/backuphelpers/backupvars.go, pkg/backuphelpers/backupvars_test.go	Removes BackupConfig, BackupVar, Enqueueable, and associated tests (argument generation, listener notification).
Main CLI wiring cmd/cluster-etcd-operator/main.go	Stops registering the backupserver subcommand; replaces deprecated ioutil.Discard with io.Discard and drops math/rand seeding.
Module tidy for cron deps go.mod	Removes direct cron module requirements and leaves one entry indirect (adjusts cron-related dependency declarations).

Request-backup: CR templating

Layer / File(s)	Summary
EtcdBackup YAML template added bindata/etcd/etcd-backup-cr.yaml	Adds an EtcdBackup CR manifest with templated metadata and spec.pvcName.
Decode-and-mutate flow pkg/cmd/request-backup/requestbackup.go	Replaces programmatic CR construction with runtime scheme + universal decoder that loads the bindata YAML template, type-asserts to *operatorv1alpha1.EtcdBackup, sets Name/Namespace/Spec.PVCName, and updates the Job ownerReference Name/UID. Adds imports for runtime/serializer and bindata.

Prune & controller refactors

Layer / File(s)	Summary
Prune retention enum + branching pkg/cmd/prune-backups/prune.go, pkg/cmd/prune-backups/prune_test.go	Replaces local retention-type constants with backupv1alpha1.RetentionType values; Validate() and Run() now switch on the API enum. Tests updated to use API enum strings and call folders.Sort().
Directory sorting modernization pkg/cmd/prune-backups/prune.go	Replaces custom sort.Interface with backupDirStats.Sort() implemented via slices.SortFunc; callers use folders.Sort().
Controller small refactors pkg/operator/backupcontroller/backupcontroller.go, pkg/operator/periodicbackupcontroller/periodicbackupcontroller.go, pkg/operator/targetconfigcontroller/targetconfigcontroller.go	Switches to slices utilities for sorting, changes running-job gating to count-running-jobs helper, simplifies condition updates to range loops, consolidates retention init-container logic to a switch, and adds TargetConfigController.Enqueue() while removing a namespace event handler.
Test fixture arch-specific env var pkg/etcdenvvar/envvarcontroller_test.go	Adds package init() to include ETCD_UNSUPPORTED_ARCH in defaultEnvResult for arm64/s390x during tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 10 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	❓ Inconclusive	Custom check targets Ginkgo tests (Describe, Context, Eventually). Repository uses Go's testing package. Quality principles partially apply but framework mismatch makes verification ambiguous.	Clarify scope: if check applies to Go testing, tests show mixed quality (good cleanup, lack assertion messages). If Ginkgo-only, not applicable.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Backup refactor' is related to the main changes in the PR, which involve removing backup-related code and restructuring backup functionality across multiple files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	Project uses standard Go testing.T framework, not Ginkgo. All modified test files have static, hardcoded test names. Custom Ginkgo check is not applicable.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests are added in this PR. The PR is a backup refactor that removes/modifies unit tests and non-test code files. The custom check is not applicable.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests (It(), Describe(), Context(), When()) were added in this PR. All modified test files use standard Go testing.T patterns. Check not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	PR is a backup refactoring that does not introduce new scheduling constraints. No required anti-affinity, topology spread, node selectors, or other topology-breaking patterns are added.
Ote Binary Stdout Contract	✅ Passed	All process-level entry points properly configure logging to stderr. gRPC logging redirected to stderr, klog defaults to stderr, no direct stdout writes. The removed backupserver.go had no violations.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests are added in this PR. The repository uses standard Go testing, not Ginkgo. All modified test files are unit tests. The check is not applicable.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

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
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/cmd/request-backup/requestbackup.go (1)

153-153: 💤 Low value

Use a type assertion with ok check for defensive decoding.

The direct type assertion will panic if the decoded object isn't the expected type. While the template is controlled in-repo, a defensive check provides clearer error messages during development if the template is modified incorrectly.

♻️ Proposed fix

-	etcdBackup := obj.(*operatorv1alpha1.EtcdBackup)
+	etcdBackup, ok := obj.(*operatorv1alpha1.EtcdBackup)
+	if !ok {
+		return fmt.Errorf("expected *operatorv1alpha1.EtcdBackup, got %T", obj)
+	}

🤖 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/request-backup/requestbackup.go` at line 153, The direct type
assertion "etcdBackup := obj.(*operatorv1alpha1.EtcdBackup)" can panic if
decoding returns an unexpected type; change it to a safe comma-ok assertion
(e.g., etcdBackup, ok := obj.(*operatorv1alpha1.EtcdBackup)) and handle the
false branch by returning or logging a clear error describing the unexpected
type (including the actual type of obj) so decoding failures surface safely in
the code path that decodes into obj.

🤖 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/prune-backups/prune.go`:
- Around line 78-82: The CLI default for retention type in NewPruneCommand is
set to the string "None", but Validate() now treats an empty RetentionType as
the "none" case, causing mismatch and "unknown retention type" errors; update
NewPruneCommand to use an empty string (or the appropriate zero-value constant)
as the default for the retention type flag so that RetentionType("") is passed
through and the switch in prune.go (using RetentionType(r.RetentionType)) hits
the case "" / no-op path. Locate the flag/default setup in NewPruneCommand and
change the default from "None" to "" (or backupv1alpha1.RetentionType(\"\")/zero
value) to align with Validate and the switch handling.

---

Nitpick comments:
In `@pkg/cmd/request-backup/requestbackup.go`:
- Line 153: The direct type assertion "etcdBackup :=
obj.(*operatorv1alpha1.EtcdBackup)" can panic if decoding returns an unexpected
type; change it to a safe comma-ok assertion (e.g., etcdBackup, ok :=
obj.(*operatorv1alpha1.EtcdBackup)) and handle the false branch by returning or
logging a clear error describing the unexpected type (including the actual type
of obj) so decoding failures surface safely in the code path that decodes into
obj.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 835369cd-1043-49b1-bf13-cae8c248be38

📥 Commits

Reviewing files that changed from the base of the PR and between c0614ca and ebf00ec.

📒 Files selected for processing (13)

• bindata/etcd/etcd-backup-cr.yaml
• cmd/cluster-etcd-operator/main.go
• pkg/backuphelpers/backupvars.go
• pkg/backuphelpers/backupvars_test.go
• pkg/cmd/backuprestore/backupserver.go
• pkg/cmd/backuprestore/backupserver_test.go
• pkg/cmd/prune-backups/prune.go
• pkg/cmd/prune-backups/prune_test.go
• pkg/cmd/request-backup/requestbackup.go
• pkg/etcdenvvar/envvarcontroller_test.go
• pkg/operator/backupcontroller/backupcontroller.go
• pkg/operator/periodicbackupcontroller/periodicbackupcontroller.go
• pkg/operator/targetconfigcontroller/targetconfigcontroller.go

💤 Files with no reviewable changes (5)

• pkg/cmd/backuprestore/backupserver.go
• pkg/backuphelpers/backupvars_test.go
• pkg/cmd/backuprestore/backupserver_test.go
• pkg/operator/targetconfigcontroller/targetconfigcontroller.go
• pkg/backuphelpers/backupvars.go

[dusk125]

/jira refresh

[openshift-ci-robot]

@dusk125: This pull request explicitly references no jira issue.

Details

In response to this:

/jira refresh

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.

[dusk125]

/retest

[dusk125]

/cc @tjungblu
/cc @bhperry
/cc @atiratree

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bertinatto for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dusk125]

/retest-required

[openshift-ci]

@dusk125: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[bhperry]

/lgtm