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 7 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 |
WalkthroughThe pull request adds Azure self-managed backup and restore workflows for HyperShift. It provisions OADP resources, runs scheduled and optional CI jobs, and cleans up Azure storage and identities. ChangesAzure self-managed backup and restore
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIJob
participant AzureWorkflow
participant OADPSetup
participant GuestCluster
participant Azure
participant OADPDestroy
CIJob->>AzureWorkflow: Start Azure backup and restore test
AzureWorkflow->>OADPSetup: Provision OADP prerequisites
OADPSetup->>Azure: Create storage and managed identity
OADPSetup->>GuestCluster: Configure workload identity and OADP
AzureWorkflow->>GuestCluster: Run backup, restore, and conformance tests
AzureWorkflow->>OADPDestroy: Request resource cleanup
OADPDestroy->>Azure: Delete storage and managed identity
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 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
…pgrade Add TECH_PREVIEW_NO_UPGRADE: "true" to the e2e-v2-aws-backuprestore periodic in the release-4.23 and release-5.0 configs.
…ewNoUpgrade Add TECH_PREVIEW_NO_UPGRADE: "true" to hypershift-self-managed-azure-e2e-backuprestore in main, release-4.23, and release-5.0 configs.
|
/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. |
|
/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. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.23-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. |
|
Hitting this when running the BackupRestoreEtcdSnapshot test: Will need to investigate this. Will need to follow https://redhat.atlassian.net/browse/CNTRLPLANE-3167 |
|
@mgencur: The following tests 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. |
…lero and etcd-backup on self-managed Azure oadp-setup now provisions one managed identity federated to both the velero and etcd-backup-job ServiceAccounts, so both Velero's object-store plugin and the HCPEtcdBackup controller authenticate via Workload Identity instead of the shared root Service Principal, which had no federated credential and caused AADSTS70025 failures. Adds the Workload-Identity pod labels/SA annotation OADP needs, sets useAAD on the BSL, and drops the now-dead standalone etcd-backup credential secret. oadp-destroy cleans up the renamed identity marker files.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/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. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.sh (2)
186-187: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the secret creation idempotent.
oc create secretfails ifcloud-credentialsalready exists inopenshift-adp. Underset -ethe step then aborts. The rest of the script usesoc apply, which is idempotent.oc create secret generic cloud-credentials -n openshift-adp \ --from-file cloud="${AZURE_CREDS_FILE}" \ --dry-run=client -o yaml | oc apply -f -🤖 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 186 - 187, Update the cloud-credentials creation command in the OADP setup script to use a client-side dry-run piped into oc apply, preserving the existing secret name, namespace, and cloud credentials file while making repeated runs succeed.
55-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd retry to the blob container creation.
--auth-mode loginrequires a data-plane RBAC role for the service principal on the newly created account. Azure RBAC propagation is delayed for tens of seconds after account creation. This single attempt can fail withAuthorizationPermissionMismatchand abort the step. The script already retries the role assignment and identity lookups for the same reason.Wrap this call in the same retry pattern used at Lines 158-170.
🤖 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 55 - 60, Update the blob container creation command near the “Creating blob container” message to use the same retry pattern as the role-assignment and identity-lookup logic around the referenced retry block. Retry Azure failures caused by delayed RBAC propagation while preserving the existing container name, storage account, login authentication, and output settings.
🤖 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/step-registry/hypershift/self-managed-azure/oadp-destroy/hypershift-self-managed-azure-oadp-destroy-commands.sh`:
- Around line 30-32: Guard both marker-file reads in the cleanup script: before
reading STORAGE_RESOURCEGROUP_MARKER and OADP_MI_RG_MARKER, verify each file
exists; when either is missing, record the failure in OVERALL_RESULT and
continue cleanup so set -e does not abort before managed identity removal.
Preserve the existing reads when the markers are present.
In
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.sh`:
- Around line 16-21: Update the CLUSTER_NAME assignment to tolerate a
non-matching grep by appending || true within the command substitution, allowing
the subsequent empty-result guard to execute and print diagnostics. Preserve the
existing HostedCluster lookup and failure handling.
- Around line 99-107: Update the retry loop around the OADP_MI_CLIENT_ID and
OADP_MI_PRINCIPAL_ID assignments so transient az identity show failures do not
terminate the script under set -e. Suppress or otherwise safely capture
command-substitution failures, allowing the loop to reach its retry message and
continue until both IDs are populated or the existing retry limit is exhausted.
---
Nitpick comments:
In
`@ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-commands.sh`:
- Around line 186-187: Update the cloud-credentials creation command in the OADP
setup script to use a client-side dry-run piped into oc apply, preserving the
existing secret name, namespace, and cloud credentials file while making
repeated runs succeed.
- Around line 55-60: Update the blob container creation command near the
“Creating blob container” message to use the same retry pattern as the
role-assignment and identity-lookup logic around the referenced retry block.
Retry Azure failures caused by delayed RBAC propagation while preserving the
existing container name, storage account, login authentication, and output
settings.
🪄 Autofix
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: 9e3f3ac1-98c3-4959-a023-68f83d85e8a9
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (14)
- ci-operator/step-registry/hypershift/self-managed-azure/oadp-setup/hypershift-self-managed-azure-oadp-setup-ref.metadata.json
- ci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/hypershift-self-managed-azure-oadp-destroy-ref.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-main.yaml
- ci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/OWNERS
- ci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/OWNERS
- ci-operator/step-registry/hypershift/self-managed-azure/oadp-destroy/hypershift-self-managed-azure-oadp-destroy-ref.metadata.json
- ci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/hypershift-self-managed-azure-e2e-backuprestore-workflow.yaml
- ci-operator/step-registry/hypershift/self-managed-azure/e2e-backuprestore/hypershift-self-managed-azure-e2e-backuprestore-workflow.metadata.json
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0__periodics.yaml
- ci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0.yaml
- 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/step-registry/hypershift/self-managed-azure/oadp-setup/OWNERS
- ci-operator/step-registry/hypershift/self-managed-azure/OWNERS
| if [[ -f "${STORAGE_ACCOUNT_MARKER}" ]]; then | ||
| STORAGE_ACCOUNT_NAME="$(cat "${STORAGE_ACCOUNT_MARKER}")" | ||
| RESOURCEGROUP="$(cat "${STORAGE_RESOURCEGROUP_MARKER}")" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A missing resource-group marker aborts the whole cleanup.
Line 32 reads ${STORAGE_RESOURCEGROUP_MARKER} without checking that the file exists. The setup step writes the storage account name and its resource group on consecutive lines, but it can fail between the two writes. cat then fails and set -e terminates the step at Line 32. The managed identity cleanup at Lines 61-85 never runs, and the identity leaks in the shared os4-common resource group.
Guard the read and record the failure in OVERALL_RESULT instead of exiting. The same pattern applies at Line 63 for ${OADP_MI_RG_MARKER}.
🐛 Proposed fix
-if [[ -f "${STORAGE_ACCOUNT_MARKER}" ]]; then
+if [[ -f "${STORAGE_ACCOUNT_MARKER}" && -f "${STORAGE_RESOURCEGROUP_MARKER}" ]]; then
STORAGE_ACCOUNT_NAME="$(cat "${STORAGE_ACCOUNT_MARKER}")"
RESOURCEGROUP="$(cat "${STORAGE_RESOURCEGROUP_MARKER}")"🤖 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-destroy/hypershift-self-managed-azure-oadp-destroy-commands.sh`
around lines 30 - 32, Guard both marker-file reads in the cleanup script: before
reading STORAGE_RESOURCEGROUP_MARKER and OADP_MI_RG_MARKER, verify each file
exists; when either is missing, record the failure in OVERALL_RESULT and
continue cleanup so set -e does not abort before managed identity removal.
Preserve the existing reads when the markers are present.
| CLUSTER_NAME="$(oc get hostedcluster -n clusters -o jsonpath='{.items[*].metadata.name}' | tr ' ' '\n' | grep "${CLUSTER_PREFIX}" | head -n1)" | ||
| if [[ -z "${CLUSTER_NAME}" ]]; then | ||
| echo "!!! Unable to find a public HostedCluster in the 'clusters' namespace" | ||
| oc get hostedcluster -n clusters | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The empty-result guard at Lines 17-21 is unreachable.
CLUSTER_NAME="$(...)" is a simple assignment, so its exit status is the exit status of the command substitution. If grep matches nothing, it returns 1 and set -e terminates the script at Line 16. The diagnostic output from oc get hostedcluster -n clusters never runs.
Add || true to the substitution so the explicit check runs.
🐛 Proposed fix
-CLUSTER_NAME="$(oc get hostedcluster -n clusters -o jsonpath='{.items[*].metadata.name}' | tr ' ' '\n' | grep "${CLUSTER_PREFIX}" | head -n1)"
+CLUSTER_NAME="$(oc get hostedcluster -n clusters -o jsonpath='{.items[*].metadata.name}' | tr ' ' '\n' | grep "${CLUSTER_PREFIX}" | head -n1 || true)"📝 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.
| CLUSTER_NAME="$(oc get hostedcluster -n clusters -o jsonpath='{.items[*].metadata.name}' | tr ' ' '\n' | grep "${CLUSTER_PREFIX}" | head -n1)" | |
| if [[ -z "${CLUSTER_NAME}" ]]; then | |
| echo "!!! Unable to find a public HostedCluster in the 'clusters' namespace" | |
| oc get hostedcluster -n clusters | |
| exit 1 | |
| fi | |
| CLUSTER_NAME="$(oc get hostedcluster -n clusters -o jsonpath='{.items[*].metadata.name}' | tr ' ' '\n' | grep "${CLUSTER_PREFIX}" | head -n1 || true)" | |
| if [[ -z "${CLUSTER_NAME}" ]]; then | |
| echo "!!! Unable to find a public HostedCluster in the 'clusters' namespace" | |
| oc get hostedcluster -n clusters | |
| exit 1 | |
| fi |
🤖 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 16 - 21, Update the CLUSTER_NAME assignment to tolerate a
non-matching grep by appending || true within the command substitution, allowing
the subsequent empty-result guard to execute and print diagnostics. Preserve the
existing HostedCluster lookup and failure handling.
| for attempt in $(seq 1 5); do | ||
| OADP_MI_CLIENT_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query clientId -o tsv)" | ||
| OADP_MI_PRINCIPAL_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query principalId -o tsv)" | ||
| if [[ -n "${OADP_MI_CLIENT_ID}" && -n "${OADP_MI_PRINCIPAL_ID}" ]]; then | ||
| break | ||
| fi | ||
| echo "Attempt ${attempt}/5: identity not fully propagated yet, retrying in 10s..." | ||
| sleep 10 | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
set -e defeats this retry loop.
Lines 100-101 assign command substitutions directly. If az identity show fails transiently, the assignment exits nonzero and set -e terminates the script before the retry and before the error message at Lines 108-111. The loop only helps when az succeeds but returns an empty value.
Suppress the failure so the loop can retry.
🐛 Proposed fix
- OADP_MI_CLIENT_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query clientId -o tsv)"
- OADP_MI_PRINCIPAL_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query principalId -o tsv)"
+ OADP_MI_CLIENT_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query clientId -o tsv || true)"
+ OADP_MI_PRINCIPAL_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query principalId -o tsv || true)"📝 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.
| for attempt in $(seq 1 5); do | |
| OADP_MI_CLIENT_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query clientId -o tsv)" | |
| OADP_MI_PRINCIPAL_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query principalId -o tsv)" | |
| if [[ -n "${OADP_MI_CLIENT_ID}" && -n "${OADP_MI_PRINCIPAL_ID}" ]]; then | |
| break | |
| fi | |
| echo "Attempt ${attempt}/5: identity not fully propagated yet, retrying in 10s..." | |
| sleep 10 | |
| done | |
| for attempt in $(seq 1 5); do | |
| OADP_MI_CLIENT_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query clientId -o tsv || true)" | |
| OADP_MI_PRINCIPAL_ID="$(az identity show --name "${OADP_MI_NAME}" --resource-group "${OADP_MI_RESOURCEGROUP}" --query principalId -o tsv || true)" | |
| if [[ -n "${OADP_MI_CLIENT_ID}" && -n "${OADP_MI_PRINCIPAL_ID}" ]]; then | |
| break | |
| fi | |
| echo "Attempt ${attempt}/5: identity not fully propagated yet, retrying in 10s..." | |
| sleep 10 | |
| done |
🤖 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 99 - 107, Update the retry loop around the OADP_MI_CLIENT_ID and
OADP_MI_PRINCIPAL_ID assignments so transient az identity show failures do not
terminate the script under set -e. Suppress or otherwise safely capture
command-substitution failures, allowing the loop to reach its retry message and
continue until both IDs are populated or the existing retry limit is exhausted.
3 participants
https://redhat.atlassian.net/browse/CNTRLPLANE-3201
Summary by CodeRabbit