-
Notifications
You must be signed in to change notification settings - Fork 23
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
Active Processor Count uses total CPUs #136
Comments
I'm leaning towards adding an option that would allow you to disable the ActiveProcessorCount helper. For backward compatibility, this would default to using the ActiveProcessorCount helper. By allowing this to be disabled, we'd let the JVM detect the total number of CPUs. All of the versions of the JVM that we have shipped for a long time now support detection of CPU count within a container. https://bugs.openjdk.java.net/browse/JDK-8146115 It is done a bit differently though and it does seem to attempt to take into consideration reduced CPU shares. This would leave three options for users:
|
Some historical context on this feature is available here -> #53 (comment) |
We added manual -XX:ActiveProcessorCount=1 to the java options at startup. we use spring-boot-admin and the process reports active CPUs as 1 and the thread pools are sized accordingly so it looks like -XX:ActiveProcessorCount=1 takes precedence over 32. |
Based on past experience, when you have multiple arguments with different values like this the JVM will consistently pick the right-most value in the list. I suspect it's processing them in order, so when it processes the argument the second time, it just changes the value from 32 to 1, but I've not looked at the source code so that is a guess. Anyway, in your case it should consistently pick 1. |
Hi @dmikusa , is there any updates on this issue? We are impacted by it. See Azure/Azure-Spring-Apps#36. Looking forward to an option to disable ActiveProcessorCount helper. |
Actually, as we are forced to override all of the memory settings the memory calculator sets. I would appreciate it if we had just a flag to disable it completely if this is possible somehow |
hello! |
I've been strongly against disabling the memory calculator because it provides a vital function. If you're having issues, it's usually just forcing you to address them now versus at 3am when your app is crashing (ie. It fails fast so you can fix stuff now). But as @anthonydahanne said please open an issue with more specifics about your problem. This issue is not regarding the memory calculator, so please open a new issue for memory calculator questions. I also think that we need to do a review of Java 17 and the upcoming Java 21 and see how they work in a container. Java has been improving its container awareness so perhaps we can disable memory calculator in newer versions if the JVM functions correctly. Thanks |
Hi @dmikusa, For the code active_processor_count looks like a bug as we calculate the active processor count by node CPU but not container request CPU, so in some small application customer request just 1 CPU, but still get 16 active processor count. Can we just remove the active processor call from helper main before we have conclusion #320 In fact from JDK8, JDK can get the right CPU numbers and we needn't to inject one https://bugs.openjdk.org/browse/JDK-8140793 |
@showpune (or anyone interested) - I'm not opposed to removing this, but I think we need a quick proof of concept first. I'd like to see what the JVM is telling us and compare that to what we get from Golang (i.e., the helper). There are two cases to compare for each major Java version: 1.) When using container limits for the number of CPUs and 2.) When using container limits for a number of CPU shares. I think it's important we examine the results from both to make sure we're giving users the best experience. Thanks |
I agree with @dmikusa : we can't just remove this functionality out of sheer hope or just for 1 single use case. We need facts / use cases that it really breaks apps. @showpune I invite you to show us how the
|
Hi @dmikusa , @anthonydahanne As an alternative solution, can we use /sys/fs/cgroup/cpu to get the CPU processor count? |
It wouldn't surprise me if that is what our Go helper is doing now. I'm open to any option, but the same request applies. We need to see a.) a use case for the change, especially if it'll change the default, and b.) a proof of concept that demonstrates the new solution works better than the present solution. |
Hi, |
What about using runtimepb.NumCPU() instead of runtime.NumCPU()? |
A workaround to this bug is to set the following runtime environment variable: This will fallback to the default (and correct) Java container support behavior which is to honor the container cpu limits. |
Tried to use the JAVA_TOOL_TOPTIONS, but it didn't have any affect on the starting pod Build Configuration: |
In my case the -XX:ActiveProcessorCount is set to a value, that slows down the application startup by 2x - e.g. 15s -> 32s It would be nice to disable this. A workaround in Kubernetes is to set entrypoint to point directly to:
Which turns off other buildpack features - because the original entrypoint no longer works. |
Now the
|
@keskad Can you expand on your environment? What value for ActiveProcessorCount is being picked by the buildpack that causes the start-up times to be slower? What are the constraints put on your application by the container orchestrator? Is it limiting your CPU count to 1, and/or are there CPU share limitations put on the app too? Thanks. Just trying to understand the circumstances behind that statement, so we can test/reproduce. |
For those limitations in docker-compose.yaml: resources:
limits:
cpus: '0.75'
memory: 1024M
reservations:
cpus: '0.10'
memory: 256M and this specs:
The value of |
I confirm what is mentioned in previous comment, and the impact can be huge, so big +1 from me to fix this issue. I was struggling to understand why the
And it seems due to the fact the hardware number of CPU are taken in account to compute You can reproduce with:
That gives me on my machine Now, if I override the value to set the one I would expect:
I see on my machine something like My use case is to simulate cheap servers on my powerful laptop to be able to perform benchmarks for Spring projects, and I also would like to use that feature in my upcoming talks. As I can see in #136 (comment), there seems also to have impact on production. I know close to nothing in Go but maybe the proposal in #136 (comment) is worth to explore:
|
@sdeleuze Thanks for providing some details. I think what I'd like to do is just have the buildpacks backoff and not provide that helper for newer Java versions. The JDK is supposed to be able to detect CPU usage in containers correctly now, so we don't really need to do this in the buildpack. I've got a PR up that does this for Java 17+. There's a test image up here You can try that by running Thanks! |
I tried but got errors like:
I am probably doing something wrong, any chance you could share the Spring Boot Maven configuration I am expected to use to test it (I am not using Also I am wondering if we could just avoid setting |
I got the same error with maven and
@dmikusa did you publish your buildpack with a pack exp that supports dual arch? |
I picked 17 arbitrarily. I wasn't sure exactly what versions of Java had the container-aware CPU sizing improvements. I didn't have time to dig in, so I thought I'd just start here. We can absolutely tweak this to different versions, or just remove it altogether if we're confident that all the fixes in this area were backported to Java 8. Changing Java 8/11 behaviors makes me nervous though, they've been out in the field a while and people expect things not to change there. Maybe a feature flag is necessary here? We could have Java 8/11 be default enabled, so behavior doesn't change, but users can opt-in, and 17+ it defaults to on. Anyway, curious to know what everyone thinks about it. Thanks for all the feedback! |
Disregard that. That was me screwing things up. I'd built this demo buildpack as multi-arch, so you need to use a multi-arch builder too. That's why it's picking the wrong arch. Try this command:
For SB Build Tools, use what @anthonydahanne posted. Add |
Ok that's fine if you prefer to focus on Java 17+ at least initially, and IMO no need for a feature flag (can be customized via Java options). I will test and provide a feedback tomorrow (AFK right now). |
I was already using |
@sdeleuze Sorry. We had a bug in our build script and it was not creating the binaries for amd64 correctly. I've fixed that and have a new image out, can you try |
Np, with this new version, I confirm my sample application using Buildpacks Java 21 and CDS is starting 4x faster without having ActiveProcessorCount customization, so all good! |
OK, so there's definitely performance implications to the way that this is being set currently by the buildpack. Thanks to everyone for providing details and information on this topic. We're going to make the following changes: #388 This disables the active processor count helper for versions of Java 17 and newer. This essentially allows the JVM to use its logic to auto-configure this value. We picked 17 because it is newish and people are still migrating to it (thus it doesn't have a lot of users' established expectations like Java 8/11), and we believe it to have solid logic for detecting container CPU limits and configuring itself correctly. It's possible that Java 8/11 would work as well but this is riskier to change as we don't want to break existing user apps/expectations. It's possible users could be adjusted to the buildpacks current method of operation and that while making this change would be correct, it might disrupt their existing apps. For users of Java 8/11, if you're seeing this issue you can use this workaround:
This works because when you manually set Or of course you can upgrade to Java 17+ which is awesome in its own right. |
Only contribute active-processor-count helper for Java versions < 17
What happened?
The ActiveProcessorCount helper's results can be confusing.
In general, I want to limit the CPU resources using Kubernetes or Docker for my application and I'd like the application to automatically adjust to these changes. The Java buildpack has the ActiveProcessorCount helper to do this.
If you limit the number of CPUs available to a container using Kubernetes limits or
docker run --cpus=X
the active processor count code will pull the total CPUs, not the limited number of CPUs. What!?This happens because these methods of limiting CPU resources are based on CPU shares, so you do technically have access to all of the CPUs on the machine. You just have a limited quantity of time on the CPUs relative to all of the other processes running at the time.
This can sometimes cause performance problems because assumptions can be made that if you have 8 CPUs you have full access to 8 CPUs.
It would be nice if there were a way for the processor count to scale down based on CPU limits/shares in a similar way to how it will scale down if you use CPU affinity and bind the process to only a subset of the CPUs.
When using CPU limits/shares, ActiveProcessorCount reports the total number of CPUs on the system.
Build Configuration
paketo-buildpacks/java
See here for more details.
The text was updated successfully, but these errors were encountered: