Ensure sandbox toleration #535
Conversation
rajivnathan
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
ranakan19,
sbryzak and
mfrancisc
as code owners
| } | ||
|
|
||
| // no need to check original contents of tolerations, if there were existing tolerations the sandbox one will be appended; if there were no tolerations then the patch will add the sandbox toleration | ||
| tolerations = append(tolerations, sandboxToleration) |
There was a problem hiding this comment.
What if there is already a sandbox toleration. We will have two copies of the same toleration with the same key:
{
"effect": "NoSchedule",
"key": "sandbox-cnv",
"operator": "Exists",
},
{
"effect": "NoSchedule",
"key": "sandbox-cnv",
"operator": "Exists",
}
right? Or I'm reading it wrong?
There was a problem hiding this comment.
yeah, I thought about adding a check for that case but
- the webhook is on create, so the toleration needs to be in the original VM for that to happen which is pretty unlikely
- it works correctly even if it adds a duplicate (I tested it)
I mostly ignored it because of 1...but now that I think about it again it's possible that a user copies the VM yaml for a VM that was created and has the toleration added by the webhook and later creates another VM manually using that yaml. In that case it'd add the toleration again which is ugly. So I'll add a check and avoid adding a toleration if there's an existing toleration with the sandbox-cnv key.
There was a problem hiding this comment.
Yeah... and if in the future we start invoking the webhook on updates too (not just on create) then every update on the VM resource would result in another duplicate of the toleration.
There was a problem hiding this comment.
Handled duplicate check in a0ee25a . Thanks
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #535 +/- ##
=======================================
Coverage 82.73% 82.74%
=======================================
Files 29 29
Lines 3256 3281 +25
=======================================
+ Hits 2694 2715 +21
- Misses 420 423 +3
- Partials 142 143 +1
|
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, rajivnathan 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 |
rajivnathan
merged commit 7476d92
into
codeready-toolchain:master
https://issues.redhat.com/browse/SANDBOX-499
Updates the member webhook to ensure the sandbox toleration is set on VM resources so that the VM can be scheduled on the BM node.
e2e tests: codeready-toolchain/toolchain-e2e#898