[metrics 2/x] Configure Prometheus Operator #687

zeeke · 2024-04-23T17:00:42Z

Deploy the needed configuration to make the prometheus
operator to find and scrape the sriov-network-metrics-exporter
endpoints, including the ServiceMonitor, Role and RoleBinding

depends on:

[metrics 1/x] Support sriov-network-metrics-exporter #655

github-actions · 2024-04-23T17:00:53Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-04-24T15:51:48Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-04-24T16:51:37Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2024-04-24T16:57:33Z

Pull Request Test Coverage Report for Build 9903758109

Details

12 of 16 (75.0%) changed or added relevant lines in 1 file are covered.
98 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.02%) to 41.0%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/sriovoperatorconfig_controller.go	12	16	75.0%

Files with Coverage Reduction	New Missed Lines	%
controllers/generic_network_controller.go	2	76.4%
controllers/sriovoperatorconfig_controller.go	34	64.19%
controllers/helper.go	62	68.75%

Totals
Change from base Build 9894164129:	0.02%
Covered Lines:	5599
Relevant Lines:	13656

💛 - Coveralls

github-actions · 2024-04-24T17:06:29Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-05-17T16:31:58Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-05-17T16:53:56Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

SchSeba · 2024-05-22T12:23:58Z

bindata/manifests/metrics-exporter/metrics-service.yaml

@@ -18,6 +18,7 @@ spec:
      name: sriov-network-metrics
      port: {{ .MetricsExporterPort }}
      targetPort: {{ .MetricsExporterPort }}
+{{ if .IsOpenshift }}


I think here we can also support k8s

lets check if the ServiceMonitor CRD exist in the cluster and deploy it instead of checking only for openshift WDYT?

good idea, working on that

Done, I used the unstructured client to check if the ServiceMonitore resource definition is available in the cluster. Does it sound good?

github-actions · 2024-05-28T07:23:06Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-05-28T08:10:11Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-05-28T08:12:43Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-05-28T08:17:05Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-05-28T09:29:40Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

SchSeba · 2024-05-29T10:49:21Z

deploy/role.yaml

@@ -32,6 +32,7 @@ rules:
  verbs:
  - get
  - create
+  - list


I think we need this one also under the config folder so it will be generated for the bundle

the contents of the config/ folder is currently out of sync with openshit/sriov-network-operator (e.g. see config/rbac/role.yaml here and here).

I would align that in a separated PR. Does it sound good?

SchSeba · 2024-05-29T10:50:47Z

controllers/helper.go

+
+func isPrometheusOperatorInstalled(ctx context.Context, client k8sclient.Reader) bool {
+	u := &uns.UnstructuredList{}
+	u.SetGroupVersionKind(schema.GroupVersionKind{


I was thinking (maybe that is not the right way) to do a kubectl get crd servicemonitor not to search if there is any server monitor object in the cluster :)

I think we can try getting the CRD (e.g. see this).

The drawback is that we have to add the permission (ClusterRole,ClusterRoleBinding,...) to make the operator read that CustomResourceDefinition resource, but it might end up cleaner.

I think it should be ok to add get for CRD that should not expose the operator to any security issues :)

I had to add the permission to the ClusterRole (instead of Role), as the CustomResourceDefinition is not namespaced.
For the same reason, I had to add a non-namespace client to the SriovOperatorConfigReconicler.
please, take a look

SchSeba · 2024-06-03T11:27:45Z

controllers/sriovoperatorconfig_controller.go

+		}
+
+		if r.PlatformHelper.IsOpenshiftCluster() {
+			err = utils.AddLabelToNamespace(ctx, vars.Namespace, "openshift.io/cluster-monitoring", "true", r.Client)


I think we need to put this in the namespace creation template and not let the operator have permission to upgrade namespace that sounds a security risk

Sounds good. Maybe we can leverage the operatorframework.io/cluster-monitoring annotation in the CSV, in the openshift fork

https://github.com/openshift/enhancements/blob/master/enhancements/olm/olm-managed-operator-metrics.md#fulfilling-namespace-and-rbac-requirements

adrianchiris · 2024-06-25T12:56:59Z

@zeeke can you rebase this one ?

github-actions · 2024-06-27T09:33:32Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-07-03T15:15:28Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-07-03T15:35:05Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-07-03T16:04:17Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-07-05T08:47:58Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke · 2024-07-05T08:48:16Z

@adrianchiris , @SchSeba please take another look

SchSeba

just some small comments :)

SchSeba · 2024-07-11T10:37:58Z

bindata/manifests/metrics-exporter/metrics-service.yaml

+subjects:
+- kind: ServiceAccount
+  name: prometheus-k8s
+  namespace: openshift-monitoring


this is good for openshift on vanilla k8s please add a variable to the helmchart something like
https://github.com/metallb/metallb/blob/21dd75560f3b8614c14b1bb55a79dbcc231e36a7/charts/metallb/templates/servicemonitor.yaml#L192

Added environment variables and Helm stuff to make the subject configurable.

deploy/clusterrole.yaml

SchSeba · 2024-07-11T10:45:43Z

pkg/utils/cluster.go

@@ -161,3 +161,28 @@ func AnnotateNode(ctx context.Context, nodeName string, key, value string, c cli

 	return AnnotateObject(ctx, node, key, value, c)
 }
+
+func AddLabelToNamespace(ctx context.Context, namespaceName, key, value string, c client.Client) error {
+	ns := &corev1.Namespace{}


I was not able to find where we use this one.

in general I think we should document the need to add a label for monitoring on namespace creation and not add a rbac to allow the operator to update namespace object

I removed the function. I will re-add it in the e2e test PR.

About permissions, the operator already had the RBAC to write on namespaces (see deploy/clusterrole.yaml and openshift CSV). No permission has been added in this PR for namespaces.

I'll take care of documenting the namespace configuration in OpenShift

github-actions · 2024-07-12T06:38:34Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-07-12T06:55:13Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke · 2024-07-16T10:02:10Z

@adrianchiris , @SchSeba please take another look

SchSeba · 2024-07-17T14:50:26Z

hack/env.sh

@@ -41,3 +41,5 @@ export DEV_MODE=${DEV_MODE:-"FALSE"}
 export OPERATOR_LEADER_ELECTION_ENABLE=${OPERATOR_LEADER_ELECTION_ENABLE:-"false"}
 export METRICS_EXPORTER_SECRET_NAME=${METRICS_EXPORTER_SECRET_NAME:-"metrics-exporter-cert"}
 export METRICS_EXPORTER_PORT=${METRICS_EXPORTER_PORT:-"9110"}
+export METRICS_EXPORTER_PROMETHEUS_OPERATOR_SERVICE_ACCOUNT=${METRICS_EXPORTER_PROMETHEUS_OPERATOR_SERVICE_ACCOUNT:-"prometheus-k8s"}


I think we need to override this one in the openshift CI file no?

ok, moving there

zeeke · 2024-07-18T12:57:33Z

I got rid of the CustomResourceDefinition clusterrole access and now the installation of the Prometheus operator is inferred by the presence of the METRICS_EXPORTER_PROMETHEUS_OPERATOR_NAMESPACE environment variable.

@adrianchiris @SchSeba please take another look

SchSeba

Great work! much cleaner now thanks for working on this!

hack/run-e2e-conformance-virtual-ocp.sh

deployment/sriov-network-operator-chart/README.md

deployment/sriov-network-operator-chart/values.yaml

controllers/sriovoperatorconfig_controller.go

deployment/sriov-network-operator-chart/values.yaml

Package `github.com/prometheus-operator/prometheus-operator/pkg/client` can be used for testing purpose. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

Deploy the needed configuration to make the prometheus operator to find and scrape the sriov-network-metrics-exporter endpoints, including the ServiceMonitor, Role and RoleBinding. Resources are installed only if the Prometheus operator is installed. When useing `ServiceMonitors`, Prometheus Operator needs permissions to read Services,Endpoint and Pods in the monitored namespace (i.e. the SRIOV operator ns). Make the ServiceAccount subject configurable via environment variables. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

SchSeba

just a small nit

SchSeba · 2024-07-30T16:52:25Z

controllers/suite_test.go

@@ -137,6 +138,10 @@ var _ = BeforeSuite(func() {
 	Expect(err).NotTo(HaveOccurred())
 	err = os.Setenv("METRICS_EXPORTER_KUBE_RBAC_PROXY_IMAGE", "mock-image")
 	Expect(err).NotTo(HaveOccurred())
+	err = os.Setenv("METRICS_EXPORTER_PROMETHEUS_OPERATOR_SERVICE_ACCOUNT", "k8s-prometheus")


do you need the new variable here?

no, I set it on the fly here:
https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/687/files#diff-35c949584634dae6fb47d82556f92d648986892e843a64d1eb9bd294375a61dcR370

So I can test both values

hack/run-e2e-conformance-virtual-ocp.sh

adrianchiris

LGtM!

zeeke changed the title ~~[metrics 2/3] Configure Prometheus Operator~~ [metrics 2/x] Configure Prometheus Operator Apr 23, 2024

zeeke force-pushed the metrics-exporter-prometheus branch from 06e880c to 75e8305 Compare April 24, 2024 15:51

zeeke force-pushed the metrics-exporter-prometheus branch from 75e8305 to 06c86e1 Compare April 24, 2024 16:51

zeeke force-pushed the metrics-exporter-prometheus branch from 06c86e1 to 37fbdf4 Compare April 24, 2024 17:06

zeeke force-pushed the metrics-exporter-prometheus branch from 37fbdf4 to 7999f7e Compare May 17, 2024 16:31

zeeke force-pushed the metrics-exporter-prometheus branch from 7999f7e to 58b9fb3 Compare May 17, 2024 16:53

SchSeba reviewed May 22, 2024

View reviewed changes

zeeke mentioned this pull request May 22, 2024

[metrics 3/x] end-to-end tests #702

Merged

zeeke force-pushed the metrics-exporter-prometheus branch from 58b9fb3 to 9b5e4cf Compare May 28, 2024 07:22

zeeke force-pushed the metrics-exporter-prometheus branch from 9b5e4cf to 2bddf42 Compare May 28, 2024 08:10

zeeke force-pushed the metrics-exporter-prometheus branch from 2bddf42 to cec70bd Compare May 28, 2024 08:12

zeeke force-pushed the metrics-exporter-prometheus branch from cec70bd to 6da499a Compare May 28, 2024 08:16

zeeke force-pushed the metrics-exporter-prometheus branch from 6da499a to e87a82d Compare May 28, 2024 09:29

SchSeba reviewed May 29, 2024

View reviewed changes

SchSeba reviewed Jun 3, 2024

View reviewed changes

zeeke force-pushed the metrics-exporter-prometheus branch from e87a82d to 17dabd0 Compare June 27, 2024 09:33

zeeke force-pushed the metrics-exporter-prometheus branch from 7aacaae to 2faab1d Compare July 3, 2024 15:34

zeeke force-pushed the metrics-exporter-prometheus branch from 2faab1d to 19e5be7 Compare July 3, 2024 16:04

zeeke removed the hold label Jul 5, 2024

zeeke force-pushed the metrics-exporter-prometheus branch from 19e5be7 to 41106a5 Compare July 5, 2024 08:47

zeeke mentioned this pull request Jul 5, 2024

CNF-12007: d/s: CSV adjustment for sriov-network-metrics-exporter openshift/sriov-network-operator#951

Closed

SchSeba reviewed Jul 11, 2024

View reviewed changes

zeeke force-pushed the metrics-exporter-prometheus branch from a39a440 to 108ef15 Compare July 12, 2024 06:55

SchSeba reviewed Jul 17, 2024

View reviewed changes

zeeke force-pushed the metrics-exporter-prometheus branch 2 times, most recently from 19cc04e to 83801a5 Compare July 17, 2024 17:19

SchSeba approved these changes Jul 22, 2024

View reviewed changes

adrianchiris reviewed Jul 22, 2024

View reviewed changes

zeeke force-pushed the metrics-exporter-prometheus branch from 83801a5 to 4af0a6a Compare July 29, 2024 07:31

zeeke added 2 commits July 29, 2024 09:33

Add prometheus-operator/pkg/client dependency

7d30a1c

Package `github.com/prometheus-operator/prometheus-operator/pkg/client` can be used for testing purpose. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

zeeke force-pushed the metrics-exporter-prometheus branch from 4af0a6a to 3dff029 Compare July 29, 2024 07:34

zeeke requested review from adrianchiris and SchSeba July 30, 2024 16:34

SchSeba approved these changes Jul 30, 2024

View reviewed changes

adrianchiris approved these changes Jul 31, 2024

View reviewed changes

adrianchiris merged commit 57e1e90 into k8snetworkplumbingwg:master Jul 31, 2024
13 checks passed

@@ @@ -32,6 +32,7 @@ rules: @@
                 verbs:
                 - get
                 - create
+                - list

[metrics 2/x] Configure Prometheus Operator #687

[metrics 2/x] Configure Prometheus Operator #687

Conversation

zeeke commented Apr 23, 2024 • edited Loading

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

coveralls commented Apr 24, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9903758109

Details

💛 - Coveralls

github-actions bot commented Apr 24, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 28, 2024

github-actions bot commented May 28, 2024

github-actions bot commented May 28, 2024

github-actions bot commented May 28, 2024

github-actions bot commented May 28, 2024

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

adrianchiris commented Jun 25, 2024

github-actions bot commented Jun 27, 2024

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 5, 2024

zeeke commented Jul 5, 2024

SchSeba 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

github-actions bot commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

zeeke commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeke commented Jul 18, 2024

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

zeeke commented Apr 23, 2024 •

edited

Loading

coveralls commented Apr 24, 2024 •

edited

Loading