PB-862: use Kubernetes native sidecar mechanism

Author: zhming0

internal/controller/controller.go

@@ -216,16 +216,6 @@ func Run(ctx context.Context, logger *slog.Logger, k8sClient kubernetes.Interfac
 	// But it does bring a trade-off of more likely reservation expiration.
 	reserver := reserver.New(logger.With("component", "reserver"), agentClient, limiter)
 
-	// PodCompletionWatcher watches k8s for pods where the agent has terminated,
-	// in order to clean up the pod. This is necessary because "sidecars" are
-	// not internally managed by buildkite-agent, and would continue running
-	// forever, preventing the pod being cleaned up.
-	completions := scheduler.NewPodCompletionWatcher(logger.With("component", "completions"), k8sClient)
-	if err := completions.RegisterInformer(ctx, informerFactory); err != nil {
-		logger.Error("failed to register completions informer", "error", err)
-		return
-	}
-
 	// JobWatcher watches for jobs in bad conditions to clean up:
 	// * Jobs that fail without ever creating a pod
 	// * Jobs that stall forever without ever creating a pod

internal/controller/scheduler/completions.go

@@ -202,33 +202,3 @@ var (
 		Help:      "Count of failures to delete pod forcefully by podWatcher",
 	}, []string{"delete_reason", "error_reason"})
 )
-
-// Completion watcher metrics
-
-var (
-	completionWatcherOnAddEventCounter = promauto.NewCounter(prometheus.CounterOpts{
-		Namespace: promNamespace,
-		Subsystem: "completion_watcher",
-		Name:      "onadd_events_total",
-		Help:      "Count of OnAdd informer events",
-	})
-	completionWatcherOnUpdateEventCounter = promauto.NewCounter(prometheus.CounterOpts{
-		Namespace: promNamespace,
-		Subsystem: "completion_watcher",
-		Name:      "onupdate_events_total",
-		Help:      "Count of OnUpdate informer events",
-	})
-
-	completionWatcherJobCleanupsCounter = promauto.NewCounter(prometheus.CounterOpts{
-		Namespace: promNamespace,
-		Subsystem: "completion_watcher",
-		Name:      "cleanups_total",
-		Help:      "Count of jobs with finished agents successfully cleaned up",
-	})
-	completionWatcherJobCleanupErrorsCounter = promauto.NewCounterVec(prometheus.CounterOpts{
-		Namespace: promNamespace,
-		Subsystem: "completion_watcher",
-		Name:      "cleanup_errors_total",
-		Help:      "Count of errors during attempts to clean up a job with a finished agent",
-	}, []string{"reason"})
-)

internal/controller/scheduler/metrics.go

@@ -3,6 +3,7 @@ package scheduler
 import (
 	"cmp"
 	"context"
+	"log/slog"
 	"regexp"
 	"slices"
 	"strconv"
@@ -16,7 +17,6 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/jedib0t/go-pretty/v6/table"
-	"log/slog"
 	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -581,12 +581,6 @@ func (w *podWatcher) stopWatchingForImageFailure(jobUUID uuid.UUID) {
 // All container-\d containers will have the agent installed as their PID 1.
 // Therefore, their lifecycle is well monitored in our backend, allowing us to terminate them if they fail to start.
 //
-// However, sidecar containers are completely unmonitored.
-// We avoid terminating jobs due to sidecar image pull backoff watcher
-// to prevent customer confusion.
-//
-// Most importantly, the CI can still pass (in theory) even if sidecars fail.
-//
 // (The name "system container" is subject to more debate.)
 func isSystemContainer(containerStatus *corev1.ContainerStatus) bool {
 	name := containerStatus.Name

internal/controller/scheduler/pod_watcher.go

@@ -24,9 +24,10 @@ import (
 	"github.com/buildkite/agent/v3/agent"
 	"github.com/buildkite/agent/v3/clicommand"
 
+	"log/slog"
+
 	"github.com/distribution/reference"
 	"github.com/jedib0t/go-pretty/v6/table"
-	"log/slog"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
@@ -503,7 +504,9 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI
 			}
 			maps.Copy(kjob.Spec.Template.Annotations, sidecarAnnotation)
 
-			podSpec.Containers = append(podSpec.Containers, c)
+			// Per https://kubernetes.io/docs/concepts/workloads/pods/sidecar-containers/
+			c.RestartPolicy = ptr.To(corev1.ContainerRestartPolicyAlways)
+			podSpec.InitContainers = append(podSpec.InitContainers, c)
 
 			sidecarCounterCount += 1
 		}
@@ -536,7 +539,7 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI
 	// The coordinating agent container are expecting those managed containers to connect.
 	//
 	// Calculating this imperatively is risky given we lack control over the context, this is subject to refactor.
-	managedContainerCount := len(podSpec.Containers) + systemContainerCount - sidecarCounterCount
+	managedContainerCount := len(podSpec.Containers) + systemContainerCount
 
 	agentContainer := corev1.Container{
 		Name:         AgentContainerName,

internal/controller/scheduler/scheduler.go

@@ -23,6 +23,10 @@ steps:
                   mv /tmp/extra-volume-mount-sidecar/pass-the-parcel /tmp/extra-volume-mount/pass-the-parcel
                   ls -lah /tmp/extra-volume-mount-sidecar
                   ls -lah /tmp/extra-volume-mount
+                  # 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
           podSpec:
             containers:
               - image: alpine:latest

internal/integration/fixtures/extra-volume-mounts-sidecar.yaml

@@ -2,13 +2,9 @@ steps:
   - label: ":wave:"
     agents:
       queue: "{{.queue}}"
+    image: curlimages/curl:latest
+    command: curl localhost:80
     plugins:
       - kubernetes:
           sidecars:
             - image: nginx:latest
-          podSpec:
-            containers:
-              - image: curlimages/curl:latest
-                name: curl
-                command:
-                  - curl --retry 5 --retry-all-errors localhost:80