[ci-operator]: expand the pod lifecycle metrics to include the state of the machinesets#4938
[ci-operator]: expand the pod lifecycle metrics to include the state of the machinesets#4938droslean wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
/hold |
WalkthroughAdds MachineAutoscaler awareness to metrics: dependency updates, scheme registration, listing autoscalers at init, passing autoscaler data into PodLifecyclePlugin, new types for per-machine-set and workload capacity, and test updates to validate workload capacity aggregation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
/test e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droslean The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e |
|
/test e2e |
|
/retest |
|
/test e2e |
…of the machinesets Signed-off-by: Nikolaos Moraitis <nmoraiti@redhat.com>
|
/test e2e |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/metrics/pods_test.go (1)
21-23: Consider registering the autoscalingv1beta1 scheme for test consistency.The test
init()only registersmachinev1beta1scheme. While the current tests don't create autoscaler objects via the fake client (they're passed directly), adding the autoscaling scheme registration would improve consistency with production code.♻️ Suggested improvement
func init() { machinev1beta1.AddToScheme(scheme.Scheme) + autoscalingv1beta1.SchemeBuilder.AddToScheme(scheme.Scheme) }pkg/metrics/pods.go (1)
167-213: Consider scoping MachineSet listing to the machine-api namespace.The
client.Listfor MachineSets on line 170 fetches cluster-wide. In OpenShift clusters, MachineSets typically reside inopenshift-machine-api. Scoping the list could improve performance and avoid unexpected results from MachineSets in other namespaces.♻️ Suggested improvement
func (p *PodLifecyclePlugin) getWorkloadCounts(workload string) WorkloadNodeCount { ret := WorkloadNodeCount{Workload: workload} machineSetList := &machinev1beta1.MachineSetList{} - if err := p.client.List(p.ctx, machineSetList); err != nil { + if err := p.client.List(p.ctx, machineSetList, ctrlruntimeclient.InNamespace(MachineAPINamespace)); err != nil { p.logger.WithError(err).Warn("Failed to list MachineSets") return WorkloadNodeCount{} }
|
@droslean: 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. |
/cc @openshift/test-platform