mirror of
https://github.com/coollabsio/coolify.git
synced 2026-11-06 13:10:37 -04:00
fix(servers): skip patch check results in bulk settings loads
ServerManagerJob and the Docker cleanup step of ScheduledJobManager load the settings of every server with SELECT *. That reads the patch_check_result JSON, which holds the full update list of each server. With 12,709 rows, the scan took about 4 ms, but the full fetch took 390-536 ms and 294 MB of PHP memory. Without the column it took 74 ms and 111 MB. Add ServerSetting::withoutPatchCheckResult() and use it in both loops. Queued jobs reload the full settings through SerializesModels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
c34f5f8e3b
commit
3d956c110c
@@ -154,3 +154,6 @@
|
||||
|
||||
## Validate input safely
|
||||
- In PCRE, `$` also matches before a final `\n`. In validation regexes, use `\z` or reject control characters first, or a trailing newline passes.
|
||||
|
||||
## Measure database reads end to end
|
||||
- On PostgreSQL 15, `EXPLAIN (ANALYZE, BUFFERS)` does not detoast or send the selected columns, so a wide `jsonb`/`json`/`text` column does not show in it. Time the full fetch (`psql \timing` with `-o /dev/null`, or PDO `fetchAll`) as well before you blame or rule out an index.
|
||||
|
||||
@@ -17,6 +17,7 @@ use Cron\CronExpression;
|
||||
use Illuminate\Bus\Queueable;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
use Illuminate\Database\Eloquent\Builder;
|
||||
use Illuminate\Database\Eloquent\Relations\HasOne;
|
||||
use Illuminate\Database\Eloquent\Relations\MorphTo;
|
||||
use Illuminate\Foundation\Bus\Dispatchable;
|
||||
use Illuminate\Queue\InteractsWithQueue;
|
||||
@@ -741,7 +742,7 @@ class ScheduledJobManager implements ShouldQueue
|
||||
|
||||
private function getServersForCleanupQuery(): Builder
|
||||
{
|
||||
$query = Server::with('settings')
|
||||
$query = Server::with(['settings' => fn (HasOne $query) => $query->withoutPatchCheckResult()])
|
||||
->whereNotNull('ip')
|
||||
->where('ip', '!=', '')
|
||||
->whereNotIn('ip', Server::PLACEHOLDER_IPS);
|
||||
|
||||
@@ -8,6 +8,7 @@ use App\Models\Team;
|
||||
use Illuminate\Bus\Queueable;
|
||||
use Illuminate\Contracts\Queue\ShouldBeEncrypted;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
use Illuminate\Database\Eloquent\Relations\HasOne;
|
||||
use Illuminate\Foundation\Bus\Dispatchable;
|
||||
use Illuminate\Queue\InteractsWithQueue;
|
||||
use Illuminate\Queue\SerializesModels;
|
||||
@@ -78,11 +79,12 @@ class ServerManagerJob implements ShouldBeEncrypted, ShouldQueue
|
||||
|
||||
private function getServers(): Collection
|
||||
{
|
||||
$allServers = Server::with(['settings', 'cloudProviderToken']);
|
||||
$relations = ['settings' => fn (HasOne $query) => $query->withoutPatchCheckResult(), 'cloudProviderToken'];
|
||||
$allServers = Server::with($relations);
|
||||
|
||||
if (isCloud()) {
|
||||
$servers = $allServers->whereRelation('team.subscription', 'stripe_invoice_paid', true)->get();
|
||||
$own = Team::find(0)->servers()->with(['settings', 'cloudProviderToken'])->get();
|
||||
$own = Team::find(0)->servers()->with($relations)->get();
|
||||
|
||||
return $servers->merge($own)->unique('id')->values();
|
||||
} else {
|
||||
|
||||
@@ -5,10 +5,12 @@ namespace App\Models;
|
||||
use App\Enums\ServerRole;
|
||||
use App\Enums\TrafficIpMode;
|
||||
use Illuminate\Contracts\Encryption\DecryptException;
|
||||
use Illuminate\Database\Eloquent\Builder;
|
||||
use Illuminate\Database\Eloquent\Casts\Attribute;
|
||||
use Illuminate\Database\Eloquent\Model;
|
||||
use Illuminate\Support\Carbon;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
use Illuminate\Support\Facades\Schema;
|
||||
use OpenApi\Attributes as OA;
|
||||
|
||||
#[OA\Schema(
|
||||
@@ -231,6 +233,9 @@ class ServerSetting extends Model
|
||||
'geoip_maxmind_license_key',
|
||||
];
|
||||
|
||||
/** @var list<string>|null */
|
||||
private static ?array $columnsWithoutPatchCheckResult = null;
|
||||
|
||||
protected static function booted()
|
||||
{
|
||||
static::creating(function ($setting) {
|
||||
@@ -465,6 +470,17 @@ class ServerSetting extends Model
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Selects every column except patch_check_result, which holds the full update list of the last patch
|
||||
* check (often tens of kilobytes). Use it for instance-wide server loops that do not read the result.
|
||||
*/
|
||||
public function scopeWithoutPatchCheckResult(Builder $query): Builder
|
||||
{
|
||||
self::$columnsWithoutPatchCheckResult ??= array_values(array_diff(Schema::getColumnListing($this->getTable()), ['patch_check_result']));
|
||||
|
||||
return $query->select(self::$columnsWithoutPatchCheckResult);
|
||||
}
|
||||
|
||||
/**
|
||||
* Summary of the saved patch check, or null when the server was never checked.
|
||||
* Results saved before security and reboot detection count as 0 and false.
|
||||
|
||||
@@ -0,0 +1,118 @@
|
||||
<?php
|
||||
|
||||
use App\Jobs\DockerCleanupJob;
|
||||
use App\Jobs\ScheduledJobManager;
|
||||
use App\Jobs\ServerManagerJob;
|
||||
use App\Jobs\ServerPatchCheckJob;
|
||||
use App\Models\InstanceSettings;
|
||||
use App\Models\Server;
|
||||
use App\Models\ServerSetting;
|
||||
use App\Models\Team;
|
||||
use Illuminate\Database\Eloquent\Relations\HasOne;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Carbon;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
beforeEach(function () {
|
||||
Server::flushIdentityMap();
|
||||
Cache::flush();
|
||||
config(['constants.coolify.self_hosted' => true]);
|
||||
InstanceSettings::forceCreate(['id' => 0, 'instance_timezone' => 'UTC']);
|
||||
$this->team = Team::factory()->create();
|
||||
$this->patchResult = ['package_manager' => 'apt', 'total_updates' => 1, 'updates' => [['package' => 'openssl', 'new_version' => '3.0.13-2']]];
|
||||
});
|
||||
|
||||
afterEach(function () {
|
||||
Carbon::setTestNow();
|
||||
});
|
||||
|
||||
function createBulkLoadServer(Team $team, array $settings): Server
|
||||
{
|
||||
$server = Server::factory()->create(['team_id' => $team->id]);
|
||||
$server->settings->forceFill([
|
||||
'is_reachable' => true,
|
||||
'is_usable' => true,
|
||||
'server_timezone' => 'UTC',
|
||||
...$settings,
|
||||
])->saveQuietly();
|
||||
|
||||
return $server;
|
||||
}
|
||||
|
||||
/**
|
||||
* @return list<string>
|
||||
*/
|
||||
function serverSettingsQueriesDuring(Closure $callback): array
|
||||
{
|
||||
$queries = [];
|
||||
DB::listen(function ($query) use (&$queries) {
|
||||
if (str_contains($query->sql, 'from "server_settings"')) {
|
||||
$queries[] = $query->sql;
|
||||
}
|
||||
});
|
||||
$callback();
|
||||
|
||||
return $queries;
|
||||
}
|
||||
|
||||
it('loads every settings column except the patch result and matches each settings row to its server', function () {
|
||||
$first = createBulkLoadServer($this->team, ['docker_cleanup_frequency' => '0 1 * * *', 'patch_check_result' => $this->patchResult]);
|
||||
$second = createBulkLoadServer($this->team, ['docker_cleanup_frequency' => '0 2 * * *', 'patch_check_frequency' => 'daily']);
|
||||
|
||||
$servers = Server::query()
|
||||
->whereKey([$first->id, $second->id])
|
||||
->with(['settings' => fn (HasOne $query) => $query->withoutPatchCheckResult()])
|
||||
->get()
|
||||
->keyBy('id');
|
||||
|
||||
foreach ([$first, $second] as $server) {
|
||||
$full = ServerSetting::query()->where('server_id', $server->id)->firstOrFail()->getAttributes();
|
||||
unset($full['patch_check_result']);
|
||||
|
||||
expect($servers[$server->id]->settings->getAttributes())->toBe($full);
|
||||
}
|
||||
expect($servers[$first->id]->settings->docker_cleanup_frequency)->toBe('0 1 * * *')
|
||||
->and($servers[$second->id]->settings->docker_cleanup_frequency)->toBe('0 2 * * *')
|
||||
->and($servers[$second->id]->settings->patch_check_frequency)->toBe('daily')
|
||||
->and($servers[$first->id]->settings->is_reachable)->toBeTrue();
|
||||
});
|
||||
|
||||
it('does not read the patch results of all servers in the server manager, and its jobs still get them', function () {
|
||||
Queue::fake();
|
||||
Carbon::setTestNow(Carbon::create(2026, 10, 28, 12, 0, 0, 'UTC'));
|
||||
$server = createBulkLoadServer($this->team, ['patch_check_frequency' => '* * * * *', 'patch_check_result' => $this->patchResult]);
|
||||
|
||||
$queries = serverSettingsQueriesDuring(fn () => (new ServerManagerJob)->handle());
|
||||
|
||||
expect($queries)->not->toBeEmpty();
|
||||
foreach ($queries as $sql) {
|
||||
expect($sql)->not->toContain('patch_check_result')->not->toContain('*');
|
||||
}
|
||||
|
||||
Queue::assertPushed(ServerPatchCheckJob::class, function (ServerPatchCheckJob $job) use ($server) {
|
||||
$restored = unserialize(serialize($job));
|
||||
|
||||
return $restored->server->id === $server->id
|
||||
&& $restored->server->settings->patch_check_result === $this->patchResult;
|
||||
});
|
||||
});
|
||||
|
||||
it('does not read the patch results of all servers when it schedules Docker cleanups', function () {
|
||||
Queue::fakeExcept(ScheduledJobManager::class);
|
||||
Carbon::setTestNow(Carbon::create(2026, 10, 28, 12, 0, 0, 'UTC'));
|
||||
$server = createBulkLoadServer($this->team, ['docker_cleanup_frequency' => '* * * * *', 'patch_check_result' => $this->patchResult]);
|
||||
|
||||
$queries = serverSettingsQueriesDuring(fn () => (new ScheduledJobManager)->handle());
|
||||
|
||||
expect(collect($queries)->filter(fn (string $sql) => str_contains($sql, '"server_id" in')))->not->toBeEmpty();
|
||||
foreach ($queries as $sql) {
|
||||
if (str_contains($sql, '"server_id" in')) {
|
||||
expect($sql)->not->toContain('patch_check_result')->not->toContain('*');
|
||||
}
|
||||
}
|
||||
Queue::assertPushed(DockerCleanupJob::class, fn (DockerCleanupJob $job) => $job->server->id === $server->id);
|
||||
});
|
||||
Reference in New Issue
Block a user