dual stack portmap support #4116
Conversation
openshift-ci-robot
added
release-note
|
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 @mrunalp |
|
@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. |
|
/ok-to-test Thank you for the PR @aojea ! |
openshift-ci-robot
added
ok-to-test
server/sandbox_network.go
Outdated
| HostNetwork: false, | ||
| }, "lo") | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to add hostport mapping for sandbox %s(%s): %v", sb.Name(), sb.ID(), err) |
There was a problem hiding this comment.
| return nil, nil, fmt.Errorf("failed to add hostport mapping for sandbox %s(%s): %v", sb.Name(), sb.ID(), err) | |
| return nil, nil, errors.Wrapf(err, "add hostport mapping for sandbox %s (%s)", sb.Name(), sb.ID()) |
There was a problem hiding this comment.
yeah, but is this way all over the file 🙃
There was a problem hiding this comment.
better address it in another PR, to have it consistent, maybe this logging was on porpuse?
openshift-ci-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
/retest |
Yep: |
aojea
force-pushed
the
dualstack
branch
2 times, most recently
from
05af125 to
cb45d93
Compare
current portmap is hardcoded to use IPv4 only. We can add IPv6 support just detecting the IP family from the CNI configuration, however, this won't work for dual stack. We add a portManager handler per IP family and use the corresponding one based on the sandbox registered IPs. Signed-off-by: Antonio Ojea <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #4116 +/- ##
==========================================
- Coverage 41.58% 41.51% -0.07%
==========================================
Files 110 110
Lines 9050 9065 +15
==========================================
Hits 3763 3763
- Misses 4947 4961 +14
- Partials 340 341 +1 |
|
/retest |
|
LGTM, though I agree we should factor this behavior out of server in a follow up. thanks for volunteering @saschagrunert |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, mrunalp, 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 |
|
@danwinship Could you give us a LGTM before we merge this? |
|
/lgtm |
|
@danwinship: changing LGTM is restricted to collaborators 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. |
|
/lgtm |
|
/test integration_rhel |
|
/retest |
|
/test e2e_cgroupv2 lot of storage failures that does not look related to the PR |
They are not, indeed. The e2e_crun_cgroupv2 is broken since quite some time and not mandatory ( |
|
/cherry-pick release-1.19 |
|
@mrunalp: once the present PR merges, I will cherry-pick it on top of release-1.19 in a new PR and assign it to you. 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. |
|
@mrunalp: new pull request created: #4117 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. |
current portmap is hardcoded to use IPv4 only. We can add IPv6
support just detecting the IP family from the network configuration,
however, this won't work for dual stack.
We add a portManager handler per IP family and use the corresponding
one based on the sandbox registered IPs.
Signed-off-by: Antonio Ojea [email protected]
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1872128