WINC-545: Improve handling of metrics configuration #2485

mansikulkarni96 · 2024-10-18T20:01:42Z

This PR adds a new metric controller that watches the WMCO namespace to create a service monitor when monitoring is enabled. This PR also removes the code where WMCO creates and manages endpoints object. An endpoint named kubelet in the kube-system namespace contains all the node IP addresses. WMCO can rely on this endpoint and create the serviceMonitor which will change the port from 10250 to 9182 which is our desired metric port for the windows-exporter service.

openshift-ci-robot · 2024-10-18T20:01:46Z

@mansikulkarni96: This pull request references WINC-545 which is a valid jira issue.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-10-18T20:01:46Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mansikulkarni96 · 2024-10-18T20:03:13Z

/test aws-e2e-operator

mansikulkarni96 · 2024-10-21T19:11:06Z

/approve cancel

openshift-ci · 2024-10-21T19:11:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sebsoto for approval. For more information see the Kubernetes Code Review Process.

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

mansikulkarni96 · 2024-10-22T04:39:55Z

/test aws-e2e-operator

mansikulkarni96 · 2024-10-24T17:59:18Z

/test aws-e2e-operator

openshift-ci-robot · 2024-10-24T18:04:51Z

@mansikulkarni96: This pull request references WINC-545 which is a valid jira issue.

In response to this:

This PR adds a new metric controller that watches the WMCO namespace to create a service monitor when monitoring is enabled. This PR also removes the code where WMCO creates and manages endpoints object. An endpoint named kubelet in the kube-system namespace contains all the node IP addresses. WMCO can rely on this endpoint and create the serviceMonitor which will change the port from 10250 to 9182 which is our desired metric port for the windows-exporter service.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

mansikulkarni96 · 2024-10-28T17:42:20Z

/test aws-e2e-operator

openshift-ci · 2024-10-28T21:14:57Z

@mansikulkarni96: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

sebsoto

Thanks @mansikulkarni96, its good to see the amount of code this removes 😄

Made an initial review, PTAL at my comments

sebsoto · 2024-10-31T13:17:28Z

bundle/manifests/windows-machine-config-operator.clusterserviceversion.yaml

@@ -211,6 +211,8 @@ spec:
          - namespaces
          verbs:
          - get
+          - list


Please squash this into the commit the kubebuilder directives are being added.
Else this will cause revert issues if one of the commits needs to be reverted

sebsoto · 2024-10-31T13:21:59Z

pkg/metrics/metrics.go

 )

 //+kubebuilder:rbac:groups="",resources=services;services/finalizers,verbs=create;get;delete
-//+kubebuilder:rbac:groups="",resources=endpoints,verbs=create;get;delete;update;patch


I dont see this change reflected in the bundle. Is another file still making endpoint API calls?

sebsoto · 2024-10-31T13:22:53Z

pkg/metrics/metrics.go

+	monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	monclient "github.com/prometheus-operator/prometheus-operator/pkg/client/versioned/typed/monitoring/v1"


Sort imports. This will fail lint otherwise

sebsoto · 2024-10-31T13:25:04Z

pkg/metrics/metrics.go

 			Endpoints: []monv1.Endpoint{
 				{
 					HonorLabels:     true,
 					Interval:        "30s",
 					Path:            "/metrics",
-					Port:            "metrics",
+					Port:            "https-metrics",


Does this have a functionality change?

sebsoto · 2024-10-31T13:28:55Z

pkg/metrics/metrics.go

 							TargetLabel: "instance",
 							SourceLabels: []monv1.LabelName{
 								"__meta_kubernetes_endpoint_address_target_name",
 							},
 						},
+						{


There is a lot of 'magic' happening here, requiring maintainers to be familiar with these actions and what they may be doing. Please add a short comment for each explaining what they are doing and why they need to be done, if it is not too obvious.

saifshaikh48 · 2024-11-05T22:36:59Z

pkg/metrics/metrics_test.go

this deletion would be better suited in the next commit

saifshaikh48 · 2024-11-05T22:37:56Z

pkg/metrics/metrics.go

 //+kubebuilder:rbac:groups="",resources=services;services/finalizers,verbs=create;get;delete
 //+kubebuilder:rbac:groups="",resources=namespaces,verbs=get
 //+kubebuilder:rbac:groups="",resources=nodes,verbs=list
 //+kubebuilder:rbac:groups="monitoring.coreos.com",resources=servicemonitors,verbs=create;get;delete
 //+kubebuilder:rbac:groups="",resources=events,verbs=*


can we move the ones that are still relevant to metric controller?

saifshaikh48 · 2024-11-05T22:39:10Z

pkg/metrics/metrics.go

 const (
-	// metricsPortName specifies the portname used for Prometheus monitoring
+	// PortName specifies the portname used for Prometheus monitoring
 	PortName = "metrics"
 	// Host is the host address used by Windows metrics
 	Host = "0.0.0.0"


is this used anywhere? if not we can delete

This commit removes the code responsible for creating and managing metric endpoints object. There is an endpoint named kubelet in the kube-system namespace which contains all the node IP addresses. WMCO can rely on this endpoint and create the serviceMonitor which will change the port from 10250 to 9182 which is our desired metric port for the windows-exporter service

This commit adds a new metricController that watches the WMCO namespace to create service monitor when monitoring is enabled. Add namespace resource list and watch rbac.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 18, 2024

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 Oct 18, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2024

mansikulkarni96 force-pushed the WINC-545 branch 3 times, most recently from 621433f to b151bf7 Compare October 21, 2024 19:09

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2024

mansikulkarni96 force-pushed the WINC-545 branch 5 times, most recently from 34e5f29 to ff5ff69 Compare October 24, 2024 17:58

mansikulkarni96 force-pushed the WINC-545 branch from ff5ff69 to 9e728d5 Compare October 24, 2024 18:09

sebsoto requested changes Oct 31, 2024

View reviewed changes

mansikulkarni96 force-pushed the WINC-545 branch 2 times, most recently from 884f8c9 to 375f4ec Compare November 4, 2024 21:00

saifshaikh48 reviewed Nov 5, 2024

View reviewed changes

mansikulkarni96 force-pushed the WINC-545 branch from 375f4ec to 9356a60 Compare November 6, 2024 15:53

mansikulkarni96 added 2 commits November 6, 2024 11:00

[controllers] Introduce metricController

25bf04c

This commit adds a new metricController that watches the WMCO namespace to create service monitor when monitoring is enabled. Add namespace resource list and watch rbac.

mansikulkarni96 force-pushed the WINC-545 branch from 9356a60 to 25bf04c Compare November 6, 2024 16:03

mansikulkarni96 requested review from saifshaikh48 and sebsoto November 6, 2024 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WINC-545: Improve handling of metrics configuration #2485

WINC-545: Improve handling of metrics configuration #2485

mansikulkarni96 commented Oct 18, 2024 •

edited

Loading

openshift-ci-robot commented Oct 18, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Oct 18, 2024

mansikulkarni96 commented Oct 18, 2024

mansikulkarni96 commented Oct 21, 2024

openshift-ci bot commented Oct 21, 2024

mansikulkarni96 commented Oct 22, 2024

mansikulkarni96 commented Oct 24, 2024

openshift-ci-robot commented Oct 24, 2024 •

edited by openshift-ci bot

Loading

mansikulkarni96 commented Oct 28, 2024

openshift-ci bot commented Oct 28, 2024

sebsoto left a comment

sebsoto Oct 31, 2024

sebsoto Oct 31, 2024

sebsoto Oct 31, 2024

sebsoto Oct 31, 2024

sebsoto Oct 31, 2024

saifshaikh48 Nov 5, 2024

saifshaikh48 Nov 5, 2024

saifshaikh48 Nov 5, 2024

		monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
		monclient "github.com/prometheus-operator/prometheus-operator/pkg/client/versioned/typed/monitoring/v1"

WINC-545: Improve handling of metrics configuration #2485

Are you sure you want to change the base?

WINC-545: Improve handling of metrics configuration #2485

Conversation

mansikulkarni96 commented Oct 18, 2024 • edited Loading

openshift-ci-robot commented Oct 18, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Oct 18, 2024

mansikulkarni96 commented Oct 18, 2024

mansikulkarni96 commented Oct 21, 2024

openshift-ci bot commented Oct 21, 2024

mansikulkarni96 commented Oct 22, 2024

mansikulkarni96 commented Oct 24, 2024

openshift-ci-robot commented Oct 24, 2024 • edited by openshift-ci bot Loading

mansikulkarni96 commented Oct 28, 2024

openshift-ci bot commented Oct 28, 2024

sebsoto 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

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

mansikulkarni96 commented Oct 18, 2024 •

edited

Loading

openshift-ci-robot commented Oct 18, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 24, 2024 •

edited by openshift-ci bot

Loading