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

Custom fleet autoscaler resync interval #2171

Merged

Conversation

jie-bao
Copy link
Contributor

@jie-bao jie-bao commented Jul 7, 2021

What type of PR is this?
/kind feature

What this PR does / Why we need it:
Add a new property sync to FleetAutoScaler for supporting custom sync interval.
See more details in: #1955

Which issue(s) this PR fixes:

Closes #1955

Special notes for your reviewer:
None

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3b54a562-568d-460e-bb3d-352d190c3a1c

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5dcdd297-bcef-480b-a43b-96bec6376f04

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: d950e74b-3b09-47b5-b931-cefd25b734b3

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

Copy link
Collaborator

@steven-supersolid steven-supersolid left a comment

Choose a reason for hiding this comment

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

Just approving the package-lock.json change which is unrelated but at least syncs package-lock and package again :)

@markmandel
Copy link
Member

Handholding testing, because change in CRDs. (I can't wait until this gets fixed), and restarting tests!

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: def1f3c2-5347-41cf-b966-580d6432ca59

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 826e136c-0548-4f9f-bec9-437e4d5b4063

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

@jie-bao
Copy link
Contributor Author

jie-bao commented Jul 8, 2021

Handholding testing, because change in CRDs. (I can't wait until this gets fixed), and restarting tests!

Sorry, I found a bug and fixed it. But the CI is blocked on the e2e-stable again. Is there a way I can work around it by myself?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0eb30eaa-b0a7-43e1-9bdb-61a010b31784

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

@markmandel
Copy link
Member

Huh, that's odd. I removed the helm install. I'll do it again.

I wish there was a way for you to fix things (or for it not to be a problem, but I'll handhold it through).

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 4ceac768-a542-49cf-96b9-8dc4add0c933

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

@markmandel
Copy link
Member

--- FAIL: TestAutoscalerBasicFunctions (408.14s)
    fleetautoscaler_test.go:103: error waiting for fleet condition on fleet simple-fleet-qctp9
FAIL
time="2021-07-08 18:08:45.322" level=info msg="Namespace 1625767270 is deleted"
FAIL	agones.dev/agones/test/e2e	454.668s
FAIL
make[1]: *** [test-e2e-integration] Error 1
Makefile:273: recipe for target 'test-e2e-integration' failed
make[1]: Leaving directory '/workspace/build'
Makefile:265: recipe for target 'test-e2e' failed
make: *** [test-e2e] Error 2

Seems like a legitimate failure, but I'll run it again for you, just so we can be sure 👍🏻

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 13d3c2b5-fe1d-4f3b-b0a0-2f285c773e22

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

@markmandel
Copy link
Member

Nope, looks like a legit issue, same failure on e2e-feature-gates

--- FAIL: TestAutoscalerBasicFunctions (389.41s)
    fleetautoscaler_test.go:103: error waiting for fleet condition on fleet simple-fleet-mlq8d
FAIL
time="2021-07-08 18:58:56.791" level=info msg="Namespace 1625770241 is deleted"
FAIL	agones.dev/agones/test/e2e	495.298s
FAIL
Makefile:273: recipe for target 'test-e2e-integration' failed

@jie-bao jie-bao force-pushed the custom-fleet-autoscaler-resync-interval branch from 2b4ff91 to c55a67c Compare August 2, 2021 03:45
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 070e46c9-ce14-47f7-8239-38ce892112c6

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: c14d9960-700f-4d7c-ba78-f6f1b624b18f

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

@jie-bao
Copy link
Contributor Author

jie-bao commented Aug 2, 2021

Hi, above build failed. But it doesn't look like an issue involved by this PR. Could anyone help retry it?

@roberthbailey
Copy link
Member

Here's the error:

Starting e2e integration test!
go test -mod=vendor -race -parallel=32 agones.dev/agones/test/e2e --kubeconfig /root/.kube/config \
	--gameserver-image=gcr.io/agones-images/simple-game-server:0.3 \
	--pullsecret= \
	--feature-gates="" \
	--namespace=""
time="2021-08-02 04:51:31.777" level=info msg="Starting e2e test(s)" featureGates="CustomFasSyncInterval=false&Example=true&NodeExternalDNS=false&PlayerAllocationFilter=false&PlayerTracking=false&RollingUpdateOnReady=true&SDKWatchSendOnExecute=true&StateAllocationFilter=false" gameServerImage="gcr.io/agones-images/simple-game-server:0.3" namespace= perfOutputDir= pullSecret= stressTestLevel=0 version=
time="2021-08-02 04:51:31.849" level=info msg="cause for namespace deletion error" reason=NotFound
time="2021-08-02 04:51:31.849" level=error msg="failed to cleanup e2e namespaces" error="namespaces \"1627879575\" not found"
FAIL	agones.dev/agones/test/e2e	0.159s
FAIL
Makefile:273: recipe for target 'test-e2e-integration' failed

@roberthbailey
Copy link
Member

I restarted the test.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 9dbf9786-1aa4-44b1-9736-2908c5143fdd

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/2171/head:pr_2171 && git checkout pr_2171
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-337a5f1

@jie-bao jie-bao requested a review from markmandel August 4, 2021 02:41
@jie-bao
Copy link
Contributor Author

jie-bao commented Aug 4, 2021

Hi @markmandel , could you take a look of this PR again? I think it might be ready :)

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Sorry about the delay! Just caught a minor documentation thing that i missed first time - but I think that is the last item!

Co-authored-by: Mark Mandel <markmandel@google.com>
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b1aabd91-6f58-4088-8cae-4c72d87137d5

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/2171/head:pr_2171 && git checkout pr_2171
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-5a23022

@markmandel
Copy link
Member

markmandel commented Aug 9, 2021

One final final thing (looks like I wasn't clear in my last comment)

The changes you made to the example yaml in site/content/en/docs/Reference/fleetautoscaler.md - add them into https://github.com/googleforgames/agones/blob/main/examples/fleet.yaml (it's basically the same thing content, just gives you two different ways to look at it) - that file is a full Fleet specification, so you can see all the options in a yaml file.

That's it. Once that's done, will approve and merge! 👍🏻 nice work!

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 000cf049-61a2-47eb-b656-d2fa1be0d688

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/2171/head:pr_2171 && git checkout pr_2171
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-088a908

@jie-bao
Copy link
Contributor Author

jie-bao commented Aug 10, 2021

One final final thing (looks like I wasn't clear in my last comment)

The changes you made to the example yaml in site/content/en/docs/Reference/fleetautoscaler.md - add them into https://github.com/googleforgames/agones/blob/main/examples/fleet.yaml (it's basically the same thing content, just gives you two different ways to look at it) - that file is a full Fleet specification, so you can see all the options in a yaml file.

That's it. Once that's done, will approve and merge! 👍🏻 nice work!

I think you meant https://github.com/googleforgames/agones/blob/main/examples/fleetautoscaler.yaml since there's no FleetAutoScaler in fleet.yaml. I updated it.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e5a8b30b-6a2b-4633-84fe-edfacd5c060e

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/2171/head:pr_2171 && git checkout pr_2171
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-4c88875

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

I think you meant https://github.com/googleforgames/agones/blob/main/examples/fleetautoscaler.yaml since there's no FleetAutoScaler in fleet.yaml. I updated it.

Yes! That's the one! 🥳

@markmandel markmandel merged commit 0c1fd4f into googleforgames:main Aug 10, 2021
@markmandel markmandel added this to the 1.17.0 milestone Aug 10, 2021
@markmandel markmandel added the kind/feature New features for Agones label Aug 10, 2021
@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jie-bao, markmandel, steven-supersolid

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

The pull request process is described 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable custom resync period for FleetAutoscaler
6 participants