-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
integration: init release upgrade testing #9262
Conversation
Skipping CI for Draft Pull Request. |
171d0e8
to
6c827c4
Compare
6c827c4
to
72cc895
Compare
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 is pretty neat! A few comments, but I'm generally happy with it.
require.NoError(t, previousProc.kill(syscall.SIGTERM)) | ||
require.NoError(t, previousProc.wait()) |
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.
Maybe a timeout here? Or (see other comment about pdeathsig) have the parent goroutine in a locked OS thread exit, causing SIGKILL to be sent?
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 changed the wait()
to wait(timeout time.Duration)
. And also add new goroutine to wait for containerd's exit event with locked OS thread. PTAL
|
||
// downloadPreviousReleaseBinary downloads the latest version of previous release | ||
// into the target dir. | ||
func downloadPreviousReleaseBinary(t *testing.T, targetDir string) { |
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.
We may eventually want a way to supply a local containerd archive instead of downloading one, especially if we want this test to be usable for distros who build their own containerd. But I don't think we need to do that now.
The TestUpgrade downloads the latest of previous release's binary and use them to setup pods and then use current release to recover the existing pods. Signed-off-by: Wei Fu <fuweid89@gmail.com>
72cc895
to
2fab240
Compare
@samuelkarp Updated with your suggestion. PTAL. Thanks! The test result is here. |
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.
LGTM!
// upgradeVerifyCaseFunc is used to verify the behavior after upgrade. | ||
type upgradeVerifyCaseFunc func(t *testing.T, criRuntimeService cri.RuntimeService) | ||
|
||
// TODO: Support Windows |
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.
nit: link to #3757 for these TODOs?
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.
oops. Just saw this comment. I will add the link in the follow-up. Thanks!
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.
LGTM
RIP legacy CRI in #9228.
The pull request is to init release upgrade testing cases to check sandbox-like CRI implementation is compatible with v1.x release. It's part of #3757