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

Swap performance testing implementation to use Hyperfoil. #34

Merged
merged 10 commits into from
Nov 4, 2020
Merged

Conversation

whitingjr
Copy link
Collaborator

This PR will replace the JMeter injector with Hyperfoil.
Included in this PR are an integration with Ansible playbooks. Updates to the documentation for users to get started with the replacement injector.

@whitingjr whitingjr requested a review from eguzki March 26, 2020 13:39
vagrant/3scale.csv Outdated Show resolved Hide resolved
deployment/hosts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
eguzki
eguzki previously requested changes Apr 2, 2020
Copy link
Member

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Some changes are requested with initial review.

@whitingjr whitingjr requested a review from eguzki June 3, 2020 14:37
@whitingjr whitingjr closed this Jun 3, 2020
@whitingjr whitingjr reopened this Jun 15, 2020
@whitingjr whitingjr dismissed eguzki’s stale review July 1, 2020 11:55

The initial review relate to an outdated commit.

@eguzki
Copy link
Member

eguzki commented Jul 2, 2020

In README.md, TOC has not been updated

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@eguzki
Copy link
Member

eguzki commented Jul 2, 2020

what is deployment/files/report.sh file for? It is too big and git does not show and my IDE has issues opening it.

It contains

#TODO create script :)

:O

@whitingjr
Copy link
Collaborator Author

whitingjr commented Jul 3, 2020

The purpose of deployment/files/report.sh is for generating the report html file from the JSON data file.
On second thoughts I will remove this file. Replacing it with a containerized version. To generate the report with a docker run command.

@eguzki
Copy link
Member

eguzki commented Jul 6, 2020

some comments left. Waiting for feedback

@whitingjr
Copy link
Collaborator Author

Feedback provided to comments.

@davidor
Copy link

davidor commented Jul 9, 2020

I'm not familiar with the codebase, so I will not comment about the code changes, but I'd like to give my opinion as a user of the tool.

We've been using this tool to benchmark many versions of 3scale. Jmeter provides all we need, which includes things like: ability to configure the number of connections, the throughput, and nice reports. Why are we changing to Hyperfoil? What problems does it solve?

Also, I'm a bit concerned about changing the tool because from the moment we change, we won't be able to compare with previous results. Has anyone tried to run the same benchmarks we've run for the latest version of 3scale with hyperfoil to see how results compare?

@whitingjr
Copy link
Collaborator Author

whitingjr commented Jul 9, 2020

We've been using this tool to benchmark many versions of 3scale. Jmeter provides all we need, which includes things like: ability to configure the number of connections, the throughput, and nice reports. Why are we changing to Hyperfoil? What problems does it solve?

Also, I'm a bit concerned about changing the tool because from the moment we change, we won't be able to compare with previous results. Has anyone tried to run the same benchmarks we've run for the latest version of 3scale with hyperfoil to see how results compare?

Swapping out the driver to use Hyperfoil has been advocated so that load testing can be achieved easily in a microservices architecture. The driver also supports the ability to distribute the load generation. Amongst one or more Agents.
You can still configure number of connections, throughput and get a nice report afterwards.
Doing a baseline run comparison will give you a good idea of comparability. I'll send you a report (offline) I created that demonstrates the measured difference for a prior (0.6) version of Hyperfoil.

@eguzki
Copy link
Member

eguzki commented Jul 13, 2020

JMeter also supports Distributed Testing https://jmeter.apache.org/usermanual/jmeter_distributed_testing_step_by_step.html

@eguzki
Copy link
Member

eguzki commented Jul 15, 2020

The following params

shared_connections: 64
injector_hyperfoil_target_port: 443

are duplicated in several files called

deployment/group_vars/all.yml

and

deployment/inventory/group_vars/all.yml

Please, make sure more attributes are not duplicated and only used files are being committed.

README.md Outdated Show resolved Hide resolved
@whitingjr
Copy link
Collaborator Author

Maybe, we should remove "CONCURRENCY" from the exposed configuration. And have shared_connections a value big enough to have all agents/executors get at least 1 connection.

I was thinking the same.

@eguzki
Copy link
Member

eguzki commented Jul 28, 2020

I just tested this branch, and still get the same error:

TASK [include_role : hyperfoil.hyperfoil_test] ***********************************************************************************************************************************************************************************************************************************
task path: /home/eguzki/git/perftest-toolkit2/deployment/run.yml:4
ERROR! conflicting action statements: shell, warn

The error appears to be in '/home/eguzki/.ansible/roles/hyperfoil.hyperfoil_test/tasks/main.yml': line 34, column 3, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

    test_files: []
- name: Upload benchmark template
  ^ here

@whitingjr
Copy link
Collaborator Author

I just tested this branch, and still get the same error:

TASK [include_role : hyperfoil.hyperfoil_test] ***********************************************************************************************************************************************************************************************************************************
task path: /home/eguzki/git/perftest-toolkit2/deployment/run.yml:4
ERROR! conflicting action statements: shell, warn

The error appears to be in '/home/eguzki/.ansible/roles/hyperfoil.hyperfoil_test/tasks/main.yml': line 34, column 3, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

    test_files: []
- name: Upload benchmark template
  ^ here

Can you raise an issue on the hyperfoil_test project here please ?

@eguzki
Copy link
Member

eguzki commented Jul 28, 2020

It would be better if you do to explain configuration and environment. I am not familiar with hyperfoil parameters and configuration.

@whitingjr
Copy link
Collaborator Author

whitingjr commented Jul 28, 2020

It would be better if you do to explain configuration and environment. I am not familiar with hyperfoil parameters and configuration.

It isn't anything to do with Hyperfoil as far as I can see. Don't you think this is an Ansible error reported for the shell module execution ?
Have you tried either:
a) editing /home/eguzki/.ansible/roles/hyperfoil.hyperfoil_test/tasks/main.yml to remove the warn parameter. On line 37
b) adding an indent for the warn parameter.

- name: Upload benchmark template 
  shell:
      "{{ lookup('template', 'templates/curl.j2') }} \
    http://{{ hyperfoil_controller_host }}:{{ hyperfoil_controller_port }}/benchmark"
  register: curl_cmd
  failed_when: curl_cmd.rc > 1
    warn: no

then execute the run.yml playbook again to see if this solves the problem ?

@eguzki
Copy link
Member

eguzki commented Jul 28, 2020

If you suggest to modify some third party source of code, please open issue to that repo suggesting to them, not to me. If it fails to me, it could fail to anyone else using it.

@whitingjr
Copy link
Collaborator Author

whitingjr commented Jul 28, 2020

If you suggest to modify some third party source of code, please open issue to that repo suggesting to them, not to me. If it fails to me, it could fail to anyone else using it.

I don't mind creating the issue against the hyperfoil_test project for you to help you with this issue.
Can you at least attempt trying the fix b) I've suggested above to the file /home/eguzki/.ansible/roles/hyperfoil.hyperfoil_test/tasks/main.yml. Then communicate with me with the results of this fix. To see if that solves the problem on your environment ?

@eguzki
Copy link
Member

eguzki commented Jul 28, 2020

Even worse message, Syntax Error.

TASK [include_role : hyperfoil.hyperfoil_test] ***********************************************************************************************************************************************************************************************************************************
task path: /home/eguzki/git/perftest-toolkit2/deployment/run.yml:4
ERROR! Syntax Error while loading YAML.
  mapping values are not allowed in this context

The error appears to be in '/home/eguzki/.ansible/roles/hyperfoil.hyperfoil_test/tasks/main.yml': line 40, column 9, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

  failed_when: curl_cmd.rc > 1
    warn: no
        ^ her

Does it work for you?

@whitingjr
Copy link
Collaborator Author

whitingjr commented Jul 28, 2020

Does it work for you?

On a RHEL/Fedora platform it has always worked.
Thanks for attempting by adding an indent. Can you do another attempt please. This time removing the offending line ?

@eguzki
Copy link
Member

eguzki commented Jul 28, 2020

Last attempt. This PR is not the place to test your roles.

fatal: [192.168.57.3]: FAILED! => {"changed": false, "msg": "AnsibleUndefinedVariable: 'dict object' has no attribute 'injector_target'"}

@whitingjr
Copy link
Collaborator Author

whitingjr commented Jul 28, 2020

Last attempt. This PR is not the place to test your roles.

fatal: [192.168.57.3]: FAILED! => {"changed": false, "msg": "AnsibleUndefinedVariable: 'dict object' has no attribute 'injector_target'"}

From this test it looks like the issue with the Ansible "conflicting action statements: shell, warn" has been solved. I'll raise an issue with the relevant hyperfoil_test project.
Created issue is here.

@eguzki
Copy link
Member

eguzki commented Aug 25, 2020

Is it ready to be tested again?

I see there are new changes. New review is needed.

@whitingjr
Copy link
Collaborator Author

Is it ready to be tested again?

I see there are new changes. New review is needed.

Yes it is ready to be tested.

@eguzki
Copy link
Member

eguzki commented Oct 8, 2020

Proposed refactor: #54

waiting for you input @whitingjr

@eguzki
Copy link
Member

eguzki commented Oct 15, 2020

Another proposed refactor: #59

@eguzki eguzki merged commit 8be51d8 into master Nov 4, 2020
@eguzki eguzki deleted the hyperfoil branch November 4, 2020 16:50
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.

3 participants