Flaky: Fix test for TestGameServerUnhealthyAfterDeletingPod #758
Conversation
Change in behaviour on Unhealthy GameServers that are not in Fleets (deleted vs just being marked as Unhealthy), caused this e2e test to be flaky. Added a TODO to review if we are still happy with this, since release is coming up, and having stable tests is most important right now.
markmandel
added
kind/bug
|
/cc @ilkercelikyilmaz just a heads up about this - we might want to review this behaviour at some point. Originally GameServers not in fleet that dropped into But maybe this is fine, and we can leave it as is 🤷♂️ Just wanted to put it on your radar, in case you had thoughts. In theory the gameserverset controller should have taken care of this for the issue you were facing, but for some strange reason it didn't 😢 |
|
Build Succeeded 👏 Build Id: 15a8bfe8-2dd1-48e2-b09a-3919337dd009 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:
|
|
|
||
| if err != nil { | ||
| logrus.WithError(err).Warn("error retrieving gameserver") | ||
| return false, nil |
There was a problem hiding this comment.
Should this be return false, err? The func returns (bool, error) but none of the cases actually return a non-nil error.
There was a problem hiding this comment.
That's a good question - looking at the code, if any error is returned from the function, then PollImmeadiate will fail, and return an error - in this case, I don't think we actually want that to happen, we want it to retry and try again if something weird happens - because it may be a timing issue or self resolve?
|
lgtm overall, just one nit on error handling. |
I was hoping that GameserverSet controller to take care of this but it wasn't. Actually it was taking care of it when the Pod restarted. |
|
Build Failed 😱 Build Id: 91b84eb0-0dec-4c84-860f-245aa2b3a310 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: 457ca927-1e6b-45ed-8b8e-258f7457f353 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: 4982ba19-2ef0-4d52-91b1-e00e98e41a83 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 👏 Build Id: 3809452d-9026-474e-bc18-a9a4eef7edc8 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:
|
Change in behaviour on Unhealthy GameServers that are not in Fleets (deleted vs just being marked as Unhealthy), caused this e2e test to be flaky.
Added a TODO to review if we are still happy with this, since release is coming up, and having stable tests is most important right now.