NETOBSERV-2402: Adding lokistack status to console plugin configmap #2142
Conversation
|
@OlivierCazade: This pull request references NETOBSERV-2402 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
OlivierCazade
force-pushed
the
loki-status
branch
from
8d89c3d to
6815b9b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2142 +/- ##
==========================================
- Coverage 73.52% 72.80% -0.72%
==========================================
Files 80 80
Lines 9064 9131 +67
==========================================
- Hits 6664 6648 -16
- Misses 1992 2069 +77
- Partials 408 414 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if mgr.ClusterInfo.HasLokiStack() { | ||
| builder.Watches(&lokiv1.LokiStack{}, &handler.EnqueueRequestForObject{}) |
There was a problem hiding this comment.
ideally, we would enqueue only when our configured lokistack is affected, not all lokistacks
but I guess that's fine, as we don't expect hundreds of lokistacks out there :-)
memodi
left a comment
memodi
left a comment
There was a problem hiding this comment.
Hi @OlivierCazade - adding review comments from Claude, I focused on must-fix and high impact fixes including better error reporting.
Besides the comments, it also pointed out missing test coverage on the operator side:
Missing in Operator:
- Test for LokiStack status embedding in configmap
- Test for namespace defaulting logic
- Test for behavior when LokiStack is not found
- Test for behavior when LokiStack CRD is not available
| if mgr.ClusterInfo.HasLokiStack() { | ||
| builder.Watches(&lokiv1.LokiStack{}, &handler.EnqueueRequestForObject{}) |
There was a problem hiding this comment.
from Claude review:
Problem: This will create reconcile requests for LokiStack objects, not FlowCollectors. When a LokiStack named "logging-loki" changes, it will try to reconcile a FlowCollector named "logging-loki", which likely doesn't exist.
Fix Required:
builder.Watches(&lokiv1.LokiStack{}, handler.EnqueueRequestsFromMapFunc(
func(ctx context.Context, obj client.Object) []reconcile.Request {
lokiStack := obj.(*lokiv1.LokiStack)
var flowCollectors flowslatest.FlowCollectorList
if err := mgr.GetClient().List(ctx, &flowCollectors); err != nil {
log.FromContext(ctx).Error(err, "Failed to list FlowCollectors")
return []reconcile.Request{}
}
var requests []reconcile.Request
for _, fc := range flowCollectors.Items {
if fc.Spec.Loki.Mode == flowslatest.LokiModeLokiStack &&
fc.Spec.Loki.LokiStack.Name == lokiStack.Name {
ns := fc.Spec.Loki.LokiStack.Namespace
if ns == "" {
ns = fc.Namespace
}
if ns == lokiStack.Namespace {
requests = append(requests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: fc.Name,
Namespace: fc.Namespace,
},
})
}
}
}
return requests
},
))
There was a problem hiding this comment.
I did not think about making a k8s query inside the handler to filter Lokistacks.
The approach I thought was to wait for the flowcollector to be created and start a dedicated controller with a static flowcollector name. This was adding a lot of complexity and I was not sure if it was worth it.
This looks like a more simple solution, to the price of a k8s querry in the handler function.
@jotak what do you think ?
There was a problem hiding this comment.
Not sure we need to overcomplicate things here. It's basically my comment here: #2142 (comment) ; having a couple of false-positive reconcile events is not so important; we're talking about lokistack objects, it's not expected to have many, and they don't change often.
btw I think claude answer is wrong the enqueue request is not for a flow-collector named after the loki stack, it's for any flow-collector? (EnqueueRequestForObject{} with empty params)
There was a problem hiding this comment.
If we really want to narrow down to our configured lokistack, in other situation we just keep in controller state the last-time seen element that we want to check (we do that in a couple of places for flowcollector.spec.namespace iirc); we could do the same with the configured lokistack.
Like this: https://github.com/netobserv/network-observability-operator/blob/main/internal/controller/flp/flp_controller.go#L60
| lokiStack = nil | ||
| log.FromContext(ctx).Info("Could not get the LokiStack resource.") |
There was a problem hiding this comment.
from Claude review:
Problems:
- Logs at Info level instead of Warning
- Actual error is discarded
- Can't distinguish between "not found" and "permission denied"
- Should surface in FlowCollector status
Fix:
if err := r.Client.Get(ctx, types.NamespacedName{Name: desired.Loki.LokiStack.Name, Namespace: ns}, lokiStack); err != nil {
lokiStack = nil
if apierrors.IsNotFound(err) {
log.FromContext(ctx).Info("LokiStack resource not found, status will not be available",
"name", desired.Loki.LokiStack.Name,
"namespace", ns)
} else {
log.FromContext(ctx).Error(err, "Failed to get LokiStack resource",
"name", desired.Loki.LokiStack.Name,
"namespace", ns)
}
// TODO: Consider surfacing this in FlowCollector status
}
|
PR needs rebase. 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. |
5 participants
Description
Add Lokistack status to the plugin configmap.
The Lokistack is also now watched to update any status change.
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.