Adding felix service metric port

[vikastigera]

Description

Changes done to add felix service metric
port in calico-node service. This helps in
removing the manual step required by the client
to enable felix metric for BYO prometheus.

https://tigera.atlassian.net/browse/EV-5305

*** Additional changes required to update felix-metrics-service-monitor.yaml to this :

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  labels:
    team: network-operators
  name: felix-metrics
spec:
  endpoints:
    - honorLabels: true
      interval: 5s
      port: felix-metrics-port
      scheme: http
      scrapeTimeout: 5s
  namespaceSelector:
    matchNames:
      - calico-system
  selector:
    matchLabels:
      k8s-app: calico-node

Testing

For PR author

• Tests for change.
• If changing pkg/apis/, run make gen-files
• If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

• Milestone set according to targeted release.
• Appropriate labels:
  • kind/bug if this is a bugfix.
  • kind/enhancement if this is a a new feature.
  • enterprise if this PR applies to Calico Enterprise only.

[tmjd]

There is a PrometheusMetricsPort in FelixConfig that should be used instead of always using a built-in default value.

[rene-dekker]

@tmjd
There is a PrometheusMetricsPort in FelixConfig that should be used instead of always using a built-in default value.

There is also in migration/core.go a migration that sets installation.spec.NodeMetricsPort. Should we perhaps:

• In the core_controller verify installation.spec.NodeMetricsPort and store it in a new variable "nodeMetricsPort"
• If variable does not exist, use *felixConfiguration.Spec.PrometheusMetricsPort if it exists
• pass add new variable to the NodeConfiguration object
• In node.go, if NodeConfiguration.NodeMetricsPort is not nil, we add it to the service.

[tmjd]

Was felixConfiguration.Spec.PrometheusMetricsPort a new field that has been added? I'm wondering if that field was needed if we already had NodeMetricsPort in the operator.

If both NodeMetricsPort and PrometheusMetricsPort are set and are different should the operator report that as a problem?

[vikastigera]

Was felixConfiguration.Spec.PrometheusMetricsPort a new field that has been added? I'm wondering if that field was needed if we already had NodeMetricsPort in the operator.

If both NodeMetricsPort and PrometheusMetricsPort are set and are different should the operator report that as a problem?

PrometheusMetricsPort is not a new field.
I have updated the code to use NodeMetricsPort and in case it is nil it will use PrometheusMetricsPort or default value. (

operator/api/v1/installation_types.go

Line 119 in ea0f4fb

// If specified, this overrides any FelixConfiguration resources which may exist. If omitted, then

)

[tmjd]

So I guess one thing I'm unclear of, should the installationl.NodeMetricsPort and felixConfig.PrometheusMetricsPort be configuring the same thing or are they different?
I think the answer is they are different and also I think I was wrong originally mentioning NodeMetricsPort since it is used to configure calico-metrics-port but PrometheusMetricsPort is used for felix-metrics-port. For some reason I was thinking they were for the same thing but I think clearly they are not.

[vikastigera]

@tmjd
As per the documentation and code :
"NodeMetricsPort specifies which port calico/node serves prometheus metrics on. By default, metrics are not enabled.If specified, this overrides any FelixConfiguration resources which may exist. If omitted, then prometheus metrics may still be configured through FelixConfiguration."

Changes I have done is specific to enabling Felix metric for "Bring your own Prometheus" use case (https://docs.tigera.io/calico-enterprise/latest/operations/monitor/prometheus/byo-prometheus#scrape-metrics-from-specific-components).
Also, calico-metrics-port and felix-metrics-port are for different purpose.

[tmjd]

Since the two metrics ports are unrelated, other than providing some metrics information. I don't think there is any reason we should use NodeMetricsPort here.
If this is needed then I'd suggest this logic should be moved to the core_controller.

[vikastigera]

updated to only use PrometheusMetricsPort

[tmjd]

LGTM as far as operator API and operator code quality. Was there a review that this is appropriate for Calico/Enterprise, for example should the felix metrics port be exposed on the same service as the node metrics or should there be a separate service?

[vikastigera]

LGTM as far as operator API and operator code quality. Was there a review that this is appropriate for Calico/Enterprise, for example should the felix metrics port be exposed on the same service as the node metrics or should there be a separate service?

Rene and I had discussion to add this in same service as we are using using "calico-node" in the selector. Also we discussed that ServiceMonitor also needs to be updated, I have updated the code for that.

[tmjd]

@rene-dekker did you want to review this then to make sure it follows what you discussed with @vikastigera ?

[rene-dekker]

Is there a reason that we log and move on as opposed to degrading and returning? Or would that create some sort of deadlock with the core controller?

[tmjd]

Good question, I wouldn't expect it to be problematic.

[vikastigera]

Updated

[rene-dekker]

Was there a review that this is appropriate for Calico/Enterprise, for example should the felix metrics port be exposed on the same service as the node metrics or should there be a separate service?

I think the vision we have around scraping is that we prefer users to get the metrics by scraping our prometheus and have our prometheus scrape node/felix. Users are struggling with the mTLS configuration and by leveraging the ExternalPrometheus option that could be simplified. With that in mind, I don't see any drawback from this, unless you can think of any reason.

[tmjd]

Was there a review that this is appropriate for Calico/Enterprise, for example should the felix metrics port be exposed on the same service as the node metrics or should there be a separate service?

I think the vision we have around scraping is that we prefer users to get the metrics by scraping our prometheus and have our prometheus scrape node/felix. Users are struggling with the mTLS configuration and by leveraging the ExternalPrometheus option that could be simplified. With that in mind, I don't see any drawback from this, unless you can think of any reason.

I wasn't trying to suggest a problem with it, I just wanted to make sure someone had reviewed this change from the aspect of is this the right solution for Enterprise and addressed the problem it was targeting.

[rene-dekker]

lgtm