Rework labels, deprecate old compatibility switches #150
Conversation
Formerly, the labels of all manifests were all defined decentrally and containing some wrong entries. Now we have them unified, using the template in _helpers.yaml and concluding to the best practices of Helm
It makes no sense to have a service in such case, as these sidecar containers are there solely to show up the built-in "Zabbix server" host's interface availability to get green. This is done by using "localhost", which no Service is needed for. A Service indeed implements loadbalancing over all matched pods, which in this case would lead to connections being round-robined between Zabbix Agent containers on different Zabbix Server pods, if using Zabbix Server HA, which is senseless.
There is no such thing as a "ContainerLabel" or "ContainerAnnotation"...
| helm.sh/chart: {{ include "zabbix.chart" . }} | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| {{ include "zabbix.selectorLabels" . }} | ||
| {{- if .Chart.AppVersion }} | ||
| app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} | ||
| {{- end }} | ||
| app.kubernetes.io/managed-by: {{ .Release.Service }} |
There was a problem hiding this comment.
Hi @fibbs
Here, can we change app.kubernetes.io/managed-by: {{ .Release.Service }}by app.kubernetes.io/managed-by: Helm?
| {{- if .Values.zabbixServer.zabbixServerHA.enabled }} | ||
| {{ .Values.zabbixServer.zabbixServerHA.haLabelsSidecar.labelName }}: "active" | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
|
|
||
| {{- if and .Values.zabbixAgent.enabled .Values.zabbixAgent.runAsDaemonSet }} |
There was a problem hiding this comment.
Hi @fibbs!
Why the zabbix-agent service was removed from here?
| helm.sh/chart: {{ include "zabbix.chart" . }} | ||
| app.kubernetes.io/instance: {{ .Release.Name }}-zabbix-server | ||
| app.kubernetes.io/managed-by: {{ .Release.Service }}-zabbix-server | ||
| {{- include "zabbix.labels" . | nindent 4 }} |
There was a problem hiding this comment.
Hi @fibbs!
Is only this labbels sufficient to reference by zabbix-server and HA files?
| # -- Labels to add to the statefulset | ||
| statefulSetLabels: {} | ||
| extraStatefulSetLabels: {} | ||
| # -- Annotations to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Annotations to add to the containers | ||
| containerAnnotations: {} | ||
| extraPodAnnotations: {} | ||
| # -- Labels to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Labels to add to the statefulset | ||
| statefulSetLabels: {} | ||
| extraStatefulSetLabels: {} | ||
| # -- Annotations to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Annotations to add to the containers | ||
| containerAnnotations: {} | ||
| extraPodAnnotations: {} | ||
| # -- Labels to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Labels to add to the deployment | ||
| deploymentLabels: {} | ||
| extraDeploymentLabels: {} | ||
| # -- Annotations to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Annotations to add to the containers | ||
| containerAnnotations: {} | ||
| extraPodAnnotations: {} | ||
| # -- Labels to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Labels to add to the deployment | ||
| deploymentLabels: {} | ||
| extraDeploymentLabels: {} | ||
| # -- Annotations to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Annotations to add to the containers | ||
| containerAnnotations: {} | ||
| extraPodAnnotations: {} | ||
| # -- Labels to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Labels to add to the deployment | ||
| deploymentLabels: {} | ||
| extraDeploymentLabels: {} | ||
| # -- Annotations to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Annotations to add to the containers | ||
| containerAnnotations: {} | ||
| extraPodAnnotations: {} | ||
| # -- Labels to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Labels to add to the deployment | ||
| deploymentLabels: {} | ||
| extraDeploymentLabels: {} | ||
| # -- Annotations to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
| # -- Annotations to add to the containers | ||
| containerAnnotations: {} | ||
| extraPodAnnotations: {} | ||
| # -- Labels to add to the containers |
There was a problem hiding this comment.
Need change:
-containers
+pods
There was a problem hiding this comment.
Great work @fibbs!
Thanks for your time and contruibution. The helm chart is better now.
I made some comments to fix in this PR.
aeciopires
added
bug
What this PR does / why we need it:
Adjust labels of all manifests: Formerly, the labels of all manifests were all defined decentrally and containing some wrong entries. Now we have them unified, using the template in
_helpers.yamland concluding to the best practices of HelmThis is a breaking-breaking change
It will force you to re-install the Release, due to
selectorlabel matchers being immutable for Deployments, Statefulsets, ....Also I have removed the service for the Zabbix Agent in sidecar mode: tt makes no sense to have a service in such case, as these sidecar containers are there solely to show up the built-in "Zabbix server" host's interface availability to get green. This is done by using "localhost" as a Zabbix host interface address, which no Service is needed for.
A Service indeed implements loadbalancing over all matched pods, which in this case would lead to connections being round-robined between Zabbix Agent containers on different Zabbix Server pods, if using Zabbix Server HA, which is senseless.
I have also raised version of Zabbix to 7.0.8!
Some values in
values.yamlhave been changed: Their names were misleading: there is no such thing as a "ContainerLabel" or "ContainerAnnotation"... as the lowest level resource one can set labels on is the Pod.Last but not least, I have reworked the NOTES.txt to give better instructions on how to get access to Zabbix Web.
Which issue this PR fixes
(optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged)Actually, @aeciopires, I changed mind regarding version for the chart. After this PR, it is a very breaking change so that 7.0.0 would actually be the better new major version. I have already changed
README.md.gotmplaccordingly.Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]