hostport manager clean up host ports #4542
Conversation
The host port manager, when removing hostports, exists fast if there are no iptables rules to delete. However, it was missing to call the method that remove the hostports bound, so there is a possibility that we leak sockets. Signed-off-by: Antonio Ojea <aojea@redhat.com>
openshift-ci-robot
added
release-note-none
|
Hi @aojea. Thanks for your PR. I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
openshift-ci-robot
added
the
needs-ok-to-test
|
/assign @danwinship until the upstream review finish kubernetes/kubernetes#98755 It is a bit ironic, but since Conformance broke due to the bug in hostport, now the fix may get upstream. |
|
@aojea: GitHub didn't allow me to assign the following users: danwinship. Note that only cri-o members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
openshift-ci-robot
added
the
do-not-merge/hold
Codecov Report
@@ Coverage Diff @@
## master #4542 +/- ##
=======================================
Coverage 40.18% 40.18%
=======================================
Files 115 115
Lines 9327 9327
=======================================
Hits 3748 3748
Misses 5161 5161
Partials 418 418 |
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
openshift-ci-robot
added
the
approved
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, 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 |
|
/test integration_fedora |
| if len(existingChainsToRemove) == 0 { | ||
| return nil | ||
| return hm.closeHostports(hostportMappings) | ||
| } | ||
|
|
There was a problem hiding this comment.
There's a return(err) at line https://github.com/cri-o/cri-o/pull/4542/files#diff-1e8c8253b73c319cc3d39e1e6eebdbff23129076dfe7814b74754f640f68b1c5R261 and I think we can leave behind the hostportmappings there too. Would it be better to create a defer function to close them there and to do the unlock too?
There was a problem hiding this comment.
If there is an error programming the iptables rules, it will be possible that the iptables rules for the portmaps are still active so we should keep holding the ports and not close them.
|
@aojea: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
/retest |
|
/cherry-pick release-1.20 |
|
/cherry-pick release-1.19 |
|
@haircommander: new pull request created: #4799 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. |
|
@haircommander: new pull request could not be created: failed to create pull request against cri-o/cri-o#release-1.21 from head openshift-cherrypick-robot:cherry-pick-4542-to-release-1.21: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between cri-o:release-1.21 and openshift-cherrypick-robot:cherry-pick-4542-to-release-1.21"}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"} 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. |
|
@haircommander: new pull request created: #4800 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. |
The host port manager, when Remove the hostport from the system, exists fast if there are no
iptables rules to delete.
However, it was missing to call the method that remove the hostports bound, so there is a possibility that we leak sockets.
Signed-off-by: Antonio Ojea aojea@redhat.com
Thanks to @johnbelamaric
kubernetes/kubernetes#98755 (comment)