fix(scheduler): pod-spec-patch was not working on default init containers #484
Conversation
defgenx
force-pushed
the
fix-pod-spec-patch-init-container
branch
2 times, most recently
from
30f1247 to
a9f5427
Compare
defgenx
force-pushed
the
fix-pod-spec-patch-init-container
branch
from
a9f5427 to
2d9b15c
Compare
defgenx
changed the title
There was a problem hiding this comment.
Thanks for raising this PR, @defgenx ! I have a question, see below.
| @@ -560,7 +560,7 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI | |||
| ) | |||
| } | |||
|
|
|||
| initContainers := []corev1.Container{w.createWorkspaceSetupContainer(podSpec, workspaceVolume)} | |||
| podSpec.InitContainers = []corev1.Container{w.createWorkspaceSetupContainer(podSpec, workspaceVolume)} | |||
There was a problem hiding this comment.
If the user has supplied a podSpec that already defines one or more init containers, this would replace them with the workspace setup container.
It would also make the workspace setup container patchable, which would make it easy to break the way the agent is designed to work in the pod. Is that the intent of this PR? What's the use case you have in mind?
There was a problem hiding this comment.
Thanks for reviewing and answering !
Yes true, my bad. We can move to an `append" here instead. I forgot about plugins to be honest.
From my understanding, pod-spec-patch is an advanced feature allowing anyone to patch the podSpec default values at runtime.
In our case we want to change the resources and the image pull policy of the copy-agent init container. It looks like it's nothing new because it was possible with the old pod-spec-patch implementation; making me think it's more a regression than a feature.
It's also possible to patch the containers of the agent right now so it would make sense to allow patching the init containers too. What do you think about it ?
Let me know if you have anything else in mind, but controlling the cost is really important for us and since we bumped from an old agent version to the last one, we cannot do that anymore so I tried to make it works again. :)
There was a problem hiding this comment.
I see, thanks for clarifying. I think the init container commands and args should still be protected from modification (with an override in the form of #449), but allowing the resources to be limited makes total sense.
Since these default containers should require bare minimum resources on pretty much any system, I wonder if they should be generated with default limits - that way you wouldn't have to patch them at all!
There was a problem hiding this comment.
Yes we can keep the command protected but I feel we should be able to patch the rest (as for containers). I can update the PR so we raise an error if the command is touched. Is it ok for you ?
If we could have a configuration to define the resources by default for the copy-agent init container, I would use it instead. We would still need to be able to change the pull policy too or anything which is configurable except the command.
Can we keep this PR to make it works like for containers and then add the feature to configure the copy-agent in another PR later?
EDIT: made the fix to prevent command from default init containers (only copy-agent right now) to be modified.
Description
This PR resolves an issue where the
PodSpecPatchfor defaultinitContainerswas not functioning correctly in the scheduler.Changes
podSpecPatchwhen the init-container exists.