Kubevirt datamover e2e tests#2238
Open
weshayutin wants to merge 1 commit into
Open
Conversation
First pass at kubevirt-datamover E2E tests: 1. Installs upstream build of HCO (required to test latest CNV code) 2. Sets up requirements for kubevirt datamover: volume policy, DPA, etc. 3. Uses defined cirros VM with snapshotMoveData=true + volume policy 4. More verbose console dump of vmbackup, dataupload and velero details 5. Adds virtctl to ci-Dockerfile for KubeVirt VM operations 6. Includes sample application manifests for cirros, fedora, centos, windows VMs 7. Adds cleanup and annotation helper scripts Cherry-picked from PR openshift#2174 (openshift/oadp-operator, branch: kubevirt-datamover-e2e) Original target: oadp-1.6, squashed onto oadp-dev Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
WalkthroughThis PR adds end-to-end testing infrastructure for KubeVirt datamover backups with Changed Block Tracking (CBT). It extends the test framework to support community HCO installation via OLM, adds CBT feature gate and label selector enablement, introduces Velero volume policy management for datamover routing, implements VM lifecycle helpers, and provides a new CBT-focused backup/restore test scenario alongside manual testing manifests and documentation. ChangesKubeVirt Datamover CBT E2E Testing
🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 5 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/lib/virt_storage_helpers.go (1)
407-411:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing DeepCopy causes mutation of original storage class.
Line 407 assigns the storage class pointer directly without calling
DeepCopy(), meaning subsequent modifications (lines 408-411) will mutate the originaldefaultStorageClassobject. This is inconsistent with the correct pattern used inCreateImmediateModeStorageClass(line 369) and can cause unintended side effects if the original storage class is reused.🐛 Proposed fix
- wffcStorageClass := defaultStorageClass + wffcStorageClass := defaultStorageClass.DeepCopy() wffcStorageClass.VolumeBindingMode = ptr.To[storagev1.VolumeBindingMode](storagev1.VolumeBindingWaitForFirstConsumer) wffcStorageClass.Name = name🤖 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 `@tests/e2e/lib/virt_storage_helpers.go` around lines 407 - 411, The code assigns wffcStorageClass := defaultStorageClass and then mutates it, which alters the original defaultStorageClass; change the assignment to use a DeepCopy of defaultStorageClass (e.g., call defaultStorageClass.DeepCopy()) before modifying VolumeBindingMode, Name, ResourceVersion, and Annotations so mutations only affect wffcStorageClass (mirror the pattern used in CreateImmediateModeStorageClass).
🧹 Nitpick comments (6)
docs/design/specs/2026-04-21-kubevirt-datamover-e2e-test-design.md (2)
153-163: ⚡ Quick winAdd language specifier to the fenced code block.
The pseudo-code sequence block should specify a language (e.g.,
textoryaml) for proper rendering and to satisfy markdown linting rules.📝 Proposed fix
-``` +```text 1. EnsureVirtInstallation() — existing flow, installs HCO with empty spec 2. EnableCBTFeatureGate() — patch HCO: spec.featureGates.incrementalBackup = true🤖 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 `@docs/design/specs/2026-04-21-kubevirt-datamover-e2e-test-design.md` around lines 153 - 163, The fenced pseudocode block listing EnsureVirtInstallation(), EnableCBTFeatureGate(), EnableCBTLabelSelector(), etc. lacks a language specifier which breaks markdown linting and rendering; update that fenced code block to include a language (for example add ```text or ```yaml) immediately after the opening backticks so the sequence renders correctly and satisfies linters.
194-196: ⚡ Quick winAdd language specifier to the HTTP endpoint code block.
The REST endpoint example should specify a language (e.g.,
httportext) for proper rendering and to satisfy markdown linting rules.📝 Proposed fix
-``` +```http PUT /apis/subresources.kubevirt.io/v1/namespaces/{namespace}/virtualmachines/{name}/start</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/design/specs/2026-04-21-kubevirt-datamover-e2e-test-design.mdaround
lines 194 - 196, The markdown code block containing the REST endpoint "PUT
/apis/subresources.kubevirt.io/v1/namespaces/{namespace}/virtualmachines/{name}/start"
needs a language specifier for proper rendering and linting; update the
triple-backtick fence so it starts with a language tag (e.g.,http ortext) immediately before the PUT line and keep the closing ``` unchanged so
the block becomes a fenced code block labeled with the chosen language.</details> <!-- cr-comment:v1:70d149046b28d83bd1119149 --> </blockquote></details> <details> <summary>tests/e2e/sample-applications/virtual-machines/kubevirt-dm/cleanup.sh (3)</summary><blockquote> `26-26`: _💤 Low value_ **Unused variable flagged by shellcheck.** The `RED` color variable is defined but never used in the script. Consider removing it or using it if error messages should be highlighted in red. <details> <summary>♻️ Proposed fix</summary> ```diff -RED='\033[0;31m'; YELLOW='\033[1;33m'; GREEN='\033[0;32m'; CYAN='\033[0;36m'; NC='\033[0m' +YELLOW='\033[1;33m'; GREEN='\033[0;32m'; CYAN='\033[0;36m'; NC='\033[0m' ``` Or add an `error()` function if you want to use it: ```bash RED='\033[0;31m'; YELLOW='\033[1;33m'; GREEN='\033[0;32m'; CYAN='\033[0;36m'; NC='\033[0m' error() { echo -e "${RED}[ERROR]${NC} $*"; } ``` </details> Based on learnings from static analysis tools. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/cleanup.sh` at line 26, Remove the unused RED color variable or use it for error messages: either delete RED from the color definitions (leaving YELLOW, GREEN, CYAN, NC) or add an error helper (e.g., an error() function that prints messages prefixed with RED and resets with NC) and call that function where cleanup failures or error conditions are reported in cleanup.sh so RED is actually used. ``` </details> <!-- cr-comment:v1:dece61ba3f6c2d2468c896c5 --> _Source: Linters/SAST tools_ --- `41-51`: _💤 Low value_ **Missing double quotes in command arguments.** Shellcheck recommends double-quoting variable `$ns_flag` (line 44) to prevent word splitting and globbing issues. <details> <summary>♻️ Proposed fix</summary> ```diff wait_for_deletion() { local kind="$1" name="$2" ns_flag="${3:-}" timeout="${4:-60}" local elapsed=0 - while oc get "$kind" "$name" $ns_flag &>/dev/null; do + while oc get "$kind" "$name" "$ns_flag" &>/dev/null; do if (( elapsed >= timeout )); then warn "Timed out waiting for $kind/$name to be deleted" return 1 fi sleep 2; (( elapsed += 2 )) done } ``` </details> Based on learnings from static analysis tools. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/cleanup.sh` around lines 41 - 51, In wait_for_deletion, quote the namespace flag variable to avoid word-splitting/globbing: update the oc get invocation in function wait_for_deletion to use "$ns_flag" (and similarly ensure any other uses of ns_flag in that function are quoted) so the command arguments are passed safely when ns_flag is empty or contains spaces/globs. ``` </details> <!-- cr-comment:v1:7c00a209d10428ad898cf9b3 --> _Source: Linters/SAST tools_ --- `33-39`: _💤 Low value_ **Missing double quotes in command arguments.** Shellcheck recommends double-quoting variables `$ns_flag` (lines 35-36) to prevent word splitting and globbing issues. While unlikely to cause problems in this context, it's a shell scripting best practice. <details> <summary>♻️ Proposed fix</summary> ```diff delete_if_exists() { local kind="$1" name="$2" ns_flag="${3:-}" - if oc get "$kind" "$name" $ns_flag &>/dev/null; then - oc delete "$kind" "$name" $ns_flag --ignore-not-found + if oc get "$kind" "$name" "$ns_flag" &>/dev/null; then + oc delete "$kind" "$name" "$ns_flag" --ignore-not-found deleted "$kind/$name" fi } ``` </details> Based on learnings from static analysis tools. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/cleanup.sh` around lines 33 - 39, The delete_if_exists function uses an unquoted variable ns_flag in oc commands which can cause word-splitting/globbing; update the two oc invocations inside delete_if_exists to quote "${ns_flag}" so the commands read oc get "$kind" "$name" "${ns_flag}" and oc delete "$kind" "$name" "${ns_flag}" --ignore-not-found, leaving the rest of the function (including deleted "$kind/$name") unchanged. ``` </details> <!-- cr-comment:v1:6bcb68838fd98da6aa0ece53 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>tests/e2e/lib/backup.go (1)</summary><blockquote> `22-22`: _⚡ Quick win_ **Using deprecated `k8s.io/utils/pointer` package.** The `k8s.io/utils/pointer` package is deprecated in favor of `k8s.io/utils/ptr`. Line 123 uses `pointer.Bool(false)`, but `tests/e2e/lib/virt_storage_helpers.go` already uses the newer `ptr.To[T]()` pattern (line 370). This inconsistency should be resolved by migrating to the `ptr` package. <details> <summary>♻️ Proposed migration to ptr package</summary> ```diff - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ``` And on line 123: ```diff - DefaultVolumesToFsBackup: pointer.Bool(false), + DefaultVolumesToFsBackup: ptr.To(false), ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/lib/backup.go` at line 22, Replace use of the deprecated k8s.io/utils/pointer with k8s.io/utils/ptr: update the import in tests/e2e/lib/backup.go from "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and change occurrences of pointer.Bool(false) (the call referenced in the review) to ptr.To(false) or ptr.To[bool](false) so it matches the ptr.To[T]() pattern used elsewhere (e.g., virt_storage_helpers.go); ensure all other pointer.* usages in the file are similarly migrated. ``` </details> <!-- cr-comment:v1:6dbfd69e955c75276c2e3594 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@tests/e2e/lib/backup.go:
- Around line 61-66: The volume policy currently has empty conditions in the
kubevirtVolumePolicyData constant and the manifest (volume-policy.yaml), which
makes the policy apply to all volumes; update both the kubevirtVolumePolicyData
constant and the volume-policy.yaml to scope the policy to only CBT-enabled PVCs
by adding a label-based condition that matches the CBT label used in your tests
(e.g., add a condition selecting PVCs with the CBT label such as
cdi.kubevirt.io/cbt: "true" or the actual label key/value your tests use), so
the policy will skip CSI snapshots only for those CBT-labeled PVCs.In
@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/centos-stream10-cbt.yaml:
- Around line 30-48: The SecurityContextConstraints resource named
mysql-persistent-scc is overly permissive: change allowPrivilegeEscalation and
allowPrivilegedContainer to false, replace RunAsAny for
runAsUser/seLinuxContext/fsGroup/supplementalGroups with more restrictive
policies (e.g., use MustRunAsRange for runAsUser and fsGroup where applicable),
limit volumes instead of '*' and remove unnecessary users if possible; update
the SecurityContextConstraints named mysql-persistent-scc to apply those tighter
settings to follow least-privilege guidelines while keeping the manifest name
(mysql-persistent-scc) and resource kind SecurityContextConstraints intact so
tests continue to reference it.- Around line 162-192: The cloud-init userData block contains hardcoded
credentials (password: dog8code and MySQL user 'test' with password 'test') —
replace these with references to secrets or templated variables and document
their usage: remove literal "password: dog8code" under userData and substitute a
cloud-init secret/variable, and change the mysql -e commands that create user
'test'@'localhost' IDENTIFIED BY 'test' to consume the MySQL password from a
secret or template variable; update any deployment templating (e.g., YAML
placeholders or secret-mounting mechanism) and add a short comment above
userData explaining where the secrets come from and how to provide them in
CI/test environments.In
@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/fedora-todolist-cbt.yaml:
- Around line 17-35: The SecurityContextConstraints resource named
mysql-persistent-scc is overly permissive; update it to follow least privilege
by setting allowPrivilegeEscalation: false and allowPrivilegedContainer: false,
change runAsUser/fsGroup/supplementalGroups/seLinuxContext from RunAsAny to more
restrictive policies (e.g., runAsUser: MustRunAsRange and appropriate
fsGroup/supplementalGroups ranges), and replace volumes: ['*'] with only the
specific volume types required by the MariaDB/todolist VM; keep the metadata
name mysql-persistent-scc and users entries unchanged while tightening these
fields to remove unnecessary privileges.- Around line 144-187: The cloud-init userData contains hardcoded credentials in
the chpasswd/users block and in runcmd MySQL commands (e.g., the chpasswd
entries under chpasswd -> users, and the mysql -uroot -e "... CREATE USER 'test'
... IDENTIFIED BY 'test' ..." and grant commands in runcmd); remove these
plain-text passwords and replace them with a secret-backed mechanism or
documented placeholders: pull passwords from a Kubernetes/VM secret or reference
an encrypted/templated variable (document the requirement), update userData (the
userData YAML block, chpasswd/users and the runcmd MySQL create/grant lines) to
use placeholders or secret injection instead of literal values, and ensure tests
or CI inject the actual credentials via the chosen secret mechanism.In
@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/README.md:
- Around line 29-31: Remove or update the ambiguous note "this may be outdated"
in the README section that describes enabling the IncrementalBackup and
UtilityVolumes feature gates on the KubeVirt CR: either verify the statement and
delete the note, or replace it with a clear, up-to-date explanation (e.g.,
confirm which KubeVirt versions/configurations enable IncrementalBackup and
UtilityVolumes and document any steps or caveats). Ensure the text around
"IncrementalBackup", "UtilityVolumes" and "KubeVirt CR" reflects the verified
behavior.In
@tests/e2e/virt_backup_restore_suite_test.go:
- Around line 168-247: runCBTVmBackup currently waits for namespace deletion via
lib.IsNamespaceDeleted but never explicitly deletes the namespace first; call
lib.DeleteNamespace(v.Clientset, brCase.Namespace) (as done in
runVmBackupAndRestore) at the start of runCBTVmBackup, assert the error is nil
(gomega.Expect(err).To(gomega.BeNil())), then keep the existing
gomega.Eventually(lib.IsNamespaceDeleted(...)) wait; reference runCBTVmBackup,
lib.DeleteNamespace, and lib.IsNamespaceDeleted to locate the spots to modify.
Outside diff comments:
In@tests/e2e/lib/virt_storage_helpers.go:
- Around line 407-411: The code assigns wffcStorageClass := defaultStorageClass
and then mutates it, which alters the original defaultStorageClass; change the
assignment to use a DeepCopy of defaultStorageClass (e.g., call
defaultStorageClass.DeepCopy()) before modifying VolumeBindingMode, Name,
ResourceVersion, and Annotations so mutations only affect wffcStorageClass
(mirror the pattern used in CreateImmediateModeStorageClass).
Nitpick comments:
In@docs/design/specs/2026-04-21-kubevirt-datamover-e2e-test-design.md:
- Around line 153-163: The fenced pseudocode block listing
EnsureVirtInstallation(), EnableCBTFeatureGate(), EnableCBTLabelSelector(), etc.
lacks a language specifier which breaks markdown linting and rendering; update
that fenced code block to include a language (for example add ```text orcorrectly and satisfies linters. - Around line 194-196: The markdown code block containing the REST endpoint "PUT /apis/subresources.kubevirt.io/v1/namespaces/{namespace}/virtualmachines/{name}/start" needs a language specifier for proper rendering and linting; update the triple-backtick fence so it starts with a language tag (e.g., ```http or ```text) immediately before the PUT line and keep the closing ``` unchanged so the block becomes a fenced code block labeled with the chosen language. In `@tests/e2e/lib/backup.go`: - Line 22: Replace use of the deprecated k8s.io/utils/pointer with k8s.io/utils/ptr: update the import in tests/e2e/lib/backup.go from "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and change occurrences of pointer.Bool(false) (the call referenced in the review) to ptr.To(false) or ptr.To[bool](false) so it matches the ptr.To[T]() pattern used elsewhere (e.g., virt_storage_helpers.go); ensure all other pointer.* usages in the file are similarly migrated. In `@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/cleanup.sh`: - Line 26: Remove the unused RED color variable or use it for error messages: either delete RED from the color definitions (leaving YELLOW, GREEN, CYAN, NC) or add an error helper (e.g., an error() function that prints messages prefixed with RED and resets with NC) and call that function where cleanup failures or error conditions are reported in cleanup.sh so RED is actually used. - Around line 41-51: In wait_for_deletion, quote the namespace flag variable to avoid word-splitting/globbing: update the oc get invocation in function wait_for_deletion to use "$ns_flag" (and similarly ensure any other uses of ns_flag in that function are quoted) so the command arguments are passed safely when ns_flag is empty or contains spaces/globs. - Around line 33-39: The delete_if_exists function uses an unquoted variable ns_flag in oc commands which can cause word-splitting/globbing; update the two oc invocations inside delete_if_exists to quote "${ns_flag}" so the commands read oc get "$kind" "$name" "${ns_flag}" and oc delete "$kind" "$name" "${ns_flag}" --ignore-not-found, leaving the rest of the function (including deleted "$kind/$name") unchanged.🪄 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:
7c7e7ef0-0909-4719-a113-c499811108d3📒 Files selected for processing (23)
CLAUDE.mdMakefilebuild/ci-Dockerfiledocs/design/specs/2026-04-21-kubevirt-datamover-e2e-test-design.mddocs/developer/testing/TESTING.mdgo.modtests/e2e/e2e_suite_test.gotests/e2e/lib/backup.gotests/e2e/lib/virt_helpers.gotests/e2e/lib/virt_storage_helpers.gotests/e2e/sample-applications/virtual-machines/cirros-test/cirros-test-cbt.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/README.mdtests/e2e/sample-applications/virtual-machines/kubevirt-dm/annotate_hco.shtests/e2e/sample-applications/virtual-machines/kubevirt-dm/backup-cirros.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/backup-fedora.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/backup-windows.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/centos-stream10-cbt.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/cirros-vm-cbt.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/cleanup.shtests/e2e/sample-applications/virtual-machines/kubevirt-dm/fedora-todolist-cbt.yamltests/e2e/sample-applications/virtual-machines/kubevirt-dm/volume-policy.yamltests/e2e/templates/default_settings.jsontests/e2e/virt_backup_restore_suite_test.go
Comment on lines
+61
to
+66
| const kubevirtVolumePolicyData = `version: v1 | ||
| volumePolicies: | ||
| - conditions: {} | ||
| action: | ||
| type: skip | ||
| ` |
Contributor
There was a problem hiding this comment.
Volume policy scope mismatch spans code and manifest. The kubevirtVolumePolicyData constant in backup.go and the volume-policy.yaml manifest both define conditions: {} (empty), causing the policy to apply to all volumes unconditionally. The comment in backup.go line 69-70 states the policy should "skip CSI snapshots for CBT-labeled PVCs," but empty conditions will skip ALL CSI snapshots. Verify whether unconditional skip is the intended behavior for datamover tests; if not, add label-based conditions to scope the policy to CBT-enabled PVCs only.
🤖 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 `@tests/e2e/lib/backup.go` around lines 61 - 66, The volume policy currently
has empty conditions in the kubevirtVolumePolicyData constant and the manifest
(volume-policy.yaml), which makes the policy apply to all volumes; update both
the kubevirtVolumePolicyData constant and the volume-policy.yaml to scope the
policy to only CBT-enabled PVCs by adding a label-based condition that matches
the CBT label used in your tests (e.g., add a condition selecting PVCs with the
CBT label such as cdi.kubevirt.io/cbt: "true" or the actual label key/value your
tests use), so the policy will skip CSI snapshots only for those CBT-labeled
PVCs.
Comment on lines
+30
to
+48
| - kind: SecurityContextConstraints | ||
| apiVersion: security.openshift.io/v1 | ||
| metadata: | ||
| name: mysql-persistent-scc | ||
| allowPrivilegeEscalation: true | ||
| allowPrivilegedContainer: true | ||
| runAsUser: | ||
| type: RunAsAny | ||
| seLinuxContext: | ||
| type: RunAsAny | ||
| fsGroup: | ||
| type: RunAsAny | ||
| supplementalGroups: | ||
| type: RunAsAny | ||
| volumes: | ||
| - '*' | ||
| users: | ||
| - system:admin | ||
| - system:serviceaccount:mysql-persistent:mysql-persistent-sa |
Contributor
There was a problem hiding this comment.
Overly permissive SecurityContextConstraints for test workload.
The SCC grants allowPrivilegeEscalation: true, allowPrivilegedContainer: true, and RunAsAny for user/SELinux/fsGroup/supplementalGroups. This exceeds the principle of least privilege.
Per coding guidelines: "Flag privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN capability, running as root without justification, and allowPrivilegeEscalation: true in container/K8s manifests."
For this test workload (CentOS Stream 10 VM running MariaDB/todolist app), these broad permissions appear unnecessary. Consider the same restrictions as suggested for the Fedora manifest: using MustRunAsRange, setting allowPrivilegeEscalation: false, allowPrivilegedContainer: false, and restricting volumes.
🤖 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
`@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/centos-stream10-cbt.yaml`
around lines 30 - 48, The SecurityContextConstraints resource named
mysql-persistent-scc is overly permissive: change allowPrivilegeEscalation and
allowPrivilegedContainer to false, replace RunAsAny for
runAsUser/seLinuxContext/fsGroup/supplementalGroups with more restrictive
policies (e.g., use MustRunAsRange for runAsUser and fsGroup where applicable),
limit volumes instead of '*' and remove unnecessary users if possible; update
the SecurityContextConstraints named mysql-persistent-scc to apply those tighter
settings to follow least-privilege guidelines while keeping the manifest name
(mysql-persistent-scc) and resource kind SecurityContextConstraints intact so
tests continue to reference it.
Source: Coding guidelines
Comment on lines
+162
to
+192
| userData: |- | ||
| #cloud-config | ||
| user: cloud-user | ||
| password: dog8code | ||
| chpasswd: { expire: False } | ||
| packages: | ||
| - mariadb-server | ||
| - policycoreutils-python-utils | ||
| - unzip | ||
| - wget | ||
| runcmd: | ||
| - systemctl stop firewalld | ||
| - systemctl disable firewalld | ||
| - systemctl start mariadb | ||
| - systemctl enable mariadb | ||
| - mysql -uroot -e "CREATE DATABASE todolist; USE todolist; CREATE USER 'test'@'localhost' IDENTIFIED BY 'test';" | ||
| - mysql -uroot -e "grant all privileges on todolist.* to test@'localhost' identified by 'test'; FLUSH PRIVILEGES;" | ||
| - pushd /home/cloud-user/ | ||
| - wget https://github.com/weshayutin/todolist-mariadb-go/releases/download/testing3/todolist-linux-amd64.zip | ||
| - unzip todolist-linux-amd64.zip | ||
| - chown -R cloud-user:cloud-user /home/cloud-user | ||
| - semanage fcontext --add --type bin_t '/home/cloud-user/todolist-linux-amd64' | ||
| - restorecon -Fv /home/cloud-user/todolist-linux-amd64 | ||
| - cp systemd/todolist-mariadb.service /etc/systemd/system/ | ||
| - popd | ||
| - systemctl daemon-reload | ||
| - systemctl start todolist-mariadb.service | ||
| - systemctl enable todolist-mariadb.service | ||
| - systemctl status todolist-mariadb.service | ||
| - systemctl disable cloud-init | ||
| name: cloudinitdisk |
Contributor
There was a problem hiding this comment.
Hardcoded credentials in cloud-init userData.
The cloud-init script contains hardcoded passwords:
- Line 165:
dog8code - Lines 177-178: MySQL user
testwith passwordtest
While this is a test manifest, hardcoded credentials should be documented or handled via secrets for better security hygiene.
🤖 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
`@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/centos-stream10-cbt.yaml`
around lines 162 - 192, The cloud-init userData block contains hardcoded
credentials (password: dog8code and MySQL user 'test' with password 'test') —
replace these with references to secrets or templated variables and document
their usage: remove literal "password: dog8code" under userData and substitute a
cloud-init secret/variable, and change the mysql -e commands that create user
'test'@'localhost' IDENTIFIED BY 'test' to consume the MySQL password from a
secret or template variable; update any deployment templating (e.g., YAML
placeholders or secret-mounting mechanism) and add a short comment above
userData explaining where the secrets come from and how to provide them in
CI/test environments.
Source: Coding guidelines
Comment on lines
+17
to
+35
| - kind: SecurityContextConstraints | ||
| apiVersion: security.openshift.io/v1 | ||
| metadata: | ||
| name: mysql-persistent-scc | ||
| allowPrivilegeEscalation: true | ||
| allowPrivilegedContainer: true | ||
| runAsUser: | ||
| type: RunAsAny | ||
| seLinuxContext: | ||
| type: RunAsAny | ||
| fsGroup: | ||
| type: RunAsAny | ||
| supplementalGroups: | ||
| type: RunAsAny | ||
| volumes: | ||
| - '*' | ||
| users: | ||
| - system:admin | ||
| - system:serviceaccount:mysql-persistent:mysql-persistent-sa |
Contributor
There was a problem hiding this comment.
Overly permissive SecurityContextConstraints for test workload.
The SCC grants allowPrivilegeEscalation: true, allowPrivilegedContainer: true, and RunAsAny for user/SELinux/fsGroup/supplementalGroups. This exceeds the principle of least privilege.
Per coding guidelines: "Flag privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN capability, running as root without justification, and allowPrivilegeEscalation: true in container/K8s manifests."
For this test workload (Fedora VM running MariaDB/todolist app), these broad permissions appear unnecessary. Consider:
- Using
MustRunAsRangeinstead ofRunAsAny - Setting
allowPrivilegeEscalation: falseunless privilege escalation is specifically required - Setting
allowPrivilegedContainer: false - Restricting volumes to only those needed (instead of
'*')
🤖 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
`@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/fedora-todolist-cbt.yaml`
around lines 17 - 35, The SecurityContextConstraints resource named
mysql-persistent-scc is overly permissive; update it to follow least privilege
by setting allowPrivilegeEscalation: false and allowPrivilegedContainer: false,
change runAsUser/fsGroup/supplementalGroups/seLinuxContext from RunAsAny to more
restrictive policies (e.g., runAsUser: MustRunAsRange and appropriate
fsGroup/supplementalGroups ranges), and replace volumes: ['*'] with only the
specific volume types required by the MariaDB/todolist VM; keep the metadata
name mysql-persistent-scc and users entries unchanged while tightening these
fields to remove unnecessary privileges.
Source: Coding guidelines
Comment on lines
+144
to
+187
| userData: |- | ||
| #cloud-config | ||
| ssh_pwauth: true | ||
| users: | ||
| - default | ||
| - name: test | ||
| lock_passwd: false | ||
| shell: /bin/bash | ||
| sudo: ALL=(ALL) NOPASSWD:ALL | ||
| homedir: /home/test | ||
| chpasswd: | ||
| expire: false | ||
| users: | ||
| - name: fedora | ||
| password: dog8code | ||
| type: text | ||
| - name: test | ||
| password: dog8code | ||
| type: text | ||
| packages: | ||
| - mariadb-server | ||
| - unzip | ||
| - wget | ||
| runcmd: | ||
| - systemctl stop firewalld | ||
| - systemctl disable firewalld | ||
| - systemctl start mariadb | ||
| - systemctl enable mariadb | ||
| - mysql -uroot -e "CREATE DATABASE todolist; USE todolist; CREATE USER 'test'@'localhost' IDENTIFIED BY 'test';" | ||
| - mysql -uroot -e "grant all privileges on todolist.* to test@'localhost' identified by 'test'; FLUSH PRIVILEGES;" | ||
| - pushd /home/test/ | ||
| - wget https://github.com/weshayutin/todolist-mariadb-go/releases/download/testing3/todolist-linux-amd64.zip | ||
| - unzip todolist-linux-amd64.zip | ||
| - chown -R test:test /home/test | ||
| - semanage fcontext --add --type bin_t '/home/test/todolist-linux-amd64' | ||
| - restorecon -Fv /home/test/todolist-linux-amd64 | ||
| - cp systemd/todolist-mariadb.service /etc/systemd/system/ | ||
| - popd | ||
| - systemctl daemon-reload | ||
| - systemctl start todolist-mariadb.service | ||
| - systemctl enable todolist-mariadb.service | ||
| - systemctl status todolist-mariadb.service | ||
| - systemctl disable cloud-init | ||
| name: cloudinitdisk |
Contributor
There was a problem hiding this comment.
Hardcoded credentials in cloud-init userData.
The cloud-init script contains hardcoded passwords:
- Lines 158, 161:
dog8code - Lines 172-173: MySQL user
testwith passwordtest
While this is a test manifest, hardcoded credentials should be documented or handled via secrets for better security hygiene.
🤖 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
`@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/fedora-todolist-cbt.yaml`
around lines 144 - 187, The cloud-init userData contains hardcoded credentials
in the chpasswd/users block and in runcmd MySQL commands (e.g., the chpasswd
entries under chpasswd -> users, and the mysql -uroot -e "... CREATE USER 'test'
... IDENTIFIED BY 'test' ..." and grant commands in runcmd); remove these
plain-text passwords and replace them with a secret-backed mechanism or
documented placeholders: pull passwords from a Kubernetes/VM secret or reference
an encrypted/templated variable (document the requirement), update userData (the
userData YAML block, chpasswd/users and the runcmd MySQL create/grant lines) to
use placeholders or secret injection instead of literal values, and ensure tests
or CI inject the actual credentials via the chosen secret mechanism.
Source: Coding guidelines
Comment on lines
+29
to
+31
| **note:** this may be outdated | ||
| This enables both the `IncrementalBackup` and `UtilityVolumes` feature gates on | ||
| the KubeVirt CR automatically. |
Contributor
There was a problem hiding this comment.
Clarify or remove "may be outdated" note.
Line 29 includes **note:** this may be outdated, which creates uncertainty about the accuracy of the documented behavior. Either verify the information is current and remove the note, or update the documentation with correct information.
🤖 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 `@tests/e2e/sample-applications/virtual-machines/kubevirt-dm/README.md` around
lines 29 - 31, Remove or update the ambiguous note "this may be outdated" in the
README section that describes enabling the IncrementalBackup and UtilityVolumes
feature gates on the KubeVirt CR: either verify the statement and delete the
note, or replace it with a clear, up-to-date explanation (e.g., confirm which
KubeVirt versions/configurations enable IncrementalBackup and UtilityVolumes and
document any steps or caveats). Ensure the text around "IncrementalBackup",
"UtilityVolumes" and "KubeVirt CR" reflects the verified behavior.
Comment on lines
+168
to
+247
| func runCBTVmBackup(brCase VmBackupRestoreCase, updateLastBRcase func(brCase VmBackupRestoreCase), v *lib.VirtOperator) { | ||
| updateLastBRcase(brCase) | ||
|
|
||
| backupName, _ := prepareBackupAndRestore(brCase.BackupRestoreCase, func() {}) | ||
|
|
||
| gomega.Eventually(lib.IsNamespaceDeleted(kubernetesClientForSuiteRun, brCase.Namespace), time.Minute*2, time.Second*5).Should(gomega.BeTrue()) | ||
| err := lib.CreateNamespace(v.Clientset, brCase.Namespace) | ||
| gomega.Expect(err).To(gomega.BeNil()) | ||
|
|
||
| err = lib.InstallApplication(v.Client, brCase.Template) | ||
| if err != nil { | ||
| fmt.Printf("Failed to install VM template %s: %v", brCase.Template, err) | ||
| } | ||
| gomega.Expect(err).To(gomega.BeNil()) | ||
|
|
||
| log.Printf("Waiting for VM %s/%s to reach Running status", brCase.Namespace, brCase.Name) | ||
| err = wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 15*time.Minute, true, func(ctx context.Context) (bool, error) { | ||
| status, err := v.GetVmStatus(brCase.Namespace, brCase.Name) | ||
| if err != nil { | ||
| log.Printf("VM %s/%s not yet available: %v", brCase.Namespace, brCase.Name, err) | ||
| return false, nil | ||
| } | ||
| return status == "Running", nil | ||
| }) | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
|
|
||
| err = v.WaitForVMReady(brCase.Namespace, brCase.Name, 5*time.Minute) | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
|
|
||
| if brCase.InitDelay > 0 { | ||
| log.Printf("Waiting %v for VM %s/%s to finish booting (cloud-init, etc.)", brCase.InitDelay, brCase.Namespace, brCase.Name) | ||
| time.Sleep(brCase.InitDelay) | ||
| } | ||
|
|
||
| log.Printf("VM %s/%s is fully booted, CBT enabled, proceeding with backup", brCase.Namespace, brCase.Name) | ||
|
|
||
| log.Printf("Creating kubevirt volume policy ConfigMap for custom action routing") | ||
| err = lib.EnsureKubevirtVolumePolicy(dpaCR.Client, namespace) | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
|
|
||
| log.Printf("Creating backup %s with kubevirt volume policy for case %s", backupName, brCase.Name) | ||
| err = lib.CreateBackupWithVolumePolicy(dpaCR.Client, namespace, backupName, []string{brCase.Namespace}, true) | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
|
|
||
| // Verify that the kubevirt-datamover controller creates a VirtualMachineBackupTracker | ||
| // in the VM namespace during the backup, confirming VEP-25 incremental backup is active. | ||
| vmbtSeen := false | ||
| vmbtCheckDone := make(chan struct{}) | ||
| go func() { | ||
| defer close(vmbtCheckDone) | ||
| for i := 0; i < 60; i++ { | ||
| found, checkErr := v.CheckVMBackupTrackerExists(brCase.Namespace) | ||
| if checkErr == nil && found { | ||
| log.Printf("VirtualMachineBackupTracker observed in %s — VEP-25 incremental backup confirmed", brCase.Namespace) | ||
| vmbtSeen = true | ||
| return | ||
| } | ||
| time.Sleep(5 * time.Second) | ||
| } | ||
| log.Printf("VirtualMachineBackupTracker was not observed in %s during backup window", brCase.Namespace) | ||
| }() | ||
|
|
||
| gomega.Eventually(lib.IsKubevirtDMBackupDone(dpaCR.Client, dynamicClientForSuiteRun, namespace, backupName), brCase.BackupTimeout, time.Second*10).Should(gomega.BeTrue()) | ||
| <-vmbtCheckDone | ||
| describeBackup := lib.DescribeBackup(dpaCR.Client, namespace, backupName) | ||
| ginkgo.GinkgoWriter.Println(describeBackup) | ||
|
|
||
| succeeded, err := lib.IsBackupCompletedSuccessfully(kubernetesClientForSuiteRun, dpaCR.Client, namespace, backupName) | ||
| gomega.Expect(err).ToNot(gomega.HaveOccurred()) | ||
| gomega.Expect(succeeded).To(gomega.Equal(true)) | ||
| log.Printf("Backup for case %s succeeded", brCase.Name) | ||
|
|
||
| gomega.Expect(vmbtSeen).To(gomega.BeTrue(), "expected VirtualMachineBackupTracker to be observed during backup (VEP-25 incremental backup)") | ||
|
|
||
| err = v.RemoveVm(brCase.Namespace, brCase.Name, 5*time.Minute) | ||
| gomega.Expect(err).To(gomega.BeNil()) | ||
| err = lib.DeleteNamespace(v.Clientset, brCase.Namespace) | ||
| gomega.Expect(err).To(gomega.BeNil()) | ||
| gomega.Eventually(lib.IsNamespaceDeleted(kubernetesClientForSuiteRun, brCase.Namespace), time.Minute*5, time.Second*5).Should(gomega.BeTrue()) | ||
| } |
Contributor
There was a problem hiding this comment.
Missing explicit namespace deletion at the start of runCBTVmBackup.
runCBTVmBackup only waits for namespace deletion at line 173 (gomega.Eventually(lib.IsNamespaceDeleted(...))) without explicitly deleting it first, unlike runVmBackupAndRestore which explicitly calls DeleteNamespace at lines 94-96. This assumes the previous test cleaned up properly.
If a previous test fails early or is skipped, the namespace may persist and cause line 173 to timeout after 2 minutes, leading to cascading test failures.
For consistency and robustness, explicitly delete the namespace before waiting for deletion:
🔧 Proposed fix to add explicit namespace deletion
func runCBTVmBackup(brCase VmBackupRestoreCase, updateLastBRcase func(brCase VmBackupRestoreCase), v *lib.VirtOperator) {
updateLastBRcase(brCase)
backupName, _ := prepareBackupAndRestore(brCase.BackupRestoreCase, func() {})
+ _ = v.RemoveVm(brCase.Namespace, brCase.Name, 2*time.Minute)
+ err := lib.DeleteNamespace(v.Clientset, brCase.Namespace)
+ gomega.Expect(err).To(gomega.BeNil())
gomega.Eventually(lib.IsNamespaceDeleted(kubernetesClientForSuiteRun, brCase.Namespace), time.Minute*2, time.Second*5).Should(gomega.BeTrue())
- err := lib.CreateNamespace(v.Clientset, brCase.Namespace)
+
+ err = lib.CreateNamespace(v.Clientset, brCase.Namespace)
gomega.Expect(err).To(gomega.BeNil())🤖 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 `@tests/e2e/virt_backup_restore_suite_test.go` around lines 168 - 247,
runCBTVmBackup currently waits for namespace deletion via lib.IsNamespaceDeleted
but never explicitly deletes the namespace first; call
lib.DeleteNamespace(v.Clientset, brCase.Namespace) (as done in
runVmBackupAndRestore) at the start of runCBTVmBackup, assert the error is nil
(gomega.Expect(err).To(gomega.BeNil())), then keep the existing
gomega.Eventually(lib.IsNamespaceDeleted(...)) wait; reference runCBTVmBackup,
lib.DeleteNamespace, and lib.IsNamespaceDeleted to locate the spots to modify.
|
@weshayutin: 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. |
shubham-pampattiwar
approved these changes
Jun 11, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shubham-pampattiwar, 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
Contributor
Author
|
@sseago oadp-dev :) review please |
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.
First pass at kubevirt-datamover E2E tests:
FYI: in a follow up pr I should document where the HCO is coming from:
https://quay.io/repository/kubevirt/hyperconverged-cluster-index?tab=tags
Cherry-picked from PR #2174 (openshift/oadp-operator, branch: kubevirt-datamover-e2e)
Original target: oadp-1.6, squashed onto oadp-dev
Summary by CodeRabbit
Tests
Documentation