NETOBSERV-2402 : Add LokiStack operator status integration #1122
Conversation
OlivierCazade
force-pushed
the
lokistack-status
branch
from
43f2e6c to
01cb822
Compare
|
/retest |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1122 +/- ##
==========================================
- Coverage 53.60% 52.91% -0.70%
==========================================
Files 205 209 +4
Lines 10500 10960 +460
Branches 1296 1391 +95
==========================================
+ Hits 5629 5799 +170
- Misses 4357 4611 +254
- Partials 514 550 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
pkg/handler/loki.go
Outdated
| if h.Cfg.Loki.Status != nil { | ||
| for _, conditions := range h.Cfg.Loki.Status.Conditions { | ||
| if conditions.Reason == "ReadyComponents" { | ||
| if conditions.Status == "True" { | ||
| return []byte("ready"), 200, nil | ||
| } | ||
| break | ||
| } | ||
| } | ||
| return []byte("pending"), 400, nil | ||
| } |
There was a problem hiding this comment.
On second thought, I think that would be something to do on the operator side, rather than here. One of the reason is, by embedding the whole Loki Status in the console config, we make it very dependent on any status change, even if we don't care about that change (which if I'm correct generates a plugin restart)
Having a first-pass done on the operator, to extract what we want, would avoid that.
There was a problem hiding this comment.
Hi @OlivierCazade - I used Claude to perform code review for this PR and netobserv/network-observability-operator#2142 together in combination:
Besides other comments I added from that review, one of the things it highlighted architecture wise was the impact on Pod restarts which appears reasonable: , I'd like to get your thoughts:
Problem: The operator embeds the entire LokiStackStatus into the configmap. Any status change triggers:
1. ConfigMap update with new digest
2. Plugin deployment rollout (all pods restart)
LokiStack status can change frequently during:
- Reconciliation loops
- Component health fluctuations
- Scaling events
- Transient failures
Impact: Users may experience frequent plugin restarts and UI disconnections.
Recommendation:
- Monitor this behavior in testing
- Consider only embedding specific status fields (Ready condition only)
- Or implement status change filtering to only reconcile on meaningful changes
Better Alternative:
// In consoleplugin_objects.go
type LokiStackCondition struct {
Type string `json:"type"`
Status string `json:"status"`
}
type SimplifiedLokiStatus struct {
Conditions []LokiStackCondition `json:"conditions"`
}
// Only copy the Ready condition
if lokiStack != nil {
for _, cond := range lokiStack.Status.Conditions {
if cond.Type == lokiv1.ConditionReady {
config.Loki.Status = &SimplifiedLokiStatus{
Conditions: []LokiStackCondition{{
Type: string(cond.Type),
Status: string(cond.Status),
}},
}
break
}
}
config.Loki.StatusURL = ""
}
It also pointed out other missing unit test coverage:
Missing in Console Plugin:
- Test for status condition checking logic
- Test for different LokiStack status states
- Test for error responses when status URL is blocked
pkg/handler/loki.go
Outdated
| if h.Cfg.Loki.Status != nil { | ||
| for _, conditions := range h.Cfg.Loki.Status.Conditions { | ||
| if conditions.Reason == "ReadyComponents" { | ||
| if conditions.Status == "True" { | ||
| return []byte("ready"), 200, nil | ||
| } | ||
| break | ||
| } | ||
| } | ||
| return []byte("pending"), 400, nil | ||
| } |
There was a problem hiding this comment.
from Claude review:
Problems:
1. Uses Reason instead of Type for condition matching
2. Only checks one specific condition reason
3. Doesn't check condition type properly according to LokiStack API
4. Returns 400 (Bad Request) for "pending" - should be 503 (Service Unavailable)
Fix Required: Based on LokiStack API, should check Type field:
if h.Cfg.Loki.Status != nil {
for _, condition := range h.Cfg.Loki.Status.Conditions {
if condition.Type == lokiv1.ConditionReady {
if condition.Status == "True" {
return []byte("ready"), 200, nil
}
break
}
}
return []byte("pending"), 503, nil // Service Unavailable, not Bad Request
}
OlivierCazade
force-pushed
the
lokistack-status
branch
from
01cb822 to
5ffd74d
Compare
|
[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 |
Integrate with LokiStack operator to use its status conditions instead of querying the Loki status endpoint. This adds support for detecting Loki readiness through the operator's status API when available. Changes: - Add github.com/grafana/loki/operator/apis/loki dependency - Add LokiStackStatus field to Loki config - Check operator status in getLokiStatus before querying status URL - Prevent status URL usage when using Loki operator
OlivierCazade
force-pushed
the
lokistack-status
branch
from
5ffd74d to
0452f4e
Compare
Co-authored-by: Mehul Modi <memodi@redhat.com>
3 participants
Description
Integrate with LokiStack operator to use its status conditions instead of querying the Loki status endpoint. This adds
support for detecting Loki readiness through the operator's status API when available.
Changes:
Dependencies
The associated operator PR
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.