Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mmakwana30 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request refactors the integration test infrastructure by splitting test execution into SNO and MNO environments, migrating test utilities to a restructured directory ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/Makefile`:
- Around line 34-35: The Makefile target integration-docker-run is invoking a
non-existent suite path openshift/lvm-operator/test/integration/qe_tests; update
the command in integration-docker-run to invoke the actual suites defined in
integration.go (qe_tests/sno and qe_tests/mno) — either run them both (e.g.,
call run-suite twice or pass both suite paths) or replace the single path with
the correct concrete suite path(s) so the docker run command executes the
existing qe_tests/sno and qe_tests/mno suites.
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 1579-1582: The error from cmd.CombinedOutput() is ignored; update
the error handling to return a non-nil error (and a safe zero value for the
bool) instead of an empty block. Inside the if err != nil { ... } handle, return
false and wrap the error with context (e.g., fmt.Errorf("running %v failed: %w",
cmd.Args, err)) and include trimmed output if helpful; add fmt to imports if
missing. This ensures the function (in lvms_utils.go where cmd.CombinedOutput()
is called) surfaces command failures instead of silently continuing.
- Around line 361-364: The getRandomNum function currently calls rand.Seed on
every invocation which is deprecated and unsafe; remove the per-call rand.Seed
and instead either rely on the auto-seeded global PRNG (drop the Seed call and
use rand.Int63n directly in getRandomNum) or create a package-level
math/rand.Rand (e.g., r := rand.New(rand.NewSource(time.Now().UnixNano()))
initialized once in init()) and call r.Int63n in getRandomNum so seeding happens
only once and not on every call.
- Around line 1856-1891: The YAML produced by
createLVMClusterWithPathsAndOptionalPaths is malformed: it omits metadata.name
and places paths/optionalPaths under thinPoolConfig instead of under
deviceSelector, and the fmt.Sprintf argument order must match the updated
placeholders; fix by adding a metadata.name: %s line (using the function's name
parameter), move the pathsJSON and optionalPathsJSON keys out of thinPoolConfig
into a deviceSelector block under the device class (deviceSelector:\n
paths: %s\n optionalPaths: %s), keep thinPoolConfig only for
thinPoolConfig fields, and update the fmt.Sprintf call in
createLVMClusterWithPathsAndOptionalPaths so the placeholder order is
metadata.name, namespace, deviceClass, pathsJSON, optionalPathsJSON before
calling createLVMClusterFromJSON.
- Around line 1551-1553: The empty error handling after calling
resourceExists("lvmcluster", name, namespace) should be fixed: handle the
returned err instead of ignoring it—either propagate it (e.g., return exists,
err or return false, err from the enclosing function) or log the error with
context using the existing logger and then decide on a fallback; update the code
around the resourceExists call (variables exists and err) to implement one of
these behaviors so the error is not silently dropped.
- Around line 1236-1246: The shell snippet in lvms_utils.go has an unclosed
outer if (if [ "$lv_count" -gt 1 ]) causing a syntax error; close that block by
inserting a matching fi immediately after the inner orphaned-check if block (the
block that echoes "HAS_ORPHANED_LVS" and exits) so the structure is: if [
"$lv_count" -gt 1 ]; then ... if [ orphaned ]; then ... fi fi before the final
echo "OK".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2256d975-16da-413b-9dbf-44fbeedf408f
⛔ Files ignored due to path filters (1)
test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
test/integration/Makefiletest/integration/go.modtest/integration/integration.gotest/integration/qe_tests/lvms.gotest/integration/qe_tests/lvms_utils.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2146 +/- ##
==========================================
+ Coverage 51.73% 52.23% +0.50%
==========================================
Files 52 52
Lines 3901 3896 -5
==========================================
+ Hits 2018 2035 +17
+ Misses 1715 1698 -17
+ Partials 168 163 -5 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/go.mod (1)
16-24:⚠️ Potential issue | 🟠 MajorAdd
k8s.io/kubernetesto the require block.Integration tests import
k8s.io/kubernetes/test/e2e/framework(confirmed intest/integration/qe-tests/lvms_utils.goandlvms.go), butk8s.io/kubernetesonly appears in the replace block, not in require. Without a require entry, the replace directive won't be applied and the build will fail or behave unexpectedly. Add a require statement fork8s.io/kubernetesbefore the replace block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/go.mod` around lines 16 - 24, The go.mod is missing a require entry for k8s.io/kubernetes, so the replace directive for k8s.io/kubernetes won't be applied; add a require line for "k8s.io/kubernetes" (e.g., require k8s.io/kubernetes v1.30.1) in the require block before the replace block so imports like k8s.io/kubernetes/test/e2e/framework resolve and the replace mapping is used.
♻️ Duplicate comments (1)
test/integration/Makefile (1)
35-35:⚠️ Potential issue | 🟠 MajorUse concrete suite names in
integration-docker-run.
run-suiteis still called with.../qe_tests, while suites are registered as.../qe_tests/snoand.../qe_tests/mno. This can skip tests or fail in Docker runs.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed fix
integration-docker-run: integration-docker-build - docker run --rm $(FULL_IMAGE) run-suite openshift/lvm-operator/test/integration/qe_tests + docker run --rm $(FULL_IMAGE) run-suite -c 1 openshift/lvm-operator/test/integration/qe_tests/sno + docker run --rm $(FULL_IMAGE) run-suite -c 1 openshift/lvm-operator/test/integration/qe_tests/mno🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/Makefile` at line 35, The docker command in the integration-docker-run target uses a generic suite path "openshift/lvm-operator/test/integration/qe_tests" which doesn't match registered suites (e.g., ".../qe_tests/sno" and ".../qe_tests/mno"); update the docker run invocation to call the concrete suite names (referencing the existing suite identifiers like "openshift/lvm-operator/test/integration/qe_tests/sno" and "openshift/lvm-operator/test/integration/qe_tests/mno") so Docker runs the specific suites (either run them separately or pass both concrete suite paths) instead of the generic directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/integration/go.mod`:
- Around line 16-24: The go.mod is missing a require entry for
k8s.io/kubernetes, so the replace directive for k8s.io/kubernetes won't be
applied; add a require line for "k8s.io/kubernetes" (e.g., require
k8s.io/kubernetes v1.30.1) in the require block before the replace block so
imports like k8s.io/kubernetes/test/e2e/framework resolve and the replace
mapping is used.
---
Duplicate comments:
In `@test/integration/Makefile`:
- Line 35: The docker command in the integration-docker-run target uses a
generic suite path "openshift/lvm-operator/test/integration/qe_tests" which
doesn't match registered suites (e.g., ".../qe_tests/sno" and
".../qe_tests/mno"); update the docker run invocation to call the concrete suite
names (referencing the existing suite identifiers like
"openshift/lvm-operator/test/integration/qe_tests/sno" and
"openshift/lvm-operator/test/integration/qe_tests/mno") so Docker runs the
specific suites (either run them separately or pass both concrete suite paths)
instead of the generic directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 107274df-5538-4464-aed1-bc8018937e4a
⛔ Files ignored due to path filters (1)
test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
test/integration/Makefiletest/integration/go.modtest/integration/integration.gotest/integration/qe_tests/lvms.gotest/integration/qe_tests/lvms_utils.go
✅ Files skipped from review due to trivial changes (1)
- test/integration/qe_tests/lvms_utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/integration.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/Dockerfile`:
- Line 7: The Dockerfile uses "COPY . ./" before "WORKDIR /workspace", which
places the repo contents in the image root and causes subsequent build steps
(the Go builds in the Dockerfile) to run from the wrong module context; fix by
either moving the "WORKDIR /workspace" line above the "COPY . ./" instruction or
change the COPY destination to the explicit workspace path (e.g., replace "COPY
. ./" with "COPY . /workspace") so that the repository is copied into the
intended /workspace module before the Go build steps run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7d74692c-68b2-4d28-a81d-39c4caac5c83
⛔ Files ignored due to path filters (1)
test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
test/integration/Dockerfiletest/integration/Makefiletest/integration/go.modtest/integration/integration.gotest/integration/qe-tests/lvms.gotest/integration/qe-tests/lvms_utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/integration.go
533bce3 to
074cacf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/integration/qe_tests/lvms_utils.go (2)
509-516: TLS verification disabled for pod exec.Disabling TLS verification (
config.Insecure = true) weakens security posture. While acceptable in test code, consider documenting why this is necessary (certificate chain issues with in-cluster exec).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/qe_tests/lvms_utils.go` around lines 509 - 516, The execCommandInPod function currently disables TLS verification on a copied kube config (setting config.Insecure, config.TLSClientConfig.Insecure, CAData/CAFile) which weakens security; update the code by adding a concise explanatory comment above the config modifications in execCommandInPod (referencing TestClient.Config and the copied config variable) stating that TLS verification is intentionally disabled only for integration tests due to in-cluster certificate/exec handshake issues, and document any assumptions or risks (e.g., not for production) so future readers understand why this exception exists.
2175-2191: Passphrase visible in process list.The passphrase is passed via
echowhich exposes it inpsoutput and potentially shell history. For test code this is low risk, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/qe_tests/lvms_utils.go` around lines 2175 - 2191, The passphrase is exposed via shell piping (echo) in setDiskEncryptPassphrase; instead create a temporary file on each worker with restrictive perms, write the passphrase into it, use cryptsetup's --key-file option for both luksFormat and luksOpen, then securely remove the temp file; update the calls that build cmd/openCmd (and where execCommandInNode is invoked) to: create temp (mktemp), chmod 600, write passphrase, run cryptsetup ... --key-file <tempfile> <diskName>, then rm -f <tempfile>, using the existing diskName, workerName, execCommandInNode and setDiskEncryptPassphrase identifiers to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 1507-1514: The loop over pvList ignores errors from the PV
update/delete calls; update the block that iterates pvList.Items (the pv
variable) to capture and handle the returned errors from
tc.Clientset.CoreV1().PersistentVolumes().Update(...) and Delete(...): check
each call's error, log a clear error message including pv.Name and the error,
and return or retry/collect failures as appropriate (e.g., continue on non-fatal
errors or fail the test on fatal ones) so orphaned PVs are not silently left
behind; ensure you reference the same pv.Finalizers modification and use the
returned objects/errors from Update and Delete for correct handling.
---
Nitpick comments:
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 509-516: The execCommandInPod function currently disables TLS
verification on a copied kube config (setting config.Insecure,
config.TLSClientConfig.Insecure, CAData/CAFile) which weakens security; update
the code by adding a concise explanatory comment above the config modifications
in execCommandInPod (referencing TestClient.Config and the copied config
variable) stating that TLS verification is intentionally disabled only for
integration tests due to in-cluster certificate/exec handshake issues, and
document any assumptions or risks (e.g., not for production) so future readers
understand why this exception exists.
- Around line 2175-2191: The passphrase is exposed via shell piping (echo) in
setDiskEncryptPassphrase; instead create a temporary file on each worker with
restrictive perms, write the passphrase into it, use cryptsetup's --key-file
option for both luksFormat and luksOpen, then securely remove the temp file;
update the calls that build cmd/openCmd (and where execCommandInNode is invoked)
to: create temp (mktemp), chmod 600, write passphrase, run cryptsetup ...
--key-file <tempfile> <diskName>, then rm -f <tempfile>, using the existing
diskName, workerName, execCommandInNode and setDiskEncryptPassphrase identifiers
to locate and change the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e0a8aebc-80da-4787-a394-d7bf20ab6603
⛔ Files ignored due to path filters (1)
test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
test/integration/Dockerfiletest/integration/Makefiletest/integration/go.modtest/integration/integration.gotest/integration/qe-tests/lvms.gotest/integration/qe-tests/lvms_utils.gotest/integration/qe_tests/lvms.gotest/integration/qe_tests/lvms_utils.go
💤 Files with no reviewable changes (2)
- test/integration/qe-tests/lvms.go
- test/integration/qe-tests/lvms_utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/integration/Dockerfile
- test/integration/go.mod
9ff83bc to
e96b80e
Compare
|
@coderabbitai perform full review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/integration/qe_tests/lvms_utils.go (3)
25-28:debugPodCounterappears unused.The variable is declared but not referenced. Remove if not needed, or add a comment if planned for future use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/qe_tests/lvms_utils.go` around lines 25 - 28, debugPodCounter is declared but never used; either remove the declaration of debugPodCounter from the file (leaving debugPodMutex if that's still used) or, if you intend to keep it for future use, add a brief comment next to the debugPodCounter declaration explaining its planned purpose; update the symbol name debugPodCounter accordingly so the intent is clear to reviewers.
554-582: Disk operations use unescaped shell commands.Commands like
fdisk,pvcreateuse string concatenation with disk names. While inputs are from cluster state, consider using%qfor safer quoting if disk names ever contain special characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/qe_tests/lvms_utils.go` around lines 554 - 582, The shell commands in createLogicalVolumeOnDisk build command strings by concatenating disk, vgName and lvName which can break for names with spaces/special chars; update each command (createPartitionCmd, unmountCmd, createPV, createVG, createLV) to build via fmt.Sprintf using %q (or a proper shell-escaping helper) to quote diskName, partitionName, vgName and lvName before calling execCommandInNodeWithError so arguments are safely quoted; reference createLogicalVolumeOnDisk and execCommandInNodeWithError when making the change.
1021-1143: Large embedded shell script for VG cleanup.This 120-line script is comprehensive but hard to maintain inline. Consider extracting to a ConfigMap or separate file if this grows further. Current approach works for test utilities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/qe_tests/lvms_utils.go` around lines 1021 - 1143, The embedded 120-line shell script inside forceDestroyVGOnNode (stored in cleanupScript and passed to exec.Command("oc", "debug", ...)) is hard to maintain; move the script out into a separate resource (e.g., a test asset file or a ConfigMap) and have forceDestroyVGOnNode load that file at runtime (or stream it into the debug session) instead of embedding it inline—update forceDestroyVGOnNode to read the external script into a string or provide its path and pass that content/path to the oc debug invocation (preserving the current formatting and behavior), so maintenance and diffs are much simpler.test/integration/qe_tests/testclient.go (1)
192-201: Stderr is discarded, losing diagnostic info.When commands fail, only stdout is logged. Consider capturing stderr for better debugging:
- cmd.Stderr = io.Discard + var stderr bytes.Buffer + cmd.Stderr = &stderr err := cmd.Run() if err != nil { - logf("Command failed: oc %s\nStdout: %s\nError: %v", - strings.Join(cmdArgs, " "), stdout.String(), err) + logf("Command failed: oc %s\nStdout: %s\nStderr: %s\nError: %v", + strings.Join(cmdArgs, " "), stdout.String(), stderr.String(), err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/qe_tests/testclient.go` around lines 192 - 201, The test currently discards stderr (cmd.Stderr = io.Discard) so failures only log stdout; create a bytes.Buffer named stderr, set cmd.Stderr = &stderr (alongside the existing stdout buffer), run cmd.Run(), and when err != nil include stderr.String() in the processLogger/logf output (and/or return it along with stdout) referencing the existing cmd, stdout, cmdArgs and cmd.Run() variables so diagnostic error output is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 632-638: The helper execCommandInNodeWithError should not rely on
fragile string matching for "error"; instead, change it to rely on real command
exit status/errors from execCommandInNode (or update execCommandInNode to return
(string, error) if it currently doesn't) and propagate that error; remove the
strings.Contains check and use the returned error from execCommandInNode (or
inspect the process exit code) to decide whether to return an error, referencing
execCommandInNode and execCommandInNodeWithError to locate the change.
---
Nitpick comments:
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 25-28: debugPodCounter is declared but never used; either remove
the declaration of debugPodCounter from the file (leaving debugPodMutex if
that's still used) or, if you intend to keep it for future use, add a brief
comment next to the debugPodCounter declaration explaining its planned purpose;
update the symbol name debugPodCounter accordingly so the intent is clear to
reviewers.
- Around line 554-582: The shell commands in createLogicalVolumeOnDisk build
command strings by concatenating disk, vgName and lvName which can break for
names with spaces/special chars; update each command (createPartitionCmd,
unmountCmd, createPV, createVG, createLV) to build via fmt.Sprintf using %q (or
a proper shell-escaping helper) to quote diskName, partitionName, vgName and
lvName before calling execCommandInNodeWithError so arguments are safely quoted;
reference createLogicalVolumeOnDisk and execCommandInNodeWithError when making
the change.
- Around line 1021-1143: The embedded 120-line shell script inside
forceDestroyVGOnNode (stored in cleanupScript and passed to exec.Command("oc",
"debug", ...)) is hard to maintain; move the script out into a separate resource
(e.g., a test asset file or a ConfigMap) and have forceDestroyVGOnNode load that
file at runtime (or stream it into the debug session) instead of embedding it
inline—update forceDestroyVGOnNode to read the external script into a string or
provide its path and pass that content/path to the oc debug invocation
(preserving the current formatting and behavior), so maintenance and diffs are
much simpler.
In `@test/integration/qe_tests/testclient.go`:
- Around line 192-201: The test currently discards stderr (cmd.Stderr =
io.Discard) so failures only log stdout; create a bytes.Buffer named stderr, set
cmd.Stderr = &stderr (alongside the existing stdout buffer), run cmd.Run(), and
when err != nil include stderr.String() in the processLogger/logf output (and/or
return it along with stdout) referencing the existing cmd, stdout, cmdArgs and
cmd.Run() variables so diagnostic error output is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4f6b80f5-c6aa-4b48-bc49-3934db528143
⛔ Files ignored due to path filters (1)
test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
test/integration/Makefiletest/integration/go.modtest/integration/integration.gotest/integration/qe-tests/lvms.gotest/integration/qe-tests/lvms_utils.gotest/integration/qe_tests/lvms.gotest/integration/qe_tests/lvms_utils.gotest/integration/qe_tests/testclient.go
💤 Files with no reviewable changes (2)
- test/integration/qe-tests/lvms.go
- test/integration/qe-tests/lvms_utils.go
|
/retest |
|
/retest |
|
@mmakwana30: 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. |
Tests results on SNO:
https://privatebin.corp.redhat.com/?a616b7804c75c274#HfPW3YQvUBfSMRf2LcnmsAhtqUVgVbdMMt5mdXpuYeae
Tests results on MNO:
https://privatebin.corp.redhat.com/?b0ab88525c881d0e#E6trhZ1VjQGgNoG3ZBjfNsMXJox4SZ7SiCjpLWZz9676