Open
Conversation
openshift-ci
bot
added
the
size/XXL
Contributor
|
[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:
WalkthroughIntegration test entrypoint and Makefile were split into separate SNO and MNO runs; integration test Docker build context expanded; go.mod dependency set was adjusted (removed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.26% +0.53%
==========================================
Files 52 52
Lines 3901 3901
==========================================
+ Hits 2018 2039 +21
+ Misses 1715 1698 -17
+ Partials 168 164 -4 🚀 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
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
Contributor
|
@mmakwana30: The following tests 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. |
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.
Logs: https://privatebin.corp.redhat.com/?a28906199661f4a1#DbpjsRYRXvWY9PYuyeSsuVEeXuBMP9n3raN2aBESmc3C