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

Support IPv6 for nerdctl network #1558

Merged
merged 1 commit into from
Oct 29, 2023
Merged

Support IPv6 for nerdctl network #1558

merged 1 commit into from
Oct 29, 2023

Conversation

yuchanns
Copy link
Contributor

@yuchanns yuchanns commented Nov 27, 2022

Fix #1547

FYI: cni-ipam

Tasks:

  • Support in the network create command.
  • Support --ip6 argument for the container run command.
  • Integration tests.
  • Documentation.
  • ...

Signed-off-by: Hanchin Hsieh me@yuchanns.xyz
Co-authored-by: Zheao Li me@manjusaka.me

@fahedouch
Copy link
Member

this may inspire you https://github.com/containerd/nerdctl/pull/127/files :-D

@yuchanns
Copy link
Contributor Author

@containerd/nerdctl-maintainers Hey guys I'm encountering issues.

The CI cannot pass when allocating the IPv6 address but test cases work well locally on my laptop. Later @Zheaoli figured out that the problem is lacking IPv6 support in test runners. And I reproduced the scene on the local host after disabling IPv6 support and confirmed that.

The matter is that GHA seems not to support IPv6 on test runners yet (and not going to) according to actions/runner-images#668.

Any ideas?

@yuchanns
Copy link
Contributor Author

yuchanns commented Feb 21, 2023

Blocked by lacking IPv6 test support. There is proposal #2031

@AkihiroSuda
Copy link
Member

Blocked by lacking IPv6 test support. There is proposal #2031

I think we can merge this PR without CI as an experimental feature.

pkg/ocihook/ocihook.go Outdated Show resolved Hide resolved
@aojea
Copy link

aojea commented Oct 26, 2023

Blocked by lacking IPv6 test support. There is proposal #2031

the runners support IPv6 , you can run IPv6 internally in the machine, we have CI on kind running ipv6 only clusters.

What is not supported is IPv6 connectivity, the runners can not communicate externally with IPv6

@aojea
Copy link

aojea commented Oct 26, 2023

can you rebase and push again the PR to take a look at the logs of the failures?

@yuchanns
Copy link
Contributor Author

yuchanns commented Oct 27, 2023

Great thanks to @Zheaoli for being my copilot and helping to solve the IPv6 permission problem. 👏🏻


Done rebasing.

NOTE: I need a moment to recall this PR since it's been a while since I last worked on this PR.


Fine. I will take a look later today (UTC+8).

@aojea
Copy link

aojea commented Oct 27, 2023

it is a permissions problem

ailed (add): failed to set bridge addr: could not add IP address

it looks like a permission problems , I can't remember now but I think that you may need to tune a bit more the way you run the tests inside the container

run: docker run -t --rm --privileged test-integration

kind runs something like
ERROR: failed to create cluster: command "docker run --name test-cluster-control-plane --hostname test-cluster-control-plane --label io.x-k8s.kind.role=control-plane --privileged --security-opt seccomp=unconfined --security-opt apparmor=unconfined --tmpfs /tmp --tmpfs /run --volume /var --volume /lib/modules:/lib/modules:ro --net kind --restart=on-failure:1 --init=false --cgroupns=private ..

and it runs and creates bridges inside kind nodes in github CI https://github.com/aojea/kindnet/blob/master/.github/workflows/e2e.yml#L63-L66 , so it is definitively possible

Dockerfile Outdated Show resolved Hide resolved
@yuchanns yuchanns marked this pull request as ready for review October 27, 2023 17:34
pkg/labels/labels.go Outdated Show resolved Hide resolved
@@ -200,6 +239,8 @@ jobs:
sudo apt-get install -y expect
- name: "Ensure that the integration test suite is compatible with Docker"
run: go test -timeout 20m -v -exec sudo ./cmd/nerdctl/... -args -test.target=docker -test.kill-daemon
- name: "Ensure that the IPv6 integration test suite is compatible with Docker"
run: go test -timeout 20m -v -exec sudo ./cmd/nerdctl/... -args -test.target=docker -test.kill-daemon -test.ipv6
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need reconfiguring sysctl and /etc/docker/daemon.json ?

Copy link
Contributor Author

@yuchanns yuchanns Oct 29, 2023

Choose a reason for hiding this comment

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

No. Fun fact 😂
image

When the parameter --test.ipv6 is enabled, those non-IPv6 tests will not run. I do this because when it comes to nerdctl, IPv6 and non-IPv6 tests can not run together. (integration-tests-ipv6 uses host network that results in some problems with non-IPv6 tests. This may caused by running nerdctl tests nested inside docker and qemu).

I'd like to take your advice if there is a better idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow the integration-tests-ipv6 fails to run nested inside docker and qemu, despite sysctl and daemon being configured. Later @Zheaoli figured out a solution by using network=host to run it successfully. However, this leads to non-IPv6 test failure. So I have to separate the IPv6 test.

Copy link

Choose a reason for hiding this comment

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

hmm, it should be able to run nested, but nested networks are complex and hard to debug, maybe you are missing certain sysctl or something, who knows

- name: "Register QEMU (tonistiigi/binfmt)"
run: docker run --privileged --rm tonistiigi/binfmt --install all
- name: "Run integration tests"
run: docker run --network host -t --rm --privileged test-integration-ipv6
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment to explain why we need (and why we can safely(?) use) --network host

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Co-authored-by: Zheao Li <me@manjusaka.me>
Signed-off-by: Hanchin Hsieh <me@yuchanns.xyz>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda mentioned this pull request Oct 29, 2023
@AkihiroSuda AkihiroSuda added this to the v1.7.0 milestone Oct 29, 2023
@AkihiroSuda AkihiroSuda merged commit 4201370 into containerd:main Oct 29, 2023
22 checks passed
This was referenced Oct 29, 2023
@yuchanns yuchanns deleted the 1547 branch October 30, 2023 00:22
@yankay
Copy link
Contributor

yankay commented Oct 30, 2023

Thanks @yuchanns @AkihiroSuda
amazing

AkihiroSuda added a commit to AkihiroSuda/nerdctl that referenced this pull request Jun 6, 2024
Hanchin Hsieh (yuchanns) served as a Reviewer of nerdctl from November 2022 to June 2024.

Hanchin has made significant contributions such as the addition of
syslog driver (containerd#1377) and IPv6 networking (containerd#1558).

We show our huge appreciation to Hanchin.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

network create flag --ipv6 support
5 participants