Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for limiting the number of CPU cores calcualted for parallelisation #129

Merged
merged 7 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions infection.json5
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@
"Fidry\\CpuCoreCounter\\CpuCoreCounter::getDefaultFinders"
]
},
"CastInt": {
"ignore": [
// This is a bug or case handled by strict types. Not sure why
// infection can't detect it.V
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// infection can't detect it.V
// Infection can't detect it.

"Fidry\\CpuCoreCounter\\Finder\\EnvVariableFinder::isPositiveInteger"
]
},
"CastString": {
"ignore": [
// I can't find a case in practice where this would happen
Expand All @@ -41,6 +48,12 @@
"Fidry\\CpuCoreCounter\\Executor\\ProcOpenExecutor::execute"
]
},
"GreaterThan": {
"ignore": [
// This is an actual false positive.
"Fidry\\CpuCoreCounter\\CpuCoreCounter::getAvailableForParallelisation"
]
},
"IncrementInteger": {
"ignore": [
"Fidry\\CpuCoreCounter\\CpuCoreCounter::getAvailableForParallelisation"
Expand Down
6 changes: 6 additions & 0 deletions phpstan.src.neon
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,9 @@ parameters:
- src

tmpDir: .build/phpstan/src/

ignoreErrors:

# This is a sanity check
- path: src/Finder/EnvVariableFinder.php
message: '#find\(\) should return int\<1\, max\>\|null but returns int\|null\.#'
26 changes: 23 additions & 3 deletions src/CpuCoreCounter.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace Fidry\CpuCoreCounter;

use Fidry\CpuCoreCounter\Finder\CpuCoreFinder;
use Fidry\CpuCoreCounter\Finder\EnvVariableFinder;
use Fidry\CpuCoreCounter\Finder\FinderRegistry;

final class CpuCoreCounter
Expand All @@ -37,15 +38,27 @@ public function __construct(?array $finders = null)
}

/**
* @param positive-int $reservedCpus
* @param positive-int $reservedCpus
* @param positive-int|null $limit If no limit is given, all available CPUs will be returned.
*
* @return positive-int
*/
public function getAvailableForParallelisation(int $reservedCpus = 1): int
{
public function getAvailableForParallelisation(
int $reservedCpus = 1,
?int $limit = null
): int {
$limit = null === $limit
? self::getKubernetesLimit()
: $limit;

$count = $this->getCountWithFallback(1);

$availableCpus = $count - $reservedCpus;

if (null !== $limit && $availableCpus > $limit) {
$availableCpus = $limit;
}

return max(1, $availableCpus);
}

Expand Down Expand Up @@ -113,4 +126,11 @@ public function getFinderAndCores(): array

throw NumberOfCpuCoreNotFound::create();
}

public static function getKubernetesLimit(): ?int
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Kubernetes the only environment with this issue? Maybe other infra orchestrators also apply here, so maybe more generic name like getLogicalCpuLimit() or getInfrastructureCpuLimit() would be better?

Anyway, I've just entered debug mode on our CI runner in Gitlab (which is run on Kubernetes) and I don't see KUBERNETES_CPU_LIMIT there. How is it supposed to work in practice? I don't think setting this manually is good, as we have runner tiers and these have different CPU limits. It should be somehow detected automatically what is the actual CPU limit inside container.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: being inside container on k8s it's possible to do cat /sys/fs/cgroup/cpu.max and you get something like 400000 100000, where the first value is a quota, the second is period. Basically this is alternative way of retrieving data described here. Actual available CPUs can be calculated by dividing quota / period. Just tested it again on our cluster and it works, but cgroup must be available in order to provide this data.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Kubernetes the only environment with this issue?

so far yes: I didn't see any other, and worst case you can always pass the $limit
parameter.

so maybe more generic name like getLogicalCpuLimit() or getInfrastructureCpuLimit() would be better?

I will introduce such a method if there is more coming, meanwhile the current one is strictly about kubernetes. The new one would likely leverage the existing kubernetes one and potentially more I guess.

Anyway, I've just entered debug mode on our CI runner in Gitlab (which is run on Kubernetes) and I don't see KUBERNETES_CPU_LIMIT there. How is it supposed to work in practice?

You tell me 😅 . I've never used kubernetes... so I don't know. It was my understanding from the reported issue that it was available as an environment variable as per the following quote:

it is actually overridden and limited to 2 in gitlab CI via the KUBERNETES_CPU_LIMIT environment variable

{
$finder = new EnvVariableFinder('KUBERNETES_CPU_LIMIT');

return $finder->find();
}
}
69 changes: 69 additions & 0 deletions src/Finder/EnvVariableFinder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php

/*
* This file is part of the Fidry CPUCounter Config package.
*
* (c) Théo FIDRY <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Fidry\CpuCoreCounter\Finder;

use function getenv;
use function preg_match;
use function sprintf;
use function var_export;

final class EnvVariableFinder implements CpuCoreFinder
{
/** @var string */
private $environmentVariableName;

public function __construct(string $environmentVariableName)
{
$this->environmentVariableName = $environmentVariableName;
}

public function diagnose(): string
{
$value = getenv($this->environmentVariableName);

return sprintf(
'parse(getenv(%s)=%s)=%s',
$this->environmentVariableName,
var_export($value, true),
self::isPositiveInteger($value) ? $value : 'null'
);
}

public function find(): ?int
{
$value = getenv($this->environmentVariableName);

return self::isPositiveInteger($value)
? (int) $value
: null;
}

public function toString(): string
{
return sprintf(
'getenv(%s)',
$this->environmentVariableName
);
}

/**
* @param string|false $value
*/
private static function isPositiveInteger($value): bool
{
return false !== $value
&& 1 === preg_match('/^\d+$/', $value)
&& (int) $value > 0;
}
}
143 changes: 140 additions & 3 deletions tests/CpuCoreCounterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace Fidry\CpuCoreCounter\Test;

use Closure;
use Exception;
use Fidry\CpuCoreCounter\CpuCoreCounter;
use Fidry\CpuCoreCounter\Finder\CpuCoreFinder;
Expand All @@ -22,6 +23,7 @@
use PHPUnit\Framework\TestCase;
use function get_class;
use function is_array;
use function sprintf;

/**
* @covers \Fidry\CpuCoreCounter\CpuCoreCounter
Expand All @@ -30,6 +32,21 @@
*/
final class CpuCoreCounterTest extends TestCase
{
/**
* @var null|Closure(): void
*/
private $cleanupEnvironmentVariables;

protected function tearDown(): void
{
$cleanupEnvironmentVariables = $this->cleanupEnvironmentVariables;

if (null !== $cleanupEnvironmentVariables) {
($cleanupEnvironmentVariables)();
$this->cleanupEnvironmentVariables = null;
}
}

public function test_it_can_get_the_number_of_cpu_cores(): void
{
$counter = new CpuCoreCounter();
Expand Down Expand Up @@ -154,19 +171,29 @@ public static function cpuCoreFinderProvider(): iterable
/**
* @dataProvider availableCpuCoreProvider
*
* @param list<CpuCoreFinder> $finders
* @param positive-int $expected
* @param list<CpuCoreFinder> $finders
* @param array<string, string|null> $environmentVariables
* @param positive-int $expected
*/
public function test_it_can_get_the_number_of_available_cpu_cores_for_parallelisation(
array $finders,
array $environmentVariables,
?int $reservedCpus,
?int $limit,
int $expected
): void {
$this->setUpEnvironmentVariables($environmentVariables);

$counter = new CpuCoreCounter($finders);

// Sanity check: this is due to not being able to use named parameters.
if (null === $reservedCpus) {
self::assertNull($limit);
}

$actual = null === $reservedCpus
? $counter->getAvailableForParallelisation()
: $counter->getAvailableForParallelisation($reservedCpus);
: $counter->getAvailableForParallelisation($reservedCpus, $limit);

self::assertSame($expected, $actual);
}
Expand All @@ -175,32 +202,112 @@ public static function availableCpuCoreProvider(): iterable
{
yield 'no finder' => [
[],
[],
null,
null,
1,
];

yield 'no finder, multiple CPUs reserved' => [
[],
[],
3,
null,
1,
];

yield 'CPU count found: kubernetes limit set and lower than the count found' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
['KUBERNETES_CPU_LIMIT' => 2],
null,
null,
2,
];
})();

yield 'CPU count found: kubernetes limit set and higher than the count found' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
['KUBERNETES_CPU_LIMIT' => 8],
null,
null,
4,
];
})();

yield 'CPU count found: kubernetes limit set and equal to the count found' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
['KUBERNETES_CPU_LIMIT' => 5],
null,
null,
4,
];
})();

yield 'CPU count found: kubernetes limit set and equal to the count found after reserved CPUs' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
['KUBERNETES_CPU_LIMIT' => 4],
null,
null,
4,
];
})();

yield 'CPU count found: kubernetes limit set and limit set' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
['KUBERNETES_CPU_LIMIT' => 2],
1,
3,
3,
];
})();

yield 'CPU count found' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
[],
null,
null,
4,
];
})();

yield 'CPU count found higher than the limit passed' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
[],
1,
3,
3,
];
})();

yield 'CPU count found, multiple CPUs reserved' => (static function () {
$finder = new DummyCpuCoreFinder(5);

return [
[$finder],
[],
2,
null,
3,
];
})();
Expand All @@ -210,9 +317,39 @@ public static function availableCpuCoreProvider(): iterable

return [
[$finder],
[],
5,
null,
1,
];
})();
}

/**
* @param array<string, string|null> $environmentVariables
*/
private function setUpEnvironmentVariables(array $environmentVariables): void
{
$cleanupCalls = [];

foreach ($environmentVariables as $environmentName => $environmentValue) {
putenv(
sprintf(
'%s=%s',
$environmentName,
$environmentValue
)
);

$cleanupCalls[] = static function () use ($environmentName): void {
putenv($environmentName);
};
}

$this->cleanupEnvironmentVariables = static function () use ($cleanupCalls): void {
foreach ($cleanupCalls as $cleanupCall) {
$cleanupCall();
}
};
}
}
Loading