diff --git a/app/Actions/Service/DeleteService.php b/app/Actions/Service/DeleteService.php index 7c544692b5..1ecc029c43 100644 --- a/app/Actions/Service/DeleteService.php +++ b/app/Actions/Service/DeleteService.php @@ -67,7 +67,7 @@ class DeleteService // One sh -c line, so non-root servers run the whole script with sudo. A leading variable // assignment would become "sudo container_ids=...", which sudo rejects. - $script = containerIdsByOwnerScript('service', $service->uuid, $filters, legacyExtraFilters: $legacyFilters, legacyId: (int) $service->id) + $script = containerIdsByOwnerScript('service', $service->uuid, $filters, legacyExtraFilters: $legacyFilters) .'; [ -z "$container_ids" ] || docker rm -f $container_ids'; instant_remote_process(['sh -c '.escapeshellarg($script)], $service->server); } diff --git a/app/Actions/Shared/ComplexStatusCheck.php b/app/Actions/Shared/ComplexStatusCheck.php index 3c400e634b..db77745a26 100644 --- a/app/Actions/Shared/ComplexStatusCheck.php +++ b/app/Actions/Shared/ComplexStatusCheck.php @@ -29,7 +29,7 @@ class ComplexStatusCheck continue; } } - $script = containerIdsByOwnerScript('application', $application->uuid, ['label=coolify.pullRequestId=0'], all: false, legacyId: (int) $application->id) + $script = containerIdsByOwnerScript('application', $application->uuid, ['label=coolify.pullRequestId=0'], all: false) .'; [ -z "$container_ids" ] || docker container inspect $container_ids --format \'{{json .}}\''; $containers = instant_remote_process(['sh -c '.escapeshellarg($script)], $server, false); $containers = format_docker_command_output_to_json($containers); diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index 52b401e23a..e2f5947dbe 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -65,8 +65,7 @@ function caddyTrafficAppKey(string $uuid, ?string $serviceName = null): string * New containers carry `coolify.{type}Uuid`. Containers created before that label carry a * numeric `coolify.{type}Id` instead. Ids change when a server moves to another instance, so * for those containers the owner is read from the compose project (Swarm: stack namespace), - * which is the resource UUID for deployments since mid-2024. Use resolveContainerOwner() to - * also fall back to the numeric id for older containers. + * which is the resource UUID for deployments since mid-2024. * * @param 'application'|'service'|'database' $type */ @@ -89,11 +88,11 @@ function containerOwnerUuid(Collection|array|string $labels, string $type): ?str } /** - * Resource that owns a container, from the given candidates. - * - * Order: UUID label, compose project / stack namespace (see containerOwnerUuid()), then the - * numeric id label of containers without a UUID label. The id is only correct on the instance - * that created the container, so it is the last fallback. + * Resource that owns a container, from the given candidates, by UUID label or compose project / + * stack namespace (see containerOwnerUuid()). Applications deployed before mid-2024 used the + * deployment directory as compose project; their compose service (the container name) starts + * with the application UUID. The numeric id label is never used: another Coolify instance that + * manages the same server can have a resource with the same id. * * @template TResource of \Illuminate\Database\Eloquent\Model * @@ -107,13 +106,13 @@ function resolveContainerOwner(Collection $resources, Collection|array|string $l $uuid = containerOwnerUuid($labels, $type); $owner = $uuid ? $resources->firstWhere('uuid', $uuid) : null; - if ($owner || filled($labels->get("coolify.{$type}Uuid"))) { + if ($owner || $type !== 'application' || filled($labels->get('coolify.applicationUuid')) || ! $labels->has('coolify.applicationId')) { return $owner; } - $legacyId = $labels->get("coolify.{$type}Id"); + $composeService = (string) $labels->get('com.docker.compose.service'); - return is_numeric($legacyId) ? $resources->first(fn ($resource) => (int) $resource->id === (int) $legacyId) : null; + return $resources->first(fn ($resource) => $composeService === $resource->uuid || str_starts_with($composeService, $resource->uuid.'-')); } /** @@ -131,13 +130,8 @@ function resolveContainerApplicationId(Collection $applications, Collection|arra $uuid = containerOwnerUuid($labels, 'application'); $id = $uuid ? Application::withTrashed()->where('uuid', $uuid)->value('id') : null; - if ($id !== null || filled($labels->get('coolify.applicationUuid'))) { - return $id !== null ? (int) $id : null; - } - $legacyId = $labels->get('coolify.applicationId'); - - return is_numeric($legacyId) ? (int) $legacyId : null; + return $id !== null ? (int) $id : null; } /** @@ -189,10 +183,9 @@ function resolveServiceContainerOwner(Collection $services, Collection|array $fl * @param 'application'|'service'|'database' $type * @param list $extraFilters Additional `--filter` values, e.g. `label=coolify.pullRequestId=0`. * @param list|null $legacyExtraFilters Filters for containers without the UUID label; defaults to $extraFilters. - * @param int|null $legacyId Local numeric id: also match older containers by `coolify.{type}Id` (may repeat a container). * @return list */ -function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters = [], string $format = '{{json .}}', bool $all = true, ?array $legacyExtraFilters = null, ?int $legacyId = null): array +function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters = [], string $format = '{{json .}}', bool $all = true, ?array $legacyExtraFilters = null): array { $filters = fn (array $values) => collect($values)->map(fn (string $filter) => '--filter '.escapeshellarg($filter))->implode(' '); $base = 'docker ps'.($all ? ' -a' : ''); @@ -203,8 +196,9 @@ function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters $base.' '.$filters(["label=coolify.{$type}Uuid={$uuid}", ...$extraFilters]).' '.$formatArgument, $base.' '.$filters(["label=coolify.{$type}Id", "label=com.docker.compose.project={$uuid}", ...$legacyFilters]).' '.$formatArgument, ]; - if ($legacyId !== null) { - $commands[] = $base.' '.$filters(["label=coolify.{$type}Id={$legacyId}", ...$legacyFilters]).' '.$formatArgument; + if ($type === 'application') { + // Deployed before mid-2024 (see resolveContainerOwner()): the container name starts with the UUID. + $commands[] = $base.' '.$filters(['label=coolify.applicationId', "name=^{$uuid}", ...$legacyFilters]).' '.$formatArgument; } return $commands; @@ -217,7 +211,7 @@ function dockerPsByOwnerCommands(string $type, string $uuid, array $extraFilters */ function containersOwnedBy(Server $server, string $type, Model $resource): Collection { - $output = instant_remote_process(dockerPsByOwnerCommands($type, $resource->uuid, legacyId: (int) $resource->id), $server); + $output = instant_remote_process(dockerPsByOwnerCommands($type, $resource->uuid), $server); return format_docker_command_output_to_json($output)->filter()->unique('ID')->values(); } @@ -229,9 +223,9 @@ function containersOwnedBy(Server $server, string $type, Model $resource): Colle * @param list $extraFilters * @param list|null $legacyExtraFilters */ -function containerIdsByOwnerScript(string $type, string $uuid, array $extraFilters = [], bool $all = true, ?array $legacyExtraFilters = null, ?int $legacyId = null): string +function containerIdsByOwnerScript(string $type, string $uuid, array $extraFilters = [], bool $all = true, ?array $legacyExtraFilters = null): string { - $commands = dockerPsByOwnerCommands($type, $uuid, $extraFilters, '{{.ID}}', $all, $legacyExtraFilters, $legacyId); + $commands = dockerPsByOwnerCommands($type, $uuid, $extraFilters, '{{.ID}}', $all, $legacyExtraFilters); return 'container_ids=$({ '.implode('; ', $commands).'; } | sort -u)'; } diff --git a/tests/Feature/ContainerOwnerUuidLabelsTest.php b/tests/Feature/ContainerOwnerUuidLabelsTest.php index d8d93a175b..18155c2a63 100644 --- a/tests/Feature/ContainerOwnerUuidLabelsTest.php +++ b/tests/Feature/ContainerOwnerUuidLabelsTest.php @@ -67,7 +67,7 @@ describe('label producers', function () { }); describe('container lookups for services and databases', function () { - test('database and service lookups match the UUID label, the compose project, and the older id label', function () { + test('database and service lookups match the UUID label and the compose project, never a numeric id', function () { $privateKey = PrivateKey::factory()->create(['team_id' => $this->team->id]); $this->server->update(['private_key_id' => $privateKey->id]); $this->server->settings()->update(['is_reachable' => true, 'is_usable' => true]); @@ -97,13 +97,13 @@ describe('container lookups for services and databases', function () { expect($commands->implode("\n")) ->toContain("label=coolify.databaseUuid={$database->uuid}") ->toContain("label=com.docker.compose.project={$database->uuid}") - ->toContain("label=coolify.databaseId={$database->id}") + ->not->toContain("label=coolify.databaseId={$database->id}") ->toContain("label=coolify.serviceUuid={$service->uuid}") ->toContain("label=com.docker.compose.project={$service->uuid}") - ->toContain("label=coolify.serviceId={$service->id}"); + ->not->toContain("label=coolify.serviceId={$service->id}"); }); - test('service part containers are matched by UUID label, compose service name, then older id', function () { + test('service part containers are matched by UUID label, then compose service name', function () { $service = Service::factory()->create([ 'environment_id' => $this->environment->id, 'server_id' => $this->server->id, @@ -117,8 +117,8 @@ describe('container lookups for services and databases', function () { expect($partOf(['coolify.serviceUuid' => $service->uuid, 'coolify.service.subType' => 'application', 'coolify.service.subUuid' => $part->uuid]))->toBe($part->id) // Moved from another instance: foreign ids, but compose project and service name still match. ->and($partOf(['coolify.serviceId' => '999', 'coolify.service.subType' => 'application', 'coolify.service.subId' => '999', 'com.docker.compose.project' => $service->uuid, 'com.docker.compose.service' => 'web']))->toBe($part->id) - // Older container with another compose project: local ids. - ->and($partOf(['coolify.serviceId' => (string) $service->id, 'coolify.service.subType' => 'application', 'coolify.service.subId' => (string) $part->id, 'com.docker.compose.project' => 'other']))->toBe($part->id); + // Another instance's container with the same numeric ids: not ours. + ->and($partOf(['coolify.serviceId' => (string) $service->id, 'coolify.service.subType' => 'application', 'coolify.service.subId' => (string) $part->id, 'com.docker.compose.project' => 'other']))->toBeNull(); }); }); @@ -138,26 +138,35 @@ describe('owner resolution', function () { ->toBe('app-uuid'); }); - test('containers from before mid-2024 fall back to their local numeric id', function () { + test('a local numeric id never makes another instance\'s container ours', function () { $applications = collect([$this->application]); - $olderContainer = ['coolify.applicationId' => (string) $this->application->id, 'com.docker.compose.project' => 'deployment-uuid']; + $localId = (string) $this->application->id; - expect(resolveContainerOwner($applications, $olderContainer, 'application')?->id)->toBe($this->application->id) - // A UUID label is never overridden by the numeric id. - ->and(resolveContainerOwner($applications, ['coolify.applicationUuid' => 'unknown', 'coolify.applicationId' => (string) $this->application->id], 'application'))->toBeNull() - ->and(dockerPsByOwnerCommands('application', 'app-uuid', legacyId: 42))->toHaveCount(3) - ->and(dockerPsByOwnerCommands('application', 'app-uuid', legacyId: 42)[2])->toContain("--filter 'label=coolify.applicationId=42'"); + expect(resolveContainerOwner($applications, ['coolify.applicationId' => $localId, 'com.docker.compose.project' => 'other-instance-uuid'], 'application'))->toBeNull() + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => $localId], 'application'))->toBeNull() + ->and(resolveContainerOwner($applications, ['coolify.applicationUuid' => 'unknown', 'coolify.applicationId' => $localId], 'application'))->toBeNull() + ->and(resolveContainerApplicationId($applications, ['coolify.applicationId' => $localId, 'com.docker.compose.project' => 'other-instance-uuid']))->toBeNull() + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => '999999', 'com.docker.compose.project' => $this->application->uuid], 'application')?->id)->toBe($this->application->id) + // Deployed before mid-2024: compose project was the deployment directory, the compose service starts with the UUID. + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => '999999', 'com.docker.compose.project' => 'deployment-dir', 'com.docker.compose.service' => $this->application->uuid.'-104512123456'], 'application')?->id)->toBe($this->application->id) + ->and(resolveContainerOwner($applications, ['coolify.applicationId' => $localId, 'com.docker.compose.project' => 'deployment-dir', 'com.docker.compose.service' => 'other-instance-uuid-104512123456'], 'application'))->toBeNull() + ->and(implode("\n", dockerPsByOwnerCommands('application', 'app-uuid')))->not->toContain('label=coolify.applicationId='); }); test('docker ps commands match the UUID label and the compose project of older containers', function () { $commands = dockerPsByOwnerCommands('application', 'app-uuid', ['label=coolify.pullRequestId=0']); - expect($commands)->toHaveCount(2) + expect($commands)->toHaveCount(3) ->and($commands[0])->toContain("--filter 'label=coolify.applicationUuid=app-uuid'") ->toContain("--filter 'label=coolify.pullRequestId=0'") ->and($commands[1])->toContain("--filter 'label=coolify.applicationId'") ->toContain("--filter 'label=com.docker.compose.project=app-uuid'") - ->toContain("--filter 'label=coolify.pullRequestId=0'"); + ->toContain("--filter 'label=coolify.pullRequestId=0'") + // Deployed before mid-2024: the container name starts with the application UUID. + ->and($commands[2])->toContain("--filter 'label=coolify.applicationId'") + ->toContain("--filter 'name=^app-uuid'") + ->toContain("--filter 'label=coolify.pullRequestId=0'") + ->and(dockerPsByOwnerCommands('service', 'service-uuid'))->toHaveCount(2); }); }); @@ -207,7 +216,7 @@ describe('status updates after a server transfer', function () { ->and($this->application->fresh()->status)->toStartWith('running'); }); - test('Sentinel push and status check still match containers from before mid-2024 by their local id', function () { + test('Sentinel push and status check still match containers from before mid-2024 by the UUID in their name', function () { $labels = [ 'coolify.managed' => 'true', 'coolify.applicationId' => (string) $this->application->id, @@ -269,8 +278,8 @@ describe('orphaned preview cleanup', function () { expect($isOrphaned(['Labels' => "coolify.applicationId=999999,coolify.pullRequestId=7,com.docker.compose.project={$this->application->uuid}"]))->toBeFalse() ->and($isOrphaned(['Labels' => "coolify.applicationUuid={$this->application->uuid},coolify.pullRequestId=7"]))->toBeFalse() ->and($isOrphaned(['Labels' => "coolify.applicationUuid={$this->application->uuid},coolify.pullRequestId=8"]))->toBeTrue() - // Containers from before mid-2024: another compose project, so the local id is used, as before. - ->and($isOrphaned(['Labels' => "coolify.applicationId={$this->application->id},coolify.pullRequestId=7,com.docker.compose.project=deployment-uuid"]))->toBeFalse() - ->and($isOrphaned(['Labels' => 'coolify.applicationId=999999,coolify.pullRequestId=7']))->toBeTrue(); + // Another instance's preview container with the same numeric id: not ours, so never removed. + ->and($isOrphaned(['Labels' => "coolify.applicationId={$this->application->id},coolify.pullRequestId=8,com.docker.compose.project=other-instance-uuid"]))->toBeFalse() + ->and($isOrphaned(['Labels' => 'coolify.applicationId=999999,coolify.pullRequestId=7']))->toBeFalse(); }); }); diff --git a/tests/Feature/DeleteResourceJobAtomicityTest.php b/tests/Feature/DeleteResourceJobAtomicityTest.php index 7696a2ffbd..5ceb748e16 100644 --- a/tests/Feature/DeleteResourceJobAtomicityTest.php +++ b/tests/Feature/DeleteResourceJobAtomicityTest.php @@ -147,8 +147,8 @@ it('removes service containers before its volumes and local metadata', function expect($commandList) ->toContain("label=coolify.serviceUuid={$service->uuid}") ->toContain("label=com.docker.compose.project={$service->uuid}") - // Containers from before mid-2024 may have another compose project; the local id still finds them. - ->toContain("label=coolify.serviceId={$service->id}") + // A numeric id can belong to another Coolify instance on the same server, so it never selects containers. + ->not->toContain("label=coolify.serviceId={$service->id}") ->toContain('sort -u') ->toContain('docker rm -f $container_ids') ->toContain("docker volume rm -f '{$service->uuid}_web-data'")