banzaicloud / koperator

Oh no! Yet another Apache Kafka operator for Kubernetes
Apache License 2.0
788 stars 198 forks source link

istio merged metrics cannot be used #1060

Open vitalii-buchyn-exa opened 1 year ago

vitalii-buchyn-exa commented 1 year ago

Description

Hello community,

We are experiencing an issue with scraping istio merged metrics with enablePrometheusMerge istio feature. In order to pull metrics by a pilot we need to add prometheus annotations to a brokerAnnotations spec of KafkaCluster object, like:

      brokerAnnotations:
        prometheus.io/path: /metrics
        prometheus.io/scrape: "true"
        prometheus.io/port: "9020"

This should result in a following env var for istio-proxy sidecar:

$ echo $ISTIO_PROMETHEUS_ANNOTATIONS
{"scrape":"true","path":"/metrics","port":"9020"}

The problem is that istio webhook overrides those annotations for a pod to make a merged metrics endpoint available for prometheus, like:

apiVersion: v1
kind: Pod
metadata:
  annotations:
    kubectl.kubernetes.io/default-container: kafka
    kubectl.kubernetes.io/default-logs-container: kafka
    prometheus.io/path: /stats/prometheus
    prometheus.io/port: "15020"
    prometheus.io/scrape: "true"

That webhook override causes koperator to instantly restart brokers.

Please let me know if any additional info is required.

Regards, Vitalii

Expected Behavior

Maybe ignore istio specific prometheus annotations. Yes, this will require manual brokers restart when prometheus config changes in kafkacluster object.

Actual Behavior

Continious pods restart.

Affected Version

kafka-operator:v0.22.0 banzaicloud-kafka:2.13-2.7.0-bzc.2 banzaicloud-cruise-control:2.5.101 jmx-exporter: banzaicloud-jmx-javaagent:0.14.0 cruise-control:2.5.101

Checklist

bartam1 commented 1 year ago

Hello @vitalii-buchyn-exa ! I think merge metrics should be false to avoid this:

brokerAnnotations:
    prometheus.istio.io/merge-metrics: "false"

I only know that this is needed for seamless workig

vitalii-buchyn-exa commented 1 year ago

hello @bartam1 !

Thank you for the suggestion, but that doesn't work. merge-metrics false annotation causes istio webhook to not override prometheus metrics and not set ISTIO_PROMETHEUS_ANNOTATIONS env var

After applying that annotation it looks like:

apiVersion: v1
kind: Pod
metadata:
  annotations:
    kubectl.kubernetes.io/default-container: kafka
    kubectl.kubernetes.io/default-logs-container: kafka
    prometheus.io/path: /metrics
    prometheus.io/port: "9020"
    prometheus.io/scrape: "true"
    prometheus.istio.io/merge-metrics: "false"
    sidecar.istio.io/inject: "true"

Our goal is to use merged metrics that should be exposed on TCP port 15020 via /stats/prometheus endpoint

bartam1 commented 1 year ago

Hello @vitalii-buchyn-exa !

Koperator reconcile the broker pods when the pod k8s resource is changed. Thus because there is a change by a 3rd party then it will put back the original state so there will be pod restart. I would try to eliminate the difference between what you have in the kafkacluster CR broker pod configuration and what the 3rd party app made on the pod

vitalii-buchyn-exa commented 1 year ago

hello @bartam1,

If we remove broker annotations from kafkacluster CR, then istio will not setup an ISTIO_PROMETHEUS_ANNOTATIONS env var and pilot prometheus will not be aware where to scrape broker' metrics.

Koperator reconcile the broker pods when the pod k8s resource is changed.

That's the issue i'm trying to emphasise. If you know another way how to make merged istio metrics to work with koperator, please let me know.

Appreciate your time and help!

vitalii-buchyn-exa commented 1 year ago

any update/help please?

can koperator just ignore existing prometheus annotations?