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.

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

[occm] The nodeSelector key is not empty in the helm chart in release v1.29.0 #2550

Open
babykart opened this issue Feb 13, 2024 · 7 comments · May be fixed by #2586
Open

[occm] The nodeSelector key is not empty in the helm chart in release v1.29.0 #2550

babykart opened this issue Feb 13, 2024 · 7 comments · May be fixed by #2586
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@babykart
Copy link

babykart commented Feb 13, 2024

**This is a BUG REPORT **:

/kind bug

What happened:
Since the release v1.29.0, the nodeSelector key in the file values.yaml of the helm chart is no longer empty and therefore requires positioning the following label node-role.kubernetes.io/controlplane: "" on the workers where the daemonSet must be deployed.

What you expected to happen:
The nodeSelector key in the file values.yaml of the helm chart should be empty.

nodeSelector: {}

How to reproduce it:
Install occm v1.29.0 with the helm chart.

Environment:

  • openstack-cloud-controller-manager(or other related binary) version: v1.29.0

Let me know if you need a PR.
Regards.

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 13, 2024
@dulek
Copy link
Contributor

dulek commented Feb 29, 2024

This got introduced in #2346. @wwentland, would you care to comment here?

As a workaround, would just setting nodeSelector: {} in your values.yaml help here? Why would you think that no nodeSelector is a preference?

@babykart
Copy link
Author

babykart commented Mar 1, 2024

@dulek The workaround doesn't help if I need to add my own nodeSelector.
The fact that there is already a default value, helm will 'add' the two values and therefore in this case I will be forced to set the default value to the nodes as well besides mine.

For example, I want to use my.corp.selector/front: "true", the nodeSelector generated by helm would be:

<...>
    spec:
      nodeSelector:
        my.corp.selector/front: "true"
        node-role.kubernetes.io/control-plane: ""
<...>

@dulek
Copy link
Contributor

dulek commented Mar 18, 2024

@babykart:

I was able to do what you need with this values.yaml:

nodeSelector:
  node-role.kubernetes.io/control-plane: null
  my.corp.selector/front: "true"

This gets my helm install --dry-run to render this:

    spec:
      nodeSelector:
        my.corp.selector/front: "true"

Based on Helm Docs.

Can I close this issue now?

@babykart
Copy link
Author

@dulek thx. I didn't know about this feature of helm.
But should we therefore consider that this is the new behavior from version 1.29?

@dulek
Copy link
Contributor

dulek commented Mar 19, 2024

I think so. I believe @wwentland motivation to change this was to follow what AWS provider does and it makes sense to me: https://github.com/kubernetes/cloud-provider-aws/blob/master/charts/aws-cloud-controller-manager/values.yaml#L14-L16. Your use case is still valid, but since we've figured out how to override the default, I think we should keep the current 1.29 behavior.

@babykart
Copy link
Author

I admit I don't fully understand what the specific AWS implementation has to do with it.
I only deploy Kubernetes clusters in on-premise environments.
If it comes to dealing with this specific implementation in the helm chart, wouldn't it make more sense to add a specific block of documentation in the README.md?

@dulek
Copy link
Contributor

dulek commented Mar 26, 2024

I admit I don't fully understand what the specific AWS implementation has to do with it. I only deploy Kubernetes clusters in on-premise environments.

But in the end AWS and OpenStack K8s clusters shouldn't be too different. The idea is that cloud-provider-openstack being part of the control plane, lands on the control plane nodes. This should basically be true for any platform and any cloud-provider.

If it comes to dealing with this specific implementation in the helm chart, wouldn't it make more sense to add a specific block of documentation in the README.md?

Sure thing, docs are always welcome! Can you prepare the PR? I'll be happy to review and approve it.

@babykart babykart linked a pull request Apr 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants