SDCICD-1844: Add ephemeral ROSA deploy script for hypershift-pr-check#3272
SDCICD-1844: Add ephemeral ROSA deploy script for hypershift-pr-check#3272minlei98 wants to merge 1 commit into
Conversation
|
There are test jobs defined for this repository which are not configured to run automatically. Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@minlei98: This pull request references SDCICD-1844 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 task 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. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: minlei98 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/ephemeral-rosa-deploy.sh`:
- Around line 72-85: Persist the ephemeral namespace so the `release` entrypoint
can reuse it later instead of starting with an empty `EPHEMERAL_NS` and exiting
early. Update the deploy path in `scripts/ephemeral-rosa-deploy.sh` alongside
the existing `cluster-id` write so it also saves `EPHEMERAL_NS` to
`${SHARED_DIR}`, and have the `release` flow read that value back before
attempting `bonfire namespace release`. Keep the fix localized around the
`deploy`/`release` handling and the `EPHEMERAL_NS`/`CLUSTER_ID` persistence
logic.
- Around line 26-28: Create the bonfire virtualenv in a unique temporary
directory instead of hardcoding /tmp/bonfire-venv, because the current setup can
fail on existing stale state or concurrent runs. Update the
ephemeral-rosa-deploy.sh setup around the venv creation and activation steps to
use a mktemp-generated directory, store it in a variable, source its
bin/activate path, and add cleanup on exit so the temporary venv is removed
automatically.
- Around line 14-15: Guard the shell variable setup in the ephemeral deploy
script so `SHARED_DIR` does not eagerly expand `ARTIFACT_DIR` when both are
unset; update the initialization around the `EPHEMERAL_CREDS_DIR` and
`SHARED_DIR` assignments to either lazily derive `SHARED_DIR` or explicitly
require `ARTIFACT_DIR`. Also make the `deploy`/`release` flow persist
`EPHEMERAL_NS` across invocations by writing it out during deployment and
reading it back during release so cleanup can find the prior namespace.
🪄 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: 81c80c09-98cd-4d63-968b-efc4b987076a
📒 Files selected for processing (2)
configs/ephemeral-cluster.yamlscripts/ephemeral-rosa-deploy.sh
| EPHEMERAL_CREDS_DIR="${EPHEMERAL_CREDS_DIR:-/usr/local/ci-secrets/ephemeral-cluster}" | ||
| SHARED_DIR="${SHARED_DIR:-${ARTIFACT_DIR}/shared}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files scripts/ephemeral-rosa-deploy.sh
echo '---'
nl -ba scripts/ephemeral-rosa-deploy.sh | sed -n '1,220p'Repository: openshift/osde2e
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba scripts/ephemeral-rosa-deploy.sh | sed -n '1,140p'Repository: openshift/osde2e
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' scripts/ephemeral-rosa-deploy.sh | cat -nRepository: openshift/osde2e
Length of output: 4543
Guard ARTIFACT_DIR initialization and persist the namespace for cleanup.
SHARED_DIR="${SHARED_DIR:-${ARTIFACT_DIR}/shared}" expands ARTIFACT_DIR at source time, so set -u aborts when both variables are unset. Initialize it lazily or require ARTIFACT_DIR explicitly. The standalone deploy|release path also loses EPHEMERAL_NS between invocations, so release cannot tear down a prior deployment unless the namespace is persisted.
🤖 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 `@scripts/ephemeral-rosa-deploy.sh` around lines 14 - 15, Guard the shell
variable setup in the ephemeral deploy script so `SHARED_DIR` does not eagerly
expand `ARTIFACT_DIR` when both are unset; update the initialization around the
`EPHEMERAL_CREDS_DIR` and `SHARED_DIR` assignments to either lazily derive
`SHARED_DIR` or explicitly require `ARTIFACT_DIR`. Also make the
`deploy`/`release` flow persist `EPHEMERAL_NS` across invocations by writing it
out during deployment and reading it back during release so cleanup can find the
prior namespace.
There was a problem hiding this comment.
| EPHEMERAL_CREDS_DIR="${EPHEMERAL_CREDS_DIR:-/usr/local/ci-secrets/ephemeral-cluster}" | |
| SHARED_DIR="${SHARED_DIR:-${ARTIFACT_DIR}/shared}" | |
| EPHEMERAL_CREDS_DIR="${EPHEMERAL_CREDS_DIR:-/usr/local/ci-secrets/ephemeral-cluster}" | |
| SHARED_DIR="${SHARED_DIR:-${ARTIFACT_DIR:-/tmp}/shared}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline =="
ast-grep outline scripts/ephemeral-rosa-deploy.sh --view expanded || true
echo
echo "== full file =="
nl -ba scripts/ephemeral-rosa-deploy.sh | sed -n '1,220p'Length of output: 636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scripts/ephemeral-rosa-deploy.sh =="
cat -n scripts/ephemeral-rosa-deploy.sh | sed -n '1,220p'Length of output: 4701
@YiqinZhang This addresses the first issue on Line 15: it avoids the set -u failure when ARTIFACT_DIR is unset.
One gap remains: the standalone release path is still not recoverable. EPHEMERAL_NS is discovered during deploy on Line 72, but ephemeral_rosa_release() on Line 89 only uses the in-memory variable. So ./scripts/ephemeral-rosa-deploy.sh release cannot clean up a previous deploy invocation.
A minimal production-grade follow-up is to persist and reload the namespace, e.g.:
EPHEMERAL_NS_FILE="${SHARED_DIR}/ephemeral-ns"
# after EPHEMERAL_NS is resolved in deploy
echo -n "${EPHEMERAL_NS}" > "${EPHEMERAL_NS_FILE}"
# at the start of release
if [[ -z "${EPHEMERAL_NS}" && -f "${EPHEMERAL_NS_FILE}" ]]; then
EPHEMERAL_NS="$(cat "${EPHEMERAL_NS_FILE}")"
fiAfter successful release, the file should be removed as well.
| EPHEMERAL_NS=$(grep -oE "namespace 'ephemeral-[^']+'" "${deploy_log}" | tail -1 | tr -d "'" | awk '{print $2}') | ||
| if [[ -z "${EPHEMERAL_NS}" ]]; then | ||
| EPHEMERAL_NS=$(bonfire namespace list --mine 2>/dev/null | awk '/ephemeral-/ {print $1; exit}') | ||
| fi | ||
| test -n "${EPHEMERAL_NS}" | ||
| bonfire namespace describe "${EPHEMERAL_NS}" | grep -q 'ROSA Cluster configuration detected' | ||
|
|
||
| local cluster_id | ||
| cluster_id=$(_ephemeral_rosa_get_cluster_id_from_capi "${EPHEMERAL_NS}") | ||
| test -n "${cluster_id}" | ||
| echo -n "${cluster_id}" > "${SHARED_DIR}/cluster-id" | ||
| echo "ephemeral namespace=${EPHEMERAL_NS} cluster_id=${cluster_id}" | ||
| export EPHEMERAL_NS | ||
| export CLUSTER_ID="${cluster_id}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Persist the namespace for the release entrypoint.
deploy writes only ${SHARED_DIR}/cluster-id. A later ./scripts/ephemeral-rosa-deploy.sh release starts with an empty EPHEMERAL_NS, hits Line 89, and exits without releasing the bonfire namespace. That leaks the reservation in the exact multi-step Prow flow this helper is meant to support.
Suggested fix
test -n "${EPHEMERAL_NS}"
bonfire namespace describe "${EPHEMERAL_NS}" | grep -q 'ROSA Cluster configuration detected'
@@
cluster_id=$(_ephemeral_rosa_get_cluster_id_from_capi "${EPHEMERAL_NS}")
test -n "${cluster_id}"
+ echo -n "${EPHEMERAL_NS}" > "${SHARED_DIR}/ephemeral-namespace"
echo -n "${cluster_id}" > "${SHARED_DIR}/cluster-id"
@@
ephemeral_rosa_release() {
+ if [[ -z "${EPHEMERAL_NS}" && -f "${SHARED_DIR}/ephemeral-namespace" ]]; then
+ EPHEMERAL_NS="$(cat "${SHARED_DIR}/ephemeral-namespace")"
+ fi
if [[ -z "${EPHEMERAL_NS}" ]]; then
return 0
fi
@@
EPHEMERAL_NS=""
+ rm -f "${SHARED_DIR}/ephemeral-namespace"
export EPHEMERAL_NS
}Also applies to: 88-96
🤖 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 `@scripts/ephemeral-rosa-deploy.sh` around lines 72 - 85, Persist the ephemeral
namespace so the `release` entrypoint can reuse it later instead of starting
with an empty `EPHEMERAL_NS` and exiting early. Update the deploy path in
`scripts/ephemeral-rosa-deploy.sh` alongside the existing `cluster-id` write so
it also saves `EPHEMERAL_NS` to `${SHARED_DIR}`, and have the `release` flow
read that value back before attempting `bonfire namespace release`. Keep the fix
localized around the `deploy`/`release` handling and the
`EPHEMERAL_NS`/`CLUSTER_ID` persistence logic.
|
/retest |
| export KUBECONFIG="${EPH_KUBECONFIG}" | ||
| set +x | ||
| oc login --token="${OC_LOGIN_TOKEN}" --server="${OC_LOGIN_SERVER}" --insecure-skip-tls-verify=true >/dev/null | ||
| set -x 2>/dev/null || true |
There was a problem hiding this comment.
| set -x 2>/dev/null || true | |
| [[ $- == *x* ]] && _was_tracing=true || _was_tracing=false | |
| set +x | |
| oc login --token="${OC_LOGIN_TOKEN}" --server="${OC_LOGIN_SERVER}" --insecure-skip-tls-verify=true >/dev/null | |
| $_was_tracing && set -x |
Provide bonfire deploy/release helpers and ephemeral-cluster config so Prow can provision a ROSA HCP cluster and run hypershift tests without cluster reserve or osde2e destroy. Merge dependency: merge this PR to openshift/osde2e main before the companion openshift/release PR (SDCICD-1844). The release job sources scripts/ephemeral-rosa-deploy.sh and configs/ephemeral-cluster.yaml from this repository at runtime. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@minlei98: 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. |
3 participants
Provide bonfire deploy/release helpers and ephemeral-cluster config so Prow can provision a ROSA HCP cluster and run hypershift tests without cluster reserve or osde2e destroy.
Summary by CodeRabbit
New Features
Bug Fixes
Chores