Skip to content

Conversation

kbelokon
Copy link
Contributor

@kbelokon kbelokon commented Jun 8, 2023

Hello,

This pull request addresses a significant issue found in the Helm template for Dragonfly deployment related to the mishandling of 'tolerations' and 'affinity'.

Problem:
The problem was that the Helm template incorrectly concatenated values within 'tolerations' and 'affinity', which led to erroneous YAML outputs. Specifically, the rendered YAML for the tolerations would incorrectly join the last toleration and 'affinity', as evidenced by the resulting YAML.

Change:
The change primarily involves updating the Helm chart's handling of 'tolerations', 'nodeSelector', and 'affinity' values. The adjustment lies in the removal of the dash (-) in the 'with' statement for each of the three values. The changes are as follows:

# Before
{{- with .Values.tolerations -}}
{{- with .Values.nodeSelector -}}
{{- with .Values.affinity -}}

# After
{{- with .Values.tolerations }}
{{- with .Values.nodeSelector }}
{{- with .Values.affinity }}

With the proposed changes, the issue with the incorrect concatenation of values in 'tolerations' and 'affinity' is resolved. The generated YAML now correctly separates each section, ensuring the successful application of the Helm chart.

I have provided the changes in values for testing:

tolerations:
  - key: key/high-memory
    operator: "Equal"
    value: "true"
    effect: "NoSchedule"
  - key: key/high-memory
    operator: "Equal"
    value: "true"
    effect: "PreferNoSchedule"
affinity:
  nodeAffinity:
    requiredDuringSchedulingIgnoredDuringExecution:
      nodeSelectorTerms:
        - matchExpressions:
            - key: key/node-kind
              operator: In
              values:
                - high-memory

Here is an example of how the previous code used to parse the values:

tolerations:
  - effect: NoSchedule
    key: key/high-memory
    operator: Equal
    value: "true"
  - effect: PreferNoSchedule
    key: key/high-memory
    operator: Equal
    value: "true"affinity:
  nodeAffinity:
    requiredDuringSchedulingIgnoredDuringExecution:
      nodeSelectorTerms:
      - matchExpressions:
        - key: key/node-kind
          operator: In
          values:
          - high-memory

Please consider merging this fix as it greatly improves the usability and robustness of the Dragonfly deployment Helm chart.

@kbelokon kbelokon force-pushed the tolerations-affinity-handling branch from f3cb389 to 8f76cef Compare June 9, 2023 07:51
@dranikpg dranikpg requested a review from Pothulapati June 9, 2023 20:51
@Pothulapati
Copy link
Contributor

Pothulapati commented Jun 14, 2023

Hey @kbelokon, Sorry for the late reply. We had a CI issue to fix. Now, It's green!

So, The change looks good. Thanks for working on it. Would you mind adding a test with the values you mentioned in the issue into the golden tests folder, along with an empty golden output file? https://github.com/dragonflydb/dragonfly/tree/main/contrib/charts/dragonfly/ci

Once you do this, Run the commands from https://github.com/dragonflydb/dragonfly/blob/main/CONTRIBUTING.md#rendering-helm-golden-files, and it should automatically add the rendered output. With this, We will have a way to know if the output changes in the future.

(If you can't, I'm more than happy to do this as a follow up and merge this as is.)

Thanks. :)

@Pothulapati Pothulapati force-pushed the tolerations-affinity-handling branch from d418375 to 122b837 Compare June 15, 2023 09:09
@Pothulapati Pothulapati force-pushed the tolerations-affinity-handling branch from 122b837 to c1da078 Compare June 15, 2023 09:09
@Pothulapati Pothulapati force-pushed the tolerations-affinity-handling branch 2 times, most recently from 2e1f035 to d29786c Compare June 15, 2023 14:35
@Pothulapati Pothulapati force-pushed the tolerations-affinity-handling branch from d29786c to eaf05e2 Compare June 15, 2023 14:40
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

Thanks for raising the PR @kbelokon! I added the tests! :)

@Pothulapati Pothulapati merged commit 956b39c into dragonflydb:main Jun 15, 2023
@kbelokon
Copy link
Contributor Author

I just saw the changes you made to the PR and your earlier message. Apologies for not catching that sooner - your message slipped through my notifications.

Thanks for making those adjustments!

@Pothulapati
Copy link
Contributor

@kbelokon Not at all. Thank YOU for taking out the time to contribute, and helping make the project better! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants