Fix/suspend server offline node (#871)

* Use handle instead of toggle & use const isnstead of string * Avoid rollback if node is unreachable * Use Enum & remove default action * Remove useless test
pelican-dev · Jan 6, 2025 · 77fd54f · 77fd54f
1 parent 18fe4f1
commit 77fd54f
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 34 deletions.
diff --git a/app/Enums/SuspendAction.php b/app/Enums/SuspendAction.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace App\Enums;
+
+enum SuspendAction: string
+{
+    case Suspend = 'suspend';
+    case Unsuspend = 'unsuspend';
+}
diff --git a/app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php b/app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php
@@ -4,6 +4,7 @@
 
 use App\Enums\ContainerStatus;
 use App\Enums\ServerState;
+use App\Enums\SuspendAction;
 use App\Filament\Admin\Resources\ServerResource;
 use App\Filament\Admin\Resources\ServerResource\RelationManagers\AllocationsRelationManager;
 use App\Filament\Components\Forms\Actions\RotateDatabasePasswordAction;
@@ -792,7 +793,11 @@ public function form(Form $form): Form
                                                         ->color('warning')
                                                         ->hidden(fn (Server $server) => $server->isSuspended())
                                                         ->action(function (SuspensionService $suspensionService, Server $server) {
-                                                            $suspensionService->toggle($server, 'suspend');
+                                                            try {
+                                                                $suspensionService->handle($server, SuspendAction::Suspend);
+                                                            } catch (\Exception $exception) {
+                                                                Notification::make()->warning()->title('Server Suspension')->body($exception->getMessage())->send();
+                                                            }
                                                             Notification::make()->success()->title('Server Suspended!')->send();
 
                                                             $this->refreshFormData(['status', 'docker']);
@@ -802,7 +807,11 @@ public function form(Form $form): Form
                                                         ->color('success')
                                                         ->hidden(fn (Server $server) => !$server->isSuspended())
                                                         ->action(function (SuspensionService $suspensionService, Server $server) {
-                                                            $suspensionService->toggle($server, 'unsuspend');
+                                                            try {
+                                                                $suspensionService->handle($server, SuspendAction::Unsuspend);
+                                                            } catch (\Exception $exception) {
+                                                                Notification::make()->warning()->title('Server Suspension')->body($exception->getMessage())->send();
+                                                            }
                                                             Notification::make()->success()->title('Server Unsuspended!')->send();
 
                                                             $this->refreshFormData(['status', 'docker']);

diff --git a/app/Filament/Admin/Resources/UserResource/RelationManagers/ServersRelationManager.php b/app/Filament/Admin/Resources/UserResource/RelationManagers/ServersRelationManager.php
@@ -3,6 +3,7 @@
 namespace App\Filament\Admin\Resources\UserResource\RelationManagers;
 
 use App\Enums\ServerState;
+use App\Enums\SuspendAction;
 use App\Models\Server;
 use App\Models\User;
 use App\Services\Servers\SuspensionService;
@@ -34,7 +35,7 @@ public function table(Table $table): Table
                     ->color('warning')
                     ->action(function (SuspensionService $suspensionService) use ($user) {
                         foreach ($user->servers()->whereNot('status', ServerState::Suspended)->get() as $server) {
-                            $suspensionService->toggle($server);
+                            $suspensionService->handle($server, SuspendAction::Suspend);
                         }
                     }),
                 Actions\Action::make('toggleUnsuspend')
@@ -43,7 +44,7 @@ public function table(Table $table): Table
                     ->color('primary')
                     ->action(function (SuspensionService $suspensionService) use ($user) {
                         foreach ($user->servers()->where('status', ServerState::Suspended)->get() as $server) {
-                            $suspensionService->toggle($server, SuspensionService::ACTION_UNSUSPEND);
+                            $suspensionService->handle($server, SuspendAction::Unsuspend);
                         }
                     }),
             ])

diff --git a/app/Http/Controllers/Api/Application/Servers/ServerManagementController.php b/app/Http/Controllers/Api/Application/Servers/ServerManagementController.php
@@ -2,6 +2,7 @@
 
 namespace App\Http\Controllers\Api\Application\Servers;
 
+use App\Enums\SuspendAction;
 use App\Http\Controllers\Api\Application\ApplicationApiController;
 use App\Http\Requests\Api\Application\Servers\ServerWriteRequest;
 use App\Models\Server;
@@ -32,7 +33,7 @@ public function __construct(
      */
     public function suspend(ServerWriteRequest $request, Server $server): Response
     {
-        $this->suspensionService->toggle($server);
+        $this->suspensionService->handle($server, SuspendAction::Suspend);
 
         return $this->returnNoContent();
     }
@@ -44,7 +45,7 @@ public function suspend(ServerWriteRequest $request, Server $server): Response
      */
     public function unsuspend(ServerWriteRequest $request, Server $server): Response
     {
-        $this->suspensionService->toggle($server, SuspensionService::ACTION_UNSUSPEND);
+        $this->suspensionService->handle($server, SuspendAction::Unsuspend);
 
         return $this->returnNoContent();
     }

diff --git a/app/Services/Servers/SuspensionService.php b/app/Services/Servers/SuspensionService.php
@@ -3,18 +3,15 @@
 namespace App\Services\Servers;
 
 use App\Enums\ServerState;
+use App\Enums\SuspendAction;
 use Filament\Notifications\Notification;
-use Webmozart\Assert\Assert;
 use App\Models\Server;
 use App\Repositories\Daemon\DaemonServerRepository;
+use Doctrine\DBAL\Exception\ConnectionException;
 use Symfony\Component\HttpKernel\Exception\ConflictHttpException;
 
 class SuspensionService
 {
-    public const ACTION_SUSPEND = 'suspend';
-
-    public const ACTION_UNSUSPEND = 'unsuspend';
-
     /**
      * SuspensionService constructor.
      */
@@ -27,11 +24,9 @@ public function __construct(
      *
      * @throws \Throwable
      */
-    public function toggle(Server $server, string $action = self::ACTION_SUSPEND): void
+    public function handle(Server $server, SuspendAction $action): void
     {
-        Assert::oneOf($action, [self::ACTION_SUSPEND, self::ACTION_UNSUSPEND]);
-
-        $isSuspending = $action === self::ACTION_SUSPEND;
+        $isSuspending = $action === SuspendAction::Suspend;
         // Nothing needs to happen if we're suspending the server, and it is already
         // suspended in the database. Additionally, nothing needs to happen if the server
         // is not suspended, and we try to un-suspend the instance.
@@ -55,11 +50,7 @@ public function toggle(Server $server, string $action = self::ACTION_SUSPEND): v
         try {
             // Tell daemon to re-sync the server state.
             $this->daemonServerRepository->setServer($server)->sync();
-        } catch (\Exception $exception) {
-            // Rollback the server's suspension status if daemon fails to sync the server.
-            $server->update([
-                'status' => $isSuspending ? null : ServerState::Suspended,
-            ]);
+        } catch (ConnectionException $exception) {
             throw $exception;
         }
     }

diff --git a/tests/Integration/Services/Servers/SuspensionServiceTest.php b/tests/Integration/Services/Servers/SuspensionServiceTest.php
@@ -3,6 +3,7 @@
 namespace App\Tests\Integration\Services\Servers;
 
 use App\Enums\ServerState;
+use App\Enums\SuspendAction;
 use Mockery\MockInterface;
 use App\Services\Servers\SuspensionService;
 use App\Tests\Integration\IntegrationTestCase;
@@ -29,11 +30,11 @@ public function testServerIsSuspendedAndUnsuspended(): void
 
         $this->repository->expects('setServer->sync')->twice()->andReturnSelf();
 
-        $this->getService()->toggle($server);
+        $this->getService()->handle($server, SuspendAction::Suspend);
 
         $this->assertTrue($server->refresh()->isSuspended());
 
-        $this->getService()->toggle($server, SuspensionService::ACTION_UNSUSPEND);
+        $this->getService()->handle($server, SuspendAction::Unsuspend);
 
         $this->assertFalse($server->refresh()->isSuspended());
     }
@@ -42,28 +43,18 @@ public function testNoActionIsTakenIfSuspensionStatusIsUnchanged(): void
     {
         $server = $this->createServerModel();
 
-        $this->getService()->toggle($server, SuspensionService::ACTION_UNSUSPEND);
+        $this->getService()->handle($server, SuspendAction::Unsuspend);
 
         $server->refresh();
         $this->assertFalse($server->isSuspended());
 
         $server->update(['status' => ServerState::Suspended]);
-        $this->getService()->toggle($server);
+        $this->getService()->handle($server, SuspendAction::Suspend);
 
         $server->refresh();
         $this->assertTrue($server->isSuspended());
     }
 
-    public function testExceptionIsThrownIfInvalidActionsArePassed(): void
-    {
-        $server = $this->createServerModel();
-
-        $this->expectException(\InvalidArgumentException::class);
-        $this->expectExceptionMessage('Expected one of: "suspend", "unsuspend". Got: "foo"');
-
-        $this->getService()->toggle($server, 'foo');
-    }
-
     private function getService(): SuspensionService
     {
         return $this->app->make(SuspensionService::class);