From 34991d2cdb2dd1bd26a500350491fe408aedf7ae Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Wed, 30 Apr 2025 10:52:17 +0400 Subject: [PATCH] Fix virtual machine resource tracking (#904) * 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 1e59e5fbb6e009dddec8274c82e185ba2442d338) Signed-off-by: Timofei Larkin --- internal/controller/workload_controller.go | 24 ++++++++++++----- .../controller/workload_controller_test.go | 26 +++++++++++++++++++ .../controller/workloadmonitor_controller.go | 13 ++++------ 3 files changed, 49 insertions(+), 14 deletions(-) create mode 100644 internal/controller/workload_controller_test.go diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index 7b7f8406..3624e0e1 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -39,6 +39,15 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } t := getMonitoredObject(w) + + if t == nil { + err = r.Delete(ctx, w) + if err != nil { + logger.Error(err, "failed to delete workload") + } + return ctrl.Result{}, err + } + err = r.Get(ctx, types.NamespacedName{Name: t.GetName(), Namespace: t.GetNamespace()}, t) // found object, nothing to do @@ -68,20 +77,23 @@ func (r *WorkloadReconciler) SetupWithManager(mgr ctrl.Manager) error { } func getMonitoredObject(w *cozyv1alpha1.Workload) client.Object { - if strings.HasPrefix(w.Name, "pvc-") { + switch { + case strings.HasPrefix(w.Name, "pvc-"): obj := &corev1.PersistentVolumeClaim{} obj.Name = strings.TrimPrefix(w.Name, "pvc-") obj.Namespace = w.Namespace return obj - } - if strings.HasPrefix(w.Name, "svc-") { + case strings.HasPrefix(w.Name, "svc-"): obj := &corev1.Service{} obj.Name = strings.TrimPrefix(w.Name, "svc-") obj.Namespace = w.Namespace return obj + case strings.HasPrefix(w.Name, "pod-"): + obj := &corev1.Pod{} + obj.Name = strings.TrimPrefix(w.Name, "pod-") + obj.Namespace = w.Namespace + return obj } - obj := &corev1.Pod{} - obj.Name = w.Name - obj.Namespace = w.Namespace + var obj client.Object return obj } diff --git a/internal/controller/workload_controller_test.go b/internal/controller/workload_controller_test.go new file mode 100644 index 00000000..816c135e --- /dev/null +++ b/internal/controller/workload_controller_test.go @@ -0,0 +1,26 @@ +package controller + +import ( + "testing" + + cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1" + corev1 "k8s.io/api/core/v1" +) + +func TestUnprefixedMonitoredObjectReturnsNil(t *testing.T) { + w := &cozyv1alpha1.Workload{} + w.Name = "unprefixed-name" + obj := getMonitoredObject(w) + if obj != nil { + t.Errorf(`getMonitoredObject(&Workload{Name: "%s"}) == %v, want nil`, w.Name, obj) + } +} + +func TestPodMonitoredObject(t *testing.T) { + w := &cozyv1alpha1.Workload{} + w.Name = "pod-mypod" + obj := getMonitoredObject(w) + if pod, ok := obj.(*corev1.Pod); !ok || pod.Name != "mypod" { + t.Errorf(`getMonitoredObject(&Workload{Name: "%s"}) == %v, want &Pod{Name: "mypod"}`, w.Name, obj) + } +} diff --git a/internal/controller/workloadmonitor_controller.go b/internal/controller/workloadmonitor_controller.go index 1c23a749..a256d757 100644 --- a/internal/controller/workloadmonitor_controller.go +++ b/internal/controller/workloadmonitor_controller.go @@ -198,15 +198,12 @@ func (r *WorkloadMonitorReconciler) reconcilePodForMonitor( ) error { logger := log.FromContext(ctx) - // Combine both init containers and normal containers to sum resources properly - combinedContainers := append(pod.Spec.InitContainers, pod.Spec.Containers...) - - // totalResources will store the sum of all container resource limits + // totalResources will store the sum of all container resource requests totalResources := make(map[string]resource.Quantity) - // Iterate over all containers to aggregate their Limits - for _, container := range combinedContainers { - for name, qty := range container.Resources.Limits { + // Iterate over all containers to aggregate their requests + for _, container := range pod.Spec.Containers { + for name, qty := range container.Resources.Requests { if existing, exists := totalResources[name.String()]; exists { existing.Add(qty) totalResources[name.String()] = existing @@ -235,7 +232,7 @@ func (r *WorkloadMonitorReconciler) reconcilePodForMonitor( workload := &cozyv1alpha1.Workload{ ObjectMeta: metav1.ObjectMeta{ - Name: pod.Name, + Name: fmt.Sprintf("pod-%s", pod.Name), Namespace: pod.Namespace, }, }