fix: #3607 Metrics data loss in K8S controller #3692

alvin-7 · 2024-03-10T10:57:03Z

This PR addresses two issues.

The first issue occurs when a Fleet is being deleted, which triggers two events: an Update (where the DeletionTimestamp is not nil) and a Delete. However, when the Update event is triggered, because the DeletionTimestamp is not nil, it eventually leads to resyncFleets. Inside resyncFleets, the Update function is called for each Fleet, resulting in a deadlock. The consequence of this is that metrics will not be recorded, as the lock cannot be acquired.

The second issue arises after a Fleet is deleted, and only then does the deletion of GameServers commence. When a GameServer's status is modified, it attempts to write the Fleet's status, as recorded in memory, into Metrics, which includes Fleets that have already been deleted. To address this issue, I made modifications so that only the metrics corresponding to the GameServer's current Fleet are recorded during this process.

Fixes #3607

agones-bot · 2024-03-10T11:10:14Z

Build Failed 😱

Build Id: 299f196e-0f1d-4108-a7c8-07a029fcb03b

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

markmandel · 2024-03-11T04:56:33Z

Thanks for taking a pass at this! Looks like some unit tests are failing. Let us know if you would like to some help debugging them.

As part of the PR description and commit, can you provide a description of what your fix does and why its required?

It would definitely help with review, as otherwise we have to reverse engineer the fix from the bug 😄

alvin-7 · 2024-03-11T05:44:52Z

Thanks for taking a pass at this! Looks like some unit tests are failing. Let us know if you would like to some help debugging them.

As part of the PR description and commit, can you provide a description of what your fix does and why its required?

It would definitely help with review, as otherwise we have to reverse engineer the fix from the bug 😄

This PR addresses two issues.

The first issue occurs when a Fleet is being deleted, which triggers two events: an Update (where the DeletionTimestamp is not nil) and a Delete. However, when the Update event is triggered, because the DeletionTimestamp is not nil, it eventually leads to resyncFleets. Inside resyncFleets, the Update function is called for each Fleet, resulting in a deadlock. The consequence of this is that metrics will not be recorded, as the lock cannot be acquired.

The second issue arises after a Fleet is deleted, and only then does the deletion of GameServers commence. When a GameServer's status is modified, it attempts to write the Fleet's status, as recorded in memory, into Metrics, which includes Fleets that have already been deleted. To address this issue, I made modifications so that only the metrics corresponding to the GameServer's current Fleet are recorded during this process.

alvin-7 · 2024-03-11T05:59:43Z

sequenceDiagram
    actor e as event
    participant c as controller
    participant l as locker

    e->>c: on fleet be deleted
    c->>+c: recordFleetChanges
    c->>+c: recordFleetDeletion
    c->>+c: resyncFleets
    c->>+l: lock
    alt In fleet.list, the deleted Fleet no longer exists.
        c ->>- e: end function
    else the deleted Fleet exists, Dead Lock
        c->>+c: recordFleetChanges
        c->>+c: recordFleetDeletion
        c->>+c: resyncFleets
        c->>+l: lock [dead lock]
    end

markmandel · 2024-03-11T06:02:42Z

TIL you can create mermaid diagrams in GitHub issues!

markmandel · 2024-03-11T06:03:32Z

Is it also possible to write a unit test, to ensure this issue doesn't show up again?

alvin-7 · 2024-03-11T08:53:02Z

Is it also possible to write a unit test, to ensure this issue doesn't show up again?

I attempted to write or modify test cases to cover this scenario, but the challenge is that it would require special modifications to the existing fakeFleetLister.List in order to reproduce this issue, otherwise it cannot be recreated.

markmandel · 2024-03-11T09:24:03Z

I attempted to write or modify test cases to cover this scenario, but the challenge is that it would require special modifications to the existing fakeFleetLister.List in order to reproduce this issue, otherwise it cannot be recreated.

What would you need to do to replicate it? I'm sure we could probably work it out.

alvin-7 · 2024-03-11T10:31:18Z

I attempted to write or modify test cases to cover this scenario, but the challenge is that it would require special modifications to the existing fakeFleetLister.List in order to reproduce this issue, otherwise it cannot be recreated.

What would you need to do to replicate it? I'm sure we could probably work it out.

Add a Standard Fleet: First, create or add a standard Fleet instance within the system.
Modify the Deletion Timestamp: Update the metadata.DeletionTimestamp attribute of the Fleet instance to the current time. This simulates the Fleet instance being in a state of deletion.
Adjust the fakeFleetLister.List Return Value: Ensure that the fakeFleetLister.List method returns this Fleet instance, which has been marked for deletion (by modifying the deletion timestamp). This step is crucial for the correct execution of the function.

I trust these steps are clear and provide a solid framework for us to replicate the Fleet deletion scenario accurately. Should you have any questions, require further clarifications, or if there's anything more I can assist you with, please do not hesitate to reach out.

agones-bot · 2024-03-11T10:52:50Z

Build Failed 😱

Build Id: 020786da-320b-4a37-9d9a-69a880fce667

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

agones-bot · 2024-03-14T10:40:06Z

Build Succeeded 👏

Build Id: 169752ed-4559-472d-8fd2-f2ed4a0300b9

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):

https://8073d4e-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3692/head:pr_3692 && git checkout pr_3692
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.40.0-dev-8073d4e-amd64

markmandel · 2024-03-15T01:33:34Z

Add a Standard Fleet: First, create or add a standard Fleet instance within the system.

Modify the Deletion Timestamp: Update the metadata.DeletionTimestamp attribute of the Fleet instance to the current time. This simulates the Fleet instance being in a state of deletion.

Adjust the fakeFleetLister.List Return Value: Ensure that the fakeFleetLister.List method returns this Fleet instance, which has been marked for deletion (by modifying the deletion timestamp). This step is crucial for the correct execution of the function.

You could absolutely do this with a new Unit Test for sure. The Fake clients for Kubernetes don't have any smarts about them, and basically only do what they are told.

Option No. 1
You could do all the manipulation via fake Watch events, which are propagated to List operations, such as what we do here:

agones/pkg/metrics/controller_test.go

Line 118 in 130c99a

c.gsWatch.Modify(gs1)

Option No. 2
Provide the exact list you want to be returned at any given point and time through AddReactor:

agones/pkg/gameserversets/controller_test.go

Lines 573 to 575 in 130c99a

    
           m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { 
        
           	return true, &agonesv1.GameServerList{Items: list}, nil 
        
           })

And that can provide you the state you need.

It still will require a new test I expect, but it would be good to test things to ensure that this doesn't regress, as right now there is no guarantee -- you found a tricky bug 😄 , so want to make sure we don't have to call you back again!

alvin-7 · 2024-03-15T06:56:51Z

Add a Standard Fleet: First, create or add a standard Fleet instance within the system.

Modify the Deletion Timestamp: Update the metadata.DeletionTimestamp attribute of the Fleet instance to the current time. This simulates the Fleet instance being in a state of deletion.

Adjust the fakeFleetLister.List Return Value: Ensure that the fakeFleetLister.List method returns this Fleet instance, which has been marked for deletion (by modifying the deletion timestamp). This step is crucial for the correct execution of the function.

You could absolutely do this with a new Unit Test for sure. The Fake clients for Kubernetes don't have any smarts about them, and basically only do what they are told.

Option No. 1 You could do all the manipulation via fake Watch events, which are propagated to List operations, such as what we do here:

agones/pkg/metrics/controller_test.go

Line 118 in 130c99a

c.gsWatch.Modify(gs1)

Option No. 2 Provide the exact list you want to be returned at any given point and time through AddReactor:

agones/pkg/gameserversets/controller_test.go

Lines 573 to 575 in 130c99a

m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) {

return true, &agonesv1.GameServerList{Items: list}, nil

})

And that can provide you the state you need.

It still will require a new test I expect, but it would be good to test things to ensure that this doesn't regress, as right now there is no guarantee -- you found a tricky bug 😄 , so want to make sure we don't have to call you back again!

Thank you very much for your guidance. I have completed writing the test cases and have submitted them. Could you please review them?

agones-bot · 2024-03-15T07:07:36Z

Build Failed 😱

Build Id: b70a2e76-7a62-4197-9e0f-8cd15259465a

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

markmandel · 2024-03-15T09:37:04Z

Heading to GDC, but will check when I can 👍🏻

agones-bot · 2024-03-15T11:59:47Z

Build Succeeded 👏

Build Id: db870010-ddf6-4a05-b4e7-9b750f60e3b8

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):

https://e62a87e-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3692/head:pr_3692 && git checkout pr_3692
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.40.0-dev-e62a87e-amd64

agones-bot · 2024-03-19T19:53:31Z

Build Failed 😱

Build Id: 5a2c484c-1003-49ec-8bac-4db0ace257c4

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

agones-bot · 2024-03-19T23:32:54Z

Build Succeeded 👏

Build Id: 3126e07c-a5fd-4579-86e1-867d1cda0c5b

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):

https://59a3575-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3692/head:pr_3692 && git checkout pr_3692
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.40.0-dev-59a3575-amd64

markmandel · 2024-03-27T22:39:53Z

Just letting you know I haven't forgotten about you! I'm still recovering from GDC but this is at the top of my queue.

alvin-7 · 2024-03-28T03:42:28Z

Just letting you know I haven't forgotten about you! I'm still recovering from GDC but this is at the top of my queue.

Thank you so much for the update! I hope you had a great time at GDC and are recovering well. Please take all the time you need, and let me know if there's anything I can do to assist with the review process. Looking forward to your feedback when you're ready.

markmandel

Just a couple of small tweaks, and this is good to merge 👍🏻 nice job!

pkg/metrics/controller_test.go

Co-authored-by: Mark Mandel <markmandel@google.com>

agones-bot · 2024-03-29T04:24:12Z

Build Succeeded 👏

Build Id: 45c4b7ca-621a-4b25-a716-348c5f142a56

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):

https://9b98dda-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3692/head:pr_3692 && git checkout pr_3692
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.40.0-dev-9b98dda-amd64

alvin-7 · 2024-03-29T05:52:06Z

Just a couple of small tweaks, and this is good to merge 👍🏻 nice job!

Thank you very much for your feedback and suggestions! I have made the requested adjustments and ensured that all test cases have passed.

markmandel · 2024-04-01T22:22:34Z

Thanks! Approved and ready to go.

agones-bot · 2024-04-01T22:41:35Z

Build Failed 😱

Build Id: 5f1629d4-3913-4d02-ab01-68a46b4d338a

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

agones-bot · 2024-04-01T23:58:11Z

Build Succeeded 👏

Build Id: a8faf0fb-2034-493b-9f2d-ac7d79cd22e0

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):

https://f7a7ea7-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3692/head:pr_3692 && git checkout pr_3692
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.40.0-dev-f7a7ea7-amd64

alvin-7 · 2024-04-02T09:35:02Z

Thanks! Approved and ready to go.

Received，and issue #3486 has already been fixed by this PR.

fix: #3607 Metrics data loss in K8S controller

11cf452

github-actions bot added kind/other size/S labels Mar 10, 2024

Merge branch 'main' into main

be0d525

roberthbailey assigned markmandel Mar 11, 2024

roberthbailey added feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) and removed feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) labels Mar 11, 2024

alvin-7 added 2 commits March 14, 2024 17:38

fix: #3607 Metrics data loss in K8S controller

cb9c366

Merge branch 'googleforgames:main' into main

8073d4e

add unit test for #3607

1af6a01

github-actions bot added the size/M label Mar 15, 2024

add unit test for #3607

e62a87e

Merge branch 'main' into main

908d1ed

Merge branch 'main' into main

59a3575

markmandel reviewed Mar 29, 2024

View reviewed changes

pkg/metrics/controller_test.go Outdated Show resolved Hide resolved

pkg/metrics/controller_test.go Outdated Show resolved Hide resolved

pkg/metrics/controller_test.go Outdated Show resolved Hide resolved

pkg/metrics/controller_test.go Outdated Show resolved Hide resolved

alvin-7 and others added 5 commits March 29, 2024 10:39

Update pkg/metrics/controller_test.go

358df77

Co-authored-by: Mark Mandel <markmandel@google.com>

Update pkg/metrics/controller_test.go

8ce2f7e

Co-authored-by: Mark Mandel <markmandel@google.com>

Update pkg/metrics/controller_test.go

3ab4ca9

Co-authored-by: Mark Mandel <markmandel@google.com>

Update pkg/metrics/controller_test.go

5fe35bf

Co-authored-by: Mark Mandel <markmandel@google.com>

Merge branch 'main' into main

9b98dda

markmandel approved these changes Apr 1, 2024

View reviewed changes

markmandel enabled auto-merge (squash) April 1, 2024 22:22

Merge branch 'main' into main

f7a7ea7

markmandel merged commit 6709f35 into googleforgames:main Apr 1, 2024
4 checks passed

alvin-7 mentioned this pull request Apr 2, 2024

Agones Controller has OOMKilled in version 1.35.0 #3486

Closed

Kalaiselvi84 added kind/bug These are bugs. and removed kind/other labels Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #3607 Metrics data loss in K8S controller #3692

fix: #3607 Metrics data loss in K8S controller #3692

alvin-7 commented Mar 10, 2024 •

edited by zmerlynn

Loading

agones-bot commented Mar 10, 2024

markmandel commented Mar 11, 2024

alvin-7 commented Mar 11, 2024

alvin-7 commented Mar 11, 2024

markmandel commented Mar 11, 2024

markmandel commented Mar 11, 2024

alvin-7 commented Mar 11, 2024

markmandel commented Mar 11, 2024

alvin-7 commented Mar 11, 2024 •

edited

Loading

agones-bot commented Mar 11, 2024

agones-bot commented Mar 14, 2024

markmandel commented Mar 15, 2024

alvin-7 commented Mar 15, 2024

agones-bot commented Mar 15, 2024

markmandel commented Mar 15, 2024

agones-bot commented Mar 15, 2024

agones-bot commented Mar 19, 2024

agones-bot commented Mar 19, 2024

markmandel commented Mar 27, 2024

alvin-7 commented Mar 28, 2024

markmandel left a comment

agones-bot commented Mar 29, 2024

alvin-7 commented Mar 29, 2024

markmandel commented Apr 1, 2024

agones-bot commented Apr 1, 2024

agones-bot commented Apr 1, 2024

alvin-7 commented Apr 2, 2024

fix: #3607 Metrics data loss in K8S controller #3692

fix: #3607 Metrics data loss in K8S controller #3692

Conversation

alvin-7 commented Mar 10, 2024 • edited by zmerlynn Loading

agones-bot commented Mar 10, 2024

markmandel commented Mar 11, 2024

alvin-7 commented Mar 11, 2024

alvin-7 commented Mar 11, 2024

markmandel commented Mar 11, 2024

markmandel commented Mar 11, 2024

alvin-7 commented Mar 11, 2024

markmandel commented Mar 11, 2024

alvin-7 commented Mar 11, 2024 • edited Loading

agones-bot commented Mar 11, 2024

agones-bot commented Mar 14, 2024

markmandel commented Mar 15, 2024

alvin-7 commented Mar 15, 2024

agones-bot commented Mar 15, 2024

markmandel commented Mar 15, 2024

agones-bot commented Mar 15, 2024

agones-bot commented Mar 19, 2024

agones-bot commented Mar 19, 2024

markmandel commented Mar 27, 2024

alvin-7 commented Mar 28, 2024

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented Mar 29, 2024

alvin-7 commented Mar 29, 2024

markmandel commented Apr 1, 2024

agones-bot commented Apr 1, 2024

agones-bot commented Apr 1, 2024

alvin-7 commented Apr 2, 2024

alvin-7 commented Mar 10, 2024 •

edited by zmerlynn

Loading

alvin-7 commented Mar 11, 2024 •

edited

Loading