-
Notifications
You must be signed in to change notification settings - Fork 455
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 tests for ListContainerStats #857
add tests for ListContainerStats #857
Conversation
Welcome @minbaev! |
/cc @mikebrow |
@minbaev: GitHub didn't allow me to request PR reviews from the following users: mikebrow. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 with nits addressed after we have the fix in containerd merged into main, and cherry picked to containerd 1.5.9 should only have to "pause" for this test case for a short period. If it takes to long for the 1.5.9 release to cycle up over this holiday feel free to add a skip.
could rebase again (it seems rebase is required to get new changes for github actions)? |
1abdac9
to
8aec4be
Compare
sure, done |
@minbaev the tests seems to fail, can we fix them? |
hmm, I don't think the tests are wrong, they pass when i run them locally against a local containerd instance with the change. we cut a 1.5.9 containerd version which should include the change can it be possible that the tests fail due some CI issue? Here is the output when I run the tests locally
|
/retest |
hey sasha .. chatted with alexander off line.. the fix for this test case still needs the cherry pick over on containerd.. (then move to containerd 1.5.10 when it's out (wip)) or we can run this test against containerd main.. or wait for containerd 1.6.0 which is imminent and has all kinds of k/k 1.23 goodness :-) |
126c9db
to
151da96
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.
LGTM
dependency error for the windows build is not related to this PR
we can move to containerd 1.5.10 or 1.6 when they become available, this commit off the top of 1.5 is fair to use for now
@saschagrunert could you please look at the tests failing. not sure the failure is related to the changes made by this pr https://github.com/kubernetes-sigs/cri-tools/runs/4900343760?check_suite_focus=true#step:12:112 on windows
|
Yes, pause is not available for windows right now. We should exclude those tests for that platform or modify them. |
@minbaev may I ask you to rebase to get the latest CI changes in? |
dffdac5
to
f6e7658
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.
/lgtm
The tests do not seem to work on windows. @minbaev can you take a look? |
@saschagrunert Hi, what would be the mechanism to exclude the failing windows test due to "k8s.gcr.io/pause:3.1" pulling error? should it be a part of this pr? |
Hm, I'm wondering why it works on the master branch compared to this PR 🤔 |
"critest dockershim / windows-2019 (pull_request) Failing after 7m — windows-2019" .. Looks to me like it's on the wrong pause tag.. 3.1 sounds .. really old probably needs 3.2 - 3.6? dockershim tests were removed... oh.. looks like the rebase didn't take .. my rebase workflowgit checkout master |
Signed-off-by: Alexander Minbaev <[email protected]>
f6e7658
to
4f533d5
Compare
@mikebrow @saschagrunert thanks for looking into that |
check the list of test buckets.. there is no "critest dockershim" test in master, but you have it in your PR's test.. thus I think your PR is not based on the current master...? or I'm reading something wrong... |
@mikebrow there is this config to set the test |
also this pr to remove dockershim, but it isn't merged |
should i just skip the failing tests in dockershim.yml file? |
nod.. just add your new 3 buckets to the skip.. since most of them are already being skipped or you could figure out how to move up the pause to 3.something that works.. Guessing 3.1 doesn't have the windows pause in it.. needs to be at least 3.2 would be my guess. |
you know it says dockershim but it's really Cloning into 'D:\a\cri-tools\cri-tools/src/github.com/Mirantis/cri-dockerd' let's see... ah here it is.. and it's already being fixed Mirantis/cri-dockerd#46 |
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
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, mikebrow, minbaev, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Alexander Minbaev [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
PR adds tests for
ListContainerStats
RPCAdds tests covering the RPC behavior after this change:
containerd/containerd#6373
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?
None