Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add tolerations and affinity in the global values #476

Merged
merged 2 commits into from
Jul 11, 2024
Merged

feat: add tolerations and affinity in the global values #476

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3ec4d0
feat: global affinity and tolerations configuration.
jvanz Jul 3, 2024
1d8e7bd
fix: nodeSelector in PodSpec configuration.
jvanz Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions charts/kubewarden-controller/templates/audit-scanner.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ spec:
items:
- key: policy-server-root-ca-pem
path: "policy-server-root-ca-pem"
{{- if .Values.global.affinity }}
affinity: {{ .Values.global.affinity | toYaml | nindent 14 }}
{{- end }}
{{- if .Values.global.tolerations }}
tolerations: {{ .Values.global.tolerations | toYaml | nindent 14 }}
{{- end }}
containers:
- name: audit-scanner
image: '{{ template "system_default_registry" . }}{{ .Values.auditScanner.image.repository }}:{{ .Values.auditScanner.image.tag }}'
Expand All @@ -53,14 +59,6 @@ spec:
nodeSelector:
{{- toYaml . | nindent 14 }}
{{- end }}
jvanz marked this conversation as resolved.
Show resolved Hide resolved
{{- with .Values.affinity }}
affinity:
{{- toYaml . | nindent 14 }}
{{- end }}
{{- with .Values.tolerations }}
tolerations:
{{- toYaml . | nindent 14 }}
{{- end }}
{{- if and .Values.resources .Values.resources.auditScanner }}
resources:
{{ toYaml .Values.resources.auditScanner | indent 14 }}
Expand Down
14 changes: 6 additions & 8 deletions charts/kubewarden-controller/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ spec:
{{- include "imagePullSecrets" .Values.imagePullSecrets | nindent 8 }}
{{- end }}
serviceAccountName: {{ include "kubewarden-controller.serviceAccountName" . }}
{{- if .Values.global.affinity }}
affinity: {{ .Values.global.affinity | toYaml | nindent 8 }}
{{- end }}
{{- if .Values.global.tolerations }}
tolerations: {{ .Values.global.tolerations | toYaml | nindent 8 }}
{{- end }}
containers:
- name: manager
args:
Expand Down Expand Up @@ -97,11 +103,3 @@ spec:
nodeSelector:
{{- toYaml . | nindent 8 }}
{{- end }}
flavio marked this conversation as resolved.
Show resolved Hide resolved
{{- with .Values.affinity }}
affinity:
{{- toYaml . | nindent 8 }}
{{- end }}
{{- with .Values.tolerations }}
tolerations:
{{- toYaml . | nindent 8 }}
{{- end }}
57 changes: 55 additions & 2 deletions charts/kubewarden-controller/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,61 @@
# by more than one chart and they ideally need to match during the
# installation of the charts consuming this values.
global:
# affinity:
# podAffinity:
# requiredDuringSchedulingIgnoredDuringExecution:
# - labelSelector:
# matchExpressions:
# - key: security
# operator: In
# values:
# - S1
# topologyKey: topology.kubernetes.io/zone
# podAntiAffinity:
# preferredDuringSchedulingIgnoredDuringExecution:
# - weight: 100
# podAffinityTerm:
# labelSelector:
# matchExpressions:
# - key: security
# operator: In
# values:
# - S2
# topologyKey: topology.kubernetes.io/zone
# nodeAffinity:
# requiredDuringSchedulingIgnoredDuringExecution:
# nodeSelectorTerms:
# - matchExpressions:
# - key: kubernetes.io/os
# operator: In
# values:
# - linux
# preferredDuringSchedulingIgnoredDuringExecution:
# - weight: 1
# preference:
# matchExpressions:
# - key: label-1
# operator: In
# values:
# - key-1
# - weight: 50
# preference:
# matchExpressions:
# - key: label-2
# operator: In
# values:
# - key-2
affinity: {}
# tolerations:
# - key: "key1"
# operator: "Equal"
# value: "value1"
# effect: "NoSchedule"
# - key: "key1"
# operator: "Equal"
# value: "value1"
# effect: "NoExecute"
tolerations: []
cattle:
systemDefaultRegistry: ghcr.io
skipNamespaces:
Expand Down Expand Up @@ -111,8 +166,6 @@ preDeleteJob:
# kubewarden-controller deployment settings:
podAnnotations: {}
nodeSelector: {}
tolerations: []
affinity: {}
tls:
# source options:
# - "cert-manager-self-signed": Scaffold cert-manager integration, and create
Expand Down
1 change: 0 additions & 1 deletion charts/kubewarden-defaults/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,3 @@ namespaceSelector:
{{- printf "%s/" .Values.global.cattle.systemDefaultRegistry -}}
{{- end -}}
{{- end -}}

7 changes: 5 additions & 2 deletions charts/kubewarden-defaults/templates/policyserver-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@ spec:
{{- if .Values.policyServer.maxUnavailable }}
maxUnavailable: {{ .Values.policyServer.maxUnavailable }}
{{- end }}
{{- if .Values.policyServer.affinity }}
affinity: {{ .Values.policyServer.affinity | toYaml | nindent 4 }}
{{- if .Values.global.affinity }}
affinity: {{ .Values.global.affinity | toYaml | nindent 4 }}
{{- end }}
{{- if .Values.global.tolerations }}
tolerations: {{ .Values.global.tolerations | toYaml | nindent 4 }}
{{- end }}
{{- if .Values.policyServer.limits }}
limits: {{ .Values.policyServer.limits | toYaml | nindent 4 }}
Expand Down
57 changes: 55 additions & 2 deletions charts/kubewarden-defaults/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,61 @@
# by more than one chart and they ideally need to match during the
# installation of the charts consuming this values.
global:
# affinity:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that instead of having the repetition on this commented configuration, this should go to common-values.yaml, and be checked that way. It may be needed to edit the make check-common-values as needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the examples in the charts values because that is sent in the helm chart bundle. Which helps users to get examples of how to configure the helm charts. For example, Artifacthub allow user to visualize the default values. If we add the comments in the common-values.yaml the users will not see an example of how to use the fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that, but the make check-common-values CI job would have failed. I would also add them into the common-values.yaml, and if the make works, it will check that they are all in sync at least.

Copy link
Member Author

@jvanz jvanz Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI failed for another reason. The CI is failing because the script is using a CLI flag not available in the yq installed in the github runner. There is a yq implementation in go lang and another one using python (the one with the required CLI flag). The script is considering one yq but the runner has another installed. Which misses the '--sort-keys' CLI flag. We need to update the script to remove the need of --sort-keys or install the right yq version.

The CI is failing for a while now. If you check the CI history they are green. But if you read the logs, the script failed with the same problem. I've opened another issue to attack this problem. It seems out of scope of this PR. I've run the make check-common-values locally and it's passing. I have the python yq installed and it does not consider comment when comparing the values:

 ~/SUSE/helm-charts │ issue756 !1 ?1  make check-common-values                                                                                                                                                                ✔ │ 11:46:13 
 ~/SUSE/helm-charts │ issue756 !1 ?1                                                                                                                                                                                          ✔ │ 14:09:58

Copy link
Member

@viccuad viccuad Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the python yq installed and it does not consider comment when comparing the values

fair enough then, approving.

# podAffinity:
# requiredDuringSchedulingIgnoredDuringExecution:
# - labelSelector:
# matchExpressions:
# - key: security
# operator: In
# values:
# - S1
# topologyKey: topology.kubernetes.io/zone
# podAntiAffinity:
# preferredDuringSchedulingIgnoredDuringExecution:
# - weight: 100
# podAffinityTerm:
# labelSelector:
# matchExpressions:
# - key: security
# operator: In
# values:
# - S2
# topologyKey: topology.kubernetes.io/zone
# nodeAffinity:
# requiredDuringSchedulingIgnoredDuringExecution:
# nodeSelectorTerms:
# - matchExpressions:
# - key: kubernetes.io/os
# operator: In
# values:
# - linux
# preferredDuringSchedulingIgnoredDuringExecution:
# - weight: 1
# preference:
# matchExpressions:
# - key: label-1
# operator: In
# values:
# - key-1
# - weight: 50
# preference:
# matchExpressions:
# - key: label-2
# operator: In
# values:
# - key-2
affinity: {}
# tolerations:
# - key: "key1"
# operator: "Equal"
# value: "value1"
# effect: "NoSchedule"
# - key: "key1"
# operator: "Equal"
# value: "value1"
# effect: "NoExecute"
tolerations: []
cattle:
systemDefaultRegistry: ghcr.io
skipNamespaces:
Expand Down Expand Up @@ -113,8 +168,6 @@ policyServer:
# certs:
# - "cert4"
sourceAuthorities: {}
# affinity for pods of the default PolicyServer
affinity: {}
# limits and requests, see https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
limits: {}
requests: {}
Expand Down
4 changes: 4 additions & 0 deletions common-values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# by more than one chart and they ideally need to match during the
# installation of the charts consuming this values.
global:
affinity: {}
tolerations: []
cattle:
systemDefaultRegistry: ghcr.io
skipNamespaces:
Expand Down Expand Up @@ -47,3 +49,5 @@ global:
default:
name: default
enabled: true
affinity: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(opening thread randomly here)

It seems that the CI job for make check-common-values is not working, possibly because we are using the wrong yq dependency (see here).

It would have catched the discrepancy with common-values.yaml and each of the other values.yaml.

Copy link
Member Author

@jvanz jvanz Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was taking a look in the CI history. That's failing for a while already. =(

I guess the ubuntu runner is running the go lang version. But we are using the python command in the script

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opening an issue for this bug. Just to avoid mixing context here.

tolerations: []
Loading