Skip to content

integration: Adds Windows equivalent for TestSandboxRemoveWithoutIPLeakage#6180

Merged
kzys merged 1 commit intocontainerd:mainfrom
claudiubelu:integration/windows-ip-leak
Apr 25, 2022
Merged

integration: Adds Windows equivalent for TestSandboxRemoveWithoutIPLeakage#6180
kzys merged 1 commit intocontainerd:mainfrom
claudiubelu:integration/windows-ip-leak

Conversation

@claudiubelu
Copy link
Copy Markdown
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 [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

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.

Details

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
Copy Markdown

theopenlab-ci Bot commented Oct 29, 2021

Build succeeded.

@claudiubelu claudiubelu force-pushed the integration/windows-ip-leak branch from 937210b to a50ec48 Compare November 2, 2021 08:41
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Nov 2, 2021

Build succeeded.

@cpuguy83
Copy link
Copy Markdown
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
Copy Markdown
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.

@dmcgowan
Copy link
Copy Markdown
Member

Can you please rebase to test with the latest test changes

…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 <[email protected]>
@claudiubelu claudiubelu force-pushed the integration/windows-ip-leak branch from a50ec48 to 894af07 Compare January 3, 2022 08:27
@claudiubelu
Copy link
Copy Markdown
Contributor Author

Can you please rebase to test with the latest test changes

Done.

/cc @gabriel-samfira

@k8s-ci-robot
Copy link
Copy Markdown

@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.

Details

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
Copy Markdown

theopenlab-ci Bot commented Jan 3, 2022

Build succeeded.

@kzys
Copy link
Copy Markdown
Member

kzys commented Jan 31, 2022

/ok-to-test

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants