pkg/metrics: Allow multi port metrics Service creation

[lilic]

Motivation for the change:

With the recent addition of Custom Resource metrics we need to expose the port in the metrics Service. This also helps users who want to expose their own metrics ports in a Service as they can just pass in another port.

Note: We also changed the name of the Service that is being created by appending -metrics to the operator name, this makes it self descriptive as we did have questions what is this service for.

TODO:

• adjust docs to match the changes to the functions

[shawn-hurley]

I think this is good from a code prespective. I just have 1 question about the MetricsService type.

WDYT of just using the ServicePort type there?

[lilic]

@shawn-hurley

WDYT of just using the ServicePort type there?

My reasoning behind is to make it easy for users to create the Services, I think those that know how to fill in the ServicePort (e.g. the TargetPort, NodePort, etc.) can create the Service themselves. This is meant to serve users that want to expose additional metrics endpoints, we had a bunch of requests for that, and our metrics endpoints. But am opening to that idea if everyone else thinks so, of course!

[joelanford]

I would tend to agree with Shawn that the MetricsService type is probably unnecessary. Here's what it would look like using ServicePorts directly:

	// Add to the below struct any other metrics ports you want to expose.
	servicePorts := []corev1.ServicePort{
		{ Name: metrics.CRPortName, TargetPort: intstr.FromInt(int(metricsPort))},
	}

	// Create Service object to expose the metrics port(s).
	_, err = metrics.CreateMetricsService(ctx, servicePorts)

To me, that seems simple enough for new users and also slightly more useful for users looking to do something slightly more custom for their metrics service ports.

[lilic]

@joelanford @shawn-hurley I made the suggested change, the additional fields were needed otherwise Service cannot be created. PTAL.

[joelanford]

Can you update the CHANGELOG and mention the removed PrometheusPortName and ExposeMetricsPort identifiers, and the added OperatorPortName, CRPortName, and CreateMetricsService identifiers?

@hasbro17 Is there a migration guide that needs to mention this change?

[joelanford]

This is a duplicate of line 62, so it can be removed.

[hasbro17]

@joelanford Still working on the migration guide but yes for v0.8.1->v0.9.x we'll need to note down these signature changes.
So long as they're marked as breaking changes in the CHANGELOG I'll be able to add them in later.

[lilic]

@hasbro17 @joelanford I added things to the CHANGELOG, please have a look at that, if the wording is clear enough, thanks!

[lilic]

/test marker

[lilic]

No idea why marker test is failing here, as the URLs work for me 🤔

[lilic]

/test marker

[joelanford]

Deprecated means that the thing still exists and will be removed in a future version, right? In this PR, we're actually removing it without deprecating it.

So that leads me to a question: should we actually deprecate ExposeMetricsPort and leave it in place for now?

[shawn-hurley]

we are pre 1.0 I think it is ok to deprecate and replace in a single release IMO. WDYT?

[lilic]

I think we said we are making breaking changes until the 1.0 release yes.

[lilic]

Maybe we could make it more in bold in the CHANGELOG? Also with the migration doc coming up that you mention @joelanford it should be easy for people to migrate IMO.

[joelanford]

Yeah, I think it's fine to remove it. Just wanted to make sure we were all in agreement to do that. I would suggest:

• Prefixing with **Breaking change**: like we've done before
• s/deprecated/removed/ to make it clear the ExposeMetricsPort is no longer present.

WDYT?

[lilic]

sgtm, done!

[shawn-hurley]

/retest

[joelanford]

/approve