Skip to content

OCPBUGS-86506: Fix lso_lvset_provisioned_PV_count metric #629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tsmetana wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −12
Open

OCPBUGS-86506: Fix lso_lvset_provisioned_PV_count metric #629

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 10 additions & 12 deletions pkg/diskmaker/controllers/lvset/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,6 @@ func (r *LocalVolumeSetReconciler) Reconcile(ctx context.Context, request ctrl.R
}

// process valid devices
var totalProvisionedPVs int
var noMatch []string
for _, blockDevice := range validDevices {
existingSymlink, err := common.GetSymlinkedForCurrentSC(symLinkDir, blockDevice.KName)
if err != nil {
Expand All @@ -293,8 +291,6 @@ func (r *LocalVolumeSetReconciler) Reconcile(ctx context.Context, request ctrl.R
if err != nil {
return ctrl.Result{}, err
}
totalProvisionedPVs = result.provisionedCount
noMatch = result.noMatch
if result.fastRequeue {
requeueTime = fastRequeueTime
}
Expand All @@ -303,7 +299,13 @@ func (r *LocalVolumeSetReconciler) Reconcile(ctx context.Context, request ctrl.R
}
}

klog.InfoS("total devices provisioned", "storagecClass", storageClassName, "count", totalProvisionedPVs)
// count all provisioned symlinks (both existing and newly created)
totalProvisionedPVs, noMatch, err := getAlreadySymlinked(symLinkDir, blockDevices)
if err != nil {
klog.ErrorS(err, "error counting provisioned symlinks")
}

klog.InfoS("total devices provisioned", "storageClass", storageClassName, "count", totalProvisionedPVs)

// update metrics for total persistent volumes provisioned
localmetrics.SetLVSProvisionedPVMetric(nodeName, storageClassName, totalProvisionedPVs)
Comment on lines +303 to 311

@coderabbitai coderabbitai Bot Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle symlink-count failures before updating metrics.

On Line 303, getAlreadySymlinked errors are only logged, but Line 311 still publishes totalProvisionedPVs (which can be zero from the failure path). This can reintroduce false 0 values for lso_lvset_provisioned_PV_count.

Suggested fix 
  totalProvisionedPVs, noMatch, err := getAlreadySymlinked(symLinkDir, blockDevices)
  if err != nil {
-     klog.ErrorS(err, "error counting provisioned symlinks")
+     return ctrl.Result{}, fmt.Errorf("error counting provisioned symlinks: %w", err)
  }

  klog.InfoS("total devices provisioned", "storageClass", storageClassName, "count", totalProvisionedPVs)

  // update metrics for total persistent volumes provisioned
  localmetrics.SetLVSProvisionedPVMetric(nodeName, storageClassName, totalProvisionedPVs)

As per coding guidelines "Never ignore error returns".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
totalProvisionedPVs, noMatch, err := getAlreadySymlinked(symLinkDir, blockDevices)
if err != nil {
klog.ErrorS(err, "error counting provisioned symlinks")
}
klog.InfoS("total devices provisioned", "storageClass", storageClassName, "count", totalProvisionedPVs)
// update metrics for total persistent volumes provisioned
localmetrics.SetLVSProvisionedPVMetric(nodeName, storageClassName, totalProvisionedPVs)
totalProvisionedPVs, noMatch, err := getAlreadySymlinked(symLinkDir, blockDevices)
if err != nil {
return ctrl.Result{}, fmt.Errorf("error counting provisioned symlinks: %w", err)
}
klog.InfoS("total devices provisioned", "storageClass", storageClassName, "count", totalProvisionedPVs)
// update metrics for total persistent volumes provisioned
localmetrics.SetLVSProvisionedPVMetric(nodeName, storageClassName, totalProvisionedPVs)
🤖 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 `@pkg/diskmaker/controllers/lvset/reconcile.go` around lines 303 - 311, The
call to getAlreadySymlinked can fail and currently we log the error but still
publish totalProvisionedPVs (which may be an invalid zero); change the flow so
that after calling getAlreadySymlinked you check err and, if non-nil, log the
error and skip updating the metric (do not call
localmetrics.SetLVSProvisionedPVMetric) or return/propagate the error
instead—update the block around getAlreadySymlinked/totalProvisionedPVs to only
call localmetrics.SetLVSProvisionedPVMetric(nodeName, storageClassName,
totalProvisionedPVs) when err == nil.

@tsmetana tsmetana Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the the getAlreadySymlinked function returns an error, the value of the totalProvisionedPVs cannot be trusted anyway, just as the metric itself. In fact, setting it to zero would make any problem more visible and increase chance somebody would take a look into the problem.

@coderabbitai coderabbitai Bot Jun 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsmetana That's a fair point — a metric that drops to 0 is arguably a better observable signal than one that silently retains a stale value, and it's consistent with an "alert on anomaly" philosophy. The error is still surfaced via klog.ErrorS, so it isn't truly ignored. I accept the reasoning and withdraw the suggestion.

Expand Down Expand Up @@ -550,10 +552,8 @@ PathLoop:
}

type processNewSymlinkResult struct {
provisionedCount int
noMatch []string
fastRequeue bool
maxCountReached bool
fastRequeue bool
maxCountReached bool
}

func (r *LocalVolumeSetReconciler) processNewSymlink(
Expand All @@ -579,9 +579,7 @@ func (r *LocalVolumeSetReconciler) processNewSymlink(
}

// validate MaxDeviceCount
alreadyProvisionedCount, noMatch, err := getAlreadySymlinked(symLinkDir, blockDevices)
result.provisionedCount = alreadyProvisionedCount
result.noMatch = noMatch
alreadyProvisionedCount, _, err := getAlreadySymlinked(symLinkDir, blockDevices)

if err != nil && lvset.Spec.MaxDeviceCount != nil {
r.eventReporter.Report(lvset, newDiskEvent(ErrorListingExistingSymlinks, "error determining already provisioned disks", "", corev1.EventTypeWarning))
Expand Down