-
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 more time and logging to extensions test #2996
Conversation
Build Succeeded 👏 Build Id: 7b1f2edd-7bc3-4101-9736-c0f7da24cd1c 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:
|
7c7d050
to
13a1725
Compare
Build Succeeded 👏 Build Id: d2d17ea8-7c98-4724-88ac-5651b27599ca 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: c5ad3f89-1f2e-4edc-8696-51573116735f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 0f3b29f1-0bd5-47b4-bf99-41e42397ff04 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:
|
} | ||
|
||
return false | ||
}, 30*time.Second, 20*time.Millisecond) |
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.
}, 30*time.Second, 20*time.Millisecond) | |
}, 5 * time.Minute, time.Second) |
We don't need to hammer the K8s API and slow down other e2e tests. Once a second should be sufficient.
|
||
assert.NoError(t, waitForAgonesExtensionsRunning(ctx)) | ||
if len(extList.Items) == 2 { |
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.
A check I would add - is the len(2) but also is the list
above different from the list we just got in extList
- that way we definitely know we have a new replacement pod, and it's up and running.
Otherwise we could get 1 pod running 1 terminating.
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.
You could likely just do a comparison of sorted pod names in both lists. A bit of code, but then we know we're in the place we expect to be.
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 this would be easier to integrate into deleteAgonesExtensionsPods
, and here's why: that function is misnamed. It's deleting exactly one pod, and that pod has a name, known to that function as list.Items[1].ObjectMeta.Name
.
It's much easier just to manually use the KubeClient.Delete, then require.Eventually() that the KubeClient.Get on the Pod fails - i.e. manually delete the single pod and just wait on that single pod to be gone.
And rename the function as deleteAgonesExtensionPod
.
I would maybe then change waitForAgonesExtensionsRunning
to actually make sure there are 2 running.
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.
Works for me 👍🏻
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.
Added a comparison to make sure that they are not equal, please take a look.
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.
This looks fine to me - it's more code than what I suggested above but it'll get the job done for sure. We can clean it up later if needed.
Build Failed 😱 Build Id: b6f35e71-db1e-456c-8535-e358b49ab69a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Look like the linter got you, but also looks good to me!
|
Had to remove the unused function. Should be good now, please take a look. |
Build Failed 😱 Build Id: e18568e1-2194-423d-ad1a-c21944efb233 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Linter got you again on the same item 😄
if you run |
Yeah, that was part of the function I removed on the latest push 😅 |
[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 Succeeded 👏 Build Id: 5cf90c26-0994-4ade-b1b0-63ed1d2dc040 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:
|
What type of PR is this?
/kind hotfix
What this PR does / Why we need it:
It attempts to fix the flaky e2e extensions test.
Which issue(s) this PR fixes:
Maybe #2992
Special notes for your reviewer:
I ran my test locally around 20+ times and it all passed. I'm not 100% sure what the issue is with the