From e38153e6e9ca42cdeb74d95c812bad981101db47 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Sun, 6 Sep 2026 07:00:52 +0500 Subject: [PATCH 1/2] fix(provider): release refused warm reservations immediately --- CHANGELOG.md | 7 ++ internal/garmproviderincus/provider/warm.go | 19 ++- .../provider/warm_reservation_test.go | 108 ++++++++++++++++++ 3 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 internal/garmproviderincus/provider/warm_reservation_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d11bc516..3b148eec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +- Release a warm worker's exact memory reservation when placement refuses + creation. Successful capacity deferrals previously skipped cleanup and + retained uncreated workers until the admission lease expired. Early + argument failures now preserve the cleanup identity, cleanup failures are + reported, and ambiguous create responses retain their reservation for + authoritative reconciliation. + - Classify warm-preemption and instance-delete waits as capacity, not timeout. A create that reclaims a warm slot and hits `context deadline exceeded` or Incus `instance is busy running a start` used to page diff --git a/internal/garmproviderincus/provider/warm.go b/internal/garmproviderincus/provider/warm.go index b679ab69..e8aa8d05 100644 --- a/internal/garmproviderincus/provider/warm.go +++ b/internal/garmproviderincus/provider/warm.go @@ -288,33 +288,42 @@ func (l *Incus) createWarm(ctx context.Context, flavor string) (name string, con if !decision.Admitted { return "", false, &decision, nil } + reservedName := name created := false + launchAttempted := false defer func() { - if err == nil { + if err == nil && deferred == nil { + return + } + // An uncertain create can still complete in Incus. Its reservation + // belongs to inventory reconciliation until existence is established. + if launchAttempted && !created && deferred == nil { return } cleanupContext, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() var cleanupErr error if created { - cleanupErr = l.DeleteInstance(cleanupContext, name) + cleanupErr = l.DeleteInstance(cleanupContext, reservedName) } else { - cleanupErr = l.admission.Release(cleanupContext, name) + cleanupErr = l.admission.Release(cleanupContext, reservedName) } if cleanupErr != nil { - err = errors.Wrapf(err, "cleaning failed warm instance %q: %v", name, cleanupErr) + err = errors2.Join(err, errors.Wrapf(cleanupErr, "cleaning failed warm instance %q", reservedName)) } }() args, err := l.getWarmCreateArgs(ctx, flavor, name) if err != nil { return "", false, nil, err } + launchAttempted = true if err = l.launchInstance(ctx, args); err != nil { if isPlacementRefusal(err) { // The scriptlet is the per-member truth and fleet-level admission // is the ledger; when they disagree under load, the member that // had ledger room lacked live room. That is a capacity deferral, - // not a failed reconcile. + // not a failed reconcile. Cleanup must still release the exact + // reservation: named return values clear both name and err here. return "", false, &admission.Decision{Admitted: false, Reason: admission.ReasonPlacementRefused, Pool: flavor}, nil } return "", false, nil, errors.Wrap(err, "launching warm instance") diff --git a/internal/garmproviderincus/provider/warm_reservation_test.go b/internal/garmproviderincus/provider/warm_reservation_test.go new file mode 100644 index 00000000..51f10688 --- /dev/null +++ b/internal/garmproviderincus/provider/warm_reservation_test.go @@ -0,0 +1,108 @@ +package provider + +import ( + "context" + "errors" + "testing" + + "github.com/NDDev-OpenNetwork/github-actions/internal/admission" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +type trackedWarmAdmission struct { + allowAllAdmission + reserved map[string]bool + released []string + releaseErr error +} + +func (a *trackedWarmAdmission) AdmitWarm(_ context.Context, _ InstanceServerInterface, _, name string) (admission.Decision, error) { + if a.reserved == nil { + a.reserved = make(map[string]bool) + } + a.reserved[name] = true + return admission.Decision{Admitted: true}, nil +} + +func (a *trackedWarmAdmission) Release(ctx context.Context, name string) error { + if err := ctx.Err(); err != nil { + return err + } + a.released = append(a.released, name) + if a.releaseErr != nil { + return a.releaseErr + } + delete(a.reserved, name) + return nil +} + +func TestWarmPlacementDeferralReleasesReservationBeforeNextAttempt(t *testing.T) { + cli := new(MockIncusServer) + p := newTestProvider(cli) + a := &trackedWarmAdmission{} + p.admission = a + prepareCreateMocks(cli, testImageDigest) + cli.On("CreateInstance", mock.Anything).Return(new(MockOperation), errors.New( + "Failed instance placement scriptlet: insufficient-memory: no fleet member has room for this worker", + )) + for range 3 { + name, consumed, deferred, err := p.createWarm(t.Context(), "nddev-linux-standard") + require.NoError(t, err) + require.Empty(t, name) + require.False(t, consumed) + require.NotNil(t, deferred) + require.Equal(t, admission.ReasonPlacementRefused, deferred.Reason) + require.Empty(t, a.reserved, "a refused warm create must not consume the next job's memory") + } + require.Len(t, a.released, 3) + for _, name := range a.released { + require.Contains(t, name, "warm-standard-") + } + cli.AssertNotCalled(t, "DeleteInstance", mock.Anything) +} + +func TestWarmPlacementDeferralReportsReleaseFailure(t *testing.T) { + cli := new(MockIncusServer) + p := newTestProvider(cli) + a := &trackedWarmAdmission{releaseErr: errors.New("journal unavailable")} + p.admission = a + prepareCreateMocks(cli, testImageDigest) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + cli.On("CreateInstance", mock.Anything).Run(func(mock.Arguments) { cancel() }).Return(new(MockOperation), errors.New( + "Failed instance placement scriptlet: insufficient-memory: no fleet member has room for this worker", + )) + _, _, _, err := p.createWarm(ctx, "nddev-linux-standard") + require.ErrorContains(t, err, "journal unavailable") + require.Len(t, a.released, 1, "cleanup must run even when the create context expired") + require.Len(t, a.reserved, 1) +} + +func TestWarmArgumentsFailureReleasesExactReservedIdentity(t *testing.T) { + cli := new(MockIncusServer) + p := newTestProvider(cli) + a := &trackedWarmAdmission{} + p.admission = a + cli.On("GetProfileNames").Return([]string{}, errors.New("profile lookup unavailable")) + _, _, _, err := p.createWarm(t.Context(), "nddev-linux-standard") + require.Error(t, err) + require.Empty(t, a.reserved) + require.Len(t, a.released, 1) + require.Contains(t, a.released[0], "warm-standard-") + cli.AssertNotCalled(t, "CreateInstance", mock.Anything) +} + +func TestWarmAmbiguousCreatePreservesReservationForReconciliation(t *testing.T) { + cli := new(MockIncusServer) + p := newTestProvider(cli) + a := &trackedWarmAdmission{} + p.admission = a + prepareCreateMocks(cli, testImageDigest) + cli.On("CreateInstance", mock.Anything).Return(new(MockOperation), errors.New("connection reset after request")) + _, _, _, err := p.createWarm(t.Context(), "nddev-linux-standard") + require.Error(t, err) + require.Len(t, a.reserved, 1, "an ambiguous create may still allocate a worker") + require.Empty(t, a.released) + cli.AssertNotCalled(t, "DeleteInstance", mock.Anything) +} From cfa97dad6149bdefdd76a4dbed950d11f1ff45b4 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Sun, 6 Sep 2026 07:02:13 +0500 Subject: [PATCH 2/2] chore(provider): release v0.1.5-nddev.125 --- config/example-runner-1.yaml | 2 +- config/example-runner-2.yaml | 2 +- config/example-runner-3.yaml | 2 +- config/example-runner-4.yaml | 2 +- config/example-services.yaml | 2 +- config/provider-derivative.yaml | 6 +++--- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/config/example-runner-1.yaml b/config/example-runner-1.yaml index 3b1e6f3b..656974e3 100644 --- a/config/example-runner-1.yaml +++ b/config/example-runner-1.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.89 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.124 + provider_version: v0.1.5-nddev.125 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-runner-2.yaml b/config/example-runner-2.yaml index 46a83a64..3769557c 100644 --- a/config/example-runner-2.yaml +++ b/config/example-runner-2.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.89 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.124 + provider_version: v0.1.5-nddev.125 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-runner-3.yaml b/config/example-runner-3.yaml index d3e6253f..4d5982f6 100644 --- a/config/example-runner-3.yaml +++ b/config/example-runner-3.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.89 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.124 + provider_version: v0.1.5-nddev.125 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-runner-4.yaml b/config/example-runner-4.yaml index d18d6ae2..e3f794e1 100644 --- a/config/example-runner-4.yaml +++ b/config/example-runner-4.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.89 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.124 + provider_version: v0.1.5-nddev.125 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-services.yaml b/config/example-services.yaml index ec6b11a9..99c88770 100644 --- a/config/example-services.yaml +++ b/config/example-services.yaml @@ -27,7 +27,7 @@ control_plane: manager_version: v0.2.1-nddev.89 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.124 + provider_version: v0.1.5-nddev.125 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/provider-derivative.yaml b/config/provider-derivative.yaml index 7dd11773..fa00cd79 100644 --- a/config/provider-derivative.yaml +++ b/config/provider-derivative.yaml @@ -16,7 +16,7 @@ artifact: garm-provider-incus # state all move together, because all three derive from here. A provider change # that does not bump it ships under the previous version, which is exactly how # runner-1 and runner-2 diverged. -derivative_version: v0.1.5-nddev.124 +derivative_version: v0.1.5-nddev.125 # The external-provider protocol GARM speaks to this binary. It moves on its own # schedule -- a provider release does not imply an interface release -- so it is @@ -37,8 +37,8 @@ runtime: queue_intent_schema_version: 6 build: - source_commit: 28bea87a35a1f4e80ba6f18fb922829c7afa834d - binary_sha256: 9ad4bae4b9b8f2625fee5baf1f31f2a05085ebbecb5b2aba6741a52f961b328f + source_commit: e38153e6e9ca42cdeb74d95c812bad981101db47 + binary_sha256: a381f312e29f1407f6fd678b2b75989b156b99a268376978e5911e40c534d2dc go_version: go1.26.7 cgo_enabled: false target_os: linux