install upstream nightly build of kubevirt and HCO#2178
install upstream nightly build of kubevirt and HCO#2178weshayutin wants to merge 2 commits intoopenshift:oadp-devfrom
Conversation
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as oc / curl
participant Repo as GitHub Tarball
participant OLM as OpenShift OLM / Catalog
participant Operator as HCO Operator
participant HCO as HyperConverged CR
User->>CLI: run install_hco.sh (envs)
CLI->>CLI: preflight checks (oc, curl, tar, auth)
CLI->>Repo: download & extract deploy/kustomize
CLI->>OLM: create gRPC CatalogSource in openshift-marketplace
OLM-->>CLI: Poll until connectionState READY
CLI->>OLM: create Namespace, OperatorGroup, Subscription (maybe startingCSV)
OLM->>Operator: InstallPlan created -> Installed
Operator-->>CLI: operator Deployment becomes Available
CLI->>HCO: apply HyperConverged CR (with optional emulation patch)
HCO-->>CLI: poll until HCO status Available
sequenceDiagram
participant User
participant CLI as oc / kubectl
participant Workloads as VMs/DVs/VMIs
participant Storage as HPP / PVs / SCs
participant OLM as CSV/Subscription/OperatorGroup/CatalogSource
participant Cluster as CRDs/RBAC/Webhooks
User->>CLI: run uninstall_hco.sh
CLI->>CLI: detect HCO namespace, auth check
CLI->>Workloads: patch KubeVirt/CDI uninstallStrategy=RemoveWorkloads
CLI->>Workloads: delete VMs/VMIs/DataVolumes/DataImportCrons (all namespaces)
CLI->>Storage: delete HostPath Provisioner, StorageClasses, image streams
Storage-->>CLI: patch PVs to nullify claimRef for reclamation
CLI->>OLM: delete HyperConverged CR, wait for deployments/daemonsets to stop
CLI->>OLM: remove CSVs, Subscriptions, OperatorGroups, CatalogSource
CLI->>Cluster: attempt deletion of kubevirt-related CRDs / RBAC / webhooks
CLI->>CLI: delete HCO and OS-images namespaces, final status check
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (2)
.gitignore (1)
50-50: Narrow ignore scope to the intended directory.If this is meant to ignore only the repo-root install artifact, prefer
/kustomize/instead ofkustomizeto avoid unintentionally ignoring nested paths with the same name.Proposed diff
- kustomize + /kustomize/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 50, The .gitignore entry 'kustomize' is too broad and may ignore any nested directories named 'kustomize'; change it to the repo-root directory form '/kustomize/' so only the intended top-level install artifact is ignored—update the .gitignore line replacing "kustomize" with "/kustomize/" to narrow the scope.hack/install_upstream_kubevirt/install_hco.sh (1)
145-165: Downloaded kustomize content is not used after validation.
download_kustomize()introduces a hard GitHub dependency and extra failure path, but extracted manifests are never consumed by install steps. Either use the artifacts or drop this step.Refactor direction
- download_kustomize # validates connectivity; WORK_DIR set for cleanup apply_catalogsourceOr, if keeping the download by design, invoke the downloaded manifest workflow so this step is functionally meaningful.
Also applies to: 449-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/install_upstream_kubevirt/install_hco.sh` around lines 145 - 165, The download_kustomize() function downloads and validates kustomize manifests into KUSTOMIZE_DIR but the extracted artifacts are never used by subsequent install steps; either remove this function and any calls to it to eliminate the unused GitHub dependency, or make the step functional by invoking the downloaded workflow (e.g., execute or source KUSTOMIZE_DIR/deploy_kustomize.sh or pass KUSTOMIZE_DIR into the install flow so later functions consume the extracted manifests). Locate download_kustomize, the KUSTOMIZE_DIR variable and any callers and either delete them or wire KUSTOMIZE_DIR into the install logic so the deploy_kustomize.sh produced by download_kustomize is actually executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/install_upstream_kubevirt/install_hco.sh`:
- Around line 131-132: The current preflight cleanup uses OC_TOOL to delete all
ClusterServiceVersions in HCO_NAMESPACE which can remove unrelated operators;
change the command to target only HCO-related CSVs by removing the --all flag
and deleting by a specific identifier or label instead (e.g., use a variable
like HCO_CSV_NAME and run ${OC_TOOL} delete clusterserviceversions -n
"${HCO_NAMESPACE}" "${HCO_CSV_NAME}" --ignore-not-found 2>/dev/null || true, or
use a label selector such as ${OC_TOOL} delete clusterserviceversions -n
"${HCO_NAMESPACE}" -l <hco-specific-label> --ignore-not-found 2>/dev/null ||
true), referencing OC_TOOL, HCO_NAMESPACE and a new HCO_CSV_NAME or label to
ensure only HCO CSVs are removed.
In `@hack/install_upstream_kubevirt/uninstall_hco.sh`:
- Around line 128-131: The deletion is too broad: the existence check uses
OC_TOOL and HCO_NAMESPACE for the kubevirt-hyperconverged CR, but the removal
call uses oc_delete hyperconverged --all-namespaces --all which will erase every
HyperConverged CR cluster-wide; change the removal to target the specific CR and
namespace (use oc_delete hyperconverged kubevirt-hyperconverged -n
"${HCO_NAMESPACE}" || true) so only the checked instance is deleted, keeping
OC_TOOL, HCO_NAMESPACE, and oc_delete as the referenced symbols to locate the
lines to update.
- Around line 83-87: The uninstall script patches the wrong field: it sets
spec.enableCommonBootImageImport=false but the installer writes the flag under
spec.workloadSources.enableCommonBootImageImport; update the patch call that
uses ${OC_TOOL} patch hyperconverged kubevirt-hyperconverged (namespace
${HCO_NAMESPACE}) to target "spec.workloadSources.enableCommonBootImageImport"
instead of "spec.enableCommonBootImageImport" so the background import is
actually disabled during teardown.
---
Nitpick comments:
In @.gitignore:
- Line 50: The .gitignore entry 'kustomize' is too broad and may ignore any
nested directories named 'kustomize'; change it to the repo-root directory form
'/kustomize/' so only the intended top-level install artifact is ignored—update
the .gitignore line replacing "kustomize" with "/kustomize/" to narrow the
scope.
In `@hack/install_upstream_kubevirt/install_hco.sh`:
- Around line 145-165: The download_kustomize() function downloads and validates
kustomize manifests into KUSTOMIZE_DIR but the extracted artifacts are never
used by subsequent install steps; either remove this function and any calls to
it to eliminate the unused GitHub dependency, or make the step functional by
invoking the downloaded workflow (e.g., execute or source
KUSTOMIZE_DIR/deploy_kustomize.sh or pass KUSTOMIZE_DIR into the install flow so
later functions consume the extracted manifests). Locate download_kustomize, the
KUSTOMIZE_DIR variable and any callers and either delete them or wire
KUSTOMIZE_DIR into the install logic so the deploy_kustomize.sh produced by
download_kustomize is actually executed.
🪄 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: Enterprise
Run ID: f1fdbd8b-3591-47b6-a800-5f9f58e994bf
📒 Files selected for processing (3)
.gitignorehack/install_upstream_kubevirt/install_hco.shhack/install_upstream_kubevirt/uninstall_hco.sh
| ${OC_TOOL} delete clusterserviceversions \ | ||
| -n "${HCO_NAMESPACE}" --all --ignore-not-found 2>/dev/null || true |
There was a problem hiding this comment.
Preflight cleanup deletes unrelated CSVs in the namespace.
Line 131-132 uses --all, which will remove every CSV in ${HCO_NAMESPACE}. If users override HCO_NAMESPACE, this can disrupt other operators.
Suggested fix
- ${OC_TOOL} delete clusterserviceversions \
- -n "${HCO_NAMESPACE}" --all --ignore-not-found 2>/dev/null || true
+ ${OC_TOOL} delete clusterserviceversions \
+ -n "${HCO_NAMESPACE}" \
+ --selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" \
+ --ignore-not-found 2>/dev/null || 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.
| ${OC_TOOL} delete clusterserviceversions \ | |
| -n "${HCO_NAMESPACE}" --all --ignore-not-found 2>/dev/null || true | |
| ${OC_TOOL} delete clusterserviceversions \ | |
| -n "${HCO_NAMESPACE}" \ | |
| --selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" \ | |
| --ignore-not-found 2>/dev/null || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/install_upstream_kubevirt/install_hco.sh` around lines 131 - 132, The
current preflight cleanup uses OC_TOOL to delete all ClusterServiceVersions in
HCO_NAMESPACE which can remove unrelated operators; change the command to target
only HCO-related CSVs by removing the --all flag and deleting by a specific
identifier or label instead (e.g., use a variable like HCO_CSV_NAME and run
${OC_TOOL} delete clusterserviceversions -n "${HCO_NAMESPACE}" "${HCO_CSV_NAME}"
--ignore-not-found 2>/dev/null || true, or use a label selector such as
${OC_TOOL} delete clusterserviceversions -n "${HCO_NAMESPACE}" -l
<hco-specific-label> --ignore-not-found 2>/dev/null || true), referencing
OC_TOOL, HCO_NAMESPACE and a new HCO_CSV_NAME or label to ensure only HCO CSVs
are removed.
| ${OC_TOOL} patch hyperconverged kubevirt-hyperconverged \ | ||
| -n "${HCO_NAMESPACE}" \ | ||
| --type='merge' \ | ||
| --patch='{"spec":{"enableCommonBootImageImport":false}}' \ | ||
| || true |
There was a problem hiding this comment.
Patch path for boot image import is inconsistent and likely ineffective.
Line 86 patches spec.enableCommonBootImageImport, but this PR’s installer writes that flag under spec.workloadSources.enableCommonBootImageImport (see hack/install_upstream_kubevirt/install_hco.sh, Line 385-386). That mismatch can leave background imports running during teardown.
Suggested fix
- ${OC_TOOL} patch hyperconverged kubevirt-hyperconverged \
- -n "${HCO_NAMESPACE}" \
- --type='merge' \
- --patch='{"spec":{"enableCommonBootImageImport":false}}' \
+ ${OC_TOOL} patch hyperconged kubevirt-hyperconverged \
+ -n "${HCO_NAMESPACE}" \
+ --type='merge' \
+ --patch='{"spec":{"workloadSources":{"enableCommonBootImageImport":false}}}' \
|| 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.
| ${OC_TOOL} patch hyperconverged kubevirt-hyperconverged \ | |
| -n "${HCO_NAMESPACE}" \ | |
| --type='merge' \ | |
| --patch='{"spec":{"enableCommonBootImageImport":false}}' \ | |
| || true | |
| ${OC_TOOL} patch hyperconverged kubevirt-hyperconverged \ | |
| -n "${HCO_NAMESPACE}" \ | |
| --type='merge' \ | |
| --patch='{"spec":{"workloadSources":{"enableCommonBootImageImport":false}}}' \ | |
| || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/install_upstream_kubevirt/uninstall_hco.sh` around lines 83 - 87, The
uninstall script patches the wrong field: it sets
spec.enableCommonBootImageImport=false but the installer writes the flag under
spec.workloadSources.enableCommonBootImageImport; update the patch call that
uses ${OC_TOOL} patch hyperconverged kubevirt-hyperconverged (namespace
${HCO_NAMESPACE}) to target "spec.workloadSources.enableCommonBootImageImport"
instead of "spec.enableCommonBootImageImport" so the background import is
actually disabled during teardown.
| if ${OC_TOOL} get hyperconverged kubevirt-hyperconverged \ | ||
| -n "${HCO_NAMESPACE}" &>/dev/null 2>&1; then | ||
| oc_delete hyperconverged --all-namespaces --all || true | ||
| fi |
There was a problem hiding this comment.
HyperConverged deletion scope is broader than intended.
Line 128-129 checks for kubevirt-hyperconverged in ${HCO_NAMESPACE}, but Line 130 deletes all HyperConverged CRs cluster-wide. This can remove unrelated installs.
Suggested fix
if ${OC_TOOL} get hyperconverged kubevirt-hyperconverged \
-n "${HCO_NAMESPACE}" &>/dev/null 2>&1; then
- oc_delete hyperconverged --all-namespaces --all || true
+ oc_delete hyperconverged kubevirt-hyperconverged -n "${HCO_NAMESPACE}" || true
fi📝 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.
| if ${OC_TOOL} get hyperconverged kubevirt-hyperconverged \ | |
| -n "${HCO_NAMESPACE}" &>/dev/null 2>&1; then | |
| oc_delete hyperconverged --all-namespaces --all || true | |
| fi | |
| if ${OC_TOOL} get hyperconverged kubevirt-hyperconverged \ | |
| -n "${HCO_NAMESPACE}" &>/dev/null 2>&1; then | |
| oc_delete hyperconverged kubevirt-hyperconverged -n "${HCO_NAMESPACE}" || true | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/install_upstream_kubevirt/uninstall_hco.sh` around lines 128 - 131, The
deletion is too broad: the existence check uses OC_TOOL and HCO_NAMESPACE for
the kubevirt-hyperconverged CR, but the removal call uses oc_delete
hyperconverged --all-namespaces --all which will erase every HyperConverged CR
cluster-wide; change the removal to target the specific CR and namespace (use
oc_delete hyperconverged kubevirt-hyperconverged -n "${HCO_NAMESPACE}" || true)
so only the checked instance is deleted, keeping OC_TOOL, HCO_NAMESPACE, and
oc_delete as the referenced symbols to locate the lines to update.
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
weshayutin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
hack/install_upstream_kubevirt/install_hco.sh (1)
129-132:⚠️ Potential issue | 🟠 MajorDon't purge every CSV in the namespace.
Line 131 uses
--all, so rerunning this against a shared or preexisting${HCO_NAMESPACE}can uninstall unrelated operators. This matches the earlier finding and still needs to be scoped to HCO-owned CSVs.Scope cleanup to the HCO subscription
- ${OC_TOOL} delete clusterserviceversions \ - -n "${HCO_NAMESPACE}" --all --ignore-not-found 2>/dev/null || true + ${OC_TOOL} delete clusterserviceversions \ + -n "${HCO_NAMESPACE}" \ + --selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" \ + --ignore-not-found 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/install_upstream_kubevirt/install_hco.sh` around lines 129 - 132, The script currently deletes every ClusterServiceVersion in ${HCO_NAMESPACE} using "${OC_TOOL} delete clusterserviceversions -n \"${HCO_NAMESPACE}\" --all", which can uninstall unrelated operators; change this to only remove CSVs owned/installed by the HCO subscription: query the installed CSV from the subscription (using ${OC_TOOL} get subscription \"${SUBSCRIPTION_NAME}\" -n \"${HCO_NAMESPACE}\" and jsonpath/.status.installedCSV or otherwise filter CSVs by an HCO-specific label/owner reference) and then delete only that CSV (or the resulting list) instead of using --all; update the clusterserviceversions deletion path that references ${OC_TOOL}, ${SUBSCRIPTION_NAME}, and ${HCO_NAMESPACE} to implement this scoped deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/install_upstream_kubevirt/install_hco.sh`:
- Around line 145-165: The download_kustomize() step downloads a GitHub tarball
but its extracted KUSTOMIZE_DIR (and deploy_kustomize.sh) are never used by the
OLM install path, causing unnecessary failures on API/rate-limit errors; either
remove the download_kustomize() invocation from the main flow (so the script no
longer requires GitHub access), or make the download conditional on the
installation mode and actually consume the extracted content where needed.
Concretely, update the main script to stop calling download_kustomize() when
running the OLM installer (or drop the call entirely), or modify
download_kustomize() to return/use KUSTOMIZE_DIR/deploy_kustomize.sh in the code
paths that need it so the extraction is required only when those files will be
executed. Ensure references to KUSTOMIZE_DIR and deploy_kustomize.sh are
consistent and remove any unconditional exit-on-failure behavior if the files
are not required by the current install mode.
- Around line 326-332: The wait_for_hco_operator function currently calls
`${OC_TOOL} wait ...
--selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}"` which fails
immediately if no deployment exists; change it to first poll for at least one
matching deployment (use `${OC_TOOL} get deployments
--selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" -n
"${HCO_NAMESPACE}"` in a retry loop with a short sleep and an overall timeout)
and only after a deployment name is found call `${OC_TOOL} wait
deployment/<found-deployment-name> -n "${HCO_NAMESPACE}"
--for=condition=Available --timeout=5m`; keep using the existing PACKAGE_NAME,
HCO_NAMESPACE and OC_TOOL variables and ensure the polling loop exits with
failure if no deployment appears before the 5m window.
- Around line 39-40: The preflight check uses `${OC_TOOL} whoami` and
`${OC_TOOL} whoami --show-server`, which fails when OC_TOOL=kubectl; update the
script to either restrict OC_TOOL to "oc" (update the comment and validate early
in preflight()) or add conditional handling in preflight(): if OC_TOOL == "oc"
keep using `whoami`/`whoami --show-server`, but if OC_TOOL == "kubectl" call
`kubectl auth whoami` for user and obtain the API server with `kubectl config
view --minify -o jsonpath={.clusters[0].cluster.server}` (or another stable
kubectl command), and ensure the doc comment for OC_TOOL is updated to reflect
supported behavior and commands. Reference: OC_TOOL variable and preflight()
checks that invoke `${OC_TOOL} whoami` and `${OC_TOOL} whoami --show-server`.
---
Duplicate comments:
In `@hack/install_upstream_kubevirt/install_hco.sh`:
- Around line 129-132: The script currently deletes every ClusterServiceVersion
in ${HCO_NAMESPACE} using "${OC_TOOL} delete clusterserviceversions -n
\"${HCO_NAMESPACE}\" --all", which can uninstall unrelated operators; change
this to only remove CSVs owned/installed by the HCO subscription: query the
installed CSV from the subscription (using ${OC_TOOL} get subscription
\"${SUBSCRIPTION_NAME}\" -n \"${HCO_NAMESPACE}\" and
jsonpath/.status.installedCSV or otherwise filter CSVs by an HCO-specific
label/owner reference) and then delete only that CSV (or the resulting list)
instead of using --all; update the clusterserviceversions deletion path that
references ${OC_TOOL}, ${SUBSCRIPTION_NAME}, and ${HCO_NAMESPACE} to implement
this scoped deletion.
🪄 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: Enterprise
Run ID: f35443e4-7e3e-4f24-bc77-efb020ef7163
📒 Files selected for processing (1)
hack/install_upstream_kubevirt/install_hco.sh
| # OC_TOOL oc or kubectl binary. | ||
| # (default: oc) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does kubectl support a top-level whoamicommand likeoc whoami, and what is the supported equivalent for retrieving the current user and cluster server?
💡 Result:
No, kubectl does not support a top-level whoami command like oc whoami. The closest equivalent is kubectl auth whoami, which displays the current authenticated subject's attributes (user, groups, extras) by querying the Kubernetes API's self-subject review. This was promoted to stable in Kubernetes v1.27. For retrieving the current user and cluster server information from the kubeconfig: - Current context (including user and cluster): kubectl config current-context - Full current context details: kubectl config view --minify - Current cluster server: kubectl config view --minify -o jsonpath='{.clusters[0].cluster.server}' - Current user from kubeconfig: kubectl config view --minify -o jsonpath='{.users[0].name}' - Cluster info (API server addresses): kubectl cluster-info oc whoami (OpenShift-specific top-level command) shows the kubeconfig user name and supports flags for server/token/context, while kubectl auth whoami provides server-validated identity attributes.
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_auth/kubectl_auth_whoami/
- 2: Promote whoami kubectl command kubernetes/kubernetes#116510
- 3: https://manpages.opensuse.org/Tumbleweed/oc/oc-whoami.1.en.html
- 4: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_config/kubectl_config_current-context
- 5: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_config/kubectl_config_view
- 6: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_auth/kubectl_auth_whoami
- 7: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_cluster-info
🏁 Script executed:
# First, check if the file exists and get its size
wc -l hack/install_upstream_kubevirt/install_hco.sh
# Then read the relevant sections
echo "=== Lines 35-45 (documentation) ==="
sed -n '35,45p' hack/install_upstream_kubevirt/install_hco.sh
echo "=== Lines 108-125 (preflight function with whoami) ==="
sed -n '108,125p' hack/install_upstream_kubevirt/install_hco.sh
echo "=== Lines 70-80 (to check for other usages) ==="
sed -n '70,80p' hack/install_upstream_kubevirt/install_hco.sh
echo "=== All OC_TOOL usages ==="
rg '\$\{OC_TOOL\}' hack/install_upstream_kubevirt/install_hco.sh -nRepository: openshift/oadp-operator
Length of output: 3288
Require oc, or add real kubectl support.
Lines 112, 116–117 use ${OC_TOOL} whoami and ${OC_TOOL} whoami --show-server, but kubectl does not support these commands. The kubectl equivalent is kubectl auth whoami (stable since Kubernetes v1.27), and there is no equivalent for the --show-server flag. The script documents that OC_TOOL can be either oc or kubectl, but setting it to kubectl will fail in the preflight() check before installation even begins.
Either restrict this script to oc only, or add conditional branches to handle the different CLI tools.
Minimal fix if `oc` is the only supported client
-# OC_TOOL oc or kubectl binary.
-# (default: oc)
+# OC_TOOL oc binary.
+# (default: oc)
@@
OC_TOOL="${OC_TOOL:-oc}"
+if [[ "${OC_TOOL}" != "oc" ]]; then
+ err "This installer currently requires 'oc'."
+ exit 1
+fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/install_upstream_kubevirt/install_hco.sh` around lines 39 - 40, The
preflight check uses `${OC_TOOL} whoami` and `${OC_TOOL} whoami --show-server`,
which fails when OC_TOOL=kubectl; update the script to either restrict OC_TOOL
to "oc" (update the comment and validate early in preflight()) or add
conditional handling in preflight(): if OC_TOOL == "oc" keep using
`whoami`/`whoami --show-server`, but if OC_TOOL == "kubectl" call `kubectl auth
whoami` for user and obtain the API server with `kubectl config view --minify -o
jsonpath={.clusters[0].cluster.server}` (or another stable kubectl command), and
ensure the doc comment for OC_TOOL is updated to reflect supported behavior and
commands. Reference: OC_TOOL variable and preflight() checks that invoke
`${OC_TOOL} whoami` and `${OC_TOOL} whoami --show-server`.
| download_kustomize() { | ||
| WORK_DIR="$(mktemp -d -t hco-kustomize-XXXXXX)" | ||
| local tarball_url="https://api.github.com/repos/kubevirt/hyperconverged-cluster-operator/tarball/${HCO_GIT_REF}" | ||
|
|
||
| log "Downloading HCO kustomize manifests (git ref: '${HCO_GIT_REF}')..." | ||
| ( | ||
| cd "${WORK_DIR}" | ||
| curl -fsSL "${tarball_url}" \ | ||
| | tar --strip-components=1 -xzf - \ | ||
| --wildcards \ | ||
| "kubevirt-hyperconverged-cluster-operator-*/deploy/kustomize" | ||
| ) | ||
|
|
||
| KUSTOMIZE_DIR="${WORK_DIR}/deploy/kustomize" | ||
| if [[ ! -f "${KUSTOMIZE_DIR}/deploy_kustomize.sh" ]]; then | ||
| err "deploy_kustomize.sh not found after extraction." | ||
| err "Expected: ${KUSTOMIZE_DIR}/deploy_kustomize.sh" | ||
| exit 1 | ||
| fi | ||
| log "Kustomize dir ready: ${KUSTOMIZE_DIR}" | ||
| } |
There was a problem hiding this comment.
Remove the unused GitHub tarball prerequisite.
download_kustomize() only validates that deploy_kustomize.sh exists; nothing ever consumes ${KUSTOMIZE_DIR} afterwards. Right now a GitHub/API outage or rate limit aborts the install even though the OLM flow does not use those manifests. Either drive the install from the extracted kustomize content or drop this step from main.
Also applies to: 456-456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/install_upstream_kubevirt/install_hco.sh` around lines 145 - 165, The
download_kustomize() step downloads a GitHub tarball but its extracted
KUSTOMIZE_DIR (and deploy_kustomize.sh) are never used by the OLM install path,
causing unnecessary failures on API/rate-limit errors; either remove the
download_kustomize() invocation from the main flow (so the script no longer
requires GitHub access), or make the download conditional on the installation
mode and actually consume the extracted content where needed. Concretely, update
the main script to stop calling download_kustomize() when running the OLM
installer (or drop the call entirely), or modify download_kustomize() to
return/use KUSTOMIZE_DIR/deploy_kustomize.sh in the code paths that need it so
the extraction is required only when those files will be executed. Ensure
references to KUSTOMIZE_DIR and deploy_kustomize.sh are consistent and remove
any unconditional exit-on-failure behavior if the files are not required by the
current install mode.
| wait_for_hco_operator() { | ||
| log "Waiting for HCO operator deployment to be Available (timeout: 5m)..." | ||
| ${OC_TOOL} wait deployments \ | ||
| --selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" \ | ||
| -n "${HCO_NAMESPACE}" \ | ||
| --for=condition=Available \ | ||
| --timeout=5m |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When kubectl/oc wait is invoked with --selector and no resources currently match, does it wait for resources to appear or fail immediately?
💡 Result:
When kubectl or oc wait is invoked with --selector (or -l) and no resources currently match the selector, it fails immediately with "error: no matching resources found". It does not wait for matching resources to appear. This behavior occurs because the ResourceFinder.Visit immediately returns without error (nil) when no resources match the label selector, resulting in visitCount == 0. The code then explicitly returns errNoMatchingResources unless --for=delete is specified (in which case it ignores NotFound errors). Special cases: - For --for=create, it polls until at least one matching resource is found (using PollUntilContextTimeout checking if any resource visits successfully), then proceeds to wait for conditions. A recent fix (PR #128662, merged Dec 2024 for v1.33+) ensures proper polling even with selectors by tracking found resources explicitly. - For --for=delete, it ignores NotFound errors and waits. oc wait inherits this behavior from kubectl, as confirmed by identical documentation and source patterns. Sources confirming immediate failure for standard conditions (condition=Ready, Available, etc.) with selectors when no matches: GitHub issues #96676, #1675, #87352, #66456; StackOverflow; kubectl source code (visitCount check returns errNoMatchingResources).
Citations:
- 1: kubectl wait "pod" --for=delete errors out if no matching resources are found kubernetes/kubernetes#96676
- 2: kubectl wait --for=create doesn't work with selectors kubernetes/kubectl#1675
- 3: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/wait/wait.go
- 4: kubectl wait should wait for resources to be available kubernetes/kubernetes#87352
- 5: kubectl wait with selectors does not output anything if resource is not found kubernetes/kubernetes#66456
- 6: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_wait/
- 7: kubectl: fix wait --for=create to work correctly with label selectors kubernetes/kubernetes#128662
Poll for the deployment to exist before waiting on its Available condition.
The oc wait command with --selector fails immediately with "no matching resources found" if no deployment matches the selector at invocation time. It does not wait for matching resources to appear. This means the function can fail immediately instead of retrying over the 5-minute timeout window.
First poll for at least one matching deployment, then invoke wait on the Available condition.
One way to make this robust
wait_for_hco_operator() {
log "Waiting for HCO operator deployment to be Available (timeout: 5m)..."
+ local selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}"
+ local retries=0 max_retries=20
+
+ while ! ${OC_TOOL} get deployments -n "${HCO_NAMESPACE}" -l "${selector}" -o name 2>/dev/null | grep -q .; do
+ if (( retries == max_retries )); then
+ err "Timed out waiting for HCO operator deployment to be created."
+ ${OC_TOOL} get deployments -n "${HCO_NAMESPACE}" --show-labels || true
+ exit 1
+ fi
+ (( retries += 1 ))
+ sleep 15
+ done
+
${OC_TOOL} wait deployments \
- --selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" \
+ --selector="${selector}" \
-n "${HCO_NAMESPACE}" \
--for=condition=Available \
--timeout=5m
log "HCO operator deployment is Available."
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/install_upstream_kubevirt/install_hco.sh` around lines 326 - 332, The
wait_for_hco_operator function currently calls `${OC_TOOL} wait ...
--selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}"` which fails
immediately if no deployment exists; change it to first poll for at least one
matching deployment (use `${OC_TOOL} get deployments
--selector="operators.coreos.com/${PACKAGE_NAME}.${HCO_NAMESPACE}" -n
"${HCO_NAMESPACE}"` in a retry loop with a short sleep and an overall timeout)
and only after a deployment name is found call `${OC_TOOL} wait
deployment/<found-deployment-name> -n "${HCO_NAMESPACE}"
--for=condition=Available --timeout=5m`; keep using the existing PACKAGE_NAME,
HCO_NAMESPACE and OC_TOOL variables and ensure the polling loop exits with
failure if no deployment appears before the 5m window.
|
@weshayutin: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
Why the changes were made
So leaving this to bake for a bit
How to test the changes made
Summary by CodeRabbit
New Features
Chores