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

use new portmap plugin #1720

Closed
wants to merge 1 commit into from
Closed

Conversation

aojea
Copy link
Contributor

@aojea aojea commented Jul 9, 2020

the portmap plugin is buggy on deletion, it just blindly tries to
delete everything once it gets a DEL, no matter the IP family.

Added to the problem caused because it is being executed with the
-w option without any timeout (see go-iptables), it may hold the
lock or cause contention, making all services in the cluster to
fail.

Use the new portmap version
containernetworking/plugins#509

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aojea
To complete the pull request process, please assign bentheelder
You can assign the PR to them by writing /assign @bentheelder in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from amwat and munnerz July 9, 2020 15:55
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 9, 2020
@aojea
Copy link
Contributor Author

aojea commented Jul 9, 2020

/priority critical-urgent
/kind bug
/assign @BenTheElder
I'll try to fix the portmap plugin but is going to take me some time,
in the meantime we can run without it, my grep FU does not show any e2e exercising hosport

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. kind/bug Categorizes issue or PR as related to a bug. labels Jul 9, 2020
@aojea
Copy link
Contributor Author

aojea commented Jul 9, 2020

/hold
I'll work in the portmap plugin to solve the problem, just use this in case is urgent

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020
@aojea
Copy link
Contributor Author

aojea commented Jul 10, 2020

just validating hypothesis

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 10, 2020
@aojea
Copy link
Contributor Author

aojea commented Jul 10, 2020

this didn't run is and is the more prone to show the errors
/test pull-kind-conformance-parallel-ipv6

nothing in the finished jobs so far 🤞

@aojea
Copy link
Contributor Author

aojea commented Jul 10, 2020

promising, no trace (I'm almost crying 😄 one year chasing this and we are so close)
/test all

@aojea
Copy link
Contributor Author

aojea commented Jul 10, 2020

ok, we got it, no deadlock errors again
/test all

@aojea
Copy link
Contributor Author

aojea commented Jul 10, 2020

ok, after 3 rounds and 6 different jobs not locking problems, I think that this time we can confirm the portmap plugin was the one causing the iptables locking problem
/cc @danwinship
Will work on fixing the portmap plugin

@k8s-ci-robot k8s-ci-robot requested a review from danwinship July 10, 2020 10:26
@danwinship
Copy link
Contributor

This will break the kube e2e tests that use kind won't it?

@aojea
Copy link
Contributor Author

aojea commented Jul 10, 2020

This will break the kube e2e tests that use kind won't it?

there are no hostPort e2e tests, at least I didn't find them 🤷

@aojea
Copy link
Contributor Author

aojea commented Jul 13, 2020

/test all

@aojea aojea changed the title disable portmap plugin use new portmap plugin Jul 13, 2020
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2020
clone containernetworking/plugins#509
./build

docker build

FROM kindest/base:v20200707-e647846b
COPY bin/portmap /opt/cni/bin/portmap
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2020
@aojea
Copy link
Contributor Author

aojea commented Jul 13, 2020

here we go, with the fix here containernetworking/plugins#509

@aojea
Copy link
Contributor Author

aojea commented Jul 13, 2020

pull-kind-e2e-kubernetes and pull-kind-e2e-kubernetes-1-18 and ipv6 clean

@aojea
Copy link
Contributor Author

aojea commented Jul 13, 2020

/test all

1 similar comment
@BenTheElder
Copy link
Member

/test all

@k8s-ci-robot
Copy link
Contributor

@aojea: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kind-conformance-parallel-1-17 6731ef4 link /test pull-kind-conformance-parallel-1-17
pull-kind-conformance-parallel-1-16 6731ef4 link /test pull-kind-conformance-parallel-1-16
pull-kind-e2e-kubernetes-1-18 6731ef4 link /test pull-kind-e2e-kubernetes-1-18

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@aojea
Copy link
Contributor Author

aojea commented Jul 13, 2020

@BenTheElder this is ready to go, what will be the best way to include the plugin with the fix meanwhile is not merged in CNI?

@BenTheElder
Copy link
Member

maybe we can build the CNI plugins in a multi-step base-image dockerfile from your fork? With a TODO to switch to upstream once merged, and to switch to just consuming the release binaries once released.

@aojea
Copy link
Contributor Author

aojea commented Jul 14, 2020

/close
by #1729

@k8s-ci-robot
Copy link
Contributor

@aojea: Closed this PR.

In response to this:

/close
by #1729

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants