-
Notifications
You must be signed in to change notification settings - Fork 825
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 e2e test for leader election #3076
Conversation
Build Failed 😱 Build Id: 25733a05-2a80-4946-b0b0-cc4b63467068 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
test/e2e/controller/crash_test.go
Outdated
require.NoError(t, err, "Could not delete controller pod") | ||
} | ||
|
||
err = deleteAgonesControllerPod(ctx, willBeLeader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you are:
- killing all replicas except the first
- then killing the first
- the waiting for everything to stabilize
Why? The way you've done it here, willBeLeader
is pretty much irrelevant - you could have just killed all the pods.
The point of deleing the other pods is to shepherd the leader to willBeLeader
. But you can't just kill it after doing that, otherwise you might as well have just restarted the whole replicaset.
What we talked about offoine was close to this, but not exactly:
- delete all but one as you do in line 82-86
- wait for all running (separately, just add a parameter to waitForAgonesControllerRunning if you want to ensure the count - you can add a wantReplicas parameter and then only check it if it's non-negative or something)
- then delete
willBeLeader
- then proceed to verify the controller till works, without waiting again
the point is that shepherding it into one replica, waiting for the others to come back, then killing that one replica, is a pretty solid guarantee that you're killing the leader. The way you have it here also guarantees you killed the leader but it then waits for everything to come back up - we ideally want to show that right after the leader dies, we can still function. Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh I see I see. I think I have a better understanding now. I will try this.
Build Succeeded 👏 Build Id: 3f6bed01-131c-4cc1-9b70-2d73a9133faa 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:
|
Build Failed 😱 Build Id: 3fadbcbd-18a0-418f-88e1-e725a1ba6137 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 2b6a8bd5-43ad-45bb-9a0d-16a409e7d991 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:
|
Build Succeeded 👏 Build Id: 96227e0e-5e7b-4178-ba39-2c61d580ec6e 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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can take it out of WIP, LGTM!
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiayi, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Failed 😱 Build Id: 893bb6fb-da2d-49ea-bc4d-ebb65e76218f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Seemed to have timed out waiting for condition during test: |
Build Succeeded 👏 Build Id: 3fd23d4d-215f-4ce1-9485-cd541d9fc4b9 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:
|
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: d4a59815-523b-4704-b465-276da360d8c7 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:
|
Build Succeeded 👏 Build Id: 2c4f6376-fd22-4bbb-80b0-299b1fe7cbef 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:
|
Add e2e test for leader election
What type of PR is this?
/kind feature
What this PR does / Why we need it:
This is the e2e test that will test the controller leader election feature
Which issue(s) this PR fixes:
Work on #2797
Special notes for your reviewer: