[DO NOT MERGE] UPSTREAM: <carry>: fix kube_persistentvolume_plugin_type_counts#2673
[DO NOT MERGE] UPSTREAM: <carry>: fix kube_persistentvolume_plugin_type_counts#2673dfajmon wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@dfajmon: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dfajmon 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 |
WalkthroughPersistent volume controller now probes metrics-capable volume plugins and wires them into a dedicated metrics plugin manager; the kube-controller-manager links the local volume plugin, metrics are registered against the metrics plugin manager, and a test verifies plugin-name resolution. ChangesMetrics Plugin Manager Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@cmd/kube-controller-manager/app/core.go`:
- Around line 334-336: The error return in newPersistentVolumeBinderController
incorrectly returns three values; change the error path in the metrics volume
plugin probe to return only (nil, error) by removing the extraneous boolean and
wrap the original probe error using fmt.Errorf("failed to probe metrics volume
plugins when starting persistentvolume controller: %w", err) so the function
signature (Controller, error) is respected and the underlying error 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 55e2cc95-d7d6-404c-bb43-74e4598e3128
📒 Files selected for processing (5)
cmd/kube-controller-manager/app/core.gocmd/kube-controller-manager/app/plugins.gopkg/controller/volume/persistentvolume/metrics/metrics_test.gopkg/controller/volume/persistentvolume/pv_controller.gopkg/controller/volume/persistentvolume/pv_controller_base.go
|
@dfajmon: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
dfajmon
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
…ng plugin_name Commit 9e29f95 refactored KCM volume plugins, replacing ProbeControllerVolumePlugins (which included CSI) with ProbeProvisionableRecyclableVolumePlugins (filtered to Provisionable/Deletable/Recyclable). The CSI plugin implements none of those interfaces, so it was excluded from volumePluginMgr. metrics.Register was still using &ctrl.volumePluginMgr, so getPVPluginName() -> FindPluginBySpec() failed for CSI PVs, falling back to "N/A" instead of e.g. "kubernetes.io/csi:ebs.csi.aws.com". Fix: initialize a separate metricsPluginMgr with all plugins (ProbePersistentVolumePlugins, no filter) and pass it to metrics.Register. Fixes: OCPBUGS-44815 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@dfajmon: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/controller/volume/persistentvolume/pv_controller_base.go`:
- Around line 103-105: If P.MetricsVolumePlugins is nil/empty, fall back to
P.VolumePlugins before calling controller.metricsPluginMgr.InitPlugins so
existing callers that omit metrics plugins keep previous behavior; update the
call site that currently passes P.MetricsVolumePlugins to use a local variable
(e.g., metricsPlugins := P.MetricsVolumePlugins; if len(metricsPlugins) == 0 {
metricsPlugins = P.VolumePlugins }) and then call
controller.metricsPluginMgr.InitPlugins(metricsPlugins, nil, controller) to
preserve backward compatibility (symbols to locate: P.MetricsVolumePlugins,
P.VolumePlugins, controller.metricsPluginMgr.InitPlugins in
pv_controller_base.go).
🪄 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: 3a74ad02-73b3-4c37-9ded-7269080991d0
📒 Files selected for processing (5)
cmd/kube-controller-manager/app/core.gocmd/kube-controller-manager/app/plugins.gopkg/controller/volume/persistentvolume/metrics/metrics_test.gopkg/controller/volume/persistentvolume/pv_controller.gopkg/controller/volume/persistentvolume/pv_controller_base.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/kube-controller-manager/app/plugins.go
- pkg/controller/volume/persistentvolume/metrics/metrics_test.go
- pkg/controller/volume/persistentvolume/pv_controller.go
- cmd/kube-controller-manager/app/core.go
| if err := controller.metricsPluginMgr.InitPlugins(p.MetricsVolumePlugins, nil /* prober */, controller); err != nil { | ||
| return nil, fmt.Errorf("could not initialize metrics volume plugins for PersistentVolume Controller: %w", err) | ||
| } |
There was a problem hiding this comment.
Add a backward-compatible fallback for metrics plugins.
If MetricsVolumePlugins is omitted by existing constructor callers, metricsPluginMgr initializes empty and plugin labeling can silently regress to fallback values. Default to VolumePlugins when metrics plugins are not provided.
Suggested fix
- if err := controller.metricsPluginMgr.InitPlugins(p.MetricsVolumePlugins, nil /* prober */, controller); err != nil {
+ metricsPlugins := p.MetricsVolumePlugins
+ if len(metricsPlugins) == 0 {
+ metricsPlugins = p.VolumePlugins
+ }
+ if err := controller.metricsPluginMgr.InitPlugins(metricsPlugins, nil /* prober */, controller); err != nil {
return nil, fmt.Errorf("could not initialize metrics volume plugins for PersistentVolume Controller: %w", err)
}📝 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.
| if err := controller.metricsPluginMgr.InitPlugins(p.MetricsVolumePlugins, nil /* prober */, controller); err != nil { | |
| return nil, fmt.Errorf("could not initialize metrics volume plugins for PersistentVolume Controller: %w", err) | |
| } | |
| metricsPlugins := p.MetricsVolumePlugins | |
| if len(metricsPlugins) == 0 { | |
| metricsPlugins = p.VolumePlugins | |
| } | |
| if err := controller.metricsPluginMgr.InitPlugins(metricsPlugins, nil /* prober */, controller); err != nil { | |
| return nil, fmt.Errorf("could not initialize metrics volume plugins for PersistentVolume Controller: %w", err) | |
| } |
🤖 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/controller/volume/persistentvolume/pv_controller_base.go` around lines
103 - 105, If P.MetricsVolumePlugins is nil/empty, fall back to P.VolumePlugins
before calling controller.metricsPluginMgr.InitPlugins so existing callers that
omit metrics plugins keep previous behavior; update the call site that currently
passes P.MetricsVolumePlugins to use a local variable (e.g., metricsPlugins :=
P.MetricsVolumePlugins; if len(metricsPlugins) == 0 { metricsPlugins =
P.VolumePlugins }) and then call
controller.metricsPluginMgr.InitPlugins(metricsPlugins, nil, controller) to
preserve backward compatibility (symbols to locate: P.MetricsVolumePlugins,
P.VolumePlugins, controller.metricsPluginMgr.InitPlugins in
pv_controller_base.go).
|
@dfajmon: 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. |
2 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Summary by CodeRabbit