fix: Clear EndpointStatuses for PodMonitoring without endpoints #1197
Conversation
a76eb6e to
6134c5e
Compare
|
Note that this PR also fixes a misconfigured test that was silently failing (#1097 adds support for a nodepool format with caadvisor) |
6134c5e to
59c0c2e
Compare
There was a problem hiding this comment.
Nice! This is great and definitely one way of doing this.
I wonder if it wouldn't be better to do this on reconcile PodMonitoring loop - because the moment PodMonitoring changes, it triggers reconcile so we could automatically clear the status there. This would avoid extra API calls essentially
Actually I think I misunderstood the intention. I thought you are solving case of empty endpoint field. I think it's about no target situation (e.g. pods not selected). Let me re-review.
pkg/operator/target_status.go
Outdated
| podMonitorings, err := getPodMonitoringCRDs(ctx, kubeClient) | ||
| if err != nil { | ||
| return err | ||
| } else if len(podMonitorings) == 0 { |
There was a problem hiding this comment.
| } else if len(podMonitorings) == 0 { | |
| } | |
| if len(podMonitorings) == 0 { |
pkg/operator/target_status.go
Outdated
| } | ||
| } | ||
|
|
||
| // Any pod monitorings that exist but dont have endpoints should also be updated |
There was a problem hiding this comment.
| // Any pod monitorings that exist but dont have endpoints should also be updated | |
| // Any pod monitorings that exist but don't have endpoints should also be updated. |
pkg/operator/target_status.go
Outdated
| _, exists := hasEndpoints[pm.GetName()] | ||
| if !exists { |
There was a problem hiding this comment.
| _, exists := hasEndpoints[pm.GetName()] | |
| if !exists { | |
| if _, exists := hasEndpoints[pm.GetName()]; !exists { |
pkg/operator/target_status.go
Outdated
| if !exists { | ||
| pm.GetPodMonitoringStatus().EndpointStatuses = []monitoringv1.ScrapeEndpointStatus{} | ||
| if err := patchPodMonitoringStatus(ctx, kubeClient, pm, pm.GetPodMonitoringStatus()); err != nil { | ||
| // Same reasoning as above for error handling |
There was a problem hiding this comment.
| // Same reasoning as above for error handling | |
| // Same reasoning as above for error handling. |
bwplotka
left a comment
There was a problem hiding this comment.
LGTM, some ideas for a bit more efficiency and readable, otherwise LGTM, thanks!
59c0c2e to
35d70b2
Compare
35d70b2 to
a342d9b
Compare
Clear EndPointStatuses when PodMonitoring does not have an endpoint.
This fixes a bug where PodMonitorings without any endpoints have a stale list of endpoints.