From 3a77ae2562015ad006857deaf2823a84575e3e3c Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 2 Sep 2026 17:06:23 +0500 Subject: [PATCH 1/2] fix(provider): one warm instance that does not arrive no longer fails the pool The warm reconciler treated two ordinary events as fatal. An instance that never published its readiness evidence within the minute returned a timeout, and an instance whose delete had begun answered a file read with "Failed getting instance pool: Instance storage pool not found" rather than 404. Either aborted the whole pool's reconcile, so the depth was not restored during exactly the bursts that consume it: on 2026-09-02 that happened eleven times between 09:35Z and 11:00Z, each one also marking the services host unhealthy for a minute. An instance that never becomes ready is now deleted and recorded in the result as abandoned; the deficit refills on the next pass. An instance that vanished mid-read is skipped, the way one retired during create already was. A pool still fails on anything that is not one instance's own problem. Claude-Session: https://claude.ai/code/session_0128syXKxAGCfJGRDxUUNQXp --- internal/garmproviderincus/provider/incus.go | 6 +- internal/garmproviderincus/provider/warm.go | 45 ++++++++++- .../provider/warm_recycle_test.go | 74 +++++++++++++++++++ 3 files changed, 121 insertions(+), 4 deletions(-) diff --git a/internal/garmproviderincus/provider/incus.go b/internal/garmproviderincus/provider/incus.go index 617ddefd..9b5621c1 100644 --- a/internal/garmproviderincus/provider/incus.go +++ b/internal/garmproviderincus/provider/incus.go @@ -85,10 +85,14 @@ var ( const ( createOperationTimeout = 2 * time.Minute deleteOperationTimeout = time.Minute - stateOperationTimeout = time.Minute defaultPlacementLock = "/var/lib/gha-fleet/placement.lock" ) +// stateOperationTimeout bounds a wait on an instance's own state, the warm +// readiness poll among them. It is a var so a test can shorten it; nothing +// outside a test writes it. +var stateOperationTimeout = time.Minute + const ( directJITPhasePath = "/home/runner/actions-runner/_diag/nddev-direct-jit-phase.log" directJITPhaseMaxBytes = 4096 diff --git a/internal/garmproviderincus/provider/warm.go b/internal/garmproviderincus/provider/warm.go index 62fe10b2..b679ab69 100644 --- a/internal/garmproviderincus/provider/warm.go +++ b/internal/garmproviderincus/provider/warm.go @@ -53,8 +53,12 @@ type WarmPoolResult struct { AdmissionDecision *admission.Decision `json:"admission_decision,omitempty"` Created []string `json:"created"` Consumed []string `json:"consumed_during_create"` - Promoted []string `json:"promoted"` - DeletedExcess []string `json:"deleted_excess"` + // Abandoned names instances that were created but never published their + // readiness evidence in time. They are deleted and the deficit refills on + // the next pass. One instance that does not arrive is not a failed pool. + Abandoned []string `json:"abandoned_during_create,omitempty"` + Promoted []string `json:"promoted"` + DeletedExcess []string `json:"deleted_excess"` } type WarmDrainResult struct { @@ -200,6 +204,11 @@ func (l *Incus) ReconcileWarm(ctx context.Context, flavor string, apply bool) (W for _, name := range preparing { promoted, err := l.promoteWarmReady(ctx, name, flavor) if err != nil { + if warmInstanceRetiredDuringCreate(err) { + // It was claimed or deleted while this pass read it; the + // next pass sees the resulting inventory. + continue + } return result, err } if promoted { @@ -232,6 +241,10 @@ func (l *Incus) ReconcileWarm(ctx context.Context, flavor string, apply bool) (W for range deficit { name, consumed, decision, err := l.createWarm(ctx, flavor) if err != nil { + if errors2.Is(err, errWarmNotReady) { + result.Abandoned = append(result.Abandoned, name) + continue + } return result, err } if consumed { @@ -318,6 +331,17 @@ func (l *Incus) createWarm(ctx context.Context, flavor string) (name string, con return "", false, nil, errors.Wrap(err, "waiting for warm instance network") } if err = l.waitWarmReady(ctx, name, flavor, 2*time.Second); err != nil { + if errors2.Is(err, runnerErrors.ErrTimeout) { + // The instance exists but never published readiness. It will never + // be claimed, and leaving it costs the pool a slot, so it is + // deleted and the deficit refills on the next pass. Failing here + // instead aborted the whole pool's reconcile -- so the depth was + // not restored during exactly the bursts that consume it. + if deleteErr := l.DeleteInstance(ctx, name); deleteErr != nil { + return "", false, nil, errors.Wrapf(deleteErr, "deleting warm instance %q that never became ready", name) + } + return name, false, nil, errWarmNotReady + } if warmInstanceRetiredDuringCreate(err) { err = nil return name, true, nil, nil @@ -336,7 +360,18 @@ func warmInstanceRetiredDuringCreate(err error) bool { // hides inside "attempt count exceeded: fetching instance: ...". A warm // instance that vanished during create was consumed by useful concurrent // work, whatever the wrapper says. - return err != nil && strings.Contains(err.Error(), "Instance not found") + // + // Incus does not always answer 404 for an instance that is going away. + // Reading a file from one whose delete has begun answers "Failed getting + // instance pool: Instance storage pool not found", and that read is + // exactly what the readiness poll does: on 2026-09-02 it failed the whole + // pool's reconcile eleven times. + message := "" + if err != nil { + message = err.Error() + } + return strings.Contains(message, "Instance not found") || + strings.Contains(message, "Instance storage pool not found") } // isPlacementRefusal recognizes the scriptlet's per-member capacity refusal @@ -350,6 +385,10 @@ func isPlacementRefusal(err error) bool { strings.Contains(message, "no fleet member has room") } +// errWarmNotReady marks an instance that was created but never published its +// readiness evidence: the reconciler deletes it and carries on with the pool. +var errWarmNotReady = errors2.New("warm instance never became ready") + func (l *Incus) waitWarmReady(ctx context.Context, name, flavor string, pollInterval time.Duration) error { if pollInterval <= 0 { return fmt.Errorf("warm readiness poll interval must be positive") diff --git a/internal/garmproviderincus/provider/warm_recycle_test.go b/internal/garmproviderincus/provider/warm_recycle_test.go index a493a674..6970b52b 100644 --- a/internal/garmproviderincus/provider/warm_recycle_test.go +++ b/internal/garmproviderincus/provider/warm_recycle_test.go @@ -2,8 +2,14 @@ package provider import ( "context" + "fmt" + "io" + "strings" "testing" + "time" + "github.com/cloudbase/garm-provider-common/errors" + incus "github.com/lxc/incus/v7/client" "github.com/lxc/incus/v7/shared/api" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -122,3 +128,71 @@ func TestReconcileWarmRecyclesAnInstanceWhosePoolCapabilityMoved(t *testing.T) { require.Equal(t, 0, result.ReadyBefore) cli.AssertNotCalled(t, "DeleteInstance", mock.Anything) } + +// One warm instance that never publishes readiness is deleted and recorded, +// and the pool's reconcile carries on: failing it instead left the depth +// unrestored during exactly the bursts that consume it (2026-09-02, eleven +// aborted reconciles). +func TestWarmInstanceThatNeverBecomesReadyIsAbandonedNotFatal(t *testing.T) { + previous := stateOperationTimeout + stateOperationTimeout = 20 * time.Millisecond + t.Cleanup(func() { stateOperationTimeout = previous }) + + cli := new(MockIncusServer) + provider := newTestProvider(cli) + setWarmTarget(provider, "nddev-linux-standard", 1) + cli.On("GetInstancesFull", api.InstanceTypeAny).Return([]api.InstanceFull{}, nil).Once() + prepareCreateMocks(cli, testImageDigest) + operation := new(MockOperation) + operation.On("WaitContext", mock.Anything).Return(nil) + cli.On("CreateInstance", mock.Anything).Return(operation, nil).Once() + cli.On("UpdateInstanceState", mock.Anything, api.InstanceStatePut{Action: "start", Timeout: -1}, "").Return(operation, nil).Maybe() + // The instance is created and reachable, but its readiness evidence never + // appears, so every poll finds nothing and the wait times out. + created := warmInstance("warm-standard-unready") + created.ExpandedConfig[lifecycleKey] = lifecycleWarmPreparing + created.ExpandedConfig[warmReadyKey] = "" + created.State = &api.InstanceState{Status: "Running", Network: map[string]api.InstanceStateNetwork{ + "eth0": {Addresses: []api.InstanceStateNetworkAddress{{Family: "inet", Scope: "global", Address: "10.0.0.5"}}}, + }} + cli.On("GetInstanceFull", mock.Anything).Return(created, "etag", nil).Maybe() + cli.On("GetInstanceFile", mock.Anything, warmReadyGuestPath). + Return(io.NopCloser(strings.NewReader("")), (*incus.InstanceFileResponse)(nil), fmt.Errorf("evidence absent: %w", errors.ErrNotFound)).Maybe() + cli.On("DeleteInstance", mock.Anything).Return(operation, nil).Maybe() + cli.On("UpdateInstanceState", mock.Anything, api.InstanceStatePut{Action: "stop", Timeout: -1, Force: true}, "").Return(operation, nil).Maybe() + + result, err := provider.ReconcileWarm(context.Background(), "nddev-linux-standard", true) + require.NoError(t, err, "one unready instance must not fail the pool") + require.Len(t, result.Abandoned, 1) + require.Empty(t, result.Created) +} + +// Incus answers a file read on an instance whose delete has begun with +// "Instance storage pool not found", not with a 404; that is the instance +// going away, not a broken pool. +func TestAVanishingInstanceIsRecognisedByItsStoragePoolError(t *testing.T) { + require.True(t, warmInstanceRetiredDuringCreate(fmt.Errorf("reading warm readiness evidence: Failed getting instance pool: Instance storage pool not found"))) + require.True(t, warmInstanceRetiredDuringCreate(fmt.Errorf("attempt count exceeded: fetching instance: Instance not found"))) + require.False(t, warmInstanceRetiredDuringCreate(fmt.Errorf("storage pool is full"))) + require.False(t, warmInstanceRetiredDuringCreate(nil)) +} + +// The preparing loop reads each instance's readiness evidence. One that is +// being deleted answers with the storage-pool error; the pass must skip it +// and keep reconciling, not abort the pool (2026-09-02, 10:59Z). +func TestPreparingInstanceThatVanishesMidReadIsSkipped(t *testing.T) { + cli := new(MockIncusServer) + provider := newTestProvider(cli) + setWarmTarget(provider, "nddev-linux-standard", 0) + preparing := warmInstance("warm-standard-vanishing") + preparing.ExpandedConfig[lifecycleKey] = lifecycleWarmPreparing + cli.On("GetInstancesFull", api.InstanceTypeAny).Return([]api.InstanceFull{*preparing}, nil).Once() + cli.On("GetInstanceFull", preparing.Name).Return(preparing, "etag", nil).Maybe() + cli.On("GetInstanceFile", preparing.Name, warmReadyGuestPath). + Return(io.NopCloser(strings.NewReader("")), (*incus.InstanceFileResponse)(nil), + fmt.Errorf("Failed getting instance pool: Instance storage pool not found")).Once() + + result, err := provider.ReconcileWarm(context.Background(), "nddev-linux-standard", true) + require.NoError(t, err, "an instance that vanished mid-read must not fail the pool") + require.Empty(t, result.Promoted) +} From 5685ff456bbba1e610531c6b47fa1bc158ec9fc7 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 2 Sep 2026 17:06:26 +0500 Subject: [PATCH 2/2] chore(provider): release v0.1.5-nddev.120, a pool survives one absent instance Source 3a77ae2562015ad006857deaf2823a84575e3e3c, built twice with CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags "-buildid= -s -w -X main.version=v0.1.5-nddev.120 -X main.commit="; both builds agree on 3c07b99b8d11f1bcded6208147540522aaa37302bf2203da78937bdbd2c70368. Claude-Session: https://claude.ai/code/session_0128syXKxAGCfJGRDxUUNQXp --- 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 c7709e34..0a84522b 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.88 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.119 + provider_version: v0.1.5-nddev.120 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 da10a2b5..ccc022fe 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.88 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.119 + provider_version: v0.1.5-nddev.120 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 fa5b0c77..f60e5e79 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.88 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.119 + provider_version: v0.1.5-nddev.120 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 c2b353cc..1a9734be 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.88 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.119 + provider_version: v0.1.5-nddev.120 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 5aad1526..0da65256 100644 --- a/config/example-services.yaml +++ b/config/example-services.yaml @@ -27,7 +27,7 @@ control_plane: manager_version: v0.2.1-nddev.88 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.119 + provider_version: v0.1.5-nddev.120 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 f6c34553..4f10b5d1 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.119 +derivative_version: v0.1.5-nddev.120 # 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: de15f14e23425201b4c18cfd989beffe3ccb5d95 - binary_sha256: c3aaa8e9e17f19d1cf81d5c7e99686a10980e2f9260a5d7e00186f5f09e81069 + source_commit: 3a77ae2562015ad006857deaf2823a84575e3e3c + binary_sha256: 3c07b99b8d11f1bcded6208147540522aaa37302bf2203da78937bdbd2c70368 go_version: go1.26.7 cgo_enabled: false target_os: linux