From e0b94662e9a955455cdd2947be5f5c2cfca66634 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 3 Nov 2026 22:16:14 +0100 Subject: [PATCH] fix(deployments): apply the concurrent build limit of build servers The queue counted every running deployment against the deployment server, and the job picked a build server at random. So the deployment server limit capped builds on build servers, and the build server limit was never read. Count each build on the server that runs it, reserve a build server with a free slot when the deployment starts, and start waiting deployments of other servers when a build server slot is freed. Closes #7649 --- app/Jobs/ApplicationDeploymentJob.php | 12 +- app/Models/Application.php | 34 ++++ bootstrap/helpers/applications.php | 72 +++++++-- .../BuildServerConcurrentBuildsTest.php | 149 ++++++++++++++++++ .../Feature/BuildServerFallbackPolicyTest.php | 54 ++++++- 5 files changed, 303 insertions(+), 18 deletions(-) create mode 100644 tests/Feature/BuildServerConcurrentBuildsTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 057b6ec158..fb74d1ded1 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -530,8 +530,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue // A deployments-only server never builds. Docker image and Compose applications are exempt: // the first builds nothing and the second does not support build servers. - $mustBuildElsewhere = ! $this->server->canBuildApplications() - && ! in_array($this->application->build_pack, ['dockerimage', 'dockercompose'], true); + $mustBuildElsewhere = $this->application->mustBuildOnBuildServer($this->server); if (! $mustBuildElsewhere && ! data_get($this->application, 'settings.is_build_server_enabled')) { return; @@ -542,7 +541,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue } $team = $this->application->environment->project->team; - $buildServers = Server::buildServers($team->id)->whereKeyNot($this->server->id)->get(); + $buildServers = $this->application->buildServerCandidates($this->server); if ($buildServers->isEmpty()) { // A restart only rebuilds when the image is missing, so it may still run on the deployment server. @@ -559,7 +558,12 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue return; } - $this->build_server = $buildServers->random(); + // Admission reserved a build server with a free build slot. A forced start reserved none, and + // the reserved server can become unusable before the job runs. + $reservedId = $this->application_deployment_queue->build_server_id; + $this->build_server = ($reservedId === null ? null : $buildServers->firstWhere('id', $reservedId)) + ?? least_busy_build_server($buildServers) + ?? least_busy_build_server($buildServers, onlyWithFreeSlot: false); // Saved now: the cancel action looks for the helper container on this server. $this->application_deployment_queue->update(['build_server_id' => $this->build_server->id]); $this->application_deployment_queue->addLogEntry("Found a suitable build server ({$this->build_server->name})."); diff --git a/app/Models/Application.php b/app/Models/Application.php index 3b7de984ac..6d6cacf5fd 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -1116,6 +1116,40 @@ class Application extends BaseModel return data_get($this, 'environment.project.team'); } + /** + * A deployments-only server cannot build this application, so a build server must build it. + * Docker image applications build nothing, and Compose applications do not use build servers. + */ + public function mustBuildOnBuildServer(Server $server): bool + { + return ! $server->canBuildApplications() + && ! in_array($this->build_pack, ['dockerimage', 'dockercompose'], true); + } + + /** + * The build servers that can build a deployment of this application on $server. It is empty when + * the deployment builds on $server, or when $server is an additional server that pulls the image. + * + * @return Collection + */ + public function buildServerCandidates(Server $server): Collection + { + $pullsImage = str($this->docker_registry_image_name)->isNotEmpty() + && $this->additional_servers()->wherePivot('server_id', $server->id)->exists(); + if ($pullsImage) { + return collect(); + } + if (! $this->mustBuildOnBuildServer($server) && ! data_get($this, 'settings.is_build_server_enabled')) { + return collect(); + } + $teamId = $this->team()?->id; + if ($teamId === null) { + return collect(); + } + + return Server::buildServers($teamId)->whereKeyNot($server->id)->get(); + } + /** * Another application of the team that builds and pushes an image with the same Docker image name. * Both push the same tags (the commit SHA, `latest`, `pr-`), so one application can run the image of the other. diff --git a/bootstrap/helpers/applications.php b/bootstrap/helpers/applications.php index 908273043f..39f9702ff6 100644 --- a/bootstrap/helpers/applications.php +++ b/bootstrap/helpers/applications.php @@ -102,7 +102,7 @@ function queue_application_deployment(Application $application, string $deployme // Decide whether to start while the application and server rows are still locked, // so concurrent requests cannot both see an idle queue and start two deployments. // After a cancelled build, CancelSupersededDeploymentJob starts it once the build is stopped. - $started = $superseded['building']->isEmpty() && ($no_questions_asked || next_queuable($server_id, $application_id, $commit, $pull_request_id)); + $started = $superseded['building']->isEmpty() && ($no_questions_asked || next_queuable($deployment)); if ($started) { $deployment->update([ 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, @@ -270,8 +270,14 @@ function start_next_queued_deployments(Application $application, ?int $finished_ ->where('status', ApplicationDeploymentStatus::QUEUED) ->distinct() ->pluck('server_id'); + // A build on a build server frees a slot that deployments on other servers of the team can wait for. + $primary_server = $application->destination?->server; + $build_server_team_server_ids = $primary_server && $application->buildServerCandidates($primary_server)->isNotEmpty() + ? Server::where('team_id', $primary_server->team_id)->pluck('id') + : collect(); $server_ids = collect([$application->destination?->server_id, $finished_deployment_server_id]) ->merge($application_queued_server_ids) + ->merge($build_server_team_server_ids) ->filter(fn ($server_id) => $server_id !== null) ->unique() ->values(); @@ -306,7 +312,7 @@ function start_queued_deployment(ApplicationDeploymentQueue $deployment, bool $f return false; } - if (! $force && ! next_queuable($current->server_id, $current->application_id, $current->commit, $current->pull_request_id)) { + if (! $force && ! next_queuable($current)) { return false; } @@ -381,12 +387,18 @@ function fail_undispatchable_deployment(ApplicationDeploymentQueue $deployment, } } -function next_queuable(string $server_id, string $application_id, string $commit = 'HEAD', int $pull_request_id = 0): bool +/** + * Decide whether a queued deployment can start now. Each deployment holds one build slot of the + * server that builds it: its deployment server, or a build server of the team. When it builds on a + * build server, it reserves the least busy one with a free slot in build_server_id, which the caller + * saves with the status change. Call it while the application and deployment server rows are locked. + */ +function next_queuable(ApplicationDeploymentQueue $deployment): bool { // Check if there's already a deployment in progress for this application with the same pull_request_id // This allows normal deployments and PR deployments to run concurrently - $in_progress = ApplicationDeploymentQueue::where('application_id', $application_id) - ->where('pull_request_id', $pull_request_id) + $in_progress = ApplicationDeploymentQueue::where('application_id', $deployment->application_id) + ->where('pull_request_id', $deployment->pull_request_id) ->where('status', ApplicationDeploymentStatus::IN_PROGRESS->value) ->exists(); @@ -394,23 +406,57 @@ function next_queuable(string $server_id, string $application_id, string $commit return false; } - // Check server's concurrent build limit // A deleted server keeps its queued deployments; they cannot run anymore. - $server = Server::find($server_id); - if (! $server) { + $server = Server::find($deployment->server_id); + $application = Application::find($deployment->application_id); + if (! $server || ! $application) { return false; } - $concurrent_builds = $server->settings->concurrent_builds; - $active_deployments = ApplicationDeploymentQueue::where('server_id', $server_id) - ->where('status', ApplicationDeploymentStatus::IN_PROGRESS->value) - ->count(); - if ($active_deployments >= $concurrent_builds) { + $buildServers = $application->buildServerCandidates($server); + if ($buildServers->isEmpty()) { + return active_builds_on_server($server->id) < $server->settings->concurrent_builds; + } + + // Deployments of other servers can take the same build slots, so lock the build servers too. + Server::query()->whereKey($buildServers->modelKeys())->orderBy('id')->lockForUpdate()->get(); + $buildServer = least_busy_build_server($buildServers); + if (! $buildServer) { return false; } + $deployment->build_server_id = $buildServer->id; return true; } + +/** + * The running deployments that build on a server: its own deployments without a build server, + * and the deployments that it builds as a build server. + */ +function active_builds_on_server(int $server_id): int +{ + return ApplicationDeploymentQueue::where('status', ApplicationDeploymentStatus::IN_PROGRESS->value) + ->where(fn ($query) => $query->where('build_server_id', $server_id) + ->orWhere(fn ($query) => $query->where('server_id', $server_id)->whereNull('build_server_id'))) + ->count(); +} + +/** + * The build server with the fewest running builds, at random among equals. With $onlyWithFreeSlot, + * a server that already runs as many builds as its "Concurrent builds" setting allows is skipped, + * and null means that all servers are full. + * + * @param Collection $buildServers + */ +function least_busy_build_server(Collection $buildServers, bool $onlyWithFreeSlot = true): ?Server +{ + return $buildServers + ->map(fn (Server $buildServer) => ['server' => $buildServer, 'builds' => active_builds_on_server($buildServer->id)]) + ->filter(fn (array $candidate) => ! $onlyWithFreeSlot || $candidate['builds'] < $candidate['server']->settings->concurrent_builds) + ->shuffle() + ->sortBy('builds') + ->first()['server'] ?? null; +} /** * Start the queued deployments that can run after a cancellation. With the application of the * cancelled deployment, its queued deployments on all servers start too, as when a deployment ends. diff --git a/tests/Feature/BuildServerConcurrentBuildsTest.php b/tests/Feature/BuildServerConcurrentBuildsTest.php new file mode 100644 index 0000000000..8428fb00f9 --- /dev/null +++ b/tests/Feature/BuildServerConcurrentBuildsTest.php @@ -0,0 +1,149 @@ + Process::result(output: '')]); + config([ + 'constants.ssh.mux_enabled' => false, + 'cache.default' => 'array', + 'app.maintenance.store' => 'array', + ]); + + $settings = new InstanceSettings; + $settings->id = 0; + $settings->save(); + + $this->team = Team::factory()->create(); + $privateKey = PrivateKey::factory()->create(['team_id' => $this->team->id]); + $environment = Environment::factory()->create([ + 'project_id' => Project::factory()->create(['team_id' => $this->team->id])->id, + ]); + $ip = 40; + + $this->makeServer = function (int $concurrentBuilds, string $role = 'both') use ($privateKey, &$ip): Server { + $server = Server::factory()->create(['team_id' => $this->team->id, 'private_key_id' => $privateKey->id, 'ip' => '203.0.113.'.$ip++]); + $server->settings->update([ + 'concurrent_builds' => $concurrentBuilds, + 'server_role' => $role, + 'is_build_server' => $role === 'build', + 'is_reachable' => true, + 'is_usable' => true, + ]); + + return $server->fresh(); + }; + + $this->makeApplication = function (Server $server, bool $usesBuildServer = true) use ($environment): Application { + $destination = StandaloneDocker::where('server_id', $server->id)->firstOrFail(); + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + $application->settings->update(['is_build_server_enabled' => $usesBuildServer]); + + return $application->fresh(); + }; + + $this->deploy = function (Application $application): ApplicationDeploymentQueue { + $uuid = new_public_id(); + queue_application_deployment($application, $uuid); + + return ApplicationDeploymentQueue::where('deployment_uuid', $uuid)->firstOrFail(); + }; +}); + +afterEach(function () { + Server::flushIdentityMap(); +}); + +test('the deployment server limit does not cap builds that run on a build server', function () { + $deploymentServer = ($this->makeServer)(1); + $buildServer = ($this->makeServer)(2, 'build'); + + $first = ($this->deploy)(($this->makeApplication)($deploymentServer)); + $second = ($this->deploy)(($this->makeApplication)($deploymentServer)); + $third = ($this->deploy)(($this->makeApplication)($deploymentServer)); + + expect($first->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value) + ->and($first->build_server_id)->toBe($buildServer->id) + ->and($second->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value) + ->and($second->build_server_id)->toBe($buildServer->id) + ->and($third->status)->toBe(ApplicationDeploymentStatus::QUEUED->value) + ->and($third->build_server_id)->toBeNull(); +}); + +test('the build server limit holds for deployments of different deployment servers', function () { + $buildServer = ($this->makeServer)(1, 'build'); + + $first = ($this->deploy)(($this->makeApplication)(($this->makeServer)(5))); + $second = ($this->deploy)(($this->makeApplication)(($this->makeServer)(5))); + + expect($first->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value) + ->and($first->build_server_id)->toBe($buildServer->id) + ->and($second->status)->toBe(ApplicationDeploymentStatus::QUEUED->value); + Queue::assertPushed(ApplicationDeploymentJob::class, 1); +}); + +test('a deployment reserves the build server that has a free build slot', function () { + $deploymentServer = ($this->makeServer)(5); + $buildServers = collect([($this->makeServer)(1, 'build'), ($this->makeServer)(1, 'build')]); + + $first = ($this->deploy)(($this->makeApplication)($deploymentServer)); + $second = ($this->deploy)(($this->makeApplication)($deploymentServer)); + + expect([$first->build_server_id, $second->build_server_id]) + ->toEqualCanonicalizing($buildServers->pluck('id')->all()) + ->and($second->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value); +}); + +test('a finished build starts the deployment of another deployment server that waits for the build server', function () { + $buildServer = ($this->makeServer)(1, 'build'); + $running = ($this->deploy)(($this->makeApplication)(($this->makeServer)(5))); + $waiting = ($this->deploy)(($this->makeApplication)(($this->makeServer)(5))); + expect($waiting->status)->toBe(ApplicationDeploymentStatus::QUEUED->value); + + (new ApplicationDeploymentJob($running->id))->failed(new RuntimeException('build failed')); + + $waiting->refresh(); + expect($waiting->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value) + ->and($waiting->build_server_id)->toBe($buildServer->id); + Queue::assertPushed(ApplicationDeploymentJob::class, fn (ApplicationDeploymentJob $job) => $job->application_deployment_queue_id === $waiting->id); +}); + +test('builds on a build server do not take the build slots of the deployment server', function () { + $deploymentServer = ($this->makeServer)(1); + ($this->makeServer)(1, 'build'); + + $onBuildServer = ($this->deploy)(($this->makeApplication)($deploymentServer)); + $onDeploymentServer = ($this->deploy)(($this->makeApplication)($deploymentServer, usesBuildServer: false)); + $waiting = ($this->deploy)(($this->makeApplication)($deploymentServer, usesBuildServer: false)); + + expect($onBuildServer->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value) + ->and($onDeploymentServer->status)->toBe(ApplicationDeploymentStatus::IN_PROGRESS->value) + ->and($onDeploymentServer->build_server_id)->toBeNull() + ->and($waiting->status)->toBe(ApplicationDeploymentStatus::QUEUED->value); +}); diff --git a/tests/Feature/BuildServerFallbackPolicyTest.php b/tests/Feature/BuildServerFallbackPolicyTest.php index f7a366e1c4..b9fdcd5717 100644 --- a/tests/Feature/BuildServerFallbackPolicyTest.php +++ b/tests/Feature/BuildServerFallbackPolicyTest.php @@ -23,7 +23,7 @@ afterEach(function () { Server::flushIdentityMap(); }); -function makeBuildServerSelectionJob(Team $team, Server $deploymentServer, array $applicationAttributes = [], bool $buildServerEnabled = true, bool $restartOnly = false): array +function makeBuildServerSelectionJob(Team $team, Server $deploymentServer, array $applicationAttributes = [], bool $buildServerEnabled = true, bool $restartOnly = false, ?int $reservedBuildServerId = null): array { $project = new Project; $project->setRelation('team', $team); @@ -40,6 +40,7 @@ function makeBuildServerSelectionJob(Team $team, Server $deploymentServer, array $application->setRelation('settings', $settings); $deploymentQueue = Mockery::mock(ApplicationDeploymentQueue::class); + $deploymentQueue->shouldReceive('getAttribute')->with('build_server_id')->andReturn($reservedBuildServerId)->byDefault(); $job = (new ReflectionClass(ApplicationDeploymentJob::class))->newInstanceWithoutConstructor(); foreach ([ @@ -501,3 +502,54 @@ test('the selected build server is saved on the deployment so a cancel finds the expect($deployment->build_server_id)->toBe($buildServer->id) ->and(ApplicationDeploymentQueue::query()->find($deployment->id)->build_server_id)->toBe($buildServer->id); }); + +function makeAvailableBuildServer(Team $team, int $concurrentBuilds = 1): Server +{ + $server = Server::factory()->create(['team_id' => $team->id]); + $server->settings()->update([ + 'is_reachable' => true, + 'is_usable' => true, + 'server_role' => 'build', + 'is_build_server' => true, + 'concurrent_builds' => $concurrentBuilds, + ]); + + return $server; +} + +test('the job builds on the build server that admission reserved', function () { + $team = Team::factory()->create(); + $deploymentServer = Server::factory()->create(['team_id' => $team->id]); + makeAvailableBuildServer($team); + $reserved = makeAvailableBuildServer($team); + [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer, reservedBuildServerId: $reserved->id); + + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $reserved->id])->once()->andReturnTrue(); + $deploymentQueue->shouldReceive('addLogEntry')->once(); + + invokeBuildServerSelection($job); + + expect(selectedBuildServer($job)->is($reserved))->toBeTrue(); +}); + +test('a force-started job without a reservation builds on a build server with a free build slot', function () { + $team = Team::factory()->create(); + $deploymentServer = Server::factory()->create(['team_id' => $team->id]); + $full = makeAvailableBuildServer($team); + $free = makeAvailableBuildServer($team); + ApplicationDeploymentQueue::create([ + 'application_id' => 1, + 'deployment_uuid' => 'running-on-full-build-server', + 'status' => 'in_progress', + 'server_id' => $deploymentServer->id, + 'build_server_id' => $full->id, + ]); + [$job, $deploymentQueue] = makeBuildServerSelectionJob($team, $deploymentServer); + + $deploymentQueue->shouldReceive('update')->with(['build_server_id' => $free->id])->once()->andReturnTrue(); + $deploymentQueue->shouldReceive('addLogEntry')->once(); + + invokeBuildServerSelection($job); + + expect(selectedBuildServer($job)->is($free))->toBeTrue(); +});