Skip to content
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: Adds Windows equivalent for TestSandboxRemoveWithoutIPLeakage #6180

Merged
merged 1 commit into from Apr 25, 2022

Conversation

claudiubelu
Copy link
Contributor

Adds an equivalent TestSandboxRemoveWithoutIPLeakage for Windows, in which we assert that the IPs are not leaked when a Pod's HNS namespace dissapears and the Pod is deleted afterwards.

The test currently fails, as it seems that the Pod's IP remains "in use" after the Pod is deleted.

Sample output: https://paste.ubuntu.com/p/t47hmr2RyB/

Signed-off-by: Claudiu Belu cbelu@cloudbasesolutions.com

@k8s-ci-robot
Copy link

Hi @claudiubelu. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 29, 2021

Build succeeded.

@dmcgowan dmcgowan added this to Needs Contributor Update in Code Review Oct 30, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 2, 2021

Build succeeded.

@cpuguy83
Copy link
Member

cpuguy83 commented Nov 2, 2021

@claudiubelu What's the status of this? I see you expect the test to fail, what should we be doing with this PR? Are we waiting for a fix so the test passes?

@claudiubelu
Copy link
Contributor Author

@claudiubelu What's the status of this? I see you expect the test to fail, what should we be doing with this PR? Are we waiting for a fix so the test passes?

Sounds like an issue to me. We should find a fix for it, and merge this PR afterwards to test against it, so we're sure we don't regress. This currently only checks azure-vnet-ipam, but other CNIs should be checked as well.

@kzys kzys moved this from Needs Contributor Update to Ready For Review in Code Review Nov 30, 2021
@dmcgowan
Copy link
Member

Can you please rebase to test with the latest test changes

@dmcgowan dmcgowan moved this from Ready For Review to Needs Contributor Update in Code Review Dec 14, 2021
…akage

Adds an equivalent TestSandboxRemoveWithoutIPLeakage for Windows, in which
we assert that the IPs are not leaked when a Pod's HNS namespace dissapears
and the Pod is deleted afterwards.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
@claudiubelu
Copy link
Contributor Author

Can you please rebase to test with the latest test changes

Done.

/cc @gabriel-samfira

@k8s-ci-robot
Copy link

@claudiubelu: GitHub didn't allow me to request PR reviews from the following users: gabriel-samfira.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Can you please rebase to test with the latest test changes

Done.

/cc @gabriel-samfira

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 3, 2022

Build succeeded.

@kzys
Copy link
Member

kzys commented Jan 31, 2022

/ok-to-test

Copy link
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This looks green enough to get in.
@jterry75 @kevpar Possibly one more LGTM as this is windows related?

Comment on lines +49 to +51
if !strings.Contains(string(cniConfig), "azure-vnet-ipam") {
t.Skip("azure-vnet ipam is not in use")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test requires the azure-vnet-ipam CNI plugin? Can this test run outside Azure?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could run on any infra, but we need a CNI and these tests are running on azure right now I think.

require.True(t, checkIP(ip))

t.Logf("Kill sandbox container")
require.NoError(t, KillPid(int(info.Pid)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work for process containers but I guess thats fine given the support statement at the moment

Copy link
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@kzys kzys merged commit 2ca54ee into containerd:main Apr 25, 2022
Code Review automation moved this from Needs Contributor Update to Done Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants