Loading
Stop counting instances already being removed in scale down
What does this MR do?
Makes RemoveIfExpired() return false for an instance that is already marked for removal, so that a scale down no longer counts it a second time. removeAllInstances(), its other caller, now skips such instances through a new Instance.IsRemoving().
Why was this MR needed?
When desired() returns a scale down of n, scale() walks the instance list, oldest first, and counts every instance for which RemoveIfExpired() returns true until it has counted n (taskscaler.go#L969-977). RemoveIfExpired() does not check state.Removing (instance.go#L826-868), so an idle instance that an earlier tick marked for removal, which still has no slots and is still past its idle time, returns true again, while remove() returns early and deletes nothing (instance.go#L879-889).
That instance stays in the list until fleeting reports it deleted. Fleeting's Capacity() already counts it under Deleting, because Delete() sets removing (provisioner.go#L238-280, at the version in go.mod), so InstanceCount in desired() has already left it out (taskscaler.go#L903) and the scale down it returns is meant for other instances. After a restart, an instance restored with Removing set while the cloud reports it suspended is counted under Suspended instead, which InstanceCount leaves out too. While a slow cloud deletion is in flight, the instances being deleted that come earlier in the list count against that scale down on every tick, so an idle instance after them that goes past its idle time keeps running until the earlier deletions finish.
Behavior change: RemoveIfExpired() now returns false, and saves no state, for an instance already marked for removal. On main its only other caller is removeAllInstances() on shutdown (taskscaler.go#L396-417), which would then warn "instance couldn't be removed" for those instances. It skips them instead and logs "instance already being removed" at debug level, so for an instance that is already being deleted it neither warns nor logs "instance removed" a second time.
What's the best way to test this MR?
go test ./internal/ -run TestRemoveIfExpired -count=1 -v
go test . -run 'TestScaleDownSkipsInstancesAlreadyRemoving|TestRemoveAllInstancesSkipsInstancesAlreadyRemoving|TestScaleDownWhileDeletionInProgress' -count=1 -vTestRemoveIfExpired gets a no expire when already removing case. Two root tests build a taskscaler holding two idle instances past their idle time, the first already marked for removal, and check that scale(-1) deletes the second one, and that shutdown removes only the second one without a warning. TestScaleDownWhileDeletionInProgress, added in review, runs desired() with the real fleeting provisioner and the dummy plugin holding a deletion in Deleting, and checks that an idle instance past its idle time is still removed. All four fail on main and pass with this change, and make test passes, mocks and modules included, with Go 1.26.10, the GO_VERSION of the test job.
Other checks, and the test output on main and with this change
With the same Go, go vet ./... passes, and gofmt -l lists none of the changed files. go test -race passes on ./internal/..., ./cron/... and ./storage/..., and on the root package except for TestPotentialCapacityWithOutOfBandScaling, which also fails on main, often under -race and rarely without it, as it did in the pipeline of 9bd40557. My note of 4 November has the numbers and the cause, which this change does not touch. Removing the guard in RemoveIfExpired() makes TestRemoveIfExpired, TestScaleDownSkipsInstancesAlreadyRemoving and TestScaleDownWhileDeletionInProgress fail, and removing the skip in removeAllInstances() makes TestRemoveAllInstancesSkipsInstancesAlreadyRemoving fail.
On main (6d4e8d9a) with this MR's tests and without the fix:
--- FAIL: TestRemoveIfExpired/no_expire_when_already_removing (0.00s)
Error: Not equal:
expected: false
actual : true
FAIL gitlab.com/gitlab-org/fleeting/taskscaler/internal 0.005s
--- FAIL: TestScaleDownSkipsInstancesAlreadyRemoving (0.00s)
Error: Not equal:
expected: 1
actual : 0
Messages: idle instance past its idle time
--- FAIL: TestRemoveAllInstancesSkipsInstancesAlreadyRemoving (0.00s)
Error: Not equal:
expected: 1
actual : 2
--- FAIL: TestScaleDownWhileDeletionInProgress (16.05s)
Error: Condition never satisfied
Messages: idle instance not removed while another deletion is in progress
FAIL gitlab.com/gitlab-org/fleeting/taskscaler 16.057sWith this change:
--- PASS: TestRemoveIfExpired/no_expire_when_already_removing (0.00s)
ok gitlab.com/gitlab-org/fleeting/taskscaler/internal 0.008s
--- PASS: TestScaleDownSkipsInstancesAlreadyRemoving (0.00s)
--- PASS: TestRemoveAllInstancesSkipsInstancesAlreadyRemoving (0.00s)
--- PASS: TestScaleDownWhileDeletionInProgress (3.05s)
ok gitlab.com/gitlab-org/fleeting/taskscaler 3.057smake test on this branch:
ok gitlab.com/gitlab-org/fleeting/taskscaler 456.560s
ok gitlab.com/gitlab-org/fleeting/taskscaler/cron 0.008s
ok gitlab.com/gitlab-org/fleeting/taskscaler/internal 1.967s
ok gitlab.com/gitlab-org/fleeting/taskscaler/internal/capacity 0.010s
ok gitlab.com/gitlab-org/fleeting/taskscaler/internal/ratelimit 0.008s
ok gitlab.com/gitlab-org/fleeting/taskscaler/storage 0.013s
Mocks are up-to-date!
Go modules are tidy and complete!What are the relevant issue numbers?
I found no issue about this. No open merge request changes the scale down loop or removeAllInstances(). !109 (merged) changes RemoveIfExpired() below the check this MR adds, replacing its slot loop with hasLiveSlots(). !126 adds a third caller, removeUnclaimed(), which calls RemoveIfExpired() only for an instance that is not being removed, so this change does not affect it; its new tests pass with both changes merged. !109 (merged), !118 (merged), !123 (merged) and !126 also edit internal/instance.go or taskscaler.go, and this branch merges cleanly with each of them.
Edited by José M. Requena Plens