CNTRLPLANE-3201: ci(hypershift): add self-managed Azure OADP backup/restore E2E test - #82796
CNTRLPLANE-3201: ci(hypershift): add self-managed Azure OADP backup/restore E2E test#82796mgencur wants to merge 3 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur 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 |
WalkthroughChangesAzure self-managed backup and restore
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CIJob
participant BackupRestoreWorkflow
participant OADPSetup
participant Azure
participant GuestCluster
participant OADPDestroy
CIJob->>BackupRestoreWorkflow: Start Azure backup and restore workflow
BackupRestoreWorkflow->>OADPSetup: Configure OADP prerequisites
OADPSetup->>Azure: Create storage account and blob container
OADPSetup->>GuestCluster: Apply credentials and OADP resources
BackupRestoreWorkflow->>GuestCluster: Run backup, restore, and conformance tests
BackupRestoreWorkflow->>OADPDestroy: Clean up Azure OADP storage
OADPDestroy->>Azure: Delete storage account
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
`@ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml`:
- Around line 406-413: Set NODEPOOL_N1_VERSION to "4.22" in both periodic job
definitions:
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml
lines 406-413 and
ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml
lines 437-444. Add the value under each job’s env section while preserving the
existing job configuration.
In
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.sh`:
- Around line 113-115: Update the Velero readiness check in the setup command so
oc wait --for=condition=Available deployment/velero returns a nonzero status
when the deployment is missing or unavailable. Remove the trailing || true,
preserving the existing timeout and namespace settings so setup fails
immediately with the readiness error.
- Around line 22-23: Update the STORAGE_ACCOUNT_NAME generation to retain a
valid lowercase alphanumeric cluster stem and append a job-unique suffix derived
from PROW_JOB_ID, ensuring the complete Azure storage account name is no longer
than 24 characters. Preserve writing the generated name to SHARED_DIR so cleanup
continues to work.
In
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-ref.yaml`:
- Around line 16-18: Update the documentation block for the Azure Blob Storage
container name to state that an empty value defaults to a container named using
the hypershift-oadp-${CLUSTER_NAME} pattern, matching the command script
behavior.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e190f3be-245f-405c-b8c9-3fa5684dc5eb
⛔ Files ignored due to path filters (5)
ci-operator/jobs/openshift/hypershift/openshift-hypershift-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/hypershift/openshift-hypershift-release-4.23-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/hypershift/openshift-hypershift-release-4.23-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/hypershift/openshift-hypershift-release-5.0-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/hypershift/openshift-hypershift-release-5.0-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (17)
ci-operator/config/openshift/hypershift/openshift-hypershift-main.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yamlci-operator/step-registry/hypershift/self-managed-azure/OWNERSci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/OWNERSci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/hypershift-self-managed-azure-e2e-backuprestore-workflow.metadata.jsonci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/hypershift-self-managed-azure-e2e-backuprestore-workflow.yamlci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/OWNERSci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/hypershift-self-managed-azure-oadp-destroy-commands.shci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/hypershift-self-managed-azure-oadp-destroy-ref.metadata.jsonci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/hypershift-self-managed-azure-oadp-destroy-ref.yamlci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/OWNERSci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.shci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-ref.metadata.jsonci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-ref.yaml
| - as: e2e-v2-azure-self-managed-backuprestore | ||
| cron: 0 6 * * 0 | ||
| steps: | ||
| cluster_profile: hypershift-azure | ||
| env: | ||
| HYPERSHIFT_AZURE_LOCATION: centralus | ||
| TEST_SUITE: openshift/conformance/parallel/minimal | ||
| workflow: hypershift-self-managed-azure-e2e-backuprestore |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'as: e2e-v2-azure-self-managed-backuprestore|NODEPOOL_N1_VERSION|workflow: hypershift-self-managed-azure-e2e-backuprestore' \
ci-operator/config/openshift/hypershift
rg -n -C 4 \
'NODEPOOL_N1_VERSION|HYPERSHIFT_NODE_COUNT' \
ci-operator/step-registry/hypershift/self-managed-azure \
ci-operator/step-registry/hypershift/e2e-backuprestoreRepository: openshift/release
Length of output: 38651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow and step definitions ---'
fd -i 'backuprestore' ci-operator/step-registry/hypershift/self-managed-azure ci-operator/step-registry/hypershift/e2e-backuprestore 2>/dev/null || true
rg -n -C 8 \
'NODEPOOL_N[1-9]_VERSION|env:|workflow: hypershift-self-managed-azure-e2e-backuprestore|chain:|ref:|from:' \
ci-operator/step-registry/hypershift/self-managed-azure \
ci-operator/step-registry/hypershift/e2e-backuprestore
printf '%s\n' '--- all matching jobs and inherited configuration ---'
rg -n -C 12 \
'as: e2e-v2-azure-self-managed-backuprestore|workflow: hypershift-self-managed-azure-e2e-backuprestore|NODEPOOL_N1_VERSION' \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-main.yaml
printf '%s\n' '--- repository guidance for periodic configuration generation ---'
rg -n -C 4 \
'make update|__periodics|zz_generated_metadata|source of truth' \
ci-operator/config README.md Makefile 2>/dev/null || trueRepository: openshift/release
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant workflow file ---'
cat -n ci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/hypershift-self-managed-azure-e2e-backuprestore-workflow.yaml
printf '%s\n' '--- NODEPOOL_N1_VERSION references in the step registry ---'
rg -n -C 3 --glob '*.yaml' --glob '*.yml' \
'NODEPOOL_N1_VERSION' ci-operator/step-registry/hypershift ci-operator/step-registry/clusterbot 2>/dev/null || true
printf '%s\n' '--- exact job definitions in source configs ---'
sed -n '225,255p' ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23.yaml
sed -n '398,418p' ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml
sed -n '228,256p' ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0.yaml
sed -n '429,449p' ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml
printf '%s\n' '--- references to the workflow and variable outside generated job listings ---'
rg -n -C 3 \
'hypershift-self-managed-azure-e2e-backuprestore|NODEPOOL_N1_VERSION' \
ci-operator/step-registry/hypershift/self-managed-azure \
ci-operator/step-registry/hypershift/e2e-backuprestore \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0.yaml \
ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yamlRepository: openshift/release
Length of output: 24764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resolver definition ---'
cat -n ci-operator/step-registry/hypershift/resolve-nodepool-releases/hypershift-resolve-nodepool-releases-ref.yaml
printf '%s\n' '--- resolver implementation and consumers ---'
rg -n -C 8 \
'resolve-nodepool-releases|NODEPOOL_N1_VERSION|NODEPOOL_N2_VERSION|NODEPOOL_N3_VERSION|NODEPOOL_N4_VERSION' \
ci-operator/step-registry/hypershift \
--glob '*.yaml' --glob '*.yml' --glob '*.sh' --glob '*.py' --glob '*.go'
printf '%s\n' '--- all files in the resolver directory ---'
find ci-operator/step-registry/hypershift/resolve-nodepool-releases -maxdepth 1 -type f -printRepository: openshift/release
Length of output: 24085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n ci-operator/step-registry/hypershift/resolve-nodepool-releases/hypershift-resolve-nodepool-releases-commands.sh
printf '%s\n' '--- related node pool image consumers ---'
rg -n -C 6 \
'nodepool_release_images|NODEPOOL_N1_VERSION|NODEPOOL_N2_VERSION|NODEPOOL_N3_VERSION|NODEPOOL_N4_VERSION' \
ci-operator/step-registry/hypershift \
--glob '*.sh' --glob '*.yaml' --glob '*.yml' \
--glob '!**/resolve-nodepool-releases/**'Repository: openshift/release
Length of output: 13668
Set NODEPOOL_N1_VERSION: "4.22" in both periodic jobs.
The resolver skips empty version inputs, so these jobs omit the 4.22 node pool coverage present in the matching optional jobs.
📍 Affects 2 files
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml#L406-L413(this comment)ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml#L437-L444
🤖 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
`@ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml`
around lines 406 - 413, Set NODEPOOL_N1_VERSION to "4.22" in both periodic job
definitions:
ci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23__periodics.yaml
lines 406-413 and
ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml
lines 437-444. Add the value under each job’s env section while preserving the
existing job configuration.
| STORAGE_ACCOUNT_NAME="oadp${CLUSTER_NAME:0:20}" | ||
| STORAGE_ACCOUNT_NAME="$(echo "${STORAGE_ACCOUNT_NAME}" | tr -cd '[:lower:][:digit:]' | cut -c1-24)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -t f 'hypershift-self-managed-azure-oadp-setup-commands\.sh$' . | head -n 1)"
printf '%s\n' "FILE=$file"
cat -n "$file" | sed -n '1,140p'
printf '%s\n' '--- related storage-account name logic ---'
rg -n -C 3 'STORAGE_ACCOUNT_NAME|storage account|storageaccount|SHARED_DIR' ci-operator/step-registry ci-operator 2>/dev/null | head -n 240
printf '%s\n' '--- script metadata ---'
stat -c '%n %s bytes' "$file"Repository: openshift/release
Length of output: 36362
🌐 Web query:
Azure Storage account naming requirements globally unique lowercase alphanumeric 24 characters
💡 Result:
To create an Azure Storage account, you must adhere to the following naming requirements: 1. Length: The name must be between 3 and 24 characters in length [1][2][3]. 2. Characters: The name may contain only numbers and lowercase letters [1][2][3]. 3. Uniqueness: The name must be globally unique within Azure [2]. No two storage accounts can share the same name [2]. In addition to these rules, the name must not contain reserved words or trademarks [4]. Because these accounts have a public endpoint, these naming restrictions are necessary to ensure the generated URL is valid and unique across the global Azure platform [2][4].
Citations:
- 1: https://learn.microsoft.com/en-us/rest/api/storagerp/storage-accounts/create?view=rest-storagerp-2024-01-01
- 2: https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview
- 3: https://learn.microsoft.com/en-us/azure/storage/common/storage-account-create?tabs=azure-portal
- 4: https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
def current(name):
value = "oadp" + name[:20]
return re.sub(r"[^a-z0-9]", "", value)[:24]
def proposed(name, suffix="0123456789abcdef"):
stem = re.sub(r"[^a-z0-9]", "", name)[:12]
return "oadp" + stem + suffix
names = [
"public-hostedcluster-alpha-aaaaaaaa",
"public-hostedcluster-alpha-bbbbbbbb",
"PUBLIC-hostedcluster-alpha-aaaaaaaa",
]
for name in names:
print(f"{name!r}: current={current(name)!r}, proposed={proposed(name)!r}, proposed_length={len(proposed(name))}")
print("current collision:", current(names[0]) == current(names[1]))
print("proposed length:", len(proposed(names[0])))
PY
printf '%s\n' '--- CI job identity variables used by step-registry scripts ---'
rg -n -g '*-commands.sh' \
'\b(BUILD_ID|JOB_ID|BUILD_NUMBER|JOB_NAME|PROW_JOB_ID|POD_NAME|BUILD_TAG|JOB_SPEC)\b' \
ci-operator/step-registry | head -n 160
printf '%s\n' '--- cleanup consumers ---'
rg -n -C 5 'oadp-storage-account-name|oadp-storage-resourcegroup|STORAGE_ACCOUNT_NAME' \
ci-operator/step-registry | head -n 200Repository: openshift/release
Length of output: 46089
Make the storage account name collision-resistant.
The current logic can produce the same name for different CLUSTER_NAME values. Use a job-unique lowercase alphanumeric suffix, such as a short hash of ${PROW_JOB_ID}, and keep the complete name within 24 characters. The proposed 12-character stem plus 16-character suffix would produce 32 characters and fail Azure validation. Continue writing the generated name to ${SHARED_DIR} for cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.sh`
around lines 22 - 23, Update the STORAGE_ACCOUNT_NAME generation to retain a
valid lowercase alphanumeric cluster stem and append a job-unique suffix derived
from PROW_JOB_ID, ensuring the complete Azure storage account name is no longer
than 24 characters. Preserve writing the generated name to SHARED_DIR so cleanup
continues to work.
| # Wait for Velero pod to be ready | ||
| echo "Waiting for Velero pod to be ready..." | ||
| oc wait --for=condition=Available deployment/velero -n openshift-adp --timeout=300s || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail setup when Velero does not become available.
Line 115 ignores a missing or unavailable deployment/velero. The workflow runs the backup and restore test immediately after this step. The job then fails later with less useful diagnostics.
Remove || true. If diagnostic output is required, collect it before returning a nonzero status.
🤖 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
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.sh`
around lines 113 - 115, Update the Velero readiness check in the setup command
so oc wait --for=condition=Available deployment/velero returns a nonzero status
when the deployment is missing or unavailable. Remove the trailing || true,
preserving the existing timeout and namespace settings so setup fails
immediately with the readiness error.
| documentation: |- | ||
| The Azure Blob Storage container (bucket) name used for OADP backups. | ||
| If empty, a container will be created using the target cluster name. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the default container-name documentation.
Lines 16-18 state that the default uses only the target cluster name. The command script uses hypershift-oadp-${CLUSTER_NAME}. Update the documentation to describe the actual default.
🤖 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
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-ref.yaml`
around lines 16 - 18, Update the documentation block for the Azure Blob Storage
container name to state that an empty value defaults to a container named using
the hypershift-oadp-${CLUSTER_NAME} pattern, matching the command script
behavior.
|
/pj-rehearse periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-v2-azure-self-managed-backuprestore |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/uncc @bryan-cox @mgencur feel free to add me when its ready for review. |
a15b3ae to
97f100e
Compare
mgencur
changed the title
|
@mgencur: This pull request references CNTRLPLANE-3201 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
needs-rebase
|
@mgencur, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
Add hypershift-self-managed-azure-oadp-setup and -oadp-destroy steps that discover the public self-managed-Azure guest cluster and create/ tear down an Azure Storage Account and Blob container for OADP, along with a DataProtectionApplication, BackupStorageLocation, and VolumeSnapshotLocation, mirroring the existing AWS OADP setup. Wire these into a new hypershift-self-managed-azure-e2e-backuprestore workflow (reusing e2e-v2-azure-self-managed's pre-steps) and a new e2e-v2-azure-self-managed-backuprestore test running the minimal conformance suite after backup/restore.
Branch the e2e-v2-azure-self-managed-backuprestore presubmit to release-4.23 and release-5.0, and add a weekly periodic version of it to both release branches' __periodics.yaml files.
…dp-setup Make the target guest cluster name filter configurable via CLUSTER_PREFIX (default "public") and remove the stray [default] header from the Azure credentials file.
97f100e to
b3e25be
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@mgencur: The following test failed, say
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. |
3 participants
https://redhat.atlassian.net/browse/CNTRLPLANE-3201
Summary by CodeRabbit