CNTRLPLANE-2678: add HCPEtcdBackup controller to HyperShift Operator - #8139
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
area/api
jparrill
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. 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. |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an etcd backup feature for Hosted Control Planes. Introduces new API condition type Sequence Diagram(s)sequenceDiagram
participant User as User/Operator
participant K8s as Kubernetes API
participant Reconciler as HCPEtcdBackupReconciler
participant HCP as HostedControlPlane
participant Etcd as Etcd StatefulSet
participant Job as Backup Job
participant Storage as Object Storage
User->>K8s: Create HCPEtcdBackup CR
K8s->>Reconciler: Reconcile triggered
Reconciler->>K8s: Get HCPEtcdBackup
K8s-->>Reconciler: CR retrieved
Reconciler->>HCP: Get HostedControlPlane
HCP-->>Reconciler: HCP retrieved
Reconciler->>Etcd: Check StatefulSet ReadyReplicas
Etcd-->>Reconciler: Healthy / Unhealthy
alt etcd healthy & no active job
Reconciler->>K8s: Ensure ServiceAccount (operator ns)
K8s-->>Reconciler: ServiceAccount ready
Reconciler->>K8s: Ensure Role & RoleBinding (hcp ns)
K8s-->>Reconciler: RBAC ready
Reconciler->>K8s: Ensure NetworkPolicy (hcp ns)
K8s-->>Reconciler: NetworkPolicy ready
Reconciler->>K8s: Create Backup Job (operator ns)
K8s-->>Reconciler: Job created
else active job exists
Reconciler->>K8s: Mark HCPEtcdBackup as Rejected
K8s-->>Reconciler: Status updated
end
Job->>Storage: Upload snapshot
Storage-->>Job: Return snapshot URL
K8s->>Reconciler: Job completion event
Reconciler->>K8s: Read Pod termination message / get snapshot URL
K8s-->>Reconciler: Snapshot URL
Reconciler->>K8s: Update HCPEtcdBackup status (Succeeded/Failed)
K8s-->>Reconciler: Status updated
Reconciler->>HCP: Update EtcdBackupSucceeded condition
HCP-->>Reconciler: Condition set
Reconciler->>K8s: Enforce retention (delete old backups)
K8s-->>Reconciler: Old resources deleted
sequenceDiagram
participant HCP as HostedControlPlane
participant HostedCluster as HostedCluster Controller
participant K8s as Kubernetes API
HCP->>K8s: Update status.conditions (EtcdBackupSucceeded)
K8s-->>HostedCluster: Watch event / notify
HostedCluster->>K8s: Read HCP.status.conditions
K8s-->>HostedCluster: Returns conditions
HostedCluster->>K8s: Patch HostedCluster.status with EtcdBackupSucceeded
K8s-->>HostedCluster: Status updated
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/uncc csrwng |
|
/uncc @cblecker |
|
/auto-cc |
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hypershift-operator/controllers/etcdbackup/reconciler.go (1)
374-378: Consider failing backup if snapshot URL extraction fails.If
getSnapshotURLFromPodfails or returns empty, the backup is still marked as successful but without a usablesnapshotURL. This could result in "successful" backups that cannot be used for restore operations.Consider whether a missing snapshot URL should cause the backup to be marked as failed, or at minimum add a warning condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/etcdbackup/reconciler.go` around lines 374 - 378, The code currently swallows errors/empty results from getSnapshotURLFromPod causing backups to appear successful without a usable SnapshotURL; change the logic in reconciler.go around the getSnapshotURLFromPod call so that when err != nil or url == "" you set backup.Status.Phase to a failing state (e.g., "Failed"), populate backup.Status.Conditions or a warning condition with a clear Reason and Message that includes the error or "empty snapshot URL", and persist the status via the same status update path used elsewhere (e.g., r.client.Status().Update or r.updateStatus). Keep the existing successful branch to set backup.Status.SnapshotURL when present. Ensure the change references getSnapshotURLFromPod, backup.Status.SnapshotURL, backup.Status.Phase, and the status update method so the backup is marked failed (or has a warning condition) when URL extraction fails or is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/hostedcluster_conditions.go`:
- Around line 198-202: The EtcdBackupActive condition is semantically inverted;
change the API to express success/completion instead of activity by renaming the
ConditionType EtcdBackupActive to something like EtcdBackupSucceeded (or
EtcdBackupComplete) and update its docstring so True means the last backup
succeeded and False means it failed/in progress, then update all uses that set
or check EtcdBackupActive (e.g., HCP/HostedCluster status setters, getters, and
any callers) to use the new EtcdBackupSucceeded name and ensure the boolean
polarity is adjusted where values are set or evaluated so consumers reading
HostedCluster status see True as success.
In `@hypershift-operator/main.go`:
- Line 173: Validate opts.EtcdBackupMaxCount immediately after parsing flags
(where cmd.Flags().IntVar is used) and before the controller is wired: if the
value is less than 1 (unless 0 is intentionally allowed and documented), return
an error and exit startup so invalid bounds never reach MaxBackupCount in the
reconciler; update the startup path that wires the controller to check
opts.EtcdBackupMaxCount (and mirror this validation for the other flag group
mentioned around lines 516-523) and include a clear error message referencing
the flag name.
In `@test/integration/oadp/run.sh`:
- Around line 426-449: The fallback currently copies the entire
~/.aws/credentials into creds_content; change it to extract only the active
profile (use $AWS_PROFILE if set, otherwise "default") and serialize just that
profile's keys (aws_access_key_id, aws_secret_access_key, aws_session_token if
present) into creds_content instead of cat'ing the whole file; update the branch
that sets creds_content (the code touching access_key/secret_key/session_token,
creds_content, tmp_creds, and CTRL_AWS_CREDS_SECRET) to parse ~/.aws/credentials
for the selected profile and include only those three entries so unrelated
profiles/accounts are not included in the Secret.
---
Nitpick comments:
In `@hypershift-operator/controllers/etcdbackup/reconciler.go`:
- Around line 374-378: The code currently swallows errors/empty results from
getSnapshotURLFromPod causing backups to appear successful without a usable
SnapshotURL; change the logic in reconciler.go around the getSnapshotURLFromPod
call so that when err != nil or url == "" you set backup.Status.Phase to a
failing state (e.g., "Failed"), populate backup.Status.Conditions or a warning
condition with a clear Reason and Message that includes the error or "empty
snapshot URL", and persist the status via the same status update path used
elsewhere (e.g., r.client.Status().Update or r.updateStatus). Keep the existing
successful branch to set backup.Status.SnapshotURL when present. Ensure the
change references getSnapshotURLFromPod, backup.Status.SnapshotURL,
backup.Status.Phase, and the status update method so the backup is marked failed
(or has a warning condition) when URL extraction fails or is empty.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e6f30f0c-d4db-4460-8317-f106584ae3c5
⛔ Files ignored due to path filters (3)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/hostedcluster_conditions.gohypershift-operator/controllers/etcdbackup/reconciler.gohypershift-operator/controllers/etcdbackup/reconciler_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/main.gotest/integration/oadp/controller/controller_test.gotest/integration/oadp/run.sh
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8139 +/- ##
==========================================
+ Coverage 27.50% 27.92% +0.42%
==========================================
Files 1096 1098 +2
Lines 107277 108303 +1026
==========================================
+ Hits 29503 30243 +740
- Misses 75240 75469 +229
- Partials 2534 2591 +57
🚀 New features to boost your workflow:
|
|
|
||
| // EtcdBackupActive bubbles up from HCP. It indicates whether an etcd backup | ||
| // is currently in progress or reports the result of the most recent backup. | ||
| // True means a backup completed successfully; False means a backup is in progress |
There was a problem hiding this comment.
who consumes this?
EtcdBackupActive=false meaning a backup is in progress seems pretty counter intuitive?
// True means a backup completed successfully; False means a backup is in progress
how does this relate to BackupCompleted? why do they live in different places?
Could you present all possible permutations for these two conditions with their reasons?
There was a problem hiding this comment.
Who consumes this?
EtcdBackupActive lives on the HCP/HC so users watching HostedCluster status can see backup activity without looking at HCPEtcdBackup CRs directly. The HC controller propagates it from HCP → HC via the existing condition bubble-up mechanism.
EtcdBackupActive=false meaning a backup is in progress seems pretty counter intuitive?
I agree it's confusing. EtcdBackupActive=False meaning "in progress" is counter-intuitive. I followed the existing EtcdRecoveryActive pattern, but the semantics don't map well here since EtcdRecoveryActive=True means "recovery is happening" while here True means "backup succeeded".
Maybe I can rename to EtcdBackupSucceeded:
True= last backup succeededFalse= in progress or failed
How does it relate to BackupCompleted?
BackupCompleted lives on the HCPEtcdBackup CR (per-backup). EtcdBackupActive lives on the HCP/HC (cluster-level summary). The controller sets EtcdBackupActive on the HCP as a mirror of the latest backup's status.
Permutations
HCPEtcdBackup BackupCompleted |
Reason | HCP EtcdBackupActive |
Reason | Meaning |
|---|---|---|---|---|
| (not set) | — | (not set) | — | CR just created, not yet reconciled |
| False | EtcdUnhealthy | (not set) | — | etcd StatefulSet not ready, waiting to retry |
| False | BackupAlreadyInProgress | False | BackupAlreadyInProgress | backup Job created and running |
| False | BackupFailed | False | BackupFailed | permanent failure (missing creds, Job failed) |
| True | BackupSucceeded | True | BackupSucceeded | backup completed successfully |
There was a problem hiding this comment.
Maybe I can rename to EtcdBackupSucceeded:
True = last backup succeeded False = in progress or failed
That sounds better to me as well.
There was a problem hiding this comment.
I also think that BackupAlreadyInProgressReason could be just BackupInProgressReason with value BackupInProgress
It's a minor detail but since we're just defining the API it's worth mentioning that having "Already" in the name sounds redundant.
There was a problem hiding this comment.
Agreed, renamed to BackupInProgressReason with value "BackupInProgress". Also added BackupRejectedReason as a separate terminal state for when concurrent backups are attempted.
| hyperv1.HostedClusterRestoredFromBackup, | ||
| hyperv1.DataPlaneConnectionAvailable, | ||
| hyperv1.ControlPlaneConnectionAvailable, | ||
| hyperv1.ConditionType("EtcdBackupActive"), |
There was a problem hiding this comment.
why not use the hyperv1.EtcdBackupActive directly ?
There was a problem hiding this comment.
Done. Removed the local conditionEtcdBackupActive string constant and now use hyperv1.EtcdBackupSucceeded directly (renamed from EtcdBackupActive per enxebre/mgencur feedback).
| pod := &podList.Items[i] | ||
| for _, cs := range pod.Status.ContainerStatuses { | ||
| if cs.Name == "upload" && cs.State.Terminated != nil && cs.State.Terminated.Message != "" { | ||
| return strings.TrimSpace(cs.State.Terminated.Message), nil |
There was a problem hiding this comment.
maybe it is not necessary, but will it be more stable if the etcd-upload write some structure message into the terminator log (like: SNAPSHOT_URL=XXX in a separate line) and we parse that from the message?
There was a problem hiding this comment.
Agreed, a structured termination message (e.g. SNAPSHOT_URL=) would be more robust than relying on the raw termination log content. However, the etcd-upload subcommand is part of the CPO which is already frozen in the OCP release payload for this cycle — we cannot modify it until the next release window.
I have filed this as a follow-up improvement. In the meantime, the current approach reads the termination message as a plain URL string, which matches what etcd-upload writes today via os.WriteFile(terminationLogPath, []byte(snapshotURL), 0644).
| logger.Error(err, "failed to update HCP backup condition") | ||
| } | ||
|
|
||
| return ctrl.Result{RequeueAfter: requeueInterval}, nil |
There was a problem hiding this comment.
why we re queue the reconciliation ? it has watched the Job as well, so job status change should trigger it automatically IMO.
There was a problem hiding this comment.
The RequeueAfter after Job creation is a safety net. The Job watch handles most transitions, but there are edge cases where the watch event can be missed (e.g. controller restart, watch bookmark gap). The RequeueAfter: 10s ensures the controller eventually catches up even if a watch event is lost. This is a common pattern in controller-runtime — the watch provides responsiveness while the periodic requeue provides consistency.
| mountPathCredentials = "/etc/etcd-backup-creds" | ||
|
|
||
| // etcdClientPort is the etcd client port used in the NetworkPolicy and etcdctl endpoint. | ||
| etcdClientPort int32 = 2379 |
There was a problem hiding this comment.
Nit: It would be cool to put the port in constants, next to https://github.com/openshift/hypershift/blob/main/support/config/constants.go#L30
There was a problem hiding this comment.
Done. Moved to support/config/constants.go as EtcdClientPort = 2379, next to DefaultEtcdURL. I'm not modifying the CPO reference due to timelines, I'll do that in a follow up PR.
|
|
||
| // EtcdBackupActive bubbles up from HCP. It indicates whether an etcd backup | ||
| // is currently in progress or reports the result of the most recent backup. | ||
| // True means a backup completed successfully; False means a backup is in progress |
There was a problem hiding this comment.
Maybe I can rename to EtcdBackupSucceeded:
True = last backup succeeded False = in progress or failed
That sounds better to me as well.
|
|
||
| // EtcdBackupActive bubbles up from HCP. It indicates whether an etcd backup | ||
| // is currently in progress or reports the result of the most recent backup. | ||
| // True means a backup completed successfully; False means a backup is in progress |
There was a problem hiding this comment.
I also think that BackupAlreadyInProgressReason could be just BackupInProgressReason with value BackupInProgress
It's a minor detail but since we're just defining the API it's worth mentioning that having "Already" in the name sounds redundant.
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. 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. |
|
|
||
| // EtcdBackupSucceeded bubbles up from HCP. It indicates the result of the | ||
| // most recent etcd backup. True means the last backup completed successfully; | ||
| // False means a backup is in progress or the last backup failed. |
There was a problem hiding this comment.
will we need a history like API instead of this?
There was a problem hiding this comment.
Good question, I'd wait for feedback from users (ROSA, ARO, self-managed) before adding a history-like API. Not sure how useful it would be to put that info in the HC when other observability tools can cover the historical view. If there's demand, we can add a status.backupHistory[] in a follow-up.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, gaol, jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. 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. |
|
Just for reference. Added a new commit to cover an identified issue on pre-merge verification (thanks @gaol): The issue
Now I can see the 2nd status is rejected, but the first one failed with: message: 'Backup Job failed: Job was active longer than specified deadline' |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hypershift-operator/controllers/etcdbackup/reconciler.go`:
- Around line 271-278: The HCP condition update errors from
updateHCPBackupCondition (used when setting EtcdBackupSucceeded) must not be
dropped; make those update calls return an error to the reconciler so the
controller will requeue/retry. Replace the logger-only handling at the three
call sites (the blocks invoking updateHCPBackupCondition that currently just
logger.Error) with code that returns/wraps the error (or requeues) from the
calling reconcile path; and in the terminal-backup short-circuit path ensure you
call updateHCPBackupCondition and propagate its error instead of proceeding
silently so terminal-path sync will be retried until propagation succeeds.
- Around line 600-607: The NetworkPolicyPeer currently allows ingress from the
entire operator namespace by using NamespaceSelector (the block referencing
NetworkPolicyPeer and metav1.LabelSelector with r.OperatorNamespace); restrict
this to only the etcd backup job pods by replacing or augmenting the
NamespaceSelector with a PodSelector that matches the labels used by the backup
Job/Pod template (use the exact label keys/values applied in the etcd backup Job
spec), or use both NamespaceSelector (for r.OperatorNamespace) and PodSelector
together so the peer is limited to pods with those labels in the operator
namespace; update the MatchLabels in the metav1.LabelSelector accordingly so the
NetworkPolicy only permits traffic from the backup job pods.
- Around line 255-257: The call to createBackupJob should treat a missing
pull-secret as a terminal failure instead of requeuing; update the reconciler
block where createBackupJob is called to check for errors.IsNotFound(err)
(and/or inspect the wrapped error message from createBackupJob that references
the pull-secret), set the Backup object condition to BackupFailed with an
explanatory message, persist the status, and return without requeueing; for
other errors keep the existing fmt.Errorf return path. Ensure you reference
createBackupJob and the BackupFailed condition handling when locating the code
to modify.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c2983e88-472f-48f9-a0b7-c8eb4f67631e
📒 Files selected for processing (2)
hypershift-operator/controllers/etcdbackup/reconciler.gohypershift-operator/controllers/etcdbackup/reconciler_test.go
✅ Files skipped from review due to trivial changes (1)
- hypershift-operator/controllers/etcdbackup/reconciler_test.go
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
…sources When two HCPEtcdBackup CRs are created simultaneously, the second is rejected by the serial guard. However, the rejected backup entering a terminal state triggered cleanupResources, which deleted the NetworkPolicy and RBAC that the first (active) backup's Job still needs, causing it to timeout. Add an active Job guard to cleanupResources: before deleting shared resources, check if any backup Job is still running in the same HCP namespace. If so, skip the cleanup and log a message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
8925e2a to
b920edf
Compare
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by @gaol |
openshift-ci-robot
added
the
verified
|
@gaol: This PR has been marked as verified by 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. |
|
/retest-required |
|
@jparrill: 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. |
…penshift#8139) * feat(CNTRLPLANE-2678): add HCPEtcdBackup controller to HyperShift Operator Implements the HCPEtcdBackup reconciler that orchestrates etcd snapshot and upload Jobs. The controller watches HCPEtcdBackup CRs, validates etcd health, manages temporary RBAC and NetworkPolicy resources for cross-namespace access, creates 3-container backup Jobs (fetch-certs, snapshot, upload), tracks Job status via pod termination messages, and enforces count-based retention of completed backups. Key design decisions: - findJobForBackup runs before findActiveJob serial guard to prevent the controller from rejecting its own Job on re-reconcile - BackupRejected is a terminal state to prevent backup accumulation when concurrent backups are attempted - BackoffLimit=0 to avoid race conditions where Kubernetes retries a pod after the controller has already cleaned up RBAC - Cleanup errors are propagated (not swallowed) so controller-runtime retries on transient failures, preventing leaked resources - --etcd-backup-max-count is validated to be at least 1 Adds EtcdBackupSucceeded condition type for HCP-to-HC status propagation. Controller is registered behind the HCPEtcdBackup feature gate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * test(CNTRLPLANE-2678): add integration tests for HCPEtcdBackup controller Add integration tests that validate the HCPEtcdBackup controller end-to-end against a live management cluster with S3 and Azure Blob storage backends. Tests: - S3 happy path: backup completes with snapshotURL, RBAC/NetworkPolicy cleaned up, EtcdBackupSucceeded condition propagated to HCP - Azure Blob happy path: same validations for Azure storage - Invalid credentials: controller sets BackupFailed as terminal condition when credential Secret is missing waitForBackupCondition monitors both the CR condition and Job status to detect pod-level failures early (e.g. init container errors) instead of waiting for the full timeout. run.sh gains a `controller` subcommand that creates/destroys all cloud resources (S3 buckets, Azure RG/storage/SP, K8s Secrets) automatically via setup/teardown functions with EXIT trap. AWS credentials fallback now extracts only the [default] profile instead of copying the entire credentials file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * fix(CNTRLPLANE-2678): prevent rejected backup from deleting shared resources When two HCPEtcdBackup CRs are created simultaneously, the second is rejected by the serial guard. However, the rejected backup entering a terminal state triggered cleanupResources, which deleted the NetworkPolicy and RBAC that the first (active) backup's Job still needs, causing it to timeout. Add an active Job guard to cleanupResources: before deleting shared resources, check if any backup Job is still running in the same HCP namespace. If so, skip the cleanup and log a message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
7 participants
Summary
HCPEtcdBackupcontroller to the HyperShift Operator, implementing the reconciliation flow defined in Enhancement PR #1945HCPEtcdBackupCRs and orchestrates a 3-container backup Job (fetch-etcd-certs → etcdctl snapshot → etcd-upload) with temporary RBAC and NetworkPolicy for cross-namespace etcd accessBackupCompleted) and propagatesEtcdBackupActivecondition to the HostedControlPlaneBackupFailedterminal condition instead of retrying indefinitely--etcd-backup-max-countrun.sh controllerDependencies
etcd-backupCPO subcommand (merged)fetch-etcd-certsCPO subcommand (merged)etcd-uploadCPO subcommand (merged)Jira
CNTRLPLANE-2678
Test plan
go test ./hypershift-operator/controllers/etcdbackup/...)make run-gitlintpassesmake verifypassesTesting notes
Unit tests
go test ./hypershift-operator/controllers/etcdbackup/... -vIntegration tests
Integration tests run against a live management cluster with a HostedCluster deployed. They require:
KUBECONFIGpointing to the management clusterETCD_BACKUP_TEST_HCP_NAMESPACEset to the HCP namespace (e.g.clusters-my-hcp)TechPreviewNoUpgradefeature set)aws sts get-caller-identity) and/or Azure CLI authenticated (az account show)The
run.sh controllersubcommand handles all cloud resource lifecycle (S3 buckets, Azure storage accounts, K8s Secrets) automatically:Alternatively, run Go tests directly with manual env vars (useful for debugging):
KUBECONFIG=/path/to/kubeconfig \ ETCD_BACKUP_TEST_HCP_NAMESPACE=clusters-my-hcp \ ETCD_BACKUP_TEST_S3_BUCKET=my-bucket \ ETCD_BACKUP_TEST_S3_REGION=us-east-2 \ ETCD_BACKUP_TEST_S3_KEY_PREFIX=etcd-backups/test \ ETCD_BACKUP_TEST_S3_CREDENTIALS_SECRET=my-aws-creds \ go test -tags integration -v -timeout 10m ./test/integration/oadp/controller/...🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests