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

Cleanup load tests #2779

Closed
wants to merge 5 commits into from
Closed

Conversation

mangalpalli
Copy link
Contributor

@mangalpalli mangalpalli commented Oct 27, 2022

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix

What this PR does / Why we need it:

After building container and running this command "docker run --rm --network="host" -e "LOCUST_FILE=gameserver_allocation.py" -e "TARGET_HOST=http://127.0.0.1:8001" -p 8089:8089 locust-files:latest", it is showing "missing /usr/local/bin/locust".

LOCUST files are no longer updated, and had to be removed.

Removed all the content related to LOCUST files.

Which issue(s) this PR fixes:

Closes #2744

Special notes for your reviewer:

@mangalpalli mangalpalli added the kind/bug These are bugs. label Oct 27, 2022
@mangalpalli mangalpalli added this to the 1.28.0 milestone Oct 27, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mangalpalli
Once this PR has been reviewed and has the lgtm label, please assign dzlier-gcp for approval by writing /assign @dzlier-gcp in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markmandel markmandel changed the title Issue 2744 Cleanup load tests Oct 27, 2022
@markmandel markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Oct 27, 2022
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e532b874-467f-4db4-b6fe-2bf7728a96e8

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2779/head:pr_2779 && git checkout pr_2779
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-5ff6f61-amd64

@gongmax
Copy link
Collaborator

gongmax commented Oct 27, 2022

Do we want to remove those four files under the allocation folder? Looks like this test is not using LOCUST?

@markmandel
Copy link
Member

My original thought was to move all the contents of https://github.com/mangalpalli/agones/tree/issue-2744/test/load/allocation/grpc up to https://github.com/mangalpalli/agones/tree/issue-2744/test/load - but now I'm not sure.

Maybe it should all be moved to https://github.com/mangalpalli/agones/tree/issue-2744/test/load/allocation (remove the grpc part). Do we think we'll have more types of load tests?

What do we think? @roberthbailey ?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: ae2d8036-2987-4b6a-8bb8-e8739d0df77f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Member

That's a new flake:

{"message":"Wait for cache sync","severity":"info","time":"2022-10-28T17:36:00.749700994Z"}
{"message":"unable to calculate 'Creating' state duration of 'd9rnhlh7tx' GameServer","severity":"warning","source":"*metrics.Controller","time":"2022-10-28T17:36:00.852651182Z"}
{"message":"unable to calculate 'Ready' state duration of 'd9rnhlh7tx' GameServer","severity":"warning","source":"*metrics.Controller","time":"2022-10-28T17:36:01.853235532Z"}
--- FAIL: TestControllerGameServerCount (2.11s)
    controller_test.go:76: 
        	Error Trace:	controller_test.go:76
        	            				controller_test.go:146
        	Error:      	Not equal: 
        	            	expected: 3
        	            	actual  : 4
        	Test:       	TestControllerGameServerCount
        	Messages:   	number of timeseries does not match under metric: gameservers_count

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 4cc1bed6-21d8-44d0-a289-3b7085fc91f3

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2779/head:pr_2779 && git checkout pr_2779
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-5e86164-amd64

@roberthbailey
Copy link
Member

My original thought was to move all the contents of https://github.com/mangalpalli/agones/tree/issue-2744/test/load/allocation/grpc up to https://github.com/mangalpalli/agones/tree/issue-2744/test/load - but now I'm not sure.

I think we all agree that we should remove the files under https://github.com/googleforgames/agones/tree/main/test/load/locust-files as well as run.sh and Dockerfile under https://github.com/googleforgames/agones/tree/main/test/load. I think we should leave a README.md in test/load though. The first sentence from https://github.com/googleforgames/agones/blob/main/test/load/README.md is really good and seems like it should remain, but the rest of the file describes Locust configuration and can be removed.

Maybe it should all be moved to https://github.com/mangalpalli/agones/tree/issue-2744/test/load/allocation (remove the grpc part). Do we think we'll have more types of load tests?

What do we think? @roberthbailey ?

The only real difference between https://github.com/googleforgames/agones/blob/main/test/load/allocation/allocationload.go and https://github.com/googleforgames/agones/blob/main/test/load/allocation/grpc/allocationload.go is the type of request that they create and how they connect. Otherwise, the logic is basically the same. It wouldn't be too difficult to combine them into a single file that could connect either way (depending on command line args).

That being said, we have only really run the grpc version in recent memory. I don't know if anyone else is using the non-grpc version. But I would be ok removing it and only running load tests using the grpc client for now and if we want to extend it to also connect via the k8s api then we can extend that client rather than forking it. In that scenario we would remove the 4 files in https://github.com/googleforgames/agones/tree/main/test/load/allocation and promote the 11 files in https://github.com/googleforgames/agones/tree/main/test/load/allocation/grpc up the directory tree.

Given that the other load test directories are mostly a distraction, I agree with your original suggestion of moving the contents of the gprc allocation load test into test/load which will clean things up significantly. If we change our mind later, we can always recover the old files from git.

@markmandel
Copy link
Member

Given that the other load test directories are mostly a distraction, I agree with your original suggestion of moving the contents of the gprc allocation load test into test/load which will clean things up significantly. If we change our mind later, we can always recover the old files from git.

That sounds good to me too. I agree with everything above 👍🏻

@mangalpalli does that make sense?

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8d575c9b-0668-4e97-93a8-5478e1d107a9

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2779/head:pr_2779 && git checkout pr_2779
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-853acd8-amd64

@mangalpalli
Copy link
Contributor Author

@markmandel aligned with the discussion. Just one thing, do we still keep directory structure as
test/load/allocation/grpc/files or we move all the files under grpc as test/load/files?

@markmandel
Copy link
Member

The directory structure should be test/load/allocation/ once complete.

With a README in test/load and the rest of the files in test/load/allocation/

@mangalpalli
Copy link
Contributor Author

Hi, Just created a new PR- #2784 as the file directory structure had to be changed.

@mangalpalli
Copy link
Contributor Author

I though it would be easier as I had to revert the directory structure on existing branch.

@roberthbailey roberthbailey removed this from the 1.28.0 milestone Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug These are bugs. kind/cleanup Refactoring code, fixing up documentation, etc size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shows missing "/usr/local/bin/locust" after building container
5 participants