feat: policy server deployment tolerations. #793
Conversation
Updates the PolicyServer CRDs to allow users to define the toleration to be used in the policy server deployment. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
==========================================
- Coverage 71.73% 71.43% -0.30%
==========================================
Files 23 23
Lines 1705 1705
==========================================
- Hits 1223 1218 -5
- Misses 371 375 +4
- Partials 111 112 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -168,6 +168,7 @@ func (r *PolicyServerReconciler) updatePolicyServerDeployment(policyServer *poli | |||
| SecurityContext: podSecurityContext, | |||
| Containers: []corev1.Container{admissionContainer}, | |||
| ServiceAccountName: policyServer.Spec.ServiceAccountName, | |||
| Tolerations: policyServer.Spec.Tolerations, | |||
There was a problem hiding this comment.
Just in case, shouldn't it be done with something similar to https://github.com/kubewarden/kubewarden-controller/blob/main/internal/controller/policyserver_controller_deployment.go#L299-L300?
That way, we support the users changing the Deployment if there was no PolicyServer spec.tolerations set.
I'm not sure what informal promise do we have with our users for this behavior. I recall someone asking for this possibility of being able to manually set the affinity, but maybe it's an anachronism. If that's the case, I would like to use this PR to unify the behavior for tolerations and affinity.
There was a problem hiding this comment.
@viccuad I don't understand why we wrote the code you linked. I would have handled affinity like @jvanz just did here with tolerations...
@fabriziosestito can you chime in?
There was a problem hiding this comment.
Is not related to how we used to detect changes in the resource and trigger a reconciliation? I have a vague memory about a discussion around this. If that's the case and now we use the controller-runtime helper functions, I believe we do not need to do that anymore.
There was a problem hiding this comment.
We can remove it and use the same approach of the Tolerations field. As far as I understood the if block does not change the reconciliation behaviour, it's probably some leftover since now we use the controller-runtime CreatOr*
That way, we support the users changing the Deployment if there was no PolicyServer spec.tolerations set.
I don't think the user should be able to thinker with the Deployment, as the only interface we expose is the PolicyServer CRD
There was a problem hiding this comment.
If we agree then, I would like to follow the same approach of Tolerations for Affinity in this PR.
The policy server reconciler check if the policy server spec has a non empty affinity. If that's the case, it set the affinity in the policy server deployment. This allow users to set the affinity manually in the deployment. This should not be allowed. The policy server CRDS should be the interface to configure the affinity. Therefore, that check is removed and the affinity from the policy server spec is always set in the deployment. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
jvanz
force-pushed
the
tolerations-issue752
branch
from
60c4c94
to
6e105ed
Compare
Description
Updates the PolicyServer CRDs to allow users to define the toleration to be used in the policy server deployment.
Fix #752
Test
make testAdditional Information
I'm aware that we have a PR changing our tests (#788 ). I'm fine to wait that PR to be merged before. I'm glad to rebase this change on top of that PR after it gets merged into main.