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.