diff --git a/packages/orchestrator/pkg/sandbox/nbd/pool.go b/packages/orchestrator/pkg/sandbox/nbd/pool.go index ba6e3e1198..2d11ff561c 100644 --- a/packages/orchestrator/pkg/sandbox/nbd/pool.go +++ b/packages/orchestrator/pkg/sandbox/nbd/pool.go @@ -26,6 +26,9 @@ const ( waitOnNBDError = 50 * time.Millisecond devicePoolCloseReleaseTimeout = 10 * time.Minute sysBlockDir = "/sys/block" + // releaseRetryDelay is how long ReleaseDevice waits before retrying a device + // that is still in use. + releaseRetryDelay = 500 * time.Millisecond ) var ( @@ -366,7 +369,14 @@ func (d *DevicePool) ReleaseDevice(ctx context.Context, idx DeviceSlot, opts ... logger.L().Error(ctx, "error releasing device", zap.Int("attempt", attempt), zap.Error(err)) } - time.Sleep(500 * time.Millisecond) + // Wait on the context too: Close bounds a stuck device with WithTimeout, + // and shutdown cancels the context outright. An uninterruptible sleep + // here would make every remaining attempt outlive both. + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(releaseRetryDelay): + } } } diff --git a/packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go b/packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go new file mode 100644 index 0000000000..114de95680 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go @@ -0,0 +1,82 @@ +//go:build linux + +package nbd + +import ( + "context" + "testing" + "time" + + "github.com/bits-and-blooms/bitset" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unreachableSlot is an index no NBD device can have, so isDeviceFree always +// fails to read /sys/block/nbd/size and release() keeps returning an +// error. That drives the retry loop deterministically, whether or not the nbd +// module is loaded on the machine running the test. +const unreachableSlot = DeviceSlot(1 << 20) + +func retryingPool() *DevicePool { + return &DevicePool{ + done: make(chan struct{}), + usedSlots: bitset.New(16), + slots: make(chan DeviceSlot, 1), + } +} + +// A cancelled context must abort the release retry loop while it is backing +// off, not only when it happens to be at the top of the loop. Otherwise every +// stuck device adds a full backoff interval to orchestrator shutdown. +func TestReleaseDeviceAbortsBackoffOnContextCancel(t *testing.T) { + t.Parallel() + + pool := retryingPool() + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + // Cancel once the first failed attempt has entered the backoff. + time.AfterFunc(releaseRetryDelay/10, cancel) + + start := time.Now() + err := pool.ReleaseDevice(ctx, unreachableSlot, WithInfiniteRetry()) + elapsed := time.Since(start) + + require.ErrorIs(t, err, context.Canceled) + assert.Less(t, elapsed, releaseRetryDelay, + "ReleaseDevice slept through the cancellation instead of aborting the backoff") +} + +// The same applies to the deadline installed by WithTimeout, which Close relies +// on to bound how long a single stuck device can hold up the pool. +func TestReleaseDeviceAbortsBackoffOnTimeout(t *testing.T) { + t.Parallel() + + pool := retryingPool() + + start := time.Now() + err := pool.ReleaseDevice(t.Context(), unreachableSlot, + WithInfiniteRetry(), + WithTimeout(releaseRetryDelay/10), + ) + elapsed := time.Since(start) + + require.ErrorIs(t, err, context.DeadlineExceeded) + assert.Less(t, elapsed, releaseRetryDelay, + "ReleaseDevice slept past its own deadline") +} + +// Without infinite retry the first failure is returned immediately, so the +// backoff change must not alter that path. +func TestReleaseDeviceReturnsFirstErrorWithoutInfiniteRetry(t *testing.T) { + t.Parallel() + + pool := retryingPool() + + err := pool.ReleaseDevice(t.Context(), unreachableSlot) + + require.Error(t, err) + assert.NotErrorIs(t, err, context.Canceled) +}