search

justb4 / docker-jmeter

Docker image for Apache JMeter
MIT License
278 stars 310 forks source link

Feature Request: Make /proc/meminfo/ readout optional #61

Closed mophima closed 2 years ago

mophima commented 2 years ago

Feature/Problem Description: /proc/meminfo/ always reads out the memory of the host machine (see https://fabiokung.com/2014/03/13/memory-inside-linux-containers/). This is a problem when running the container in a cluster environment like Kubernetes or OpenShift.

Proposed Solution: Add a new, optional command line parameter to control the "freeMem" parameter, which does not get passed to the jmeter call.

justb4 commented 2 years ago

I can see that this may be an issue but isn't this always the case, for example when running multiple Docker Containers and/or assigning a specific memory maximum like with docker run --memory 512m ...?

It is hard for me to follow the new optional command line parameter logic. Like the case --). Suggestion: Isn't it easier to introduce a new variable like FREE_MEM which can be passed to the container? If set, its value will override getting freeMem from/proc/meminfo.

mophima commented 2 years ago

Im not sure about the problem apprearing with vanilla docker, but with Kubernetes/Openshift, this certainly is an issue.

The ---) case breaks the loop when no more options are available. As I understand it, this is necessary for null parameters, but I baiscally went with the implementation found here.

Suggestion: Isn't it easier to introduce a new variable like FREE_MEM which can be passed to the container? If set, its value will override getting freeMem from/proc/meminfo.

This is baiscally what I am trying to do. In line 24, a new parameter "-max_memory " is filtered and assigned to the freeMem variable. The name could of course be altered to "-FREE_MEM", if desired. The rest of the code retrieves the remaining command line parameters, which will then be added to the options list, which in turn gets passed to jmeter directly in line 64. If no -max_memory parameter is set, the code will remain as is and use the readout of /proc/meminfo.

mophima commented 2 years ago

I believe I understand now, what you were trying to say. I edited the Skript so that it should work with environment variables instead of using a cmd arg.

It now features three variables: JVM_XMS, JVM_XMN and JVM_XNX which can be set. If not, the default of reading out /proc/meminfo should be used.

Is this more what you had in mind?

justb4 commented 2 years ago

Yes, using variables is very common in Docker-based deployments, especially in Kubernetes. This is much more flexible than adapting the entrypoint command line.

Your new script has some problems, see my review, upcoming.

mophima commented 2 years ago

I changed it :)

justb4 commented 2 years ago

Ok, looks good. One last request: can you add a few lines in the README.md to document the three new optional env vars, how and when to use? Thanks! Then we are good to go/merge!

mophima commented 2 years ago

I added a description to the readme