From bf6e1ce9660e007f4148d13418353b4cc85c3552 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Fri, 9 Oct 2020 22:12:45 -0700 Subject: [PATCH] Document what is being tested a little better so it isn't just a wall of code --- .../Deployment/FindViableNodesService.php | 4 +- .../Deployment/FindViableNodesServiceTest.php | 74 +++++++++++++++---- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/app/Services/Deployment/FindViableNodesService.php b/app/Services/Deployment/FindViableNodesService.php index f71230418..d89c73f5e 100644 --- a/app/Services/Deployment/FindViableNodesService.php +++ b/app/Services/Deployment/FindViableNodesService.php @@ -97,8 +97,8 @@ class FindViableNodesService } $results = $query->groupBy('nodes.id') - ->havingRaw('(IFNULL(SUM(servers.memory), 0) + ?) < (nodes.memory * (1 + (nodes.memory_overallocate / 100)))', [ $this->memory ]) - ->havingRaw('(IFNULL(SUM(servers.disk), 0) + ?) < (nodes.disk * (1 + (nodes.disk_overallocate / 100)))', [ $this->disk ]) + ->havingRaw('(IFNULL(SUM(servers.memory), 0) + ?) <= (nodes.memory * (1 + (nodes.memory_overallocate / 100)))', [ $this->memory ]) + ->havingRaw('(IFNULL(SUM(servers.disk), 0) + ?) <= (nodes.disk * (1 + (nodes.disk_overallocate / 100)))', [ $this->disk ]) ->get() ->toBase(); diff --git a/tests/Integration/Services/Deployment/FindViableNodesServiceTest.php b/tests/Integration/Services/Deployment/FindViableNodesServiceTest.php index 1c5619cc0..47261dbe8 100644 --- a/tests/Integration/Services/Deployment/FindViableNodesServiceTest.php +++ b/tests/Integration/Services/Deployment/FindViableNodesServiceTest.php @@ -4,7 +4,9 @@ namespace Pterodactyl\Tests\Integration\Services\Deployment; use Pterodactyl\Models\Node; use InvalidArgumentException; +use Pterodactyl\Models\Server; use Pterodactyl\Models\Location; +use Pterodactyl\Models\Database; use Illuminate\Support\Collection; use Pterodactyl\Tests\Integration\IntegrationTestCase; use Pterodactyl\Services\Deployment\FindViableNodesService; @@ -12,6 +14,15 @@ use Pterodactyl\Exceptions\Service\Deployment\NoViableNodeException; class FindViableNodesServiceTest extends IntegrationTestCase { + public function setUp(): void + { + parent::setUp(); + + Database::query()->delete(); + Server::query()->delete(); + Node::query()->delete(); + } + public function testExceptionIsThrownIfNoDiskSpaceHasBeenSet() { $this->expectException(InvalidArgumentException::class); @@ -35,10 +46,11 @@ class FindViableNodesServiceTest extends IntegrationTestCase /** @var \Pterodactyl\Models\Node[] $nodes */ $nodes = [ - // This node should never be returned. + // This node should never be returned once we've completed the initial test which + // runs without a location filter. factory(Node::class)->create([ 'location_id' => $locations[0]->id, - 'memory' => 10240, + 'memory' => 2048, 'disk' => 1024 * 100, ]), factory(Node::class)->create([ @@ -55,40 +67,72 @@ class FindViableNodesServiceTest extends IntegrationTestCase ]), ]; + // Expect that all of the nodes are returned as we're under all of their limits + // and there is no location filter being provided. + $response = $this->getService()->setDisk(512)->setMemory(512)->handle(); + $this->assertInstanceOf(Collection::class, $response); + $this->assertCount(3, $response); + $this->assertInstanceOf(Node::class, $response[0]); + + // Expect that only the last node is returned because it is the only one with enough + // memory available to this instance. + $response = $this->getService()->setDisk(512)->setMemory(2049)->handle(); + $this->assertInstanceOf(Collection::class, $response); + $this->assertCount(1, $response); + $this->assertSame($nodes[2]->id, $response[0]->id); + + // Helper, I am lazy. $base = function () use ($locations) { return $this->getService()->setLocations([ $locations[1]->id ])->setDisk(512); }; + // Expect that we can create this server on either node since the disk and memory + // limits are below the allowed amount. $response = $base()->setMemory(512)->handle(); - $this->assertInstanceOf(Collection::class, $response); - $this->assertFalse($response->isEmpty()); - $this->assertSame(2, $response->count()); + $this->assertCount(2, $response); $this->assertSame(2, $response->where('location_id', $locations[1]->id)->count()); + // Expect that we can only create this server on the second node since the memory + // allocated is over the amount of memory available to the first node. $response = $base()->setMemory(2048)->handle(); - $this->assertSame(1, $response->count()); + $this->assertCount(1, $response); $this->assertSame($nodes[2]->id, $response[0]->id); + // Expect that we can only create this server on the second node since the disk + // allocated is over the limit assigned to the first node (even with the overallocate). $response = $base()->setDisk(20480)->setMemory(256)->handle(); - $this->assertSame(1, $response->count()); + $this->assertCount(1, $response); $this->assertSame($nodes[2]->id, $response[0]->id); - $response = $base()->setDisk(11263)->setMemory(256)->handle(); - $this->assertSame(2, $response->count()); + // Expect that we could create the server on either node since the disk allocated is + // right at the limit for Node 1 when the overallocate value is included in the calc. + $response = $base()->setDisk(11264)->setMemory(256)->handle(); + $this->assertCount(2, $response); + // Create two servers on the first node so that the disk space used is equal to the + // base amount available to the node (without overallocation included). $servers = Collection::make([ $this->createServerModel(['node_id' => $nodes[1]->id, 'disk' => 5120]), $this->createServerModel(['node_id' => $nodes[1]->id, 'disk' => 5120]), ]); + // Expect that we cannot create a server with a 1GB disk on the first node since there + // is not enough space (even with the overallocate) available to the node. $response = $base()->setDisk(1024)->setMemory(256)->handle(); - $this->assertSame(1, $response->count()); + $this->assertCount(1, $response); $this->assertSame($nodes[2]->id, $response[0]->id); + + // Cleanup servers since we need to test some other stuff with memory here. $servers->each->delete(); + // Expect that no viable node can be found when the memory limit for the given instance + // is greater than either node can support, even with the overallocation limits taken + // into account. $this->expectException(NoViableNodeException::class); $base()->setMemory(10000)->handle(); + // Create four servers so that the memory used for the second node is equal to the total + // limit for that node (pre-overallocate calculation). Collection::make([ $this->createServerModel(['node_id' => $nodes[2]->id, 'memory' => 1024]), $this->createServerModel(['node_id' => $nodes[2]->id, 'memory' => 1024]), @@ -96,12 +140,16 @@ class FindViableNodesServiceTest extends IntegrationTestCase $this->createServerModel(['node_id' => $nodes[2]->id, 'memory' => 1024]), ]); + // Expect that either node can support this server when we account for the overallocate + // value of the second node. $response = $base()->setMemory(500)->handle(); - $this->assertSame(2, $response->count()); + $this->assertCount(2, $response); $this->assertSame(2, $response->where('location_id', $locations[1]->id)->count()); - $response = $base()->setMemory(512)->handle(); - $this->assertSame(1, $response->count()); + // Expect that only the first node can support this server when we go over the remaining + // memory for the second nodes overallocate calculation. + $response = $base()->setMemory(640)->handle(); + $this->assertCount(1, $response); $this->assertSame($nodes[1]->id, $response[0]->id); }