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

support tolerations and node Selector #2791

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

fabiomarinetti
Copy link
Contributor

@fabiomarinetti fabiomarinetti commented Mar 26, 2024

Change Overview

This Pull Request add the support for tolerations and nodeSelector of the kanister helm chart as per issue 2707 request. I added the default empty values in the values.yaml file and added the logic for handling them in the deployment helm template.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@viveksinghggits
Copy link
Contributor

viveksinghggits commented Mar 27, 2024

@fabiomarinetti can you please add PR description (Change Overview) and detailed test plan in PR.

@fabiomarinetti
Copy link
Contributor Author

@viveksinghggits I added a short description in the PR. What about helm chart test? I don't see where it is covered.

@viveksinghggits
Copy link
Contributor

viveksinghggits commented Mar 27, 2024

I think we should add something here https://github.com/kanisterio/kanister/blob/master/pkg/testing/helm/helm_test.go
Basically we should be able to install (or dry-run) the local helm chart by setting nodeSelector and tolerations helm values and then verify that the generated/rendered deployment template has proper fields set.
Let me know if you need some help doing this.

@fabiomarinetti
Copy link
Contributor Author

Hi @viveksinghggits, I took a look into the tests, I would like to launch helm install with --dry-run, anyway even though I understand how to pass this to the helmApp I don't get how/if it is used and what it is expected to return by the install function.
In fact in the helm/client.go I don't see any mention to dry-run param.
I am not very confident with go so it is probable I miss something.

@viveksinghggits
Copy link
Contributor

Yeah, we would need to improve our helm client code as well to support --dry-run flag. we can either do that as part of this PR or another PR as well. And then we can incorporate that change in this PR.

I will discuss this in tomorrow's community call.

@hairyhum
Copy link
Contributor

@fabiomarinetti in the meanwhile can you please provide a manual test plan while we're working on improving our test tools #2796

@e-sumin e-sumin self-requested a review April 11, 2024 16:52
@fabiomarinetti
Copy link
Contributor Author

sure @hairyhum

if you have yq installed you can simply do the two following commands in your bash shell. OK means test passed, KO test failed.

[ "$(helm template kanister helm/kanister-operator/ 2>/dev/null \
      --set nodeSelector.foo=bar | yq e '. | select(.kind == "Deployment").spec.template.spec.nodeSelector.foo')" == "bar" ] && \
     echo OK || echo KO

for testing nodeSelector, while for tolerations

expected="effect: NoSchedule
key: CriticalAddonsOnly
operator: Equal
value: true"
[ "$(helm template kanister helm/kanister-operator/ 2>/dev/null \
      --set tolerations[0].key=CriticalAddonsOnly \
      --set tolerations[0].effect=NoSchedule \
      --set tolerations[0].operator=Equal \
      --set tolerations[0].value='true' | yq e '. | select(.kind == "Deployment").spec.template.spec.tolerations[0]')" == "$expected" ]  \
      && echo OK ||  echo KO

@mergify mergify bot merged commit bc2d38f into kanisterio:master Apr 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce Installation of the Operator on stateless Nodepools
3 participants