-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Added tolerations for cinder-csi-nodeplugin DaemonSet #8137
Added tolerations for cinder-csi-nodeplugin DaemonSet #8137
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @Ajarmar! |
Hi @Ajarmar. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/check-cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ajarmar Thank you for the PR
/ok-to-test
Would have been nice to add a comment before the option and some kind of example but anyhow it's fine as it is
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ajarmar, floryut The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this.
just one comment.
@@ -121,3 +121,5 @@ spec: | |||
path: {{ kube_config_dir }}/cinder-cacert.pem | |||
type: FileOrCreate | |||
{% endif %} | |||
tolerations: | |||
{{ cinder_tolerations | to_nice_yaml(indent=2) | indent(width=8) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitate to add empty tolerations into the spec.
For sample, ingress_nginx_tolerations
is checked when adding it as
Lines 33 to 36 in 1c3d082
{% if ingress_nginx_tolerations %} | |
tolerations: | |
{{ ingress_nginx_tolerations | to_nice_yaml(indent=2) | indent(width=8) }} | |
{% endif %} |
It is better to add the similar condition here like
{% if cinder_tolerations %}
tolerations:
{{ cinder_tolerations | to_nice_yaml(indent=2) | indent(width=8) }}
{% endif %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback.
I pushed this change as a separate commit - do you want me to squash the commits myself or will you squash them when you merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating the pull request.
It is better to squash multiple commits into a single by yourself before merging.
In addition, please rebase the pull request based on the latest master branch to solve molecule_tests issue.
Sorry for this inconvenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. I squashed the commits and rebased on master, hope everything looks good now.
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/check-cla |
0d606bc
to
9af2eca
Compare
Thanks for updating again. /lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it: Adds tolerations for the
cinder-csi-nodeplugin
DaemonSet. This is so that if a node needs to be tainted, the user can ensure that thecinder-csi-nodeplugin
Pods still run on all nodes. By default it's set to an empty list, so it won't cause any change unless the user specifies their desired tolerations.Which issue(s) this PR fixes:
none
Special notes for your reviewer: Any chance that this can fit into the
2.17.1
patch release?Does this PR introduce a user-facing change?: