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

Allow large test data files (#139) #177

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Conversation

amandahla
Copy link
Contributor

  • Convert CSV to GZIP BASE64 format
  • Create InitContainer that convert back to CSV and copy to volume so worker can access the CSV

@amandahla amandahla closed this Jan 24, 2022
@amandahla amandahla reopened this Jan 24, 2022
@s-radyuk
Copy link
Contributor

Hello @amandahla ! Thank you very much for your interest to Kangal!
I would like to ask you to add an integration test to your PR to make sure your change helps with large testdata files.

@amandahla
Copy link
Contributor Author

@s-radyuk Great project :-)
I'm not sure if it's ok but instead of create a new integration test, I have just created a new testdata.csv file named "testdata-jmeter.csv" (1332K). WDYT?

@s-radyuk
Copy link
Contributor

Hello again @amandahla Sorry for the delay with the answer.

Thank you for paying attention to the problem with large testdata files. We also investigated it before and found some limitations on the Kubernetes side. Here are some cases that we’ve tried and their errors:

  1. Create a loadtest with testdata file with size > 3Mb. The error thrown will be the following:
    "error": "Request entity too large: limit is 3145728".
    It's coming from Kubernetes API preventing me from creating a very big kubernetes object.

  2. Create a loadtest with testdata file 2Mb < size < 3Mb. The error thrown will be the following:
    "error": "rpc error: code = ResourceExhausted desc = trying to send message larger than max (2493228 vs. 2097152)"

  3. Create a loadtest with testdata file size < 2097152. The error thrown will be the following:
    "error": "etcdserver: request is too large"

I saw your proposal and I totally agree that dynamic volume provisioning with ReadWriteMany can help us in this case.

I also had a chance to try out the code from this PR and I've noticed the following change in behaviour (unfortunately our integration tests don't show this problem):

Config map with testdata now contains testdata.csv.gz, which is expected with the change from this PR. But for some reason the data is not extracted to the /testdata directory. Because of it JMeter process running in worker pod throws the following error in logs:
Test failed! MDC{} java.lang.IllegalArgumentException: Could not read file header line for file /testdata/testdata.csv
The JMeter process expects to find the data in CSV format in /testdata/testdata.csv but this directory is empty. This prevents a loadtest from running as designed.

@amandahla
Copy link
Contributor Author

Hi @s-radyuk , thanks for testing!

I'm really sorry but I could swear that I tested this code and it worked fine (maybe it's too hot here in Brazil and it's affecting my brain). Anyway, I have changed a few things and now I suppose that should work with bigger CSV files.

I had to modify the Spec and I'm not sure if I did it in the right place.

To work with ingress-nginx (for example), it would be necessary to change "proxy-body-size" via annotations or configmap like explained here
I did it using this command:

kubectl annotate --overwrite ingress kangal-proxy nginx.ingress.kubernetes.io/proxy-body-size="10M"
kubectl annotate --overwrite ingress nginx.ingress.kubernetes.io/proxy-body-size=10M --all

@amandahla
Copy link
Contributor Author

Update to fix split files between the workers. @thiagoarrais good catch!

@codecov-commenter
Copy link

Codecov Report

Merging #177 (ee345ec) into master (a60aec0) will increase coverage by 0.00%.
The diff coverage is 64.58%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #177   +/-   ##
=======================================
  Coverage   59.89%   59.90%           
=======================================
  Files          36       36           
  Lines        2713     2756   +43     
=======================================
+ Hits         1625     1651   +26     
- Misses        970      985   +15     
- Partials      118      120    +2     
Impacted Files Coverage Δ
pkg/backends/jmeter/backend.go 47.97% <0.00%> (-2.75%) ⬇️
pkg/backends/jmeter/resources.go 85.71% <76.31%> (-1.65%) ⬇️
pkg/proxy/proxy.go 84.79% <100.00%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a60aec0...ee345ec. Read the comment docs.

Copy link
Contributor

@lucasmdrs lucasmdrs left a comment

Choose a reason for hiding this comment

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

Hi @amandahla 👋

Thank you for taking the time for this contribution so far, we really appreciate it!

I left some small comments/suggestions, if you address them and fix the conflicts I'll make sure to review it, to move this PR forward 🙂

pkg/backends/jmeter/backend.go Outdated Show resolved Hide resolved
pkg/proxy/proxy.go Outdated Show resolved Hide resolved
…itcontainer (hellofresh#139)

* create bigger csv file to jmeter integration test
Copy link
Contributor

@lucasmdrs lucasmdrs left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the conflicts, just tested all locally and it seems to be working nicely. ✔️

Thank you very much your contribution !
💚 💙 💜 💛 ❤️

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.

None yet

5 participants