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

add metadata to agones webhook autoscaler request #3957

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

swermin
Copy link

@swermin swermin commented Aug 20, 2024

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
/kind release

What this PR does / Why we need it:
This PR extends the webhook autoscaler with metadata, so that the the service retrieving the request can use the lables/annotations to do the calculation

Which issue(s) this PR fixes:
Closes #3951

Special notes for your reviewer:

Copy link

google-cla bot commented Aug 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added kind/feature New features for Agones size/S labels Aug 20, 2024
@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 5ade9d2e-d20e-4cad-b76b-697960ab7ff0

Status: FAILURE

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

@swermin swermin force-pushed the feature/extend-webhook-autoscaler branch from a970b12 to 9ec84ce Compare August 24, 2024 18:32
Copy link
Collaborator

@igooch igooch 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 the contribution. Could you please also add unit and e2e tests?

@gongmax
Copy link
Collaborator

gongmax commented Sep 19, 2024

@swermin kindly ping on this, could you resolve the conflicts and address the above comments?

@swermin
Copy link
Author

swermin commented Sep 20, 2024

Yes ofc! I have unit tests ready and just left with the e2e tests. I’ll push the current changes and focus on the e2e tests today and this weekend if needed!

@swermin swermin force-pushed the feature/extend-webhook-autoscaler branch from 9ec84ce to f56ba00 Compare September 22, 2024 19:28
@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 166b3f1b-5297-4b2a-bc9b-3b0ec925abe5

Status: FAILURE

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

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 984531ec-7bcc-4fa4-bf36-089d5719a947

Status: FAILURE

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

@swermin
Copy link
Author

swermin commented Sep 24, 2024

@igooch do you have any good suggestions on how to do the e2e tests for this?
Current default webhook autoscaler is using the example image to test the scaling. I feel like adding support in that image feels a bit weird but I have no good idea on how to add something

@swermin swermin force-pushed the feature/extend-webhook-autoscaler branch from b9ab8fd to 8d0c86a Compare September 24, 2024 18:05
@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 0ef393db-4a59-403f-8729-4a86e54fb6d3

Status: FAILURE

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

@igooch
Copy link
Collaborator

igooch commented Oct 8, 2024

I just got back from being out of office, so I'm still catching up.

For the e2e tests, do you need to make changes to examples/autoscaler-webhook/main.go? Depending on the test you may be able to simply add MetaData to the default similar to https://github.com/googleforgames/agones/blob/c26f7371980418b56b816b13d265c9cc9d092f32/test/e2e/fleetautoscaler_test.go#L549C14-L549C38. If you do need to make changes to the examples/autoscaler-webhook/main.go, then a maintainer will need to build and push a new example autoscaler webhook image with the changes. That being said, it looks like the unit test is failing.

From the most recent build:

        	Test:       	TestApplyWebhookPolicyWithMetadata
        	            	actual  : 5
        	            	expected: 11
        	Error:      	Not equal: 
        	Error Trace:	/go/src/agones.dev/agones/pkg/fleetautoscalers/fleetautoscalers_test.go:618
    fleetautoscalers_test.go:618: 
--- FAIL: TestApplyWebhookPolicyWithMetadata (0.01s)

You'll need to run ~/agones/build$ make gen-crd-code to generate the deepcopy for pkg/apis/autoscaling/v1/fleetautoscaler.go, but even with that the test is still failing, so there's another issue going on.

@swermin swermin force-pushed the feature/extend-webhook-autoscaler branch from 8d0c86a to 9e74cc5 Compare October 21, 2024 19:04
@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 00518f36-9edb-49bc-8c2a-b755dd8f48d1

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/3957/head:pr_3957 && git checkout pr_3957
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.45.0-dev-9e74cc5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Webhook autoscaler to send fleet metadata with the request
4 participants