NetApp / trident

Storage orchestrator for containers
Apache License 2.0
762 stars 222 forks source link

tridentControllerPluginNodeSelector/tridentNodePluginNodeSelector won't work with "true" value #899

Closed kmizumar closed 4 weeks ago

kmizumar commented 7 months ago

Describe the bug

Trying to use tridentControllerPluginNodeSelector and tridentNodePluginNodeSelector with helm chart setup providing the following values per yaml file would fail:

tridentControllerPluginNodeSelector:
  node-role.kubernetes.io/worker: "true"

tridentNodePluginNodeSelector:
  node-role.kubernetes.io/worker: "true"

This bug is exactly the same one which is reported in new nodeSelectors are not working when label value contains "true".

According to this update https://github.com/NetApp/trident/issues/700#issuecomment-1195891827, it's fixed in Trident 22.07 release, but some users reported it still exists in 23.04.0, 23.07.0, 23.10, and I hit this bug with 24.02 release.

I confirmed that Parajwal V brought in this problem again on 03/06/23 98c0d9ae, which Arnav Srivastava had solved on 06/01/22 a53cf69e.

Environment Provide accurate information about the environment to help us reproduce the issue.

To Reproduce see above.

Expected behavior node selector handles the value "true" correctly.

prajwalv-netapp commented 4 months ago

Hi @kmizumar , we are using the nodeAffinity and podAffinity with IN operator, which works on the set of strings. You're trying to use node-role.kubernetes.io/worker: "true" which treats it as string and fails.

Could you try to use as below and see if it works? node-role.kubernetes.io/worker: "'true'"

E-Zurg commented 4 months ago

Hi @prajwalv-netapp, I also got the same behavior while trying to use a label with value set to "true" in the Helm values. I tried as you mentioned by putting "'true'" in the value for the key used in tridentNodePluginNodeSelector in Helm. This translates to '''true''' in the TridentOrchestrator and the operator correctly patches the deamonSet.

Nevertheless, I don't understand what you mean that the IN operator is working on a set of strings being the problem? I got the following error in the operator when trying to use "true" : json: cannot unmarshal bool into Go struct field NodeSelectorRequirement.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms.matchExpressions.values of type string, requeuing"

This seems like it is the wrong type (as described in the bug mentioned by @kmizumar) and not due to the fact that it is not a list. In the end, I think the value "true" should be usable as is.

prajwalv-netapp commented 4 months ago

Hi @prajwalv-netapp, I also got the same behavior while trying to use a label with value set to "true" in the Helm values. I tried as you mentioned by putting "'true'" in the value for the key used in tridentNodePluginNodeSelector in Helm. This translates to '''true''' in the TridentOrchestrator and the operator correctly patches the deamonSet.

Nevertheless, I don't understand what you mean that the IN operator is working on a set of strings being the problem? I got the following error in the operator when trying to use "true" : json: cannot unmarshal bool into Go struct field NodeSelectorRequirement.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms.matchExpressions.values of type string, requeuing"

This seems like it is the wrong type (as described in the bug mentioned by @kmizumar) and not due to the fact that it is not a list. In the end, I think the value "true" should be usable as is.

Hi @E-Zurg , I provided a workaround until the Trident fixes this. If you refer to the below document, it states that IN would treat each value as string but the value true is interpreted as a boolean and hence you see the error.

https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#operators

torirevilla commented 4 weeks ago

This fix will be in the 24.10 release: https://github.com/NetApp/trident/commit/3550582947e96bba5c6bf151dce416ab77dd0db1