STOR-2873: Implement metrics for mismatch symlinks#597
Open
gnufied wants to merge 7 commits intoopenshift:mainfrom
Open
STOR-2873: Implement metrics for mismatch symlinks#597gnufied wants to merge 7 commits intoopenshift:mainfrom
gnufied wants to merge 7 commits intoopenshift:mainfrom
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds LocalVolumeDeviceLink (LVDL) management: new internal handler and metrics collector, status reconciliation for ignored/rejected devices, PV provisioning refactor to structured args with device-link creation/status updates, CRD/RBAC/docs updates, many unit and e2e tests, and Prometheus alert rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/common/provisioner_utils.go (1)
215-215:⚠️ Potential issue | 🟡 MinorUse the provided
ctxinstead ofcontext.TODO().The function receives
ctxas a parameter but usescontext.TODO()forcontrollerutil.CreateOrUpdate. This could miss cancellation signals.🐛 Proposed fix
- opRes, err := controllerutil.CreateOrUpdate(context.TODO(), client, existingPV, func() error { + opRes, err := controllerutil.CreateOrUpdate(ctx, client, existingPV, func() error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/provisioner_utils.go` at line 215, The call to controllerutil.CreateOrUpdate uses context.TODO() which ignores the function's provided cancellation/timeout; update the call to pass the function parameter ctx instead (i.e., replace context.TODO() with ctx) so controllerutil.CreateOrUpdate(client, existingPV, ...) receives the passed-in context and cancellation is propagated; ensure this change is applied where controllerutil.CreateOrUpdate is invoked with client and existingPV.
🧹 Nitpick comments (6)
pkg/diskmaker/controllers/lvset/device_age_test.go (1)
69-71: Assert the new rejected-device bucket too.This test adapts to the new three-value return signature, but it still discards the new classification outcome. An explicit expectation here would keep regressions in
getValidDevicesfrom slipping through.Proposed test update
- validDevices, delayedDevices, _ := r.getValidDevices(nil, blockDevices) + validDevices, delayedDevices, rejectedDevices := r.getValidDevices(nil, blockDevices) assert.Lenf(t, validDevices, expectedValid[run], "validDevices") assert.Lenf(t, delayedDevices, len(blockDevices)-expectedValid[run], "delayedDevices") + assert.Empty(t, rejectedDevices, "rejectedDevices")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/diskmaker/controllers/lvset/device_age_test.go` around lines 69 - 71, Test currently ignores the third return value from r.getValidDevices; update the call to capture the rejected bucket (e.g., validDevices, delayedDevices, rejectedDevices := r.getValidDevices(nil, blockDevices)) and add an assertion for its length (e.g., assert.Lenf(t, rejectedDevices, expectedRejected[run], "rejectedDevices")). Add or compute expectedRejected for each run (recommended: introduce an expectedRejected slice alongside expectedValid) so the test verifies all three classification outcomes from getValidDevices.pkg/internal/diskutil.go (1)
19-23: Consider consolidating the new test seam instead of another package-global override.
CmdExecutornow joinsFilePathGlobandFilePathEvalSymLinksas process-wide mutable dependencies. That works, but it also makes command stubbing leak across tests and become order-sensitive once any related test runs in parallel. A small injected dependency bundle would keep the abstraction without adding more shared global state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/internal/diskutil.go` around lines 19 - 23, CmdExecutor, FilePathGlob and FilePathEvalSymLinks are introduced as package-global mutable seams which leak state across tests; replace them with a single injected dependency bundle (e.g., a small struct like diskDeps or DiskUtil with fields CmdExecutor utilexec.Interface, FilePathGlob func(string) ([]string, error), FilePathEvalSymLinks func(string) (string, error)) and refactor functions that currently read those globals to accept that deps struct (or be methods on a DiskUtil instance) so production code can use a package-default instance while tests construct isolated instances with stubbed executors/glob/eval functions, and update constructors and tests to inject mocks rather than mutating package globals.pkg/internal/diskutil_test.go (1)
26-44: Keep coverage forblkid's exit-status-2 path.
GetDeviceFSMaptreatsblkidexit code 2 as the normal blank-disk case, butnewFakeExecutorcan only model successfulCombinedOutputcalls. Please let the fake script(output, err)pairs and add one case for that path, otherwise regressions around unformatted devices will slip past both these tests and the downstream discovery tests.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/internal/diskutil_test.go` around lines 26 - 44, newFakeExecutor currently only models successful CombinedOutput calls, so tests can't simulate blkid returning exit status 2 which GetDeviceFSMap treats as a blank-disk path; update newFakeExecutor to accept pairs of (output string, err error) (or a small struct slice) instead of only outputs, populate each testingexec.FakeCmd.CombinedOutputScript to return the provided ([]byte(output), nil or error) and ensure one test case supplies an error that represents blkid exit status 2 (the same kind of error GetDeviceFSMap expects) so the blkid-exit-2 path is covered; adjust any call sites that create newFakeExecutor to pass the (output, err) pairs accordingly.pkg/common/symlink_utils.go (1)
52-67: Keep the new symlink scan mockable.This helper goes straight to
filepath.Glob, unlike the rest of the disk-path utilities that already route throughinternal.FilePathGlob/internal.PathEvalsToDiskLabel. That makes the new LVSet symlink lookup harder to unit-test and easier to drift from the existing filesystem hooks.♻️ Suggested change
func GetSymlinkedForCurrentSC(symlinkDir string, currentDevice internal.BlockDevice) (string, error) { - paths, err := filepath.Glob(filepath.Join(symlinkDir, "/*")) + paths, err := internal.FilePathGlob(filepath.Join(symlinkDir, "*")) if err != nil { return "", err }As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/symlink_utils.go` around lines 52 - 67, GetSymlinkedForCurrentSC currently calls filepath.Glob directly which breaks test hooks; change it to use the existing testable wrapper internal.FilePathGlob(symlinkDir, "/*") (or the equivalent internal.FilePathGlob call used elsewhere) while keeping the rest of the logic (iterating paths, calling internal.PathEvalsToDiskLabel(path, currentDevice.KName), returning filepath.Base(path) on match and propagating errors) so the symlink scan remains mockable and consistent with other disk-path utilities.test/e2e/localvolume_test.go (1)
283-309: Consider usingsets.Newfor consistency.This function manually builds a map while
eventuallyFindLVDLsForPVsusessets.New. Consider using the same pattern for consistency.♻️ Suggested refactor for consistency
func verifyLVDLFilesystemUUIDForPVs(t *testing.T, f *framework.Framework, namespace string, pvNames []string) { matcher := gomega.NewWithT(t) - pvNameSet := map[string]struct{}{} - for _, pvName := range pvNames { - pvNameSet[pvName] = struct{}{} - } + pvNameSet := sets.New(pvNames...) matcher.Eventually(func() bool { lvdlList := &localv1.LocalVolumeDeviceLinkList{} err := f.Client.List(goctx.TODO(), lvdlList, client.InNamespace(namespace)) if err != nil { t.Logf("error listing LocalVolumeDeviceLink objects while verifying filesystemUUID: %v", err) return false } uuidFoundCount := 0 for _, lvdl := range lvdlList.Items { - if _, ok := pvNameSet[lvdl.Spec.PersistentVolumeName]; !ok { + if !pvNameSet.Has(lvdl.Spec.PersistentVolumeName) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/localvolume_test.go` around lines 283 - 309, Replace the manual map-based set in verifyLVDLFilesystemUUIDForPVs with k8s sets for consistency with eventuallyFindLVDLsForPVs: import k8s.io/apimachinery/pkg/util/sets, create pvNameSet using sets.NewString(pvNames...), use pvNameSet.Has(lvdl.Spec.PersistentVolumeName) instead of map lookup, and compare uuidFoundCount to pvNameSet.Len() rather than len(pvNameSet); remove the manual population loop and adjust references accordingly.pkg/diskmaker/controllers/lvset/reconcile.go (1)
267-302: Consider extracting sharedprocessRejectedDevicesForDeviceLinkslogic.This function is nearly identical to the one in
pkg/diskmaker/controllers/lv/reconcile.go. Consider extracting to a shared helper inpkg/commonorpkg/internalto reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/diskmaker/controllers/lvset/reconcile.go` around lines 267 - 302, Extract the duplicated logic in processRejectedDevicesForDeviceLinks into a shared helper (e.g., pkg/common or pkg/internal) that accepts context, the node/namespace/runtimeConfig pieces needed, symLinkDir, storageClassName, lvset and the rejectedDevices slice and returns errors/events; reuse helper from both LocalVolumeSetReconciler.processRejectedDevicesForDeviceLinks and the near-identical function in pkg/diskmaker/controllers/lv/reconcile.go. Inside the helper keep the same calls to common.GetSymlinkedForCurrentSC, common.GetSymLinkSourceAndTarget, common.GeneratePVName and internal.NewDeviceLinkHandler/ApplyStatus and surface any failures so callers can call r.eventReporter.Report/newDiskEvent and klog.ErrorS as before; update both reconciler methods to call the new helper and remove duplicated loops and 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 `@HACKING.md`:
- Around line 9-12: The heading "Overriding diskmaker image or operator image
directly after deployment" is misleading because the steps only show deleting
the subscription object and editing the CSV to override the diskmaker image;
either update the heading to only mention "diskmaker image" or add the missing
operator-image instructions: describe how to edit the CSV object or the operator
Deployment (or ClusterServiceVersion) to change the operator image after
removing the subscription, include exact kubectl commands and which field to
patch (e.g., spec.install.spec.template.spec.containers[*].image) for the
operator, and keep the existing steps for "subscription object" and "CSV object"
referring to the "diskmaker image" override.
---
Outside diff comments:
In `@pkg/common/provisioner_utils.go`:
- Line 215: The call to controllerutil.CreateOrUpdate uses context.TODO() which
ignores the function's provided cancellation/timeout; update the call to pass
the function parameter ctx instead (i.e., replace context.TODO() with ctx) so
controllerutil.CreateOrUpdate(client, existingPV, ...) receives the passed-in
context and cancellation is propagated; ensure this change is applied where
controllerutil.CreateOrUpdate is invoked with client and existingPV.
---
Nitpick comments:
In `@pkg/common/symlink_utils.go`:
- Around line 52-67: GetSymlinkedForCurrentSC currently calls filepath.Glob
directly which breaks test hooks; change it to use the existing testable wrapper
internal.FilePathGlob(symlinkDir, "/*") (or the equivalent internal.FilePathGlob
call used elsewhere) while keeping the rest of the logic (iterating paths,
calling internal.PathEvalsToDiskLabel(path, currentDevice.KName), returning
filepath.Base(path) on match and propagating errors) so the symlink scan remains
mockable and consistent with other disk-path utilities.
In `@pkg/diskmaker/controllers/lvset/device_age_test.go`:
- Around line 69-71: Test currently ignores the third return value from
r.getValidDevices; update the call to capture the rejected bucket (e.g.,
validDevices, delayedDevices, rejectedDevices := r.getValidDevices(nil,
blockDevices)) and add an assertion for its length (e.g., assert.Lenf(t,
rejectedDevices, expectedRejected[run], "rejectedDevices")). Add or compute
expectedRejected for each run (recommended: introduce an expectedRejected slice
alongside expectedValid) so the test verifies all three classification outcomes
from getValidDevices.
In `@pkg/diskmaker/controllers/lvset/reconcile.go`:
- Around line 267-302: Extract the duplicated logic in
processRejectedDevicesForDeviceLinks into a shared helper (e.g., pkg/common or
pkg/internal) that accepts context, the node/namespace/runtimeConfig pieces
needed, symLinkDir, storageClassName, lvset and the rejectedDevices slice and
returns errors/events; reuse helper from both
LocalVolumeSetReconciler.processRejectedDevicesForDeviceLinks and the
near-identical function in pkg/diskmaker/controllers/lv/reconcile.go. Inside the
helper keep the same calls to common.GetSymlinkedForCurrentSC,
common.GetSymLinkSourceAndTarget, common.GeneratePVName and
internal.NewDeviceLinkHandler/ApplyStatus and surface any failures so callers
can call r.eventReporter.Report/newDiskEvent and klog.ErrorS as before; update
both reconciler methods to call the new helper and remove duplicated loops and
logic.
In `@pkg/internal/diskutil_test.go`:
- Around line 26-44: newFakeExecutor currently only models successful
CombinedOutput calls, so tests can't simulate blkid returning exit status 2
which GetDeviceFSMap treats as a blank-disk path; update newFakeExecutor to
accept pairs of (output string, err error) (or a small struct slice) instead of
only outputs, populate each testingexec.FakeCmd.CombinedOutputScript to return
the provided ([]byte(output), nil or error) and ensure one test case supplies an
error that represents blkid exit status 2 (the same kind of error GetDeviceFSMap
expects) so the blkid-exit-2 path is covered; adjust any call sites that create
newFakeExecutor to pass the (output, err) pairs accordingly.
In `@pkg/internal/diskutil.go`:
- Around line 19-23: CmdExecutor, FilePathGlob and FilePathEvalSymLinks are
introduced as package-global mutable seams which leak state across tests;
replace them with a single injected dependency bundle (e.g., a small struct like
diskDeps or DiskUtil with fields CmdExecutor utilexec.Interface, FilePathGlob
func(string) ([]string, error), FilePathEvalSymLinks func(string) (string,
error)) and refactor functions that currently read those globals to accept that
deps struct (or be methods on a DiskUtil instance) so production code can use a
package-default instance while tests construct isolated instances with stubbed
executors/glob/eval functions, and update constructors and tests to inject mocks
rather than mutating package globals.
In `@test/e2e/localvolume_test.go`:
- Around line 283-309: Replace the manual map-based set in
verifyLVDLFilesystemUUIDForPVs with k8s sets for consistency with
eventuallyFindLVDLsForPVs: import k8s.io/apimachinery/pkg/util/sets, create
pvNameSet using sets.NewString(pvNames...), use
pvNameSet.Has(lvdl.Spec.PersistentVolumeName) instead of map lookup, and compare
uuidFoundCount to pvNameSet.Len() rather than len(pvNameSet); remove the manual
population loop and adjust references accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 468c6b2f-d35b-45d8-b2ac-ccaa6851f532
⛔ Files ignored due to path filters (2)
vendor/k8s.io/utils/exec/testing/fake_exec.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (26)
HACKING.mdapi/v1/localvolumedevicelink_types.gocmd/diskmaker-manager/manager.goconfig/manifests/stable/local-storage-operator.clusterserviceversion.yamlconfig/manifests/stable/local.storage.openshift.io_localvolumedevicelinks.yamlpkg/common/provisioner_utils.gopkg/common/symlink_utils.gopkg/controllers/localvolume/localvolume_controller.gopkg/diskmaker/controllers/lv/create_pv_test.gopkg/diskmaker/controllers/lv/reconcile.gopkg/diskmaker/controllers/lv/reconcile_test.gopkg/diskmaker/controllers/lvset/create_pv_test.gopkg/diskmaker/controllers/lvset/device_age_test.gopkg/diskmaker/controllers/lvset/reconcile.gopkg/diskmaker/controllers/lvset/reconcile_test.gopkg/diskmaker/discovery/discovery_test.gopkg/diskmaker/event_reporter.gopkg/internal/device_link_handler.gopkg/internal/device_link_handler_test.gopkg/internal/disk_location.gopkg/internal/diskutil.gopkg/internal/diskutil_test.gopkg/localmetrics/custom_metrics_collector.gopkg/localmetrics/custom_metrics_collector_test.gotest/e2e/localvolume_test.gotest/e2e/localvolumeset_test.go
💤 Files with no reviewable changes (1)
- config/manifests/stable/local.storage.openshift.io_localvolumedevicelinks.yaml
Comment on lines
+9
to
+12
| ## Overriding diskmaker image or operator image directly after deployment | ||
|
|
||
| 1. Delete the subscription object so as OLM is no longer managing the operator. | ||
| 2. Edit the CSV object to override diskmaker image. |
There was a problem hiding this comment.
Align the heading with the actual steps.
The section promises both diskmaker and operator image overrides, but the instructions only describe changing the diskmaker image in the CSV. Either document the operator-image edit path too or narrow the heading so this workflow is not misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@HACKING.md` around lines 9 - 12, The heading "Overriding diskmaker image or
operator image directly after deployment" is misleading because the steps only
show deleting the subscription object and editing the CSV to override the
diskmaker image; either update the heading to only mention "diskmaker image" or
add the missing operator-image instructions: describe how to edit the CSV object
or the operator Deployment (or ClusterServiceVersion) to change the operator
image after removing the subscription, include exact kubectl commands and which
field to patch (e.g., spec.install.spec.template.spec.containers[*].image) for
the operator, and keep the existing steps for "subscription object" and "CSV
object" referring to the "diskmaker image" override.
Fix rbac rules and bugs to enable creation of lvdl objects
Also setowner ref
refactor code to findexisting lvldl Change UpdateStatus to ApplyStatus
Refactor the code to directly use BlockDevice Add comments about processRejectedDevicesForDeviceLinks
Add utilexec libraries
gnufied
force-pushed
the
implement-metrics-for-mismatch-symlinks
branch
from
1cf07bd to
7b34b7c
Compare
openshift-ci
bot
removed
the
needs-rebase
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 (2)
pkg/diskmaker/controllers/lv/create_pv_test.go (1)
252-254:⚠️ Potential issue | 🟠 MajorBug:
returnexits the entire test function instead of continuing to the next test case.Using
returnhere will exit the entireTestCreatePVfunction after the first test case that hasshouldErr: true, skipping all subsequent test cases intestTable.🐛 Proposed fix
if tc.shouldErr { - return + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/diskmaker/controllers/lv/create_pv_test.go` around lines 252 - 254, In TestCreatePV the test loop uses "return" when tc.shouldErr is true which exits the entire TestCreatePV function and prevents subsequent test cases in testTable from running; replace the "return" with "continue" inside the loop iterating over testTable so only the current case is skipped, locating the conditional that checks tc.shouldErr in the TestCreatePV test helper or loop and change the control flow accordingly.pkg/common/provisioner_utils.go (1)
215-215:⚠️ Potential issue | 🟡 MinorUse the passed
ctxparameter instead ofcontext.TODO().The function receives a
ctxparameter butcontrollerutil.CreateOrUpdateusescontext.TODO(). This should use the provided context for proper cancellation propagation and observability.🔧 Proposed fix
- opRes, err := controllerutil.CreateOrUpdate(context.TODO(), client, existingPV, func() error { + opRes, err := controllerutil.CreateOrUpdate(ctx, client, existingPV, func() error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/provisioner_utils.go` at line 215, Replace the hard-coded context.TODO() call in the controllerutil.CreateOrUpdate invocation with the function's passed-in ctx so cancellation and deadlines propagate; locate the CreateOrUpdate call (opRes, err := controllerutil.CreateOrUpdate(context.TODO(), client, existingPV, func() error { ... }) and swap context.TODO() for ctx, ensuring the surrounding function signature indeed accepts ctx and that imports/usage remain consistent.
🧹 Nitpick comments (2)
pkg/diskmaker/discovery/discovery_test.go (1)
17-35: Consider extractingnewFakeExecutorto a shared test utility.This helper is duplicated verbatim in
pkg/internal/diskutil_test.go. Extracting it to a shared location (e.g.,pkg/internal/testutil/exec.goor similar) would reduce duplication and ensure consistent behavior across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/diskmaker/discovery/discovery_test.go` around lines 17 - 35, The helper newFakeExecutor (which constructs a *testingexec.FakeExec with CommandScript/CombinedOutputScript) is duplicated; extract it into a shared test utility package (e.g., create pkg/internal/testutil with a file exec.go) and move the function there so both discovery.newFakeExecutor and the copy in pkg/internal/diskutil_test.go import testutil.NewFakeExecutor (or similar); update callers to use the shared symbol and keep the same signature (outputs ...string) and behavior using testingexec.FakeExec/ FakeCmd so tests remain unchanged.test/e2e/localvolume_test.go (1)
297-323: Consider usingsets.Newfor consistency.This function manually constructs
map[string]struct{}whileeventuallyFindLVDLsForPVsusessets.New. Consider using the same pattern for consistency across helper functions.♻️ Suggested refactor for consistency
func verifyLVDLFilesystemUUIDForPVs(t *testing.T, f *framework.Framework, namespace string, pvNames []string) { matcher := gomega.NewWithT(t) - pvNameSet := map[string]struct{}{} - for _, pvName := range pvNames { - pvNameSet[pvName] = struct{}{} - } + pvNameSet := sets.New(pvNames...) matcher.Eventually(func() bool { lvdlList := &localv1.LocalVolumeDeviceLinkList{} err := f.Client.List(goctx.TODO(), lvdlList, client.InNamespace(namespace)) @@ ... @@ uuidFoundCount := 0 for _, lvdl := range lvdlList.Items { - if _, ok := pvNameSet[lvdl.Spec.PersistentVolumeName]; !ok { + if !pvNameSet.Has(lvdl.Spec.PersistentVolumeName) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/localvolume_test.go` around lines 297 - 323, Replace the manual map construction in verifyLVDLFilesystemUUIDForPVs with a kubernetes sets.String for consistency with eventuallyFindLVDLsForPVs: create pvNameSet using sets.NewString(pvNames...) (from k8s.io/apimachinery/pkg/util/sets), change membership checks from "if _, ok := pvNameSet[pvName]" to pvNameSet.Has(pvName), and use pvNameSet.Len() when comparing counts; keep the rest of the function logic the same (references: verifyLVDLFilesystemUUIDForPVs, eventuallyFindLVDLsForPVs).
🤖 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 `@pkg/common/provisioner_utils.go`:
- Line 215: Replace the hard-coded context.TODO() call in the
controllerutil.CreateOrUpdate invocation with the function's passed-in ctx so
cancellation and deadlines propagate; locate the CreateOrUpdate call (opRes, err
:= controllerutil.CreateOrUpdate(context.TODO(), client, existingPV, func()
error { ... }) and swap context.TODO() for ctx, ensuring the surrounding
function signature indeed accepts ctx and that imports/usage remain consistent.
In `@pkg/diskmaker/controllers/lv/create_pv_test.go`:
- Around line 252-254: In TestCreatePV the test loop uses "return" when
tc.shouldErr is true which exits the entire TestCreatePV function and prevents
subsequent test cases in testTable from running; replace the "return" with
"continue" inside the loop iterating over testTable so only the current case is
skipped, locating the conditional that checks tc.shouldErr in the TestCreatePV
test helper or loop and change the control flow accordingly.
---
Nitpick comments:
In `@pkg/diskmaker/discovery/discovery_test.go`:
- Around line 17-35: The helper newFakeExecutor (which constructs a
*testingexec.FakeExec with CommandScript/CombinedOutputScript) is duplicated;
extract it into a shared test utility package (e.g., create
pkg/internal/testutil with a file exec.go) and move the function there so both
discovery.newFakeExecutor and the copy in pkg/internal/diskutil_test.go import
testutil.NewFakeExecutor (or similar); update callers to use the shared symbol
and keep the same signature (outputs ...string) and behavior using
testingexec.FakeExec/ FakeCmd so tests remain unchanged.
In `@test/e2e/localvolume_test.go`:
- Around line 297-323: Replace the manual map construction in
verifyLVDLFilesystemUUIDForPVs with a kubernetes sets.String for consistency
with eventuallyFindLVDLsForPVs: create pvNameSet using
sets.NewString(pvNames...) (from k8s.io/apimachinery/pkg/util/sets), change
membership checks from "if _, ok := pvNameSet[pvName]" to pvNameSet.Has(pvName),
and use pvNameSet.Len() when comparing counts; keep the rest of the function
logic the same (references: verifyLVDLFilesystemUUIDForPVs,
eventuallyFindLVDLsForPVs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f30f2bf2-6e52-4046-980d-082344ff54ad
⛔ Files ignored due to path filters (2)
vendor/k8s.io/utils/exec/testing/fake_exec.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (25)
HACKING.mdapi/v1/localvolumedevicelink_types.gocmd/diskmaker-manager/manager.goconfig/manifests/stable/local-storage-operator.clusterserviceversion.yamlconfig/manifests/stable/local.storage.openshift.io_localvolumedevicelinks.yamlpkg/common/provisioner_utils.gopkg/controllers/localvolume/localvolume_controller.gopkg/diskmaker/controllers/lv/create_pv_test.gopkg/diskmaker/controllers/lv/reconcile.gopkg/diskmaker/controllers/lv/reconcile_test.gopkg/diskmaker/controllers/lvset/create_pv_test.gopkg/diskmaker/controllers/lvset/device_age_test.gopkg/diskmaker/controllers/lvset/reconcile.gopkg/diskmaker/controllers/lvset/reconcile_test.gopkg/diskmaker/discovery/discovery_test.gopkg/diskmaker/event_reporter.gopkg/internal/device_link_handler.gopkg/internal/device_link_handler_test.gopkg/internal/disk_location.gopkg/internal/diskutil.gopkg/internal/diskutil_test.gopkg/localmetrics/custom_metrics_collector.gopkg/localmetrics/custom_metrics_collector_test.gotest/e2e/localvolume_test.gotest/e2e/localvolumeset_test.go
💤 Files with no reviewable changes (1)
- config/manifests/stable/local.storage.openshift.io_localvolumedevicelinks.yaml
✅ Files skipped from review due to trivial changes (6)
- pkg/diskmaker/controllers/lvset/device_age_test.go
- HACKING.md
- pkg/diskmaker/event_reporter.go
- config/manifests/stable/local-storage-operator.clusterserviceversion.yaml
- pkg/internal/disk_location.go
- pkg/localmetrics/custom_metrics_collector_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/diskmaker/controllers/lvset/reconcile_test.go
- pkg/controllers/localvolume/localvolume_controller.go
- test/e2e/localvolumeset_test.go
- pkg/diskmaker/controllers/lvset/create_pv_test.go
- cmd/diskmaker-manager/manager.go
- pkg/internal/diskutil.go
- pkg/internal/device_link_handler.go
gnufied
force-pushed
the
implement-metrics-for-mismatch-symlinks
branch
from
7b34b7c to
15b9838
Compare
gnufied
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@gnufied: This pull request references STOR-2873 which is a valid jira issue. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
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 `@assets/templates/localmetrics/prometheus-rule.yaml`:
- Line 49: Replace the misspelled phrase "Local Storage oprator" with "Local
Storage operator" in the Prometheus alert template where the alert summary
contains that text (the summary strings that include "Local Storage oprator" in
prometheus-rule.yaml); update both occurrences (the summary around pv {{
$labels.persistent_volume }} and the other summary occurrence) so both alert
summaries read "Local Storage operator".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 637cc55c-8687-410d-a7f8-27835d9c2338
📒 Files selected for processing (4)
assets/templates/localmetrics/prometheus-rule.yamlcmd/diskmaker-manager/manager.gopkg/localmetrics/custom_metrics_collector.gopkg/localmetrics/custom_metrics_collector_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/diskmaker-manager/manager.go
- pkg/localmetrics/custom_metrics_collector_test.go
gnufied
force-pushed
the
implement-metrics-for-mismatch-symlinks
branch
from
15b9838 to
c662ab9
Compare
Ship alertmanager rule for emitting alerts for LSO
gnufied
force-pushed
the
implement-metrics-for-mismatch-symlinks
branch
from
c662ab9 to
2ef63a1
Compare
Contributor
|
@gnufied: 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. |
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.
Fixes https://redhat.atlassian.net/browse/STOR-2873
https://redhat.atlassian.net/browse/STOR-2872