From 7960dd17e28c489ae9f6ccb279128ae76694745a Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Sat, 31 Aug 2024 17:34:54 +0300 Subject: [PATCH 1/5] Refactor logging statements in admission controller Signed-off-by: Omer Aplatony --- .../pkg/admission-controller/certs.go | 9 ++++----- .../pkg/admission-controller/main.go | 2 +- .../pkg/admission-controller/resource/pod/handler.go | 4 ++-- .../pod/recommendation/recommendation_provider.go | 12 ++++++------ .../pkg/admission-controller/resource/vpa/handler.go | 2 +- .../pkg/admission-controller/resource/vpa/matcher.go | 6 +++--- 6 files changed, 17 insertions(+), 18 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/certs.go b/vertical-pod-autoscaler/pkg/admission-controller/certs.go index f0fe41dfbb9c..6d8d06447691 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/certs.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/certs.go @@ -34,11 +34,10 @@ type certsConfig struct { func readFile(filePath string) []byte { res, err := os.ReadFile(filePath) if err != nil { - klog.Errorf("Error reading certificate file at %s: %v", filePath, err) + klog.ErrorS(err, "Error reading certificate file", "file", filePath) return nil } - - klog.V(3).Infof("Successfully read %d bytes from %v", len(res), filePath) + klog.V(3).InfoS("Successfully read bytes fron file", "bytes", len(res), "file", filePath) return res } @@ -67,9 +66,9 @@ func (cr *certReloader) start(stop <-chan struct{}) error { select { case event := <-watcher.Events: if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) { - klog.V(2).Info("New certificate found, reloading") + klog.V(2).InfoS("New certificate found, reloading") if err := cr.load(); err != nil { - klog.Errorf("Failed to reload certificate: %s", err) + klog.ErrorS(err, "Failed to reload certificate") } } case err := <-watcher.Errors: diff --git a/vertical-pod-autoscaler/pkg/admission-controller/main.go b/vertical-pod-autoscaler/pkg/admission-controller/main.go index 118bf57dc7a2..113621619f4e 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/main.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/main.go @@ -108,7 +108,7 @@ func main() { var limitRangeCalculator limitrange.LimitRangeCalculator limitRangeCalculator, err := limitrange.NewLimitsRangeCalculator(factory) if err != nil { - klog.Errorf("Failed to create limitRangeCalculator, falling back to not checking limits. Error message: %s", err) + klog.ErrorS(err, "Failed to create limitRangeCalculator, falling back to not checking limits.") limitRangeCalculator = limitrange.NewNoopLimitsCalculator() } recommendationProvider := recommendation.NewProvider(limitRangeCalculator, vpa_api_util.NewCappingRecommendationProcessor(limitRangeCalculator)) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go index 20d7549e5c34..44c0d4237a2f 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go @@ -76,10 +76,10 @@ func (h *resourceHandler) GetPatches(ar *admissionv1.AdmissionRequest) ([]resour pod.Name = pod.GenerateName + "%" pod.Namespace = namespace } - klog.V(4).Infof("Admitting pod %s", klog.KObj(&pod)) + klog.V(4).InfoS("Admitting pod", "pod", klog.KObj(&pod)) controllingVpa := h.vpaMatcher.GetMatchingVPA(&pod) if controllingVpa == nil { - klog.V(4).Infof("No matching VPA found for pod %s", klog.KObj(&pod)) + klog.V(4).InfoS("No matching VPA found for pod", "pod", klog.KObj(&pod)) return []resource_admission.PatchRecord{}, nil } pod, err := h.preProcessor.Process(pod) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go index 3272058e3e5d..9a9fbd54f034 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go @@ -55,10 +55,10 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou recommendation := vpa_api_util.GetRecommendationForContainer(container.Name, &podRecommendation) if recommendation == nil { if !addAll { - klog.V(2).Infof("no matching recommendation found for container %s, skipping", container.Name) + klog.V(2).InfoS("No recommendation found for container, skipping", "container", container.Name) continue } - klog.V(2).Infof("no matching recommendation found for container %s, using Pod request", container.Name) + klog.V(2).InfoS("No matching found for container, using Pod request", "container", container.Name) resources[i].Requests = container.Resources.Requests } else { resources[i].Requests = recommendation.Target @@ -85,10 +85,10 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou // The returned slice corresponds 1-1 to containers in the Pod. func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]vpa_api_util.ContainerResources, vpa_api_util.ContainerToAnnotationsMap, error) { if vpa == nil || pod == nil { - klog.V(2).Infof("can't calculate recommendations, one of vpa(%+v), pod(%+v) is nil", vpa, pod) + klog.V(2).InfoS("Can't calculate recommendations, one of VPA or Pod is nil", "vpa", vpa, "pod", pod) return nil, nil, nil } - klog.V(2).Infof("updating requirements for pod %s.", pod.Name) + klog.V(2).InfoS("Updating requirements for pod", "pod", pod.Name) var annotations vpa_api_util.ContainerToAnnotationsMap recommendedPodResources := &vpa_types.RecommendedPodResources{} @@ -97,7 +97,7 @@ func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod, vpa var err error recommendedPodResources, annotations, err = p.recommendationProcessor.Apply(vpa.Status.Recommendation, vpa.Spec.ResourcePolicy, vpa.Status.Conditions, pod) if err != nil { - klog.V(2).Infof("cannot process recommendation for pod %s", klog.KObj(pod)) + klog.V(2).InfoS("Cannot process recommendation for pod", "pod", klog.KObj(pod)) return nil, annotations, err } } @@ -114,7 +114,7 @@ func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod, vpa // Ensure that we are not propagating empty resource key if any. for _, resource := range containerResources { if resource.RemoveEmptyResourceKeyIfAny() { - klog.Infof("An empty resource key was found and purged for pod=%s with vpa=%s", klog.KObj(pod), klog.KObj(vpa)) + klog.InfoS("An empty resource key was found and purged", "pod", klog.KObj(pod), "vpa", klog.KObj(vpa)) } } diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go index 59927edbbe53..490865d57e85 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go @@ -88,7 +88,7 @@ func (h *resourceHandler) GetPatches(ar *v1.AdmissionRequest) ([]resource.PatchR return nil, err } - klog.V(4).Infof("Processing vpa: %v", vpa) + klog.V(4).InfoS("Processing vpa", "vpa", vpa) patches := []resource.PatchRecord{} if vpa.Spec.UpdatePolicy == nil { // Sets the default updatePolicy. diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go index cee7c97859ff..15897044b83f 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go @@ -52,7 +52,7 @@ func NewMatcher(vpaLister vpa_lister.VerticalPodAutoscalerLister, func (m *matcher) GetMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler { configs, err := m.vpaLister.VerticalPodAutoscalers(pod.Namespace).List(labels.Everything()) if err != nil { - klog.Errorf("failed to get vpa configs: %v", err) + klog.ErrorS(err, "Failed to get vpa configs") return nil } onConfigs := make([]*vpa_api_util.VpaWithSelector, 0) @@ -62,7 +62,7 @@ func (m *matcher) GetMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler } selector, err := m.selectorFetcher.Fetch(vpaConfig) if err != nil { - klog.V(3).Infof("skipping VPA object %s because we cannot fetch selector: %s", klog.KObj(vpaConfig), err) + klog.V(3).InfoS("Skipping VPA object because we cannot fetch selector", "vpa", klog.KObj(vpaConfig), "error", err) continue } onConfigs = append(onConfigs, &vpa_api_util.VpaWithSelector{ @@ -70,7 +70,7 @@ func (m *matcher) GetMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler Selector: selector, }) } - klog.V(2).Infof("Let's choose from %d configs for pod %s", len(onConfigs), klog.KObj(pod)) + klog.V(2).InfoS("Let's choose from", "configs", len(onConfigs), "pod", klog.KObj(pod)) result := vpa_api_util.GetControllingVPAForPod(pod, onConfigs, m.controllerFetcher) if result != nil { return result.Vpa From 4ce3f865cf0a0e461ee138acf514de4b8e4ac0c7 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Thu, 5 Sep 2024 05:51:42 +0300 Subject: [PATCH 2/5] revert GetControllingVPAForPod call --- .../pkg/admission-controller/resource/vpa/matcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go index 8b432f59f955..05ceff3026f7 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/matcher.go @@ -73,7 +73,7 @@ func (m *matcher) GetMatchingVPA(ctx context.Context, pod *core.Pod) *vpa_types. }) } klog.V(2).InfoS("Let's choose from", "configs", len(onConfigs), "pod", klog.KObj(pod)) - result := vpa_api_util.GetControllingVPAForPod(pod, onConfigs, m.controllerFetcher) + result := vpa_api_util.GetControllingVPAForPod(ctx, pod, onConfigs, m.controllerFetcher) if result != nil { return result.Vpa } From 07a13c95ef725a367a7ac04c94ea92c91029a6d4 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Thu, 5 Sep 2024 05:57:22 +0300 Subject: [PATCH 3/5] revert calling GetMatchingVPA --- .../pkg/admission-controller/resource/pod/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go index 49184c19b31c..7d8e704f7435 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/handler.go @@ -78,7 +78,7 @@ func (h *resourceHandler) GetPatches(ctx context.Context, ar *admissionv1.Admiss pod.Namespace = namespace } klog.V(4).InfoS("Admitting pod", "pod", klog.KObj(&pod)) - controllingVpa := h.vpaMatcher.GetMatchingVPA(&pod) + controllingVpa := h.vpaMatcher.GetMatchingVPA(ctx, &pod) if controllingVpa == nil { klog.V(4).InfoS("No matching VPA found for pod", "pod", klog.KObj(&pod)) return []resource_admission.PatchRecord{}, nil From ad259f6242f7a8f33aeecb94811561d34c8f273a Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Tue, 17 Sep 2024 09:07:18 +0300 Subject: [PATCH 4/5] Fixed typo Co-authored-by: Adrian Moisey --- vertical-pod-autoscaler/pkg/admission-controller/certs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/certs.go b/vertical-pod-autoscaler/pkg/admission-controller/certs.go index 6d8d06447691..17c906c8711c 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/certs.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/certs.go @@ -37,7 +37,7 @@ func readFile(filePath string) []byte { klog.ErrorS(err, "Error reading certificate file", "file", filePath) return nil } - klog.V(3).InfoS("Successfully read bytes fron file", "bytes", len(res), "file", filePath) + klog.V(3).InfoS("Successfully read bytes from file", "bytes", len(res), "file", filePath) return res } From 77b435badbb29aa2b636489de2790fc0d2c5316b Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Tue, 17 Sep 2024 09:07:42 +0300 Subject: [PATCH 5/5] Better statement Co-authored-by: Adrian Moisey --- .../resource/pod/recommendation/recommendation_provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go index 3e8a2f914cc2..4927274d8446 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider.go @@ -59,7 +59,7 @@ func GetContainersResources(pod *core.Pod, vpaResourcePolicy *vpa_types.PodResou klog.V(2).InfoS("No recommendation found for container, skipping", "container", container.Name) continue } - klog.V(2).InfoS("No matching found for container, using Pod request", "container", container.Name) + klog.V(2).InfoS("No match found for container, using Pod request", "container", container.Name) resources[i].Requests = container.Resources.Requests } else { resources[i].Requests = recommendation.Target