Skip to content

Commit 2b3c1eb

Browse files
saeedvazirysaeedvaziry
authored
Fix deploy restarts all workers rather than site workers (#915)
* Fix deploy restarts all workers rather than site workers * address comments * fix lint
1 parent e587b15 commit 2b3c1eb

File tree

7 files changed

+183
-2
lines changed

7 files changed

+183
-2
lines changed

‎app/Facades/SSH.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
* @method static string write(string $path, string $content, string $owner = null)
2222
* @method static string assertExecuted(mixed $commands)
2323
* @method static string assertExecutedContains(string $command)
24+
* @method static string assertNotExecutedContains(string $command, string $message = '')
2425
* @method static string assertFileUploaded(string $toPath, ?string $content = null)
2526
* @method static string getUploadedLocalPath()
2627
* @method static disconnect()

‎app/Jobs/Site/DeployJob.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,8 @@ private function handleClassicDeployment($site, $log): void
8181
if ($site->deploymentScript->shouldRestartWorkers()) {
8282
/** @var ProcessManager $processManager */
8383
$processManager = $site->server->processManager()->handler();
84-
$processManager->restartAll($site->id);
84+
$workerIds = $site->workers()->pluck('id')->toArray();
85+
$processManager->restartByIds($workerIds, $site->id);
8586
}
8687
}
8788

@@ -132,7 +133,8 @@ private function handleModernDeployment($site, $log): void
132133
if ($site->preFlightScript?->shouldRestartWorkers()) {
133134
/** @var ProcessManager $processManager */
134135
$processManager = $site->server->processManager()->handler();
135-
$processManager->restartAll($site->id);
136+
$workerIds = $site->workers()->pluck('id')->toArray();
137+
$processManager->restartByIds($workerIds, $site->id);
136138
}
137139
}
138140
}

‎app/Services/ProcessManager/ProcessManager.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,5 +28,10 @@ public function start(int $id, ?int $siteId = null): void;
2828

2929
public function restartAll(?int $siteId = null): void;
3030

31+
/**
32+
* @param array<int> $workerIds
33+
*/
34+
public function restartByIds(array $workerIds, ?int $siteId = null): void;
35+
3136
public function getLogs(string $user, string $logPath): string;
3237
}

‎app/Services/ProcessManager/Supervisor.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,26 @@ public function restartAll(?int $siteId = null): void
153153
);
154154
}
155155

156+
/**
157+
* @param array<int> $workerIds
158+
*
159+
* @throws Throwable
160+
*/
161+
public function restartByIds(array $workerIds, ?int $siteId = null): void
162+
{
163+
if (empty($workerIds)) {
164+
return;
165+
}
166+
167+
$this->service->server->ssh()->exec(
168+
view('ssh.services.process-manager.supervisor.restart-workers', [
169+
'workerIds' => $workerIds,
170+
]),
171+
'restart-workers',
172+
$siteId
173+
);
174+
}
175+
156176
/**
157177
* @throws Throwable
158178
*/

‎app/Support/Testing/SSHFake.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,18 @@ public function assertExecutedContains(string $command): void
130130
);
131131
}
132132

133+
public function assertNotExecutedContains(string $command, string $message = ''): void
134+
{
135+
foreach ($this->commands as $executedCommand) {
136+
$commandStr = (string) $executedCommand;
137+
if (str($commandStr)->contains($command)) {
138+
Assert::fail(
139+
$message ?: "The command '{$command}' should not be executed, but it was found in: {$commandStr}"
140+
);
141+
}
142+
}
143+
}
144+
133145
public function assertFileUploaded(string $toPath, ?string $content = null): void
134146
{
135147
if ($this->uploadedLocalPath === '' || $this->uploadedLocalPath === '0' || ($this->uploadedRemotePath === '' || $this->uploadedRemotePath === '0')) {

‎resources/views/ssh/services/process-manager/supervisor/restart-workers.blade.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
@foreach($workerIds as $workerId)
2+
if ! sudo supervisorctl restart {{ $workerId }}:*; then
3+
echo 'VITO_SSH_ERROR' && exit 1
4+
fi
5+
@endforeach
6+
7+
echo "Workers restarted successfully."

‎tests/Feature/ApplicationTest.php‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,12 @@
33
namespace Tests\Feature;
44

55
use App\Enums\DeploymentStatus;
6+
use App\Enums\WorkerStatus;
67
use App\Facades\SSH;
78
use App\Models\Deployment;
89
use App\Models\GitHook;
10+
use App\Models\Site;
11+
use App\Models\Worker;
912
use App\Notifications\DeploymentCompleted;
1013
use Exception;
1114
use Illuminate\Foundation\Testing\RefreshDatabase;
@@ -557,4 +560,135 @@ public static function hookData(): array
557560
],
558561
];
559562
}
563+
564+
public function test_deploy_classic_restarts_only_site_workers(): void
565+
{
566+
$sshFake = SSH::fake('fake output');
567+
Http::fake([
568+
'github.com/*' => Http::response([
569+
'sha' => '123',
570+
'commit' => [
571+
'message' => 'test commit message',
572+
'name' => 'test commit name',
573+
'email' => 'test@example.com',
574+
'url' => 'https://github.com/commit-url',
575+
],
576+
]),
577+
]);
578+
Notification::fake();
579+
580+
// Create a worker for the site being deployed
581+
$siteWorker = Worker::factory()->create([
582+
'server_id' => $this->server->id,
583+
'site_id' => $this->site->id,
584+
'status' => WorkerStatus::RUNNING,
585+
]);
586+
587+
// Create another site with workers on the same server
588+
$otherSite = Site::factory()->create([
589+
'server_id' => $this->server->id,
590+
]);
591+
$otherSiteWorker = Worker::factory()->create([
592+
'server_id' => $this->server->id,
593+
'site_id' => $otherSite->id,
594+
'status' => WorkerStatus::RUNNING,
595+
]);
596+
597+
// Enable restart workers for the deployment script
598+
$this->site->deploymentScript->update([
599+
'content' => 'git pull',
600+
'configs' => ['restart_workers' => true],
601+
]);
602+
603+
$this->actingAs($this->user);
604+
605+
$this->post(route('application.deploy', [
606+
'server' => $this->server,
607+
'site' => $this->site,
608+
]))
609+
->assertSessionDoesntHaveErrors();
610+
611+
// Verify that only the site worker restart command was executed
612+
SSH::assertExecutedContains('supervisorctl restart '.$siteWorker->id.':*');
613+
614+
// Verify that other site's worker and "restart all" are not executed
615+
$this->assertWorkerNotRestarted($otherSiteWorker->id);
616+
SSH::assertNotExecutedContains('supervisorctl restart all', 'Should not restart all workers');
617+
}
618+
619+
public function test_deploy_modern_restarts_only_site_workers(): void
620+
{
621+
$sshFake = SSH::fake('fake output');
622+
Http::fake([
623+
'github.com/*' => Http::response([
624+
'sha' => '123',
625+
'commit' => [
626+
'message' => 'test commit message',
627+
'name' => 'test commit name',
628+
'email' => 'test@example.com',
629+
'url' => 'https://github.com/commit-url',
630+
],
631+
]),
632+
]);
633+
Notification::fake();
634+
635+
$this->site->update([
636+
'type_data' => [
637+
'modern_deployment' => true,
638+
'modern_deployment_history' => 10,
639+
'modern_deployment_shared_resources' => ['.env'],
640+
],
641+
]);
642+
$this->site->ensureDeploymentScriptsExist();
643+
$this->site->refresh();
644+
645+
// Create a worker for the site being deployed
646+
$siteWorker = Worker::factory()->create([
647+
'server_id' => $this->server->id,
648+
'site_id' => $this->site->id,
649+
'status' => WorkerStatus::RUNNING,
650+
]);
651+
652+
// Create another site with workers on the same server
653+
$otherSite = Site::factory()->create([
654+
'server_id' => $this->server->id,
655+
]);
656+
$otherSiteWorker = Worker::factory()->create([
657+
'server_id' => $this->server->id,
658+
'site_id' => $otherSite->id,
659+
'status' => WorkerStatus::RUNNING,
660+
]);
661+
662+
// Enable restart workers for the pre-flight script
663+
$this->site->preFlightScript->update([
664+
'content' => 'php artisan migrate --force',
665+
'configs' => ['restart_workers' => true],
666+
]);
667+
668+
$this->actingAs($this->user);
669+
670+
$this->post(route('application.deploy', [
671+
'server' => $this->server,
672+
'site' => $this->site,
673+
]))
674+
->assertSessionDoesntHaveErrors();
675+
676+
// Verify that only the site worker restart command was executed
677+
SSH::assertExecutedContains('supervisorctl restart '.$siteWorker->id.':*');
678+
679+
// Verify that other site's worker and "restart all" are not executed
680+
$this->assertWorkerNotRestarted($otherSiteWorker->id);
681+
SSH::assertNotExecutedContains('supervisorctl restart all', 'Should not restart all workers');
682+
}
683+
684+
/**
685+
* Assert that the given worker's restart command was not executed via SSH.
686+
*/
687+
private function assertWorkerNotRestarted(int|string $workerId): void
688+
{
689+
SSH::assertNotExecutedContains(
690+
'supervisorctl restart '.$workerId.':*',
691+
"Worker {$workerId} should not be restarted"
692+
);
693+
}
560694
}

0 commit comments

Comments
 (0)