Move RollingUpdateOnReady to stable #2271

Jeffwan · 2021-09-19T23:46:14Z

What type of PR is this?

/kind feature

What this PR does / Why we need it:
Move feature gate RollingUpdateOnReady from beta to stable.

Which issue(s) this PR fixes:

Special notes for your reviewer:
I check all code references but not sure what doc we want to update. Please help give some guidance on that.

/cc @roberthbailey

agones-bot · 2021-09-19T23:54:49Z

Build Failed 😱

Build Id: a51019fc-e0b3-4221-b126-b3de94054151

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

agones-bot · 2021-09-20T19:20:41Z

Build Failed 😱

Build Id: 495a345e-a877-42ca-b16b-7abd2d2e0076

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

site/content/en/docs/Guides/feature-stages.md

site/content/en/docs/Guides/fleet-updates.md

test/e2e/fleet_test.go

roberthbailey · 2021-09-21T18:40:06Z

pkg/fleets/controller_test.go

@@ -1118,11 +1109,6 @@ func TestFeatureRollingUpdateOnReady(t *testing.T) {
 func TestControllerRollingUpdateDeployment(t *testing.T) {


This test is failing:

--- FAIL: TestControllerRollingUpdateDeployment (0.11s) --- FAIL: TestControllerRollingUpdateDeployment/attempt_to_drive_replicas_over_the_max_surge (0.02s) controller_test.go:1280: Error Trace: controller_test.go:1280 fixture.go:521 fake.go:141 fake_gameserverset.go:98 controller.go:592 controller.go:606 controller.go:415 controller_test.go:1285 Error: Not equal: expected: 65 actual : 45 Test: TestControllerRollingUpdateDeployment/attempt_to_drive_replicas_over_the_max_surge --- FAIL: TestControllerRollingUpdateDeployment/statuses_don't_match_the_spec._nothing_should_happen (0.00s) controller_test.go:1280: Error Trace: controller_test.go:1280 fixture.go:521 fake.go:141 fake_gameserverset.go:98 controller.go:492 controller.go:537 controller.go:606 controller.go:415 controller_test.go:1285 Error: Not equal: expected: 15 actual : 10 Test: TestControllerRollingUpdateDeployment/statuses_don't_match_the_spec._nothing_should_happen controller_test.go:1280: Error Trace: controller_test.go:1280 fixture.go:521 fake.go:141 fake_gameserverset.go:98 controller.go:592 controller.go:606 controller.go:415 controller_test.go:1285 Error: Not equal: expected: 15 actual : 0 Test: TestControllerRollingUpdateDeployment/statuses_don't_match_the_spec._nothing_should_happen controller_test.go:1292: Error Trace: controller_test.go:1292 Error: Not equal: expected: false actual : true Test: TestControllerRollingUpdateDeployment/statuses_don't_match_the_spec._nothing_should_happen --- FAIL: TestControllerRollingUpdateDeployment/test_smalled_numbers_of_active_and_allocated (0.00s) controller_test.go:1280: Error Trace: controller_test.go:1280 fixture.go:521 fake.go:141 fake_gameserverset.go:98 controller.go:592 controller.go:606 controller.go:415 controller_test.go:1285 Error: Not equal: expected: 3 actual : 4 Test: TestControllerRollingUpdateDeployment/test_smalled_numbers_of_active_and_allocated

I will rerun it locally and check the problem

@roberthbailey I can reproduce the test failures in my env. Seems change in controller.go is straightforward. I didn't make other changes. Not sure why this test failed. Do you have any clues?

Given that these tests were aimed at when the feature was off assert.NoError(t, utilruntime.ParseFeatures(string(utilruntime.FeatureRollingUpdateOnReady)+"=false")), it's expected they would fail now it's enabled always.

So they would either need to be updated to the correct values, or removed entirely.

I took a quick look at these tests and it isn't immediately obvious to me what we are checking for. What test coverage are we losing if we delete the entire set of tests? Or just delete the 3 that are failing?

This tests various gameserver set and rolling update deployment scenarios, and if they go beyond max surge, remove items as appropriate, etc.

Basically does rollingUpdateDeployment do as it should.

I think it's worth converting this over before moving to stable - also to ensure an extra layer that it is doing as expected.

That makes sense to me. @Jeffwan - we have about 1 more day to try and get this in before the 1.18 release, otherwise we can ship it with 1.19.

em. Seem it's better to change to the appropriate values rather than removing them? Let me check algorithm today and see if I can make the changes.

I think I figure out the problem. I manually change to use c.rollingUpdateRestBeforeFixOnReady() they all pass. They do work for case utilruntime.FeatureRollingUpdateOnReady=false.

agones/pkg/fleets/controller.go

Lines 605 to 610 in aefac73

func (c *Controller) rollingUpdateRest(ctx context.Context, fleet *agonesv1.Fleet, active *agonesv1.GameServerSet, rest []*agonesv1.GameServerSet) error {

if runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) {

return c.rollingUpdateRestFixedOnReady(ctx, fleet, active, rest)

}

return c.rollingUpdateRestBeforeFixOnReady(ctx, fleet, rest)

}

Since we already use rollingUpdateRestFixedOnReady and remove c.rollingUpdateRestBeforeFixOnReady, these test need changes.

I go though the logic and change right number in test cases "test smalled numbers of active and allocated" and "attempt to drive replicas over the max surge"

I remove "statuses don't match the spec. nothing should happen", this is because update won' be called in the past c.rollingUpdateRestBeforeFixOnReady . However, this test case is not valid anymore because c.rollingUpdateRestFixedOnReady does consider active GSS.

@roberthbailey @markmandel Please have another check.

roberthbailey · 2021-09-25T23:33:22Z

We have about a week left before the next release candidate. Do you think you will be able to work on this before the 1.18 release?

Jeffwan · 2021-09-27T17:41:28Z

@roberthbailey Let run try to solve this problem by today. If I can not make it, please take it and help on it.

agones-bot · 2021-09-28T07:11:09Z

Build Failed 😱

Build Id: 45a69a64-8887-43dc-8b87-bd9fcf891cff

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

roberthbailey · 2021-09-29T18:40:49Z

From the latest run:

FAIL
Makefile:259: recipe for target 'test-go' failed

I pulled your changes down locally and running make test-go is succeeding for me....

agones-bot · 2021-09-29T18:54:18Z

Build Failed 😱

Build Id: 2d8e7e3d-73e4-4bc0-aa58-6fb0d2d33720

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

roberthbailey · 2021-09-29T18:55:07Z

I think I was testing the wrong commits. Let me try again.

agones-bot · 2021-10-04T00:50:47Z

Build Failed 😱

Build Id: b3c93ec0-a03a-440f-b441-8e236c4c8d0a

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

Jeffwan · 2021-10-04T01:57:08Z

Is there a way to restart the test? Not sure if it's failed because of flaky test? I manually test controller_test.go and it works in my env

roberthbailey · 2021-10-04T06:46:45Z

I've restarted the test.

agones-bot · 2021-10-04T07:34:10Z

Build Failed 😱

Build Id: 45e75586-c4e8-4140-a376-31132856e309

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

roberthbailey · 2021-10-04T16:01:41Z

It's failing on the html test:

Html Test: Attempt 20
htmltest started at 07:32:50 on /tmp/website
========================================================================
site/index.html
  Non-OK status: 404 --- site/index.html --> https://afterverse.com/
========================================================================
✘✘✘ failed in 674.332884ms
1 errors in 144 documents
includes/website.mk:65: recipe for target 'hugo-test' failed

roberthbailey · 2021-10-04T16:05:28Z

I've created #2289 to skip the link check for that site.

agones-bot · 2021-10-04T17:44:43Z

Build Succeeded 👏

Build Id: 3bd9a33b-3ef7-4695-9cd4-b9a562ee1fdf

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

image: gcr.io/agones-images/agones-controller:1.18.0-131fa47
image: gcr.io/agones-images/agones-ping:1.18.0-131fa47
Linux C++ SDK (build): agonessdk-1.18.0-131fa47-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.18.0-131fa47.zip

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

https://131fa47-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2271/head:pr_2271 && git checkout pr_2271
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-131fa47

google-oss-robot · 2021-10-04T17:54:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan, roberthbailey

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:

~~OWNERS~~ [roberthbailey]

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

google-oss-robot requested a review from roberthbailey September 19, 2021 23:46

google-cla bot added the cla: yes label Sep 19, 2021

google-oss-robot added the size/M label Sep 19, 2021

Jeffwan mentioned this pull request Sep 19, 2021

Move RollingUpdateOnReady to Stable #2239

Closed

Jeffwan force-pushed the move_feature_stable branch from 0f8c9f7 to 7dddf61 Compare September 20, 2021 19:06

roberthbailey reviewed Sep 21, 2021

View reviewed changes

Jeffwan force-pushed the move_feature_stable branch from 7dddf61 to 1369291 Compare September 28, 2021 06:57

Jeffwan added 3 commits October 3, 2021 17:01

Move RollingUpdateOnReady to stable

45ff198

Address review feedbacks

7fa36a9

Fix test failures

c5c0aed

Jeffwan force-pushed the move_feature_stable branch from 1369291 to c5c0aed Compare October 4, 2021 00:04

google-oss-robot added size/L and removed size/M labels Oct 4, 2021

Merge branch 'main' into move_feature_stable

131fa47

roberthbailey approved these changes Oct 4, 2021

View reviewed changes

google-oss-robot assigned roberthbailey Oct 4, 2021

google-oss-robot added the lgtm label Oct 4, 2021

roberthbailey merged commit a748397 into googleforgames:main Oct 4, 2021

google-oss-robot added the approved label Oct 4, 2021

Jeffwan deleted the move_feature_stable branch October 4, 2021 17:58

roberthbailey added this to the 1.18.0 milestone Oct 4, 2021

roberthbailey added the kind/feature New features for Agones label Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move RollingUpdateOnReady to stable #2271

Move RollingUpdateOnReady to stable #2271

Jeffwan commented Sep 19, 2021

agones-bot commented Sep 19, 2021

agones-bot commented Sep 20, 2021

roberthbailey Sep 21, 2021

Jeffwan Sep 22, 2021

Jeffwan Sep 28, 2021

markmandel Sep 29, 2021

roberthbailey Oct 1, 2021

markmandel Oct 1, 2021

roberthbailey Oct 1, 2021

Jeffwan Oct 2, 2021

Jeffwan Oct 4, 2021

roberthbailey commented Sep 25, 2021

Jeffwan commented Sep 27, 2021

agones-bot commented Sep 28, 2021

roberthbailey commented Sep 29, 2021

agones-bot commented Sep 29, 2021

roberthbailey commented Sep 29, 2021

agones-bot commented Oct 4, 2021

Jeffwan commented Oct 4, 2021 •

edited

Loading

roberthbailey commented Oct 4, 2021

agones-bot commented Oct 4, 2021

roberthbailey commented Oct 4, 2021

roberthbailey commented Oct 4, 2021

agones-bot commented Oct 4, 2021

google-oss-robot commented Oct 4, 2021

		@@ -1118,11 +1109,6 @@ func TestFeatureRollingUpdateOnReady(t *testing.T) {
		func TestControllerRollingUpdateDeployment(t *testing.T) {

	func (c Controller) rollingUpdateRest(ctx context.Context, fleet agonesv1.Fleet, active agonesv1.GameServerSet, rest []agonesv1.GameServerSet) error {
	if runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) {
	return c.rollingUpdateRestFixedOnReady(ctx, fleet, active, rest)
	}
	return c.rollingUpdateRestBeforeFixOnReady(ctx, fleet, rest)
	}

Move RollingUpdateOnReady to stable #2271

Move RollingUpdateOnReady to stable #2271

Conversation

Jeffwan commented Sep 19, 2021

agones-bot commented Sep 19, 2021

agones-bot commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberthbailey commented Sep 25, 2021

Jeffwan commented Sep 27, 2021

agones-bot commented Sep 28, 2021

roberthbailey commented Sep 29, 2021

agones-bot commented Sep 29, 2021

roberthbailey commented Sep 29, 2021

agones-bot commented Oct 4, 2021

Jeffwan commented Oct 4, 2021 • edited Loading

roberthbailey commented Oct 4, 2021

agones-bot commented Oct 4, 2021

roberthbailey commented Oct 4, 2021

roberthbailey commented Oct 4, 2021

agones-bot commented Oct 4, 2021

google-oss-robot commented Oct 4, 2021

Jeffwan commented Oct 4, 2021 •

edited

Loading