-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
Enable operator metrics collection #273
Enable operator metrics collection #273
Conversation
39d7338
to
55bef5b
Compare
@@ -12,7 +12,6 @@ spec: | |||
- path: /metrics | |||
port: https | |||
scheme: https |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is missing for having a TLS without skipping verify?
It's basically the same reason why we have this bug: https://issues.redhat.com/browse/NETOBSERV-765
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to what we did for FLP we need to add the labels to the metric services to generate certificate and mount them in the kube-rbac-proxy container so they can be used.
55bef5b
to
5db2a1c
Compare
Added SSL and auth configuration for downstream collection, this made operator metrics endpoint incompatible with monitoring user workload. @memodi appart from review comments, the PR is ready to be tested. |
Codecov Report
@@ Coverage Diff @@
## main #273 +/- ##
==========================================
- Coverage 47.61% 47.57% -0.04%
==========================================
Files 43 43
Lines 5015 5019 +4
==========================================
Hits 2388 2388
- Misses 2416 2420 +4
Partials 211 211
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@OlivierCazade - is the idea, they would be compatible with SSL and auth configuration once operator is deployed with DOWNSTREAM_DEPLOYMENT to true? |
Yes, SSL and authentication for the metrics endpoint. And the prometheus deployment should automatically use it from the ServiceMonitor configuration |
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-operator:1b5e2fb"]. It will expire after two weeks. |
@OlivierCazade - I tried with image from this PR, I am not able to see those metrics:
am I missing something? |
Most of the changes in this PR are at the bundle level, did you also used the bundle from this branch? |
5db2a1c
to
fcc89e9
Compare
/ok-to-test |
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-a5309c1
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
fcc89e9
to
185b7a6
Compare
/ok-to-test |
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-7a7c5bf
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
monitor.yaml
Outdated
@@ -0,0 +1,8 @@ | |||
apiVersion: v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this file, do we? Is it included in the bundles, or is it just there as a helper (e.g. for make targets) ? afaict it should not be included in bundles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sorry, this was one of my test file.
config/openshift-olm/monitor.yaml
Outdated
tlsConfig: | ||
insecureSkipVerify: true | ||
caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt | ||
serverName: netobserv-metrics-service.netobserv.svc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, my understanding is that we should hardcode here the installation namespace, openshift-operators
, right?
185b7a6
to
21cb6cd
Compare
21cb6cd
to
434ced9
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
adding NETOBSERV-900 as JIRA reference |
Enable collection for operator metrics.