Conversation
Remove all packer infrastructure and operator-built image flow: - Delete packer-resource-cleanup.sh script - Remove packer from binary packages and lib.sh functions - Delete packer build functions from all cloud provider handlers - Remove download_source_code, prepare_source_code functions - Delete get_all_ami_ids orphaned function from AWS handler The operator now only supports pre-built images via PODVM_IMAGE_URI. Signed-off-by: Snir Schreiber <ssheribe@redhat.com> Assisted-by: Claude AI
Remove IMAGE_TYPE variable and related conditionals from all handlers. Since the operator-built flow has been deprecated, IMAGE_TYPE was always set to "pre-built". Simplify the code by: - Removing IMAGE_TYPE conditionals from handlers (AWS, Azure, GCP, Libvirt) - Calling prebuilt artifact functions directly - Renaming set_podvm_image_type() to validate_podvm_image_uri() - Updating documentation to reflect simplified flow Assisted-by: Claude AI Signed-off-by: Snir Schreiber <ssheribe@redhat.com>
Remove the initContainer that copied podvm-binaries.tar.gz to /payload and the associated payload volume. This was only used by the deleted prepare_source_code() function in the operator-built flow. The pre-built flow does not need podvm-binaries.tar.gz as it pulls complete pre-built images from container registries. Assisted-by: Claude AI Signed-off-by: Snir Schreiber <ssheribe@redhat.com>
Remove configuration variables that were only used by the deprecated operator-built image flow: - PODVM_DISTRO, CAA_SRC, CAA_REF, DOWNLOAD_SOURCES - INSTANCE_TYPE, VM_SIZE (never functionally used) - DISABLE_CLOUD_CONFIG, BOOT_FIPS, AGENT_POLICY - ENABLE_NVIDIA_GPU, NVIDIA_DRIVER_VERSION, NVIDIA_USERSPACE_VERSION - BASE_IMAGE_*, BASE_OS_VERSION - ORG_ID, ACTIVATION_KEY Keep CONFIDENTIAL_COMPUTE_ENABLED only where it's used. Assisted-by: Claude AI Signed-off-by: Snir Schreiber <ssheribe@redhat.com>
Rename create_image_from_prebuilt_artifact to create_gcp_image_from_prebuilt_artifact for consistency with other cloud providers Assisted-by: Claude AI Signed-off-by: Snir Schreiber <ssheribe@redhat.com>
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThis pull request removes support for building PodVM images from scratch using Packer and enforces using prebuilt images instead. The changes eliminate packer-based image creation workflows, configuration parameters, and supporting infrastructure across all cloud providers (AWS, Azure, GCP, IBM Cloud, libvirt), restructuring image handlers to only pull from prebuilt artifact URIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/peerpods/podvm/aws-podvm-image-handler.sh`:
- Line 339: The prebuilt-artifact flow in create_ami_from_prebuilt_artifact
leaves the temp S3 bucket and extended AWS credentials behind on any early exit
after calling prepare_for_prebuilt_artifact(); add a trap-based cleanup to
guarantee those resources are removed on error. Implement a local cleanup
function inside create_ami_from_prebuilt_artifact (e.g.,
cleanup_prebuilt_resources) that deletes the temp bucket and clears/revokes
extended credentials set by prepare_for_prebuilt_artifact, register it with trap
'ERR EXIT' immediately before calling prepare_for_prebuilt_artifact, and ensure
you remove/unset the trap (or call cleanup explicitly) on the normal success
path so resources are not double-handled. Ensure the cleanup function references
the same temp bucket variable and credential identifiers used by
prepare_for_prebuilt_artifact and is idempotent.
In `@config/peerpods/podvm/azure-podvm-image-handler.sh`:
- Line 369: The deletion fails because images created by
create_azure_image_from_prebuilt_artifact() use --os-vhd-uri and set
storageProfile.osDiskImage.source rather than storageProfile.source.id that
delete_image_using_id() expects; update delete_image_using_id() to first fetch
the image JSON, check storageProfile.source.id and if empty fall back to
storageProfile.osDiskImage.source (or another relevant field) and skip or adapt
the deletion step accordingly (e.g., skip deleting a non-existent managed source
id or delete the VHD blob when osDiskImage.source is present); ensure the code
branches so -C cleanup does not call az image delete with a missing source.id
and instead handles gallery/vhd-created images appropriately.
In `@config/peerpods/podvm/podvm-builder.sh`:
- Around line 367-368: The script currently calls validate_podvm_image_uri
unconditionally which breaks delete/delete-gallery; update the flow so
validate_podvm_image_uri is only invoked for the create path (e.g., check the
command variable or branch that handles "create" before calling
validate_podvm_image_uri). Locate the unconditional call to
validate_podvm_image_uri and move it into the create command handler or wrap it
with a conditional like if [ "$COMMAND" = "create" ]; then
validate_podvm_image_uri; fi so delete/delete-gallery continue to work without a
PODVM_IMAGE_URI.
- Around line 49-56: The script calls validate_podvm_image_uri which uses an
undefined error_exit and is run unconditionally, breaking delete flows; fix by
either replacing the undefined error_exit call inside validate_podvm_image_uri
with inline handling (e.g., echo to stderr and exit with non-zero status) or
move the validate_podvm_image_uri invocation so it runs only for create
operations (invoke it inside the create case block and do not call it for delete
or delete-gallery); update references to validate_podvm_image_uri, error_exit,
and the command dispatch (create / delete / delete-gallery) accordingly.
In `@config/peerpods/podvm/podvm-handling.md`:
- Around line 147-148: Update the documentation to remove the old `/payload`
workflow references (e.g. `/podvm-binaries.tar.gz` and the payload volume) and
align it with the URI-only flow: state that PODVM_IMAGE_URI must be set in the
cloud-provider configMap and that the operator pulls the pre-built image from
that URI and uploads it to the cloud provider; specifically remove or rewrite
the sentences describing an initContainer copying binaries into `/payload` and
any mention of mounting that volume so the text matches the current
osc-podvm-create-job.yaml which no longer includes the initContainer or mount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de6b92e9-e468-46ac-ac00-8fbaa8a11462
📒 Files selected for processing (17)
config/peerpods/podvm/Dockerfile.podvm-builderconfig/peerpods/podvm/aws-podvm-image-cm.yamlconfig/peerpods/podvm/aws-podvm-image-handler.shconfig/peerpods/podvm/azure-podvm-image-cm.yamlconfig/peerpods/podvm/azure-podvm-image-handler.shconfig/peerpods/podvm/bootc/README.mdconfig/peerpods/podvm/gcp-podvm-image-cm.yamlconfig/peerpods/podvm/gcp-podvm-image-handler.shconfig/peerpods/podvm/ibmcloud-podvm-image-cm.yamlconfig/peerpods/podvm/lib.shconfig/peerpods/podvm/libvirt-podvm-image-cm.yamlconfig/peerpods/podvm/libvirt-podvm-image-handler.shconfig/peerpods/podvm/osc-podvm-create-job.yamlconfig/peerpods/podvm/osc-podvm-delete-job.yamlconfig/peerpods/podvm/packer-resource-cleanup.shconfig/peerpods/podvm/podvm-builder.shconfig/peerpods/podvm/podvm-handling.md
💤 Files with no reviewable changes (6)
- config/peerpods/podvm/gcp-podvm-image-cm.yaml
- config/peerpods/podvm/aws-podvm-image-cm.yaml
- config/peerpods/podvm/azure-podvm-image-cm.yaml
- config/peerpods/podvm/lib.sh
- config/peerpods/podvm/ibmcloud-podvm-image-cm.yaml
- config/peerpods/podvm/packer-resource-cleanup.sh
| elif [[ "${IMAGE_TYPE}" == "pre-built" ]]; then | ||
| create_ami_from_prebuilt_artifact | ||
| fi | ||
| create_ami_from_prebuilt_artifact |
There was a problem hiding this comment.
Add failure-safe cleanup before making the prebuilt path unconditional.
create_ami_from_prebuilt_artifact() only deletes the temp bucket and cleans the extended credentials on the success path. Any error_exit after prepare_for_prebuilt_artifact() now leaves those resources behind, and this change makes that leak possible on every AMI create. Please wrap the prebuilt flow in a trap-based cleanup so retries do not inherit stale buckets or elevated AWS credentials.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/peerpods/podvm/aws-podvm-image-handler.sh` at line 339, The
prebuilt-artifact flow in create_ami_from_prebuilt_artifact leaves the temp S3
bucket and extended AWS credentials behind on any early exit after calling
prepare_for_prebuilt_artifact(); add a trap-based cleanup to guarantee those
resources are removed on error. Implement a local cleanup function inside
create_ami_from_prebuilt_artifact (e.g., cleanup_prebuilt_resources) that
deletes the temp bucket and clears/revokes extended credentials set by
prepare_for_prebuilt_artifact, register it with trap 'ERR EXIT' immediately
before calling prepare_for_prebuilt_artifact, and ensure you remove/unset the
trap (or call cleanup explicitly) on the normal success path so resources are
not double-handled. Ensure the cleanup function references the same temp bucket
variable and credential identifiers used by prepare_for_prebuilt_artifact and is
idempotent.
| elif [[ "${IMAGE_TYPE}" == "pre-built" ]]; then | ||
| create_azure_image_from_prebuilt_artifact | ||
| fi | ||
| create_azure_image_from_prebuilt_artifact |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the azure-podvm-image-handler.sh file
git ls-files | grep -i azure-podvm-image-handlerRepository: openshift/sandboxed-containers-operator
Length of output: 133
🏁 Script executed:
# Read the entire file to understand the functions
wc -l config/peerpods/podvm/azure-podvm-image-handler.shRepository: openshift/sandboxed-containers-operator
Length of output: 137
🏁 Script executed:
# Read around line 369 to see the context
sed -n '360,380p' config/peerpods/podvm/azure-podvm-image-handler.sh
# Also search for the function definitions we need to check
rg "^(create_image|create_azure_image_from_prebuilt_artifact|delete_image_using_id)" config/peerpods/podvm/azure-podvm-image-handler.sh -nRepository: openshift/sandboxed-containers-operator
Length of output: 565
🏁 Script executed:
# Search for function definitions
rg "^(create_image|create_azure_image_from_prebuilt_artifact|delete_image_using_id)\s*\(" config/peerpods/podvm/azure-podvm-image-handler.sh -nRepository: openshift/sandboxed-containers-operator
Length of output: 65
🏁 Script executed:
# Search for function definitions with simpler pattern
rg "^(create_image|create_azure_image_from_prebuilt_artifact|delete_image_using_id)" config/peerpods/podvm/azure-podvm-image-handler.sh -n
# Also try looking for function keyword
rg "create_image\(\)|create_azure_image_from_prebuilt_artifact\(\)|delete_image_using_id\(\)" config/peerpods/podvm/azure-podvm-image-handler.sh -nRepository: openshift/sandboxed-containers-operator
Length of output: 210
🏁 Script executed:
# Read the create_image function
sed -n '337,384p' config/peerpods/podvm/azure-podvm-image-handler.sh
# Read the create_azure_image_from_prebuilt_artifact function
sed -n '385,450p' config/peerpods/podvm/azure-podvm-image-handler.sh
# Read the delete_image_using_id function
sed -n '770,850p' config/peerpods/podvm/azure-podvm-image-handler.shRepository: openshift/sandboxed-containers-operator
Length of output: 7023
🏁 Script executed:
# Let's see what happens after the az sig image-version create command
sed -n '450,470p' config/peerpods/podvm/azure-podvm-image-handler.sh
# Also check if there are any tests or documentation about this
git ls-files | grep -i test | grep -i azure
# Check git history to see if there were previous implementations
git log --oneline --all -- config/peerpods/podvm/azure-podvm-image-handler.sh | head -20Repository: openshift/sandboxed-containers-operator
Length of output: 1947
🏁 Script executed:
# Check the specific commit that added source image deletion
git show 310f0a3 --stat
# Also check more recent commits
git log --oneline --all -20 -- config/peerpods/podvm/azure-podvm-image-handler.sh
# Look for any comments or issues in the code related to VHD or source deletion
rg "storageProfile|source\.id|osDiskImage" config/peerpods/podvm/azure-podvm-image-handler.sh -nRepository: openshift/sandboxed-containers-operator
Length of output: 2175
🏁 Script executed:
# Let's examine the exact changes from commit 310f0a3
git show 310f0a3
# Also check if there's any conditional logic around source deletion
sed -n '770,810p' config/peerpods/podvm/azure-podvm-image-handler.shRepository: openshift/sandboxed-containers-operator
Length of output: 3999
🏁 Script executed:
# Let's check what the actual state of the code is before the change
# Look for when create_azure_image_from_prebuilt_artifact was introduced
git log --all --pretty=format:"%h %s" -- config/peerpods/podvm/azure-podvm-image-handler.sh | grep -i "prebuilt\|vhd"
# Check the commit that introduced prebuilt support
git show 391be5c --statRepository: openshift/sandboxed-containers-operator
Length of output: 1498
🏁 Script executed:
# Let's see what the current code path is
# Check if there's any branching logic around creation methods
rg "create_image_using_packer|create_azure_image_from_prebuilt_artifact" config/peerpods/podvm/azure-podvm-image-handler.sh -B 5 -A 5
# Check the git history to see if there were different creation paths before
git show 391be5c | grep -A 30 "function create_image"Repository: openshift/sandboxed-containers-operator
Length of output: 3344
🏁 Script executed:
# Let's check the current state of create_image function more carefully
sed -n '337,390p' config/peerpods/podvm/azure-podvm-image-handler.sh
# Check if there's a branching based on IMAGE_TYPE
rg "IMAGE_TYPE|operator-built|pre-built" config/peerpods/podvm/azure-podvm-image-handler.sh -nRepository: openshift/sandboxed-containers-operator
Length of output: 1724
🏁 Script executed:
# Check if there's recent history showing when the branching was removed
git log --oneline -20 -- config/peerpods/podvm/azure-podvm-image-handler.sh
# Check the current HEAD to see all recent changes
git log -p --reverse -S "IMAGE_TYPE" -- config/peerpods/podvm/azure-podvm-image-handler.sh | head -200Repository: openshift/sandboxed-containers-operator
Length of output: 134
🏁 Script executed:
# Let's look at the most recent commit to see what changed
git show c6c0a41 -- config/peerpods/podvm/azure-podvm-image-handler.sh | head -100
# Check what commits changed the create_image function
git log -p -- config/peerpods/podvm/azure-podvm-image-handler.sh | grep -B 5 "function create_image" | head -50
# Check for any recent changes that may have simplified the branching
git log --oneline -30 -- config/peerpods/podvm/azure-podvm-image-handler.shRepository: openshift/sandboxed-containers-operator
Length of output: 4631
🏁 Script executed:
# Let me check what paths the create_azure_image_from_prebuilt_artifact uses
sed -n '385,470p' config/peerpods/podvm/azure-podvm-image-handler.sh | grep -A 10 "az sig image-version create"Repository: openshift/sandboxed-containers-operator
Length of output: 550
🏁 Script executed:
# Let me verify the Azure schema behavior by checking if there's any documentation or tests
rg "storageProfile|osDiskImage" config/peerpods/podvm/ -r
# Also let's check what the previous packer-based creation path was doing
git show 310f0a3~1:config/peerpods/podvm/azure-podvm-image-handler.sh | grep -A 50 "function delete_image"Repository: openshift/sandboxed-containers-operator
Length of output: 10527
Deletion will fail for all images created through this path.
create_azure_image_from_prebuilt_artifact() creates gallery image versions using --os-vhd-uri, which stores the source under storageProfile.osDiskImage.source rather than the managed-image storageProfile.source.id that delete_image_using_id() unconditionally queries. When -C attempts deletion, the source ID lookup will fail or return empty, causing az image delete to error and breaking the entire cleanup flow. Conditionally skip the source image deletion when storageProfile.source.id is empty, or detect the image creation method and apply appropriate deletion logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/peerpods/podvm/azure-podvm-image-handler.sh` at line 369, The deletion
fails because images created by create_azure_image_from_prebuilt_artifact() use
--os-vhd-uri and set storageProfile.osDiskImage.source rather than
storageProfile.source.id that delete_image_using_id() expects; update
delete_image_using_id() to first fetch the image JSON, check
storageProfile.source.id and if empty fall back to
storageProfile.osDiskImage.source (or another relevant field) and skip or adapt
the deletion step accordingly (e.g., skip deleting a non-existent managed source
id or delete the VHD blob when osDiskImage.source is present); ensure the code
branches so -C cleanup does not call az image delete with a missing source.id
and instead handles gallery/vhd-created images appropriately.
Comment on lines
+49
to
56
| # Function to validate that PODVM_IMAGE_URI is set. | ||
| function validate_podvm_image_uri() { | ||
| echo "Checking if PODVM_IMAGE_URI is set" | ||
|
|
||
| # If the value of the PODVM_IMAGE_URI is empty or not set, then build the image from scratch else use the prebuilt artifact. | ||
| # PODVM_IMAGE_URI is required as operator-built flow has been deprecated | ||
| if [[ -z "${PODVM_IMAGE_URI}" ]]; then | ||
| IMAGE_TYPE="operator-built" | ||
| echo "Initiating the operator to build the podvm image" | ||
| else | ||
| IMAGE_TYPE="pre-built" | ||
| echo "Initiating the operator to use the pre-built podvm image" | ||
| error_exit "PODVM_IMAGE_URI is required. The operator-built flow has been deprecated. Please provide a pre-built podvm image URI." | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="config/peerpods/podvm/podvm-builder.sh"
echo "Calls/imports in ${FILE}:"
rg -n 'error_exit|^\s*(source|\.)\s+.*lib\.sh' "${FILE}"
echo
echo "Definitions under config/peerpods/podvm/:"
rg -n '^\s*(function\s+)?error_exit\s*\(\)' config/peerpods/podvmRepository: openshift/sandboxed-containers-operator
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="config/peerpods/podvm/podvm-builder.sh"
echo "=== Checking beginning of ${FILE} for sourcing patterns ==="
head -100 "${FILE}" | cat -n
echo
echo "=== Searching for all source/dot patterns (including variations) ==="
rg -n '\bsource\b|\b\.\s+|^\.' "${FILE}" | head -20
echo
echo "=== Checking if lib.sh exists and location ==="
find config/peerpods/podvm -name "lib.sh" -type f
echo
echo "=== Full context around error_exit call (lines 50-60) ==="
sed -n '50,60p' "${FILE}" | cat -nRepository: openshift/sandboxed-containers-operator
Length of output: 7082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for invocations of podvm-builder.sh ==="
rg -n 'podvm-builder\.sh' --type sh --type bash
echo
echo "=== Checking if any wrapper/entry scripts source lib.sh ==="
find . -name "*.sh" -type f -exec grep -l "podvm-builder\.sh" {} \; | head -5 | xargs -I {} bash -c 'echo "=== {} ===" && head -30 {}'Repository: openshift/sandboxed-containers-operator
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for invocations of podvm-builder.sh ==="
rg -n 'podvm-builder\.sh' --type sh
echo
echo "=== Broader search for podvm-builder.sh references ==="
rg -n 'podvm-builder\.sh'
echo
echo "=== Checking the full file length to see all function calls ==="
wc -l config/peerpods/podvm/podvm-builder.sh
echo
echo "=== Looking at the main entry point / dispatch logic (last 100 lines) ==="
tail -100 config/peerpods/podvm/podvm-builder.sh | cat -nRepository: openshift/sandboxed-containers-operator
Length of output: 4519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking Dockerfile.podvm-builder for sourcing lib.sh ==="
cat config/peerpods/podvm/Dockerfile.podvm-builder
echo
echo "=== Checking if the shell scripts in /scripts source lib.sh ==="
head -20 config/peerpods/podvm/lib.shRepository: openshift/sandboxed-containers-operator
Length of output: 3369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking what variables are used in delete_podvm_image function ==="
sed -n '/^function delete_podvm_image()/,/^}/p' config/peerpods/podvm/podvm-builder.sh | grep -E 'PODVM_IMAGE_URI|IMAGE_ID|AMI_ID|IMAGE_NAME|LIBVIRT_IMAGE_ID' | head -20
echo
echo "=== Checking delete_podvm_image_gallery function ==="
sed -n '/^function delete_podvm_image_gallery()/,/^}/p' config/peerpods/podvm/podvm-builder.sh | grep -E 'PODVM_IMAGE_URI|IMAGE_ID|AMI_ID|IMAGE_NAME|LIBVIRT_IMAGE_ID' | head -20
echo
echo "=== Confirming: does any delete function reference PODVM_IMAGE_URI? ==="
rg 'PODVM_IMAGE_URI' config/peerpods/podvm/podvm-builder.shRepository: openshift/sandboxed-containers-operator
Length of output: 1666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verifying exit code meaning: searching for PODVM_IMAGE_URI ==="
if rg 'PODVM_IMAGE_URI' config/peerpods/podvm/podvm-builder.sh; then
echo "Found PODVM_IMAGE_URI"
else
echo "Exit code: $? - PODVM_IMAGE_URI NOT found in podvm-builder.sh"
fi
echo
echo "=== Confirming validate_podvm_image_uri is called unconditionally ==="
sed -n '350,380p' config/peerpods/podvm/podvm-builder.sh | cat -nRepository: openshift/sandboxed-containers-operator
Length of output: 1631
error_exit is undefined in this script; calling validate_podvm_image_uri unconditionally breaks delete operations.
validate_podvm_image_uri() calls error_exit, but podvm-builder.sh neither defines it nor sources lib.sh. Additionally, this validation runs unconditionally (line 354) before command dispatch, affecting all operations including delete and delete-gallery, which do not require PODVM_IMAGE_URI. When PODVM_IMAGE_URI is unset, this causes a command not found error instead of a clean failure, and breaks deletion workflows.
🛠️ Suggested fixes
Option 1: Replace error_exit with inline error handling:
function validate_podvm_image_uri() {
echo "Checking if PODVM_IMAGE_URI is set"
# PODVM_IMAGE_URI is required as operator-built flow has been deprecated
if [[ -z "${PODVM_IMAGE_URI}" ]]; then
- error_exit "PODVM_IMAGE_URI is required. The operator-built flow has been deprecated. Please provide a pre-built podvm image URI."
+ echo "PODVM_IMAGE_URI is required. The operator-built flow has been deprecated. Please provide a pre-built podvm image URI." >&2
+ exit 1
fi
}Option 2: Call validation only for the create command:
-# Validate that PODVM_IMAGE_URI is set
-validate_podvm_image_uriThen add inside the create case block:
create)
+ validate_podvm_image_uri
create_podvm_image
;;📝 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.
Suggested change
| # Function to validate that PODVM_IMAGE_URI is set. | |
| function validate_podvm_image_uri() { | |
| echo "Checking if PODVM_IMAGE_URI is set" | |
| # If the value of the PODVM_IMAGE_URI is empty or not set, then build the image from scratch else use the prebuilt artifact. | |
| # PODVM_IMAGE_URI is required as operator-built flow has been deprecated | |
| if [[ -z "${PODVM_IMAGE_URI}" ]]; then | |
| IMAGE_TYPE="operator-built" | |
| echo "Initiating the operator to build the podvm image" | |
| else | |
| IMAGE_TYPE="pre-built" | |
| echo "Initiating the operator to use the pre-built podvm image" | |
| error_exit "PODVM_IMAGE_URI is required. The operator-built flow has been deprecated. Please provide a pre-built podvm image URI." | |
| fi | |
| # Function to validate that PODVM_IMAGE_URI is set. | |
| function validate_podvm_image_uri() { | |
| echo "Checking if PODVM_IMAGE_URI is set" | |
| # PODVM_IMAGE_URI is required as operator-built flow has been deprecated | |
| if [[ -z "${PODVM_IMAGE_URI}" ]]; then | |
| echo "PODVM_IMAGE_URI is required. The operator-built flow has been deprecated. Please provide a pre-built podvm image URI." >&2 | |
| exit 1 | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/peerpods/podvm/podvm-builder.sh` around lines 49 - 56, The script
calls validate_podvm_image_uri which uses an undefined error_exit and is run
unconditionally, breaking delete flows; fix by either replacing the undefined
error_exit call inside validate_podvm_image_uri with inline handling (e.g., echo
to stderr and exit with non-zero status) or move the validate_podvm_image_uri
invocation so it runs only for create operations (invoke it inside the create
case block and do not call it for delete or delete-gallery); update references
to validate_podvm_image_uri, error_exit, and the command dispatch (create /
delete / delete-gallery) accordingly.
Comment on lines
+367
to
+368
| # Validate that PODVM_IMAGE_URI is set | ||
| validate_podvm_image_uri |
There was a problem hiding this comment.
Only validate PODVM_IMAGE_URI for create.
This now runs before command dispatch, so delete and delete-gallery fail unless the provider ConfigMap also has a URI populated. Those paths delete by existing image id/name and should keep working even after the build flow is deprecated.
🛠️ Suggested fix
-# Validate that PODVM_IMAGE_URI is set
-validate_podvm_image_uri
-
# Check if CLOUD_PROVIDER is set to azure or aws or libvirt
# Install the required dependencies
case "${CLOUD_PROVIDER}" in
@@
case "$1" in
create)
+ validate_podvm_image_uri
create_podvm_image
;;
delete)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/peerpods/podvm/podvm-builder.sh` around lines 367 - 368, The script
currently calls validate_podvm_image_uri unconditionally which breaks
delete/delete-gallery; update the flow so validate_podvm_image_uri is only
invoked for the create path (e.g., check the command variable or branch that
handles "create" before calling validate_podvm_image_uri). Locate the
unconditional call to validate_podvm_image_uri and move it into the create
command handler or wrap it with a conditional like if [ "$COMMAND" = "create" ];
then validate_podvm_image_uri; fi so delete/delete-gallery continue to work
without a PODVM_IMAGE_URI.
Comment on lines
+147
to
+148
| * `PODVM_IMAGE_URI` must be set in the cloud provider specific configMap (eg: `libvirt-podvm-image-cm`) | ||
| * The operator pulls the pre-built image from the specified URI and uploads it to the cloud provider |
There was a problem hiding this comment.
This doc still references the removed /payload workflow elsewhere.
Lines 147-148 describe the new URI-only flow, but Lines 54-63 in the same file still say the create job copies /podvm-binaries.tar.gz into /payload and mounts that volume. config/peerpods/podvm/osc-podvm-create-job.yaml no longer has that initContainer or mount, so the document is now internally inconsistent.
🧰 Tools
🪛 LanguageTool
[grammar] ~147-~147: Use a hyphen to join words.
Context: ...E_URImust be set in the cloud provider specific configMap (eg:libvirt-podvm-i...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/peerpods/podvm/podvm-handling.md` around lines 147 - 148, Update the
documentation to remove the old `/payload` workflow references (e.g.
`/podvm-binaries.tar.gz` and the payload volume) and align it with the URI-only
flow: state that PODVM_IMAGE_URI must be set in the cloud-provider configMap and
that the operator pulls the pre-built image from that URI and uploads it to the
cloud provider; specifically remove or rewrite the sentences describing an
initContainer copying binaries into `/payload` and any mention of mounting that
volume so the text matches the current osc-podvm-create-job.yaml which no longer
includes the initContainer or mount.
Member
|
IBM explicitly asked we keep this around as they don't have a working alternative yet. Add /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
gkurz
changed the title
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit