PB-862: use Kubernetes native sidecar mechanism#759
Conversation
| # So Kubernetes can mark this as Started | ||
| # In reality a sidecar should not be used like this. | ||
| # As this is effectively an ad-hoc initContainer. | ||
| sleep 120 |
There was a problem hiding this comment.
This is a potential breaking change. In the new sidecar implementation, the sidecar container is expected to be long running and never finish.
There was a problem hiding this comment.
Hmm, true. We'll have to call this out carefully in the release notes.
| "github.com/buildkite/agent/v3/agent" | ||
| "github.com/buildkite/roko" | ||
|
|
||
| "log/slog" |
There was a problem hiding this comment.
This actually belongs in the first group (standard library packages).
| // (our in-house built) sidecar containers. | ||
| // Now it's no longer necessary. | ||
| // | ||
| // I am leaning to keep it because: |
There was a problem hiding this comment.
I vote in favour of deleting the whole completion watcher.
If the containers are misbehaving so badly that they are blocking the pod finishing, that sounds like a serious bug we should fix. Leaving the pod running would enable more ways to diagnose.
There was a problem hiding this comment.
No objection from me. It's gone now 🫡
| # So Kubernetes can mark this as Started | ||
| # In reality a sidecar should not be used like this. | ||
| # As this is effectively an ad-hoc initContainer. | ||
| sleep 120 |
There was a problem hiding this comment.
Hmm, true. We'll have to call this out carefully in the release notes.
|
This change also adds a new minimum cluster version requirement of |
Aside from that, it'd be nice to have a fallback for a while, even though I'd also like to rip old code. I imagine a lot of broken controllers right now. |
the feature is active by default since Kubernetes v1.29
Yes and yes. |
|
I am hoping to do a scan to find out version distribution in our existing users. But we don't seem to collect this information. Nor do we have a way to engage with the crowd en mass. So we will have to make a small bet. If it helps,
but yeah, 100% on a |
|
ready for another review @DrJosh9000 🙏🏿 |
DrJosh9000
left a comment
DrJosh9000
left a comment
There was a problem hiding this comment.
I wish there were a way to test for individual feature gates in Chart.yaml rather than k8s versions
Anyway, LGTM with some minor comments
| # So Kubernetes can mark this as Started | ||
| # In reality a sidecar should not be used like this. | ||
| # As this is effectively an ad-hoc initContainer. | ||
| sleep 120 |
There was a problem hiding this comment.
A quick play with alpine:latest shows that it supports infinity - maybe we should do this?
| sleep 120 | |
| sleep infinity |
4 participants
Historically, k8s didn't have a main container vs sidecar container concept. As a result we maintain a in-house built sidecar pattern for agent-k8s stack, it means we have to implement many code to make a container sidecar: it includes relying on
activeDeadlineSecondsto kill the pod after "main" container is done etc.It also isn't ideal because it can leave a k8s job in a status of
type: Failed with reason: DeadlineExceededdespite it successfully handled the bk job.This PR changes k8s stack to use Kuberenetes' native sidecar pattern.