Add support for optional node selector labels for kube-ovn OVS/OVN pods #5804
Conversation
This change introduces a new Helm value ovsNodesLabels that allows adding custom node selector labels to ovs-ovn pods. For example: ovsNodesLabels: kube-ovn/role: ovs This is useful in environments such as OpenStack-Helm, where operators may want to schedule OVS/OVN pods only on specific nodes (e.g., compute nodes) instead of deploying them cluster-wide. When ovsNodesLabels is empty, only the default selector kubernetes.io/os: linux is applied. Signed-off-by: Saeed Padari <[email protected]>
dosubot
bot
added
the
size:S
Summary of ChangesHello @saeed-mcu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Helm value ovsNodesLabels to allow adding custom node selector labels to the ovs-ovn, kube-ovn-cni, and kube-ovn-pinger pods. While the intent is good, the implementation in the Helm templates for the daemonsets has issues with whitespace control that can lead to invalid YAML output. I've provided suggestions to use a more robust and idiomatic approach using the merge function to combine the default and custom node selectors. Additionally, I've suggested a small change in values.yaml to improve clarity for users.
| kubernetes.io/os: "linux" | ||
| {{- with .Values.ovsNodesLabels }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
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 }}| kubernetes.io/os: "linux" | ||
| {{- with .Values.ovsNodesLabels }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
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 }}| kubernetes.io/os: "linux" | ||
| {{- with .Values.ovsNodesLabels }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
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 }}| ovsNodesLabels: {} | ||
| # kube-ovn/role: ovs |
There was a problem hiding this comment.
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
|
Shouldn't those labels be added to every daemon deployed and not just the ones you put? For example, the controller? If not, there's an ovsOvn section in the chart where that value could probably fit better. Renaming it to "nodeSelector" could also help make its goal clearer. |
2 participants
This change introduces a new Helm value ovsNodesLabels that allows adding custom node selector labels to ovs-ovn pods. For example:
ovsNodesLabels:
kube-ovn/role: ovs
This is useful in environments such as OpenStack-Helm, where operators may want to schedule OVS/OVN pods only on specific nodes (e.g., compute nodes) instead of deploying them cluster-wide. When ovsNodesLabels is empty, only the default selector kubernetes.io/os: linux is applied.