Fix virtual machine resource tracking by lllamnyp · Pull Request #904 · cozystack/cozystack

Timofei Larkin (lllamnyp) · 2025-04-29T12:30:28Z

Count Workload resources for pods by requests, not limits
Do not count init container requests
Prefix Workloads for pods with pod-, just like the other types to prevent possible name collisions (closes Create Workloads for pods with a "pod-" prefix #787)

The previous version of the WorkloadMonitor controller incorrectly summed resource limits on pods, rather than requests. This prevented it from tracking the resource allocation for pods, which only had requests specified, which is particularly the case for kubevirt's virtual machine pods. Additionally, it counted the limits for all containers, including init containers, which are short-lived and do not contribute much to the total resource usage.

Summary by CodeRabbit

Bug Fixes
- Improved handling of workloads with unrecognized prefixes by ensuring they are properly deleted and not processed further.
- Corrected resource aggregation for Pods to sum container resource requests instead of limits, and now only includes normal containers.
New Features
- Added support for monitoring workloads with names prefixed by "pod-".
Tests
- Introduced unit tests to verify correct handling of workload name prefixes and monitored object creation.

* Count Workload resources for pods by requests, not limits * Do not count init container requests * Prefix Workloads for pods with `pod-`, just like the other types to prevent possible name collisions (closes #787) The previous version of the WorkloadMonitor controller incorrectly summed resource limits on pods, rather than requests. This prevented it from tracking the resource allocation for pods, which only had requests specified, which is particularly the case for kubevirt's virtual machine pods. Additionally, it counted the limits for all containers, including init containers, which are short-lived and do not contribute much to the total resource usage. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

coderabbitai · 2025-04-29T12:30:35Z

Walkthrough

This update modifies the logic for managing Workloads associated with Kubernetes Pods, Services, and PersistentVolumeClaims. The getMonitoredObject function now uses a switch statement to recognize "pod-", "svc-", and "pvc-" prefixes in Workload names, returning the appropriate Kubernetes object or nil if unrecognized. The Reconcile method deletes Workloads when no valid monitored object is found. The WorkloadMonitor controller now creates Workloads for Pods with a "pod-" prefix and sums container resource requests (excluding init containers) instead of limits. New unit tests verify the behavior of the getMonitoredObject function.

Changes

File(s)	Change Summary
internal/controller/workload_controller.go	Refactored `getMonitoredObject` to use a switch for prefixes; added logic to delete Workloads if monitored object is nil; now returns nil for unknown prefixes.
internal/controller/workload_controller_test.go	Added unit tests for `getMonitoredObject` to verify handling of prefixed and unprefixed Workload names.
internal/controller/workloadmonitor_controller.go	Changed Workload creation for Pods to use "pod-" prefix; aggregates resource requests (not limits), excludes init containers.

Sequence Diagram(s)

sequenceDiagram
    participant WorkloadMonitorController
    participant KubernetesAPI
    participant WorkloadController

    WorkloadMonitorController->>KubernetesAPI: Get Pod/Service/PVC
    WorkloadMonitorController->>KubernetesAPI: Create Workload with prefix ("pod-", "svc-", "pvc-")
    WorkloadController->>KubernetesAPI: Get Workload
    WorkloadController->>KubernetesAPI: Get Monitored Object (by prefix)
    alt Monitored Object is nil
        WorkloadController->>KubernetesAPI: Delete Workload
    else
        WorkloadController->>KubernetesAPI: Continue processing
    end

Assessment against linked issues

Objective	Addressed	Explanation
Prefix Workload names for pods with "pod-" to avoid collisions (#787)	✅

Possibly related PRs

Delete a Workload if the related object is absent #779: Implements initial deletion logic for Workloads when related objects are absent and sets up the controller, which is directly extended by this PR.

Suggested reviewers

kvaps

Poem

In the warren where prefixes grow,
"pod-", "svc-", "pvc-" now in tow!
Workloads hop with names anew,
No collisions in this bunny crew.
With tests and checks, we leap ahead—
🐇 Code is tidy, bugs are shed!
Hooray for prefixes—hip hip hooray!

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 108fc64 and ec9e26c.

📒 Files selected for processing (3)

internal/controller/workload_controller.go (2 hunks)
internal/controller/workload_controller_test.go (1 hunks)
internal/controller/workloadmonitor_controller.go (2 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

internal/controller/workload_controller_test.go (1)

api/v1alpha1/workload_types.go (1)

Workload (52-57)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Build and Test

🔇 Additional comments (7)

internal/controller/workload_controller.go (2)

43-49: Great addition for handling unrecognized workload types

This change properly handles the case when no monitored object is found for a workload by deleting it, preventing further processing of invalid workloads. This aligns with the new prefix-based identification system.

80-98: Well-structured refactoring of object type detection

The switch statement approach is more maintainable than the previous if-statements. The addition of the "pod-" prefix case aligns with the naming convention changes in the workloadmonitor controller, and returning nil for unrecognized types enables the cleanup logic in the Reconcile method.

internal/controller/workloadmonitor_controller.go (3)

215-216: Updated comment correctly reflects the implementation

The comment now correctly indicates that the totalResources map stores resource requests rather than limits, improving code clarity.

219-228: Key fix: Using resource requests instead of limits and excluding init containers

This change addresses the core issue by switching from resource limits to resource requests and excluding init containers from the total. This is the correct approach since:

Many pods specify only resource requests, not limits

Init containers are short-lived and don't significantly contribute to overall resource usage

For virtual machine pods managed by kubevirt, resource requests better represent actual allocation

This fix will provide more accurate resource tracking, especially for virtual machine pods.

249-250: Improved workload naming convention

Adding the "pod-" prefix to Workload names for pods aligns with the naming convention used for other resource types (like "pvc-" and "svc-"), providing consistency and preventing potential name collisions.

internal/controller/workload_controller_test.go (2)

10-17: Good test for unrecognized workload prefixes

This test verifies that workloads with unrecognized name prefixes return nil from getMonitoredObject, which will trigger the deletion logic in the Reconcile method. This helps ensure invalid workloads are properly cleaned up.

19-26: Appropriate test for pod-prefixed workloads

This test correctly verifies that the "pod-" prefix is recognized and returns a Pod object with the expected name. The type assertion and name check are appropriately thorough.

✨ Finishing Touches

📝 Generate Docstrings

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

Andrei Kvapil (kvaps)

`lgtm

* Count Workload resources for pods by requests, not limits * Do not count init container requests * Prefix Workloads for pods with `pod-`, just like the other types to prevent possible name collisions (closes #787) The previous version of the WorkloadMonitor controller incorrectly summed resource limits on pods, rather than requests. This prevented it from tracking the resource allocation for pods, which only had requests specified, which is particularly the case for kubevirt's virtual machine pods. Additionally, it counted the limits for all containers, including init containers, which are short-lived and do not contribute much to the total resource usage.  ## Summary by CodeRabbit - **Bug Fixes** - Improved handling of workloads with unrecognized prefixes by ensuring they are properly deleted and not processed further. - Corrected resource aggregation for Pods to sum container resource requests instead of limits, and now only includes normal containers. - **New Features** - Added support for monitoring workloads with names prefixed by "pod-". - **Tests** - Introduced unit tests to verify correct handling of workload name prefixes and monitored object creation.  (cherry picked from commit 1e59e5f) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

* Count Workload resources for pods by requests, not limits * Do not count init container requests * Prefix Workloads for pods with `pod-`, just like the other types to prevent possible name collisions (closes #787) The previous version of the WorkloadMonitor controller incorrectly summed resource limits on pods, rather than requests. This prevented it from tracking the resource allocation for pods, which only had requests specified, which is particularly the case for kubevirt's virtual machine pods. Additionally, it counted the limits for all containers, including init containers, which are short-lived and do not contribute much to the total resource usage. (cherry picked from commit 1e59e5f) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

* Count Workload resources for pods by requests, not limits * Do not count init container requests * Prefix Workloads for pods with `pod-`, just like the other types to prevent possible name collisions (closes #787) The previous version of the WorkloadMonitor controller incorrectly summed resource limits on pods, rather than requests. This prevented it from tracking the resource allocation for pods, which only had requests specified, which is particularly the case for kubevirt's virtual machine pods. Additionally, it counted the limits for all containers, including init containers, which are short-lived and do not contribute much to the total resource usage.  ## Summary by CodeRabbit - **Bug Fixes** - Improved handling of workloads with unrecognized prefixes by ensuring they are properly deleted and not processed further. - Corrected resource aggregation for Pods to sum container resource requests instead of limits, and now only includes normal containers. - **New Features** - Added support for monitoring workloads with names prefixed by "pod-". - **Tests** - Introduced unit tests to verify correct handling of workload name prefixes and monitored object creation.  (cherry picked from commit 1e59e5f) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

Timofei Larkin (lllamnyp) requested review from klinch0 and Andrei Kvapil (kvaps) as code owners April 29, 2025 12:30

Timofei Larkin (lllamnyp) assigned Andrei Kvapil (kvaps) and klinch0 Apr 29, 2025

Andrei Kvapil (kvaps) approved these changes Apr 29, 2025

View reviewed changes

klinch0 approved these changes Apr 29, 2025

View reviewed changes

Timofei Larkin (lllamnyp) merged commit 1e59e5f into main Apr 30, 2025

Timofei Larkin (lllamnyp) deleted the fix/track-vms-consumption branch April 30, 2025 06:52

Timofei Larkin (lllamnyp) added the backport Should change be backported on previous release label May 5, 2025

Timofei Larkin (lllamnyp) mentioned this pull request May 20, 2025

Prepare 0.31.0 rc.2 #966

Closed

coderabbitai Bot mentioned this pull request Jul 21, 2025

[cozystack-controller] Fix deleting workloads #1229

Merged

Navigation Menu

Uh oh!

Fix virtual machine resource tracking - #904

Fix virtual machine resource tracking#904
Timofei Larkin (lllamnyp) merged 1 commit into
mainfrom
fix/track-vms-consumption

Timofei Larkin (lllamnyp) commented Apr 29, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 29, 2025 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

Andrei Kvapil (kvaps) left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Timofei Larkin (lllamnyp) commented Apr 29, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Assessment against linked issues

Possibly related PRs

Suggested reviewers

Poem

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

Andrei Kvapil (kvaps) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Timofei Larkin (lllamnyp) commented Apr 29, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 29, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)