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

Conversation

theofidry
Copy link
Owner

@theofidry theofidry commented Jul 25, 2024

Closes #125

@theofidry theofidry changed the title Fix/k8 limit feat: Add support for limiting the number of CPU cores calcualted for parallelisation Jul 25, 2024
@theofidry theofidry marked this pull request as ready for review July 26, 2024 08:23
@theofidry theofidry merged commit 0a108d8 into main Jul 26, 2024
20 checks passed
@theofidry theofidry deleted the fix/k8-limit branch July 26, 2024 08:35
Copy link

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Sorry to comment after the merge, but I would like to discuss things before the release 🙂.


public function test_it_can_describe_itself(): void
{
$finder = new EnvVariableFinder('CI_CPU_LIMIT');
Copy link

Choose a reason for hiding this comment

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

How CI_CPU_LIMIT relates to KUBERNETES_CPU_LIMIT? Is this refactor leftover?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It doesn't directly relate, it's just a generic finder I've made available which tries to get back a positive int from the environment variable. So it can be (and is) used for KUBERNETES_CPU_LIMIT or any other.

"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.

@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect available CPUs on Kubernetes
2 participants