Adding monitoring capability to lvm-operator #49

aruniiird · 2021-12-24T12:59:10Z

Signed-off-by: Arun Kumar Mohan amohan@redhat.com

nbalacha · 2021-12-27T06:59:46Z

config/rbac/metrics_service_role.yaml

+metadata:
+  name: controller-manager-metrics-service
+rules:
+ - apiGroups: [""]


Please use the same format as the rbac yamls for uniformity.

Why does this require a cluster role?

Initially thought we require a cluster wide permissions, but moved it to Role now

All the yamls are moved to same format

leelavg · 2021-12-27T10:32:08Z

config/prometheus/monitor.yaml

@@ -5,8 +5,8 @@ kind: ServiceMonitor
 metadata:
  labels:
    control-plane: controller-manager
-  name: controller-manager-metrics-monitor
-  namespace: system
+  name: lvm-operator-controller-manager-metrics-monitor


any reason for adding the namespace as prefix? if this isn't mandatory for the metrics, can we please not use this?

This prefix makes it more readable. In fact, we should add this prefix to all the lvm-operator objects.

Yes, you should add prefix to all your resources.
Use https://github.com/red-hat-storage/lvm-operator/blob/main/config/default/kustomization.yaml#L4-L9 for prefix.
Also, try adding labels via https://github.com/red-hat-storage/lvm-operator/blob/main/config/default/kustomization.yaml#L11-L13 for easy queries later on.

this prefix usage was discussed earlier, we decided not to use this and follow ocs-operator convention.

the reason was, it's difficult to get this prefix in code where we are associating serviceaccount names with deployments

We can modify the kustomize files to include the prefix explicitly. I agree that the prefix should be added.

ack, then we need to settle on a consistent name and use that in code as well while supplying serviceaccount names

config/default/kustomization.yaml

config/prometheus/rbac-authorization-role.yaml

jmolmo · 2022-01-12T08:09:27Z

cmd/metricsexporter/metricsExporter.go

+
+func main() {
+	lvmExpFlags := flag.NewFlagSet("lvm-metrics-exporter", flag.ExitOnError)
+	lvmExpFlags.StringVar(&exporterNamespace, "namespace", "lvm-exporter",


Where is used the namespace?

Currently this metric exporter is a dummy one which don't export/expose any particular metrics. Once this is actively used then we require this flag, otherwise this is a place holder

nbalacha

Can a service scrape metrics from both the topolvm and lvm-operator nodes or are separate services required?

config/prometheus/monitor.yaml

config/prometheus/rbac-lvm-op-metric-role-n-rolebindings.yaml

nbalacha · 2022-01-12T06:25:24Z

config/prometheus/rbac-lvm-op-metric-role-n-rolebindings.yaml

+kind: RoleBinding
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: lvm-operator-controller-metric-rbac-rolebinding


Please rename this to lvm-operator-metrics

config/prometheus/rbac-lvm-op-metric-role-n-rolebindings.yaml

nbalacha · 2022-01-12T06:26:24Z

config/prometheus/rbac-topolvm-metric-role-n-rolebindings.yaml

+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: topolvm-metrics
+  namespace: lvm-operator-system


Please set the namespace to system so kustomize can update it correctly.

config/prometheus/rbac-topolvm-metric-role-n-rolebindings.yaml

config/samples/topolvm-metric-service-config.yaml

nbalacha · 2022-01-12T06:31:47Z

config/prometheus/rbac-lvm-op-metric-role-n-rolebindings.yaml

+kind: Role
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  # 'namespace' is omitted as 'ClusterRole'-s are not namespaced


Please remove this comment as this is a Role, not a ClusterRole.

Removed the comment and added the namespace: system

nbalacha · 2022-01-12T06:38:06Z

config/rbac/metrics_service_role.yaml

+metadata:
+  name: controller-manager-metrics-service
+rules:
+ - apiGroups: [""]


Why does this require a cluster role?

config/prometheus/monitor.yaml

sp98

A couple of questions here:

How is the metricsExporter.go getting invoked here?
Can the export extract metrics from different components (vgManager, node etc) or just form the operator?

sp98 · 2022-01-13T04:08:55Z

cmd/metricsexporter/metricsExporter.go

+	newLogger.Log(log.InfoLevel, "Namespace:", exporterNamespace)
+	newLogger.Log(log.DebugLevel, "Address:", exporterAddress)
+
+	// kubeConfig, err := clientcmd.BuildConfigFromFlags(kubeAPIServerURL, kubeconfigPath)


Commented out code can be removed if its not required.

Removed the commented out lines

cmd/metricsexporter/metricsExporter.go

sp98 · 2022-01-13T04:24:41Z

cmd/metricsexporter/metricsExporter.go

+	http.Handle("/metrics", promhttp.Handler())
+	http.HandleFunc("/", func(rw http.ResponseWriter, r *http.Request) {
+		rw.Write([]byte(`<html>
+             <head><title>LVM Metric Exporter</title></head>


(asking for my understanding) Why do we need this HTML ?

This is added, if user try to access server without /metrics path appended in the address (like 127.0.0.1:8080/).
This will guide them to reach correct location. It is a common practice seen in many metric exporters

.gitignore

nbalacha · 2022-01-13T09:49:34Z

Please follow the commit lint format for the commit messages :

feat:

Long Description

.vscode/launch.json

Dockerfile.exporter

Makefile

nbalacha · 2022-01-15T12:46:07Z

config/manager/manager.yaml

+      - name: metricsexporter
+        command:
+        - /metricsexporter
+        image: exporter:latest


Doesn't this require command line arguments? When I ran a make deploy, the logs show the following:

{"level":"info","ts":1642251710.3554845,"logger":"lvm-metric-exporter","msg":"Exporter set values: ","namespace":"lvm-exporter","address":":23532"}

When the namespace should be lvm-operator-system (default namespace in the kustomize config files)

Right now it is run on all the default params.
./metricsexporter -h

-address string address on which the metrics exporter should run (default ":23532") -apiserver string API server URL -kubeconfig string Path to kubeconfig file -namespace string set the namespace of the lvm metric exporter (default "lvm-exporter") -zap-devel Development Mode defaults(encoder=consoleEncoder,logLevel=Debug,stackTraceLevel=Warn). Production Mode defaults(encoder=jsonEncoder,logLevel=Info,stackTraceLevel=Error) -zap-encoder value Zap log encoding (one of 'json' or 'console') -zap-log-level value Zap Level to configure the verbosity of logging. Can be one of 'debug', 'info', 'error', or any integer value > 0 which corresponds to custom debug levels of increasing verbosity -zap-stacktrace-level value Zap Level at and above which stacktraces are captured (one of 'info', 'error', 'panic').

Dockerfile.exporter

config/manager/manager.yaml

Makefile

cmd/metricsexporter/metrics_exporter.go

nbalacha · 2022-01-17T07:01:29Z

Requesting a review from @umangachapagain as he knows monitoring.

cmd/metricsexporter/metrics_exporter.go

aruniiird · 2022-01-17T16:30:15Z

Something went wrong (completely)... and whole of my changes are gone now.... =(

leelavg · 2022-01-17T16:33:58Z

Something went wrong (completely)... and whole of my changes are gone now.... =(

Ya, just observed that
Maybe use git reflog to checkout at commit e0f1e92 or 26a88a0 (referred from above github commits) if not done already

leelavg · 2022-01-18T03:57:25Z

@aruniiird can you please do a rebase?

config/manager/manager.yaml

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

As a first step adding all the yaml files needed for the metrics monitoring. Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

leelavg

/lgtm

openshift-ci · 2022-01-18T08:21:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aruniiird, leelavg, sp98

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 24, 2021

aruniiird marked this pull request as draft December 24, 2021 12:59

nbalacha reviewed Dec 27, 2021

View reviewed changes

leelavg reviewed Dec 27, 2021

View reviewed changes

sp98 reviewed Dec 27, 2021

View reviewed changes

config/prometheus/rbac-authorization-role.yaml Outdated Show resolved Hide resolved

config/prometheus/rbac-authorization-role.yaml Outdated Show resolved Hide resolved

aruniiird force-pushed the add-monitoring-components branch 2 times, most recently from 721ac0e to 83799f7 Compare January 11, 2022 02:35

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022

aruniiird force-pushed the add-monitoring-components branch from 83799f7 to 51656b8 Compare January 11, 2022 16:49

aruniiird changed the title ~~[WIP] Adding monitoring capability to lvm-operator~~ Adding monitoring capability to lvm-operator Jan 11, 2022

aruniiird force-pushed the add-monitoring-components branch from 51656b8 to bb3ce4e Compare January 12, 2022 05:46

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2022

jmolmo reviewed Jan 12, 2022

View reviewed changes

nbalacha suggested changes Jan 12, 2022

View reviewed changes

openshift-ci bot assigned nbalacha Jan 12, 2022

aruniiird force-pushed the add-monitoring-components branch from bb3ce4e to 52eb1c6 Compare January 12, 2022 15:57

aruniiird marked this pull request as ready for review January 12, 2022 15:57

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2022

sp98 suggested changes Jan 13, 2022

View reviewed changes

openshift-ci bot assigned sp98 Jan 13, 2022

nbalacha suggested changes Jan 13, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

aruniiird force-pushed the add-monitoring-components branch 4 times, most recently from 39d2f22 to d2f3c90 Compare January 15, 2022 06:19

nbalacha suggested changes Jan 15, 2022

View reviewed changes

nbalacha requested a review from umangachapagain January 17, 2022 07:00

aruniiird force-pushed the add-monitoring-components branch from d2f3c90 to c9e9cfd Compare January 17, 2022 07:09

openshift-ci bot assigned leelavg Jan 17, 2022

aruniiird force-pushed the add-monitoring-components branch 4 times, most recently from 6c78ec9 to 26a88a0 Compare January 17, 2022 12:49

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2022

sp98 reviewed Jan 17, 2022

View reviewed changes

cmd/metricsexporter/metrics_exporter.go Outdated Show resolved Hide resolved

aruniiird force-pushed the add-monitoring-components branch 2 times, most recently from e0f1e92 to e386b7b Compare January 17, 2022 16:19

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2022

aruniiird force-pushed the add-monitoring-components branch from e386b7b to 807b805 Compare January 17, 2022 19:01

leelavg reviewed Jan 18, 2022

View reviewed changes

config/manager/manager.yaml Outdated Show resolved Hide resolved

aruniiird force-pushed the add-monitoring-components branch 2 times, most recently from 3b7c110 to 6dbc39a Compare January 18, 2022 06:25

aruniiird added 2 commits January 18, 2022 11:59

feat: adding lvm operator metric exporter

49fc9a4

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

feat: add topolvm monitoring config files

f7fe182

As a first step adding all the yaml files needed for the metrics monitoring. Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

aruniiird force-pushed the add-monitoring-components branch 2 times, most recently from 6440622 to f03789c Compare January 18, 2022 07:47

sp98 approved these changes Jan 18, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

aruniiird force-pushed the add-monitoring-components branch from f03789c to 80e5b9a Compare January 18, 2022 08:16

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

feat: add lvm operator monitoring config files

80e5b9a

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>

leelavg approved these changes Jan 18, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

nbalacha merged commit 52673fa into openshift:main Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding monitoring capability to lvm-operator #49

Adding monitoring capability to lvm-operator #49

aruniiird commented Dec 24, 2021

nbalacha Dec 27, 2021

nbalacha Jan 12, 2022

aruniiird Jan 13, 2022

aruniiird Jan 13, 2022

leelavg Dec 27, 2021

nbalacha Jan 12, 2022

umangachapagain Jan 17, 2022

leelavg Jan 17, 2022

nbalacha Jan 17, 2022

leelavg Jan 17, 2022

jmolmo Jan 12, 2022

aruniiird Jan 13, 2022

nbalacha left a comment •

edited

Loading

nbalacha Jan 12, 2022

aruniiird Jan 13, 2022

nbalacha Jan 12, 2022

aruniiird Jan 13, 2022

nbalacha Jan 12, 2022

aruniiird Jan 13, 2022

nbalacha Jan 12, 2022

sp98 left a comment

sp98 Jan 13, 2022

aruniiird Jan 13, 2022

sp98 Jan 13, 2022

aruniiird Jan 13, 2022

nbalacha commented Jan 13, 2022

nbalacha Jan 15, 2022

aruniiird Jan 17, 2022

nbalacha commented Jan 17, 2022

aruniiird commented Jan 17, 2022

leelavg commented Jan 17, 2022 •

edited

Loading

leelavg commented Jan 18, 2022

leelavg left a comment

openshift-ci bot commented Jan 18, 2022

Adding monitoring capability to lvm-operator #49

Adding monitoring capability to lvm-operator #49

Conversation

aruniiird commented Dec 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha commented Jan 17, 2022

aruniiird commented Jan 17, 2022

leelavg commented Jan 17, 2022 • edited Loading

leelavg commented Jan 18, 2022

leelavg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 18, 2022

nbalacha left a comment •

edited

Loading

leelavg commented Jan 17, 2022 •

edited

Loading