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.

Sign up for GitHub

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

Run SCTP HostPort e2e test only if the container runtime is Docker #92079

Closed
wants to merge 1 commit into from
Closed

Run SCTP HostPort e2e test only if the container runtime is Docker #92079

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions test/e2e/network/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3961,6 +3961,7 @@ var _ = SIGDescribe("SCTP [Feature:SCTP] [LinuxOnly]", func() {
})

ginkgo.It("should create a Pod with SCTP HostPort", func() {
e2eskipper.RunIfContainerRuntimeIs("docker")
Copy link
Member

Choose a reason for hiding this comment

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

I’m just wondering why we can’t handle that via the ginkgo test scope.

Copy link
Member

Choose a reason for hiding this comment

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

#92079 (comment)
we should avoid these kind of skips in the k/k framework, each CRI can implement their own tests in their infra/repo IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we hope this is going to be fixed in the runtimes soon, maybe we should wait and only do the skip if the test fails? Then as soon as the CI environment gets an updated containerd, the test will start passing (and then eventually we can remove the skip).

Copy link
Contributor

Choose a reason for hiding this comment

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

(by "wait", I mean, instead of skipping here, do it before the framework.Failf in the if !found { block below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danwinship We should be pretty careful with that, I think. We must follow up the runtime version closely.
iptables rules can be missing due to a bug we caught. So, if the runtime is already a supporting one, we must immediately turn the skip out. Otherwise we may miss bugs.

ginkgo.By("checking whether kubenet is used")
Copy link
Member

Choose a reason for hiding this comment

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

can someone explain why this is checking for kubenet?

Copy link
Contributor

Choose a reason for hiding this comment

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

The obvious way to test "SCTP HostPorts work" would be to create a pod with an SCTP HostPort, and then try to send traffic to it. But we can't do that because many hosts (including all GCE-based nodes) won't have the SCTP kernel modules so trying to actually send SCTP traffic would fail. So instead we check that it's working indirectly, by seeing that the expected iptables rules get created. But that only works if we're sure we know what iptables rules will be created, so we have to whitelist where the test can run

Copy link
Member

Choose a reason for hiding this comment

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

in the linked issues there's discussion of not loading the module so as to allow usage of userspace SCTP -- can we test that instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, yes, though we'd have to maintain some image with appropriate useful test binaries built against some userspace SCTP library, which would be a lot of work for a relatively marginal feature.

Note that there's a second set of SCTP tests, not yet complete, labeled [Feature:SCTPConnectivity] that actually check that SCTP connections work. But those tests can only be run in clusters that have the SCTP kernel module available.

Copy link
Member

Choose a reason for hiding this comment

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

ACK -- at some point i'd like to follow up about what it would take to get the module loaded.
A lot of the CI is on GCE / Ubuntu nodes these days, it may be more viable than GCE / cos.

KIND will have the containerd patch today, we'd just need the module and a job that ran this test tag, the underyling VMs are GKE/Ubuntu but e.g. we load the IPv6 modules.

node, err := e2enode.GetRandomReadySchedulableNode(cs)
framework.ExpectNoError(err)
Expand Down