Move rancherServiceMetrics into main chart #1786
Conversation
aiyengar2
force-pushed
the
move_rancher_service_metrics_to_main_chart
branch
3 times, most recently
from
f60ad33
to
1b17051
Compare
packages/rancher-monitoring/rancher-monitoring/generated-changes/patch/values.yaml.patch
Outdated
Show resolved
Hide resolved
| {{- define "rancher.serviceMonitor.namespaceSelector" -}} | ||
| {{- if .Values.rancherMonitoring.namespaceSelector }} | ||
| {{ .Values.rancherMonitoring.namespaceSelector | toYaml }} | ||
| {{- else }} | ||
| matchNames: | ||
| - cattle-system | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
- Uses the provided selector
- If nil, returns default
There was a problem hiding this comment.
Don't think rancher service will ever not be in cattle-system or it's reasonable to assume it won't be but I suppose this doesn't hurt anything.
There was a problem hiding this comment.
Also, why not just set cattle-system as the default in values.yaml?
There was a problem hiding this comment.
Just a choice of preference. I wanted to keep both fields in the helpers.tpl and set them the same way to be consistent.
There was a problem hiding this comment.
I’d prefer to leave it this way unless there’s a strong reason to have it in the values.yaml instead?
There was a problem hiding this comment.
Modified the chart to put the default in the values.yaml and drop the helper template for this.
...100.1.1+up19.0.3/templates/rancher-monitoring/dashboards/rancher/performance-dashboards.yaml
Show resolved
Hide resolved
...nitoring/100.1.1+up19.0.3/templates/rancher-monitoring/exporters/rancher/servicemonitor.yaml
Show resolved
Hide resolved
...nitoring/100.1.1+up19.0.3/templates/rancher-monitoring/exporters/rancher/servicemonitor.yaml
Show resolved
Hide resolved
|
Tested deploying this chart on a management and downstream cluster and, as expected, it only deploys performance metrics when Rancher is seen. I also tested modifying the namespaceSelector and selector on a |
| {{- define "rancher.serviceMonitor.namespaceSelector" -}} | ||
| {{- if .Values.rancherMonitoring.namespaceSelector }} | ||
| {{ .Values.rancherMonitoring.namespaceSelector | toYaml }} | ||
| {{- else }} | ||
| matchNames: | ||
| - cattle-system | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Don't think rancher service will ever not be in cattle-system or it's reasonable to assume it won't be but I suppose this doesn't hurt anything.
| {{- define "rancher.serviceMonitor.namespaceSelector" -}} | ||
| {{- if .Values.rancherMonitoring.namespaceSelector }} | ||
| {{ .Values.rancherMonitoring.namespaceSelector | toYaml }} | ||
| {{- else }} | ||
| matchNames: | ||
| - cattle-system | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Also, why not just set cattle-system as the default in values.yaml?
aiyengar2
force-pushed
the
move_rancher_service_metrics_to_main_chart
branch
from
1b17051
to
fd6aee7
Compare
How is this related to the original issue?
The issue filed calls out that there is a discrepancy in that the ServiceMonitor is only deployed if the chartLabel is set whereas the dashboard is deployed only when rancherServiceMetrics is enabled; what was seen is that if the chartLabel is found but rancherServiceMetrics is not enabled, the servicemonitor is deployed (due to the issue called out inhttps://github.com/rancher/charts-build-scripts/issues/68, where the
enabledflag is ignored for local charts) but the dashboard is not.To keep things consistent and to avoid having a whole dependency chart just for one resource (when the other resource is in the main chart anyways), this PR merges the subchart into the main chart and makes various enhancements w.r.t. what values the user can provide to configure the chart).
Breaking changes are not a concern since this chart has never been released.
Related Issue: rancher/rancher#36770