Skip to content

Do not treat empty PVC fetch results as errors - #303

Open
plkokanov wants to merge 3 commits into
gardener:masterfrom
plkokanov:fix/empty-pvc-fetch-results-no-error
Open

Do not treat empty PVC fetch results as errors#303
plkokanov wants to merge 3 commits into
gardener:masterfrom
plkokanov:fix/empty-pvc-fetch-results-no-error

Conversation

@plkokanov

Copy link
Copy Markdown
Contributor

How to categorize this PR?

/area ops-productivity
/kind cleanup

What this PR does / why we need it:
Previously, internal/target/pvcfetcher.Fetch funciton returned errors when no pods matched a selector or when matching pods had no PVC volumes. This caused the runner to set the ReasonPVCFetchError condition for situations that are not errors, e.g. when a workload is scaled down to 0.

With this PR the internal/target/pvcfetcher.Fetch no longer returns errors in such cases, and just an empty/nil list of PVCs
In such cases the runner will just print a debug log and set the RecommendationAvailable and Resizing conditions appropriately, to indicate that no PVCs were matched.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

The `internal/target/pvcfetcher.Fetch` no longer returns errors when no pods matched a selector or when matching pods had no PVC volumes. Such cases are now logged at debug level and represented in the `RecommendationAvailable` and `Resizing` conditions.

@plkokanov
plkokanov requested a review from a team as a code owner July 23, 2026 09:49
@gardener-prow gardener-prow Bot added area/ops-productivity Operator productivity related (how to improve operations) kind/cleanup Something that is not needed anymore and can be cleaned up size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Jul 23, 2026

@RadaBDimitrova RadaBDimitrova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR, I have just a wording suggestion.

Comment thread internal/periodic/periodic.go Outdated
Comment thread internal/periodic/periodic_test.go
Comment thread internal/periodic/periodic.go Outdated
Comment thread internal/periodic/periodic.go Outdated
Comment thread internal/periodic/periodic.go Outdated

@RadaBDimitrova RadaBDimitrova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/approve

@gardener-prow

gardener-prow Bot commented Aug 3, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RadaBDimitrova

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2026

@Kostov6 Kostov6 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor nit, otherwise lgtm

logger.V(2).Info("no persistentvolumeclaims found for persistentvolumeclaimautoscaler", "pvca", pvcaKey, "reason", err.Error())

reason = ReasonNoPVCsMatched
message = fmt.Sprintf("Failed to fetch PersistentVolumeClaims: %s", err.Error())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this meaningfully different from L283?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ops-productivity Operator productivity related (how to improve operations) cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/cleanup Something that is not needed anymore and can be cleaned up size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants