route bake targets across pods with random loadbalance

[areebahmeddd]

loadbalance=random on the kubernetes driver was sending all bake targets to the same pod.
the client connection was cached after the first dial, so pod selection only happened once.

fix this by creating a new connection per target when loadbalance=random is enabled.
also disable the shared local mount session optimization in this mode.

Closes #3382

[AkihiroSuda]

loadbalance=random on the kubernetes driver was sending all bake targets to the same pod. the client connection was cached after the first dial, so pod selection only happened once.

fix this by creating a new connection per target when loadbalance=random is enabled.

What about loadbalance=sticky ?

[AkihiroSuda]

Also, please squash the commits

[areebahmeddd]

What about loadbalance=sticky ?

no change. it reuses the same shared connection path

Also, please squash the commits

sure 🙂

[areebahmeddd]

gentle ping! @AkihiroSuda

the ci failure is not related to our changes.

second review

[crazy-max]

I was looking at

buildx/driver/kubernetes/podchooser/podchooser.go

Lines 29 to 45 in 0b358c7

	func (pc *RandomPodChooser) ChoosePod(ctx context.Context) (*corev1.Pod, error) {
	pods, err := ListRunningPods(ctx, pc.PodClient, pc.Deployment, pc.StatefulSet)
	if err != nil {
	return nil, err
	}
	if len(pods) == 0 {
	return nil, errors.New("no running buildkit pods found")
	}
	randSource := pc.RandSource
	if randSource == nil {
	randSource = rand.NewSource(time.Now().Unix())
	}
	rnd := rand.New(randSource) // #nosec G404 -- no strong seeding required
	n := rnd.Int() % len(pods)
	logrus.Debugf("RandomPodChooser.ChoosePod(): len(pods)=%d, n=%d", len(pods), n)
	return pods[n], nil
	}

Seems to still looks capable of picking the same pod for all targets in one bake no? ChoosePod() creates a new RNG on every call and seeds it with time.Now().Unix(). With this PR, multiple targets can create fresh clients and dial within the same second, so each call can get the same seed and produce the same first random value iiuc.

Maybe we should seed the RNG once at the package level, with concurrency handled, or use another safe random selection path? Otherwise this PR may still reproduce the original issue.

[crazy-max]

Actually after new review this doesn't feel like the right shape for the driver API. NeedsNewClientPerBuild() is added as an optional hidden interface inside DriverHandle, but the behavior is broader than the name says. The build code also uses it to disable shared local mount sessions.

Can we make that contract explicit near the Driver interface instead of hiding it in manager.go? Something like an optional RequiresUncachedClient() interface would make the build-side behavior easier to reason about. I would also rename NewClient() to something like UncachedClient() so it is clear this is deliberately bypassing DriverHandle.Client() caching, not just another constructor.

[areebahmeddd]

Maybe we should seed the RNG once at the package level, with concurrency handled, or use another safe random selection path? Otherwise this PR may still reproduce the original issue.

yea, re-seeding in ChoosePod() could lead to identical selections for concurrent dials. i'll switch it to use a shared RandSource that's seeded once and reused

Can we make that contract explicit near the Driver interface instead of hiding it in manager.go? Something like an optional RequiresUncachedClient() interface would make the build-side behavior easier to reason about. I would also rename NewClient() to something like UncachedClient() so it is clear this is deliberately bypassing DriverHandle.Client() caching, not just another constructor.

okay, i've moved it next to the Driver interface as an optional RequiresUncachedClient() interface

[areebahmeddd]

Hm, let me take a look