Skip to content

NETOBSERV-2402: Adding lokistack status to console plugin configmap #2142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OlivierCazade wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NETOBSERV-2402: Adding lokistack status to console plugin configmap #2142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/controller/consoleplugin/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type LokiConfig struct {
URL string `yaml:"url" json:"url"`
Labels []string `yaml:"labels" json:"labels"`

Status string `yaml:"status,omitempty" json:"status,omitempty"`
StatusURL string `yaml:"statusUrl,omitempty" json:"statusUrl,omitempty"`
Timeout api.Duration `yaml:"timeout,omitempty" json:"timeout,omitempty"`
TenantID string `yaml:"tenantID,omitempty" json:"tenantID,omitempty"`
Expand Down
22 changes: 21 additions & 1 deletion internal/controller/consoleplugin/consoleplugin_objects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strconv"
"time"

lokiv1 "github.com/grafana/loki/operator/apis/loki/v1"
osv1 "github.com/openshift/api/console/v1"
monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
"gopkg.in/yaml.v2"
Expand Down Expand Up @@ -493,9 +494,24 @@ func (b *builder) setFrontendConfig(fconf *cfg.FrontendConfig) error {
return nil
}

func getLokiStatus(lokiStack *lokiv1.LokiStack) string {
Copy link
Member

@jotak jotak Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not tested, but looking at the code I think there's a problem here:
In reconcileConfigMap, the lokiStack object is created and can have the following values:

  • a reference to the lokistack that was found
  • nil if an error occurred
  • a reference to an empty (0-value) struct &lokiv1.LokiStack{} when not in LokiStack mode

So in this last situation, the returned value would be pending, which seems incorrect?

IMO, what we could do:

  • if there was an error when fetching LokiStack, set this error as status (e.g. the console plugin could display something like "LokiStack not found")
  • if the lokistack shows a non-ready status (error/pending condition), set a message with that condition status
  • if the loki stack is ready, set as ready
  • if not in lokistack mode, set as an empty string

Also, I'm not sure it's useful to check for the presence of the LokiStack API: if it's configured in LokiStack mode BUT the API is not present, the config is wrong, so it's ok to just display the error message that would come up when trying to fetch LokiStack?
wdyt?

if lokiStack != nil {
for _, conditions := range lokiStack.Status.Conditions {
if conditions.Reason == "ReadyComponents" {
if conditions.Status == "True" {
return "ready"
}
break
}
}
return "pending"
}
return ""
}

// returns a configmap with a digest of its configuration contents, which will be used to
// detect any configuration change
func (b *builder) configMap(ctx context.Context) (*corev1.ConfigMap, string, error) {
func (b *builder) configMap(ctx context.Context, lokiStack *lokiv1.LokiStack) (*corev1.ConfigMap, string, error) {
config := cfg.PluginConfig{
Server: cfg.ServerConfig{
Port: int(*b.advanced.Port),
Expand All @@ -511,6 +527,10 @@ func (b *builder) configMap(ctx context.Context) (*corev1.ConfigMap, string, err
// configure loki
var err error
config.Loki, err = b.getLokiConfig()
if lokiStack != nil {
config.Loki.Status = getLokiStatus(lokiStack)
config.Loki.StatusURL = ""
}
if err != nil {
return nil, "", err
}
Expand Down
33 changes: 28 additions & 5 deletions internal/controller/consoleplugin/consoleplugin_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,16 @@ import (
appsv1 "k8s.io/api/apps/v1"
ascv2 "k8s.io/api/autoscaling/v2"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/log"

lokiv1 "github.com/grafana/loki/operator/apis/loki/v1"
flowslatest "github.com/netobserv/network-observability-operator/api/flowcollector/v1beta2"
"github.com/netobserv/network-observability-operator/internal/controller/constants"
"github.com/netobserv/network-observability-operator/internal/controller/reconcilers"
"github.com/netobserv/network-observability-operator/internal/pkg/helper"
"github.com/netobserv/network-observability-operator/internal/pkg/resources"
apierrors "k8s.io/apimachinery/pkg/api/errors"
)

// Type alias
Expand Down Expand Up @@ -83,7 +84,7 @@ func (r *CPReconciler) Reconcile(ctx context.Context, desired *flowslatest.FlowC
}
}

cmDigest, err := r.reconcileConfigMap(ctx, &builder)
cmDigest, err := r.reconcileConfigMap(ctx, &builder, &desired.Spec)
if err != nil {
return err
}
Expand Down Expand Up @@ -158,7 +159,7 @@ func (r *CPReconciler) reconcilePlugin(ctx context.Context, builder *builder, de
pluginExists := true
err := r.Get(ctx, types.NamespacedName{Name: name}, &oldPlg)
if err != nil {
if errors.IsNotFound(err) {
if apierrors.IsNotFound(err) {
pluginExists = false
} else {
return err
Expand All @@ -179,8 +180,30 @@ func (r *CPReconciler) reconcilePlugin(ctx context.Context, builder *builder, de
return nil
}

func (r *CPReconciler) reconcileConfigMap(ctx context.Context, builder *builder) (string, error) {
newCM, configDigest, err := builder.configMap(ctx)
func (r *CPReconciler) reconcileConfigMap(ctx context.Context, builder *builder, desired *flowslatest.FlowCollectorSpec) (string, error) {
lokiStack := &lokiv1.LokiStack{}
if desired.Loki.Mode == flowslatest.LokiModeLokiStack {
if r.ClusterInfo.HasLokiStack() {
ns := desired.Loki.LokiStack.Namespace
if ns == "" {
ns = desired.Namespace
}
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)
}
}
}
}

newCM, configDigest, err := builder.configMap(ctx, lokiStack)
if err != nil {
return "", err
}
Expand Down
24 changes: 12 additions & 12 deletions internal/controller/consoleplugin/consoleplugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func getAutoScalerSpecs() (ascv2.HorizontalPodAutoscaler, flowslatest.FlowCollec
func getBuilder(spec *flowslatest.FlowCollectorSpec, lk *helper.LokiConfig) builder {
info := reconcilers.Common{Namespace: testNamespace, Loki: lk, ClusterInfo: &cluster.Info{}}
b := newBuilder(info.NewInstance(map[reconcilers.ImageRef]string{reconcilers.MainImage: testImage}, status.Instance{}), spec, constants.PluginName)
_, _, _ = b.configMap(context.Background()) // build configmap to update builder's volumes
_, _, _ = b.configMap(context.Background(), nil) // build configmap to update builder's volumes
return b
}

Expand Down Expand Up @@ -223,8 +223,8 @@ func TestConfigMapUpdateCheck(t *testing.T) {
}
spec := flowslatest.FlowCollectorSpec{ConsolePlugin: plugin}
builder := getBuilder(&spec, &loki)
old, _, _ := builder.configMap(context.Background())
nEw, _, _ := builder.configMap(context.Background())
old, _, _ := builder.configMap(context.Background(), nil)
nEw, _, _ := builder.configMap(context.Background(), nil)
assert.Equal(old.Data, nEw.Data)

// update loki
Expand All @@ -239,15 +239,15 @@ func TestConfigMapUpdateCheck(t *testing.T) {
}},
}
builder = getBuilder(&spec, &loki)
nEw, _, _ = builder.configMap(context.Background())
nEw, _, _ = builder.configMap(context.Background(), nil)
assert.NotEqual(old.Data, nEw.Data)
old = nEw

// set status url and enable default tls
loki.LokiManualParams.StatusURL = "http://loki.status:3100/"
loki.LokiManualParams.StatusTLS.Enable = true
builder = getBuilder(&spec, &loki)
nEw, _, _ = builder.configMap(context.Background())
nEw, _, _ = builder.configMap(context.Background(), nil)
assert.NotEqual(old.Data, nEw.Data)
old = nEw

Expand All @@ -258,7 +258,7 @@ func TestConfigMapUpdateCheck(t *testing.T) {
CertFile: "status-ca.crt",
}
builder = getBuilder(&spec, &loki)
nEw, _, _ = builder.configMap(context.Background())
nEw, _, _ = builder.configMap(context.Background(), nil)
assert.NotEqual(old.Data, nEw.Data)
old = nEw

Expand All @@ -270,7 +270,7 @@ func TestConfigMapUpdateCheck(t *testing.T) {
CertKey: "tls.key",
}
builder = getBuilder(&spec, &loki)
nEw, _, _ = builder.configMap(context.Background())
nEw, _, _ = builder.configMap(context.Background(), nil)
assert.NotEqual(old.Data, nEw.Data)
}

Expand All @@ -286,8 +286,8 @@ func TestConfigMapUpdateWithLokistackMode(t *testing.T) {
loki := helper.NewLokiConfig(&lokiSpec, "any")
spec := flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: lokiSpec}
builder := getBuilder(&spec, &loki)
old, _, _ := builder.configMap(context.Background())
nEw, _, _ := builder.configMap(context.Background())
old, _, _ := builder.configMap(context.Background(), nil)
nEw, _, _ := builder.configMap(context.Background(), nil)
assert.Equal(old.Data, nEw.Data)

// update lokistack name
Expand All @@ -296,7 +296,7 @@ func TestConfigMapUpdateWithLokistackMode(t *testing.T) {

spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: lokiSpec}
builder = getBuilder(&spec, &loki)
nEw, _, _ = builder.configMap(context.Background())
nEw, _, _ = builder.configMap(context.Background(), nil)
assert.NotEqual(old.Data, nEw.Data)
old = nEw

Expand All @@ -306,7 +306,7 @@ func TestConfigMapUpdateWithLokistackMode(t *testing.T) {

spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: lokiSpec}
builder = getBuilder(&spec, &loki)
nEw, _, _ = builder.configMap(context.Background())
nEw, _, _ = builder.configMap(context.Background(), nil)
assert.NotEqual(old.Data, nEw.Data)
}

Expand All @@ -331,7 +331,7 @@ func TestConfigMapContent(t *testing.T) {
Processor: flowslatest.FlowCollectorFLP{SubnetLabels: flowslatest.SubnetLabels{OpenShiftAutoDetect: ptr.To(false)}},
}
builder := getBuilder(&spec, &loki)
cm, _, err := builder.configMap(context.Background())
cm, _, err := builder.configMap(context.Background(), nil)
assert.NotNil(cm)
assert.Nil(err)

Expand Down
7 changes: 7 additions & 0 deletions internal/controller/flowcollector_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"

lokiv1 "github.com/grafana/loki/operator/apis/loki/v1"
osv1 "github.com/openshift/api/console/v1"
securityv1 "github.com/openshift/api/security/v1"
appsv1 "k8s.io/api/apps/v1"
Expand All @@ -12,6 +13,7 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"

flowslatest "github.com/netobserv/network-observability-operator/api/flowcollector/v1beta2"
Expand Down Expand Up @@ -68,6 +70,11 @@ func Start(ctx context.Context, mgr *manager.Manager) (manager.PostCreateHook, e
log.Info("CNO not detected: using ovnKubernetes config and reconciler")
}

if mgr.ClusterInfo.HasLokiStack() {
builder.Watches(&lokiv1.LokiStack{}, &handler.EnqueueRequestForObject{})
Copy link
Member

@jotak jotak Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Comment on lines +73 to +74
Copy link
Member

@memodi memodi Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
      },
  ))

Copy link
Contributor Author

@OlivierCazade OlivierCazade Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@jotak jotak Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@jotak jotak Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log.Info("LokiStack CRD detected")
}

ctrl, err := builder.Build(&r)
if err != nil {
return nil, err
Expand Down
19 changes: 13 additions & 6 deletions internal/pkg/cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"sync"

"github.com/coreos/go-semver/semver"
lokiv1 "github.com/grafana/loki/operator/apis/loki/v1"
configv1 "github.com/openshift/api/config/v1"
osv1 "github.com/openshift/api/console/v1"
operatorv1 "github.com/openshift/api/operator/v1"
Expand Down Expand Up @@ -43,12 +44,13 @@ type Info struct {
}

var (
consolePlugin = "consoleplugins." + osv1.GroupVersion.String()
cno = "networks." + operatorv1.GroupVersion.String()
svcMonitor = "servicemonitors." + monv1.SchemeGroupVersion.String()
promRule = "prometheusrules." + monv1.SchemeGroupVersion.String()
ocpSecurity = "securitycontextconstraints." + securityv1.SchemeGroupVersion.String()
consolePlugin = "consoleplugins." + osv1.GroupVersion.String()
cno = "networks." + operatorv1.GroupVersion.String()
svcMonitor = "servicemonitors." + monv1.SchemeGroupVersion.String()
promRule = "prometheusrules." + monv1.SchemeGroupVersion.String()
ocpSecurity = "securitycontextconstraints." + securityv1.SchemeGroupVersion.String()
endpointSlices = "endpointslices." + discoveryv1.SchemeGroupVersion.String()
lokistacks = "lokistacks." + lokiv1.GroupVersion.String()
)

func NewInfo(ctx context.Context, cl client.Client, dcl *discovery.DiscoveryClient, onRefresh func()) (*Info, func(ctx context.Context) error, error) {
Expand All @@ -74,6 +76,7 @@ func (c *Info) fetchAvailableAPIs(ctx context.Context) error {
promRule: false,
ocpSecurity: false,
endpointSlices: false,
lokistacks: false,
}
for apiName := range apisMap {
if hasAPI(apiName, resources) {
Expand Down Expand Up @@ -281,9 +284,13 @@ func (c *Info) HasPromRule() bool {
return c.apisMap[promRule]
}

// HasEndpointSlices returns true if "endpointslices.discovery.k8s.io" API was found
func (c *Info) HasEndpointSlices() bool {
c.apisMapLock.RLock()
defer c.apisMapLock.RUnlock()
return c.apisMap[endpointSlices]
}

// HasLokiStack returns true if "lokistack" API was found
func (c *Info) HasLokiStack() bool {
return c.apisMap[lokistacks]
}