-
Notifications
You must be signed in to change notification settings - Fork 10
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
Soufianej taint k3s #291
Soufianej taint k3s #291
Conversation
Requires code from: Zun: ChameleonCloud/zun#20 Order to apply is:
|
note: add |
2c89186
to
76e722f
Compare
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.
overall LGTM, but with the following caveats:
- the noted refactoring of how the taint/toleration is specified, to avoid repeating logic throughout the jinja templates
- related to the integration with doni and zun: I would suggest adding two flags:
doni_enforce_worker_taints: false
zun_enforce_worker_taints: false
Then, separate from the deployed container versions, the operator can first toggle on the zun support, and then toggle on the doni support, to avoid unexpected breakage.
Finally, is there a migration procedure to ensure currently running/deployed containers are not un-scheduled when this is deployed?
Do we have docs on how to trigger doni to deploy the new annotation? How can an operator check if some otherwise "good" nodes aren't available because they are/aren't tainted correctly?
tolerations: | ||
- key: "{{ k3s_worker_taint.split('=')[0] }}" | ||
operator: "Equal" | ||
value: "{{ k3s_worker_taint.split('=')[1].split(':')[0] }}" | ||
effect: "{{ k3s_worker_taint.split(':')[1] }}" |
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.
It is difficult to maintain jinja templates containing logic.
I suggest putting lines into kolla/defaults.yml instead:
k3s_worker_taint_key:
k3s_worker_taint_value:
k3s_worker_taint_effect:
If necessary, you can also use those to template the string format, also in defaults.yml
# format like 'worker-node=true:NoSchedule'
k3s_worker_taint: "{{ k3s_worker_taint_key }}={{ k3s_worker_taint_value }}:{{ k3s_worker_taint_effect}}"
tolerations: | ||
- key: "{{ k3s_worker_taint.split('=')[0] }}" | ||
operator: "Equal" | ||
value: "{{ k3s_worker_taint.split('=')[1].split(':')[0] }}" | ||
effect: "{{ k3s_worker_taint.split(':')[1] }}" |
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.
suggest refactor as per above comment
- key: "{{ k3s_worker_taint.split('=')[0] }}" | ||
operator: "Equal" | ||
value: "{{ k3s_worker_taint.split('=')[1].split(':')[0] }}" | ||
effect: "{{ k3s_worker_taint.split(':')[1] }}" |
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.
suggest refactor as per above comment
76e722f
to
f77eb5a
Compare
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.
Added some comments, mostly just naming/nits for consistency and to avoid confusing future me?
kolla/defaults.yml
Outdated
@@ -92,6 +92,7 @@ enable_cinder: no | |||
|
|||
# Doni | |||
enable_doni: yes | |||
doni_enforce_worker_taints: false |
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.
this seems unused?
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.
remnant from a merge :(, I'll change it
kolla/defaults.yml
Outdated
@@ -345,6 +346,10 @@ enable_k3s: no | |||
enable_zun: no | |||
enable_zun_compute_k8s: "{{ enable_zun | bool and enable_k3s | bool }}" | |||
blazar_enable_device_plugin_k8s: "{{ enable_k3s | bool }}" | |||
# K8S worker tainting | |||
zun_enforce_worker_taint: false |
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.
should this be zun_tolerate_worker_taint
? afaik it will not prevent scheduling even if no workers are tainted?
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.
agreed
kolla/defaults.yml
Outdated
# K8S worker tainting | ||
zun_enforce_worker_taint: false | ||
doni_enable_worker_taint: false | ||
worker_taint: "{{ k8s_worker_taint.key | default('worker-node') }}={{ k8s_worker_taint.value | default('true') }}:{{ k8s_worker_taint.effect | default('NoSchedule') }}" |
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.
Is this a string or a dict? you reference worker_taint.key
in doni.conf?
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.
this is a dict, looks like my merge was a little destructive
0c33ef9
to
93cfd5d
Compare
The options to add to site-config are: - doni_enable_worker_taint: true/false, by default set to false (enables worker tainting through the k8s worker on doni devices) - zun_tolerate_worker_taint: true/false, by default set to false (enables taint enforcement by adding tolerations to worker pods) - k8s_worker_taint - key: taint key, by default set to "worker-node" - value: taint value, by default set to "true" - effect: taint effect, by default set to "NoSchedule"
93cfd5d
to
598a306
Compare
k3s defaults.yaml gets the value of k3s_worker_taint from worker_taint which is defined under kolla/defaults.yml which subsequently defines defaults for the option and takes in site values from the k8s_worker_taint option that can be specified through the site config. Added worker node taint toleration to smarter devices manager daemonsets. Furthermore, templated the nvidia device plugin daemonset and added the toleration there as well.
598a306
to
8f9944a
Compare
Added k8s worker taint configuration options
The options to add to site-config are:
the k8s worker on doni devices)
adding tolerations to worker pods)
Added taint tolerations to core deployments in k3s
k3s defaults.yaml gets the value of k3s_worker_taint from worker_taint
which is defined under kolla/defaults.yml which subsequently defines
defaults for the option and takes in site values from the
k8s_worker_taint option that can be specified through the site config.
Added worker node taint toleration to smarter devices manager
daemonsets.
Furthermore, templated the nvidia device plugin daemonset and added the
toleration there as well.
Taint deployment strategy on a running testbed:
zun_tolerate_worker_taint
toTrue
and redeploy Zundoni_enable_worker_taint
toTrue
and redeployThe above sequence ensures that no existing or simultaneous user pods get evicted and inflicts minimal downtime to core daemonsets.