Skip to content

Add support for optional node selector labels for kube-ovn OVS/OVN pods #5804

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for optional node selector labels for kube-ovn OVS/OVN pods #5804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions charts/kube-ovn-v2/templates/agent/agent-daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@ spec:
{{- end }}
nodeSelector:
kubernetes.io/os: "linux"
{{- with .Values.ovsNodesLabels }}
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines 233 to +236
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current templating for adding ovsNodesLabels can lead to incorrect YAML due to issues with whitespace control (- in {{- ... }}) and the indentation of the template directives. A more robust and idiomatic approach in Helm is to use the merge function to combine the default nodeSelector labels with the custom ones provided in Values.ovsNodesLabels. This ensures correct formatting and is easier to read and maintain.

{{- toYaml (merge .Values.ovsNodesLabels (dict "kubernetes.io/os" "linux")) | nindent 8 }}

volumes:
- name: usr-local-sbin
emptyDir: {}
Expand Down
3 changes: 3 additions & 0 deletions charts/kube-ovn-v2/templates/ovs-ovn/ovs-ovn-daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ spec:
{{- end }}
nodeSelector:
kubernetes.io/os: "linux"
{{- with .Values.ovsNodesLabels }}
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines 182 to +185
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current templating for adding ovsNodesLabels can lead to incorrect YAML due to issues with whitespace control (- in {{- ... }}) and the indentation of the template directives. A more robust and idiomatic approach in Helm is to use the merge function to combine the default nodeSelector labels with the custom ones provided in Values.ovsNodesLabels. This ensures correct formatting and is easier to read and maintain.

{{- toYaml (merge .Values.ovsNodesLabels (dict "kubernetes.io/os" "linux")) | nindent 8 }}

volumes:
- name: usr-local-sbin
emptyDir: {}
Expand Down
3 changes: 3 additions & 0 deletions charts/kube-ovn-v2/templates/pinger/pinger-daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ spec:
periodSeconds: 10
nodeSelector:
kubernetes.io/os: "linux"
{{- with .Values.ovsNodesLabels }}
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines 158 to +161
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current templating for adding ovsNodesLabels can lead to incorrect YAML due to issues with whitespace control (- in {{- ... }}) and the indentation of the template directives. A more robust and idiomatic approach in Helm is to use the merge function to combine the default nodeSelector labels with the custom ones provided in Values.ovsNodesLabels. This ensures correct formatting and is easier to read and maintain.

{{- toYaml (merge .Values.ovsNodesLabels (dict "kubernetes.io/os" "linux")) | nindent 8 }}

volumes:
- name: host-run-ovs
hostPath:
Expand Down
7 changes: 7 additions & 0 deletions charts/kube-ovn-v2/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ masterNodesLabels:
kube-ovn/role: master
# node-role.kubernetes.io/control-plane: ""

# -- Additional node selector labels for the OVS/OVN pods.
# This allows scheduling pods like ovs-ovn, kube-ovn-cni, and kube-ovn-pinger on specific nodes.
# See https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#nodeselector
# @section -- Global parameters
ovsNodesLabels: {}
# kube-ovn/role: ovs
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The indentation of the commented-out example for ovsNodesLabels is misleading. It suggests it's a child of a mapping, but ovsNodesLabels is defined as an empty map on a single line. This can be confusing for users trying to enable this feature. Adjusting the comment's indentation will improve clarity.

ovsNodesLabels: {}
#  kube-ovn/role: ovs


# -- General configuration of the network created by Kube-OVN.
# @section -- Network parameters of the CNI
# @default -- "{}"
Expand Down
Loading