resourceReqs and podLabels for mover job/deploy #1072
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1072 +/- ##
=======================================
+ Coverage 66.9% 67.3% +0.4%
=======================================
Files 55 55
Lines 7080 7117 +37
=======================================
+ Hits 4739 4795 +56
+ Misses 2061 2042 -19
Partials 280 280
|
Resolves: backube#707 Signed-off-by: Tesshu Flower <tflower@redhat.com>
tesshuflower
force-pushed
the
moverResourceRequirements
branch
from
76d6788
to
c91600a
Compare
|
/cc @JohnStrunk |
- in cases where user has set securityReqs/ resourceRequirements, and then unsets in the spec, we would not update the spec - in this case make sure the job/deploy template gets updated properly back to the default/empty values Signed-off-by: Tesshu Flower <tflower@redhat.com>
controllers/utils/utils.go
Outdated
| // Security context | ||
| if moverConfig.MoverSecurityContext != nil { | ||
| podTemplateSpec.Spec.SecurityContext = moverConfig.MoverSecurityContext | ||
| } | ||
|
|
||
| // Adjust the job/deploy containers resourceRequirements based on resourceRequirements from the moverConfig | ||
| if moverConfig.MoverResources != nil { | ||
| for i := range podTemplateSpec.Spec.Containers { | ||
| podTemplateSpec.Spec.Containers[i].Resources = *moverConfig.MoverResources | ||
| } | ||
| } |
There was a problem hiding this comment.
An observation... If a field in the moverConfig is null, we won't make any change to the template. Would this prevent us from removing these items once they are set?
I'm wondering if this should just be a simple assignment in the case of SecurityContext and perhaps assignment of an empty corev1.ResourceRequirements{} for MoverResources.
Thoughts?
There was a problem hiding this comment.
hmm this is a very good point - I think I was hoping not to reset anything that could have been set up earlier, but I think this should work if I simply set the securityContext as you said - one specific case that I made me do it this way I think was Syncthing. It sets a memory limit by default on the mover pod, which I'm allowing to be overridden.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JohnStrunk, tesshuflower 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 |
Resolves: #707
Describe what this PR does
Allows configuration of resourceRequirements and podLabels for mover jobs/deployments.
Is there anything that requires special attention?
Docs still need to be updated - will need to warn users that using these knobs can potentially crash mover pods or prevent them from being scheduled.
Related issues:
moverResourceRequirements