-
Notifications
You must be signed in to change notification settings - Fork 27
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
Migrations-1079 - Adding Benchmark Testing Environment #231
Conversation
Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #231 +/- ##
=========================================
Coverage 58.19% 58.19%
Complexity 515 515
=========================================
Files 68 68
Lines 2643 2643
Branches 245 245
=========================================
Hits 1538 1538
Misses 963 963
Partials 142 142
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Omar Khasawneh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "testingEnvironment" is going to be very overloaded. Instead of having a separate directory/project, I'd overlay everything that you have into the trafficCaptureProxyServerTest directory, merging the build.gradle files.
dependencies { | ||
implementation project(':trafficCaptureProxyServer') | ||
testImplementation project(':trafficCaptureProxyServerTest') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can fall away by merging these changes into the ...ProxyServerTest directory
import com.bmuschko.gradle.docker.tasks.image.DockerBuildImage | ||
import org.apache.tools.ant.taskdefs.condition.Os | ||
|
||
def calculateDockerHash(String projectName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be refactored into a common shared class and imported by both projects
task("buildDockerImage_${projectName}", type: DockerBuildImage) { | ||
def hash = calculateDockerHash(projectName) | ||
images.add("testenv/${dockerImageName}:$hash") | ||
images.add("testenv/${dockerImageName}:latest") | ||
inputDir = project.file("src/main/docker/${projectName}") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be refactored into a common shared class and imported by both projects
def dockerBuildDir = "build/docker/${projectName}" | ||
def artifactsDir = "${dockerBuildDir}/jars"; | ||
task("copyArtifact_${projectName}", type: Copy) { | ||
dependsOn ":${projectName}:build" | ||
dependsOn ":${projectName}:jar" | ||
from { project(":${projectName}").configurations.findByName("runtimeClasspath").files } | ||
from { project(":${projectName}").tasks.getByName('jar') } | ||
into artifactsDir | ||
include "*.jar" | ||
duplicatesStrategy = DuplicatesStrategy.WARN | ||
|
||
if (projectName == "trafficCaptureProxyServerTest") { | ||
include "*.properties" | ||
from project.file("src/main/docker/${projectName}/jmeter.properties").absolutePath | ||
into "${dockerBuildDir}" | ||
} | ||
} | ||
|
||
task "createDockerfile_${projectName}"(type: com.bmuschko.gradle.docker.tasks.image.Dockerfile) { | ||
dependsOn "copyArtifact_${projectName}" | ||
destFile = project.file("${dockerBuildDir}/Dockerfile") | ||
def baseImageOverrideProjectName = baseImageProjectOverrides.get(projectName) | ||
if (baseImageOverrideProjectName) { | ||
def dependentDockerImageName = dockerFilesForExternalServices.get(baseImageOverrideProjectName) | ||
def hashNonce = calculateDockerHash(baseImageOverrideProjectName) | ||
from "testenv/${dependentDockerImageName}:${hashNonce}" | ||
dependsOn "buildDockerImage_${baseImageOverrideProjectName}" | ||
runCommand("sed -i -e \"s|mirrorlist=|#mirrorlist=|g\" /etc/yum.repos.d/CentOS-* ; sed -i -e \"s|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g\" /etc/yum.repos.d/CentOS-*") | ||
runCommand("yum -y install nmap-ncat") | ||
} else { | ||
from 'openjdk:11-jre' | ||
if (projectName == "trafficCaptureProxyServerTest") { | ||
instruction 'COPY jmeter.properties /jmeter.properties' | ||
} | ||
runCommand("apt-get update && apt-get install -y netcat") | ||
} | ||
|
||
copyFile("jars", "/jars") | ||
// can't set the environment variable from the runtimeClasspath because the Dockerfile is | ||
// constructed in the configuration phase and the classpath won't be realized until the | ||
// execution phase. Therefore, we need to have docker run the command to resolve the classpath | ||
// and it's simplest to pack that up into a helper script. | ||
runCommand("printf \"#!/bin/sh\\njava -cp `echo /jars/*.jar | tr \\ :` \\\"\\\$@\\\" \" > /runJavaWithClasspath.sh"); | ||
runCommand("chmod +x /runJavaWithClasspath.sh") | ||
// container stay-alive | ||
defaultCommand('tail', '-f', '/dev/null') | ||
//defaultCommand('/runJavaWithClasspath.sh', '...') | ||
} | ||
} | ||
|
||
(javaContainerServices).forEach { projectName, dockerImageName -> | ||
def dockerBuildDir = "build/docker/${projectName}" | ||
task "buildDockerImage_${projectName}"(type: DockerBuildImage) { | ||
dependsOn "createDockerfile_${projectName}" | ||
inputDir = project.file("${dockerBuildDir}") | ||
images.add("testenv/${dockerImageName}:${version}") | ||
images.add("testenv/${dockerImageName}:latest") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be refactored into a common shared class and imported by both projects
image: 'testenv/jmeter:latest' | ||
networks: | ||
- testing | ||
command: /bin/sh -c "while sleep 10; do /runJavaWithClasspath.sh org.opensearch.migrations.trafficcapture.JMeterLoadTest -p 9201 -d captureproxy; done" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to run this within a loop? The JMeter configuration would be able to loop as many times as you'd like it to. Managing multiple processes in a container is less desirable than managing just one.
version: '3.7' | ||
services: | ||
nginx: | ||
image: 'testenv/nginx:latest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: A slightly more descriptive name than 'nginx' might be helpful for others
# Switch to noninteractive mode to avoid interactive prompts during package installation | ||
ENV DEBIAN_FRONTEND=noninteractive | ||
|
||
RUN apt-get update && apt-get upgrade -y && apt-get install -y --no-install-recommends vim \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to install vim? What does the process need vim for? Please leave a comment, especially if it's just nice to have for somebody exec-ing a shell within the container.
@@ -49,7 +49,7 @@ static class Parameters { | |||
@Parameter(required = false, | |||
names = {"--noCapture"}, | |||
arity = 0, | |||
description = "Directory to store trace files in.") | |||
description = "If enabled, Does NOT store trace files in.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... s/store trace files/capture traffic to ANY sink/
names = {"-d", "--domain-name"}, | ||
description = "Domain name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/domain-name/server-name/.
…ngEnvironment w/ proxyServerTest Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
…h an updated version Signed-off-by: Omar Khasawneh <[email protected]>
static class Parameters { | ||
@Parameter(required = true, | ||
names = {"-p", "--port"}, | ||
description = "Port number") | ||
int backsidePort; | ||
@Parameter(required = true, | ||
names = {"-s", "--server-name"}, | ||
description = "Server name") | ||
String domainName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http vs https will eventually be a concern too. We could also take in a URL and parse those three components. That's what the replayer or the proxy (IIRC) does, but it's weird because the path and query string wouldn't be allowed/used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this note, are the tests run to test HTTPS or HTTP? HTTPS should be the default. I can't tell from the HttpSampler interface how to set it other than by specifying a fullUrl, which we aren't doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HttpSampler has a setProtocol method, which by default is set to HTTP, added a parameter for that in next commit.
@@ -1,6 +1,30 @@ | |||
version: '3.7' | |||
services: | |||
webserver: | |||
image: 'nginx-simple:latest' | |||
image: 'migrations/nginx:latest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably choose a more descriptive name than nginx. For this project, it is fine, but the image will live in a more general migrations space. One might presume that it's a more general purpose nginx container, but it's instead highly tailored to this testing framework. Something like nginx-perf-test-webserver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I thought you meant the name of the container, not the image. I'll update that as you recommended.
} | ||
} | ||
|
||
static def createDockerfile(Project project, String projectName, Map<String, String> baseImageProjectOverrides, Map<String, String> dockerFilesForExternalServices) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need some eventual refactoring - but not for this task. There are a lot of assumptions that we're building precisely 2 kinds of dockerfiles. We also could be better off w/ static Dockerfiles for each of these. Again, those changes would not be for this PR.
@@ -1,9 +1,10 @@ | |||
plugins { | |||
id 'org.opensearch.migrations.java-library-conventions' | |||
id "com.avast.gradle.docker-compose" version "0.16.12" | |||
id "com.bmuschko.docker-java-application" version "9.3.1" | |||
id 'com.bmuschko.docker-remote-api' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this required for? I assume that the plugin is effectively being loaded by CommonUtils now. Is this line required? If so, can you document why and put a version on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id 'com.bmuschko.docker-remote-api' plugin is necessary for the remaining DockerBuildImage tasks to function properly. While this plugin is now declared in the buildSrc/build.gradle
file, it's not automatically applied to the trafficCaptureProxyServerTest/build.gradle
file and needs to be explicitly declared. As for the version number, adding it here causes a conflict because the plugin is already on the classpath due to including it in the buildSrc/build.gradle
file. So to sum it up, this line IS required but W/O a specified version. The version is managed within the buildSrc/build.gradle
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I understand the version. Thanks for the reference.
For my own future understanding, the plugin isn't imported in the common code, just some java imports from it are and the remote-api plugin is a better fit to pickup the remaining other pieces unique for this grade file.
Signed-off-by: Omar Khasawneh <[email protected]>
@@ -1,9 +1,10 @@ | |||
plugins { | |||
id 'org.opensearch.migrations.java-library-conventions' | |||
id "com.avast.gradle.docker-compose" version "0.16.12" | |||
id "com.bmuschko.docker-java-application" version "9.3.1" | |||
id 'com.bmuschko.docker-remote-api' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I understand the version. Thanks for the reference.
For my own future understanding, the plugin isn't imported in the common code, just some java imports from it are and the remote-api plugin is a better fit to pickup the remaining other pieces unique for this grade file.
resolutionStrategy.dependencySubstitution { | ||
substitute module('org.apache.xmlgraphics:batik-codec') using module('org.apache.xmlgraphics:batik-all:1.15') | ||
} | ||
exclude group: 'org.apache.logging.log4j', module: 'log4j-slf4j-impl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you need this line for? What are you excluding it from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to use it to prevent classpath issues caused by multiple logging implementations, which was occuring due to mulptiple dependencies pulling in this Log4j to SLF4J bridge as a transitive dependency.
I would get this exception without it: Exception in thread "main" java.lang.NoSuchMethodError: 'void org.apache.logging.slf4j.Log4jLoggerFactory.<init>(org.apache.logging.slf4j.Log4jMarkerFactory)'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, JMeter and the ProxyCapture project are both bringing in log4j components? Do you know what version you're fixing on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the version is 2.17.2.
+--- org.apache.logging.log4j:log4j-1.2-api:2.17.2 | | \--- org.apache.logging.log4j:log4j-api:2.17.2 -> 2.20.0 | +--- org.apache.logging.log4j:log4j-api:2.17.2 -> 2.20.0 | +--- org.apache.logging.log4j:log4j-core:2.17.2 -> 2.20.0 (*) | +--- org.apache.logging.log4j:log4j-slf4j-impl:2.17.2
dependsOn "createDockerfile_${projectName}" | ||
inputDir = project.file("${dockerBuildDir}") | ||
images.add("migrations/${dockerImageName}:${version}") | ||
images.add("migrations/${dockerImageName}:latest") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for myself. This is identical to code within dockerSolution, but it seems fair to replicate it since it is short and because a couple policies are embedded within it (like the dual image tags) that could vary by project.
image: 'migrations/jmeter:latest' | ||
networks: | ||
- testing | ||
command: /bin/sh -c "while sleep 10; do /runJavaWithClasspath.sh org.opensearch.migrations.trafficcapture.JMeterLoadTest -p 9201 -s captureproxy; done" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please kill the while part. If you want, you can put a tail -f /dev/null in at the end to keep the container alive to make it easier to run additional commands or process files. I'd like to know when a run has been done and have whatever time I need to process the results across the other containers (manually or automatically).
static class Parameters { | ||
@Parameter(required = true, | ||
names = {"-p", "--port"}, | ||
description = "Port number") | ||
int backsidePort; | ||
@Parameter(required = true, | ||
names = {"-s", "--server-name"}, | ||
description = "Server name") | ||
String domainName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this note, are the tests run to test HTTPS or HTTP? HTTPS should be the default. I can't tell from the HttpSampler interface how to set it other than by specifying a fullUrl, which we aren't doing.
…pty jmeter properties file if one is not found Signed-off-by: Omar Khasawneh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one comment
resolutionStrategy.dependencySubstitution { | ||
substitute module('org.apache.xmlgraphics:batik-codec') using module('org.apache.xmlgraphics:batik-all:1.15') | ||
} | ||
exclude group: 'org.apache.logging.log4j', module: 'log4j-slf4j-impl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, JMeter and the ProxyCapture project are both bringing in log4j components? Do you know what version you're fixing on?
Signed-off-by: Greg Schohn <[email protected]>
Description
This is a remake of PR #161, as this one uses a whole new method of creating the containers, similar to how the dockerSolution was implemented by @gregschohn, also following Greg's recommendations on PR #161.
This PR adds the ability to create an environment where JMeter tests are run repeatedly, and its' requests are sent to the capture proxy before hitting the webserver (nginx in our case).
To get the containers up, run
./gradlew trafficCaptureProxyServerTest:composeUp
from the TrafficCapture directory.Issues Resolved
Migrations-1079
Is this a backport? If so, please add backport PR # and/or commits #
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.