search

hellofresh / kangal

Run performance tests in Kubernetes cluster with Kangal
Apache License 2.0
159 stars 22 forks source link

Don't allow different image tags for JMeter master and worker Docker images #278

Closed s-radyuk closed 11 months ago

s-radyuk commented 11 months ago

Current behaviour: Kangal allows to use custom docker images when creating a loadtest object. For JMeter backend docker images for master and worker pods are defined separately. But they should still have the same tag to avoid inconsistency when running master and worker with different JMeter versions.

Solution: This PR removes the usage of separate MasterImageTag and WorkerImageTag in code. Now the same tag will be used for both master and worker pods.

codecov-commenter commented 11 months ago

Codecov Report

Merging #278 (61be5e4) into master (2f21cf4) will increase coverage by 0.03%. The diff coverage is 80.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   56.10%   56.13%   +0.03%     
==========================================
  Files          37       37              
  Lines        4112     4115       +3     
==========================================
+ Hits         2307     2310       +3     
  Misses       1682     1682              
  Partials      123      123
Files Coverage Δ
pkg/backends/jmeter/backend.go 39.90% <100.00%> (+0.83%) :arrow_up:
pkg/backends/jmeter/resources.go 69.15% <0.00%> (ø)
lucasmdrs commented 11 months ago

This change might cause some problems for some Kangal users. :warning:

Restricting the worker and master images to have the same tag would force the way they tag their custom images (if any) and not necessarily fix the issue of having different JMeter versions running.

Aside from that, for JMeter backend master and worker images are also allowed to be passed from the Proxy request or even directly in the LoadTest spec, which makes this issue way more complex than what it seems.

I recommend we leave things as it is, since it gives the users safe defaults but flexibility when they need at their own responsibility to ensure compatibility.

s-radyuk commented 11 months ago

After discussion we realized we don't have enough evidence of different usage patterns and can't really make assumptions on what is the best approach for users. We agreed to leave the current logic in place.