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

[WIP]Add ipv6 error message check to support the nerdctl #3396

Closed

Conversation

yankay
Copy link
Member

@yankay yankay commented Oct 26, 2023

Do not merge

HI dears.

I'm one of the maintainers of nerdctl, and I'm working on the nerdctl to support the kind recently.

The cluster has been successfully created by kind with nerdctl:

root@kay201:~/oss/kind# ln -s /usr/local/bin/nerdctl /usr/local/bin/docker ## Using the nerdctl instead of docker
root@kay201:~/oss/kind# export KUBE_VERSION="1.28.0"
root@kay201:~/oss/kind#  ./bin/kind create cluster --config /root/kind.yml --image release-ci.daocloud.io/docker.m.daocloud.io/kindest/node:v${KUBE_VERSION}

Creating cluster "kind" ...
 ✓ Ensuring node image (release-ci.daocloud.io/docker.m.daocloud.io/kindest/node:v1.28.0) 🖼
 ✓ Preparing nodes 📦 📦 📦 📦
 ✓ Writing configuration 📜
 ✓ Starting control-plane 🕹️
 ✓ Installing CNI 🔌
 ✓ Installing StorageClass 💾
 ✓ Joining worker nodes 🚜
Set kubectl context to "kind-kind"

root@kay201:~/oss/kind# ./kubectl get nodes
NAME                 STATUS   ROLES           AGE   VERSION
kind-control-plane   Ready    control-plane   76s   v1.28.0
kind-worker          Ready    <none>          51s   v1.28.0
kind-worker2         Ready    <none>          53s   v1.28.0
kind-worker3         Ready    <none>          52s   v1.28.0

The issues of nerdctl has been almost fixed, only 2 PR is pending:

And there needs a change of kind.

Because the nerdctl do not support the ipv6 network now, so it needs kind to skip the ipv6 config.
If the kind can read the error message of kind, and skip it , the cluster can be create successully.

Thanks :-)

@k8s-ci-robot k8s-ci-robot added the area/provider/docker Issues or PRs related to docker label Oct 26, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 26, 2023
@yankay yankay changed the title Add ipv6 unsupport for nerdctl Add ipv6 error message check to support the nerdctl Oct 26, 2023
@yankay
Copy link
Member Author

yankay commented Oct 26, 2023

/retest

@yankay yankay force-pushed the add-ipv6-unsupport-for-nerdctl branch from 6dc2409 to 239bc7d Compare October 26, 2023 03:26
@aojea
Copy link
Contributor

aojea commented Oct 26, 2023

the problem I see is that once nerdctl implement the --ipv6 flag we'll have to remove the error, right?

what is blocking nerdctl to implement the ipv6 flag? is there any link or place where I can look more?

@BenTheElder
Copy link
Member

the problem I see is that once nerdctl implement the --ipv6 flag we'll have to remove the error, right

I think it depends on how long it takes to ship IPv6, right now released nerdctl won't work even with this patch AIUI but there might be intermediate versions worth supporting and this path could be safely left in place.

I do wonder if we should fork to a nerdctl provider though, which would also let us avoid requiring the user to put a fake docker symlink in their path. We've had a bad experience with this with podman in the past versus detecting that it's actually podman and having dedicated provider / code paths.

@aojea
Copy link
Contributor

aojea commented Oct 26, 2023

I do wonder if we should fork to a nerdctl provider though, which would also let us avoid requiring the user to put a fake docker symlink in their path. We've had a bad experience with this with podman in the past versus detecting that it's actually podman and having dedicated provider / code paths.

@AkihiroSuda has some list somewhere with the missing things, his opinion will be interesting

@AkihiroSuda
Copy link
Member

what is blocking nerdctl to implement the ipv6 flag? is there any link or place where I can look more?

The lack of the CI for IPv6 is considered to be a blocker for this PR:

I think this is just a soft blocker, as Docker doesn't have IPv6 CI either (IIUC).

@AkihiroSuda
Copy link
Member

I do wonder if we should fork to a nerdctl provider though, which would also let us avoid requiring the user to put a fake docker symlink in their path.

SGTM, but I think the implementation should try to avoid the amount of the code clones (unlike podman driver).

@aojea
Copy link
Contributor

aojea commented Oct 26, 2023

The lack of the CI for IPv6 is considered to be a blocker for this PR:

we run ipv6 in github actions, what is exactly nlockling ... I'll try to take a look later

@BenTheElder
Copy link
Member

SGTM, but I think the implementation should try to avoid the amount of the code clones (unlike podman driver).

FWIW It's not too much code (~1200 loc for the entire "provider") and when we review changes to the existing drivers we ask that they be copied between them when PRs aren't obviously specific to that driver.

We can start with a copy paste and just swap docker => nerdctl, if enough code stays common and/or they get larger we can refactor out more common code now that we have N=3 examples to derive APIs from.

$ echo $(pwd) && cloc .
/usr/local/google/home/bentheelder/go/src/sigs.k8s.io/kind/pkg/cluster/internal/providers/docker
      10 text files.
       9 unique files.                              
       1 file ignored.

github.com/AlDanial/cloc v 1.96  T=0.01 s (648.8 files/s, 124063.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Go                               9            170            354           1197
-------------------------------------------------------------------------------
SUM:                             9            170            354           1197
-------------------------------------------------------------------------------

I don't think IPv6 support should be a blocker, kind already works on hosts without IPv6 and will need to continue to for the forseeable future. IPv6 is also somewhat broken in docker, kind.

I think we'll also want to get some minimal CI up, at least testing one configuration of nerdctl

@yankay
Copy link
Member Author

yankay commented Oct 30, 2023

Thanks @aojea @BenTheElder @AkihiroSuda for the PR review

The ipv6 has been supported by the nerdctl by containerd/nerdctl#1558
The kind cluster can be created successfully without the change :-)

Thank you all very much. :-)


And what's the next step to do to make kind support nerdctl ? for example:

@yankay yankay closed this Oct 30, 2023
@yankay yankay reopened this Oct 30, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yankay
Once this PR has been reviewed and has the lgtm label, please ask for approval from aojea. For more information see the Kubernetes Code Review Process.

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

@yankay yankay changed the title Add ipv6 error message check to support the nerdctl [WIP]Add ipv6 error message check to support the nerdctl Oct 30, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2023
@k8s-ci-robot
Copy link
Contributor

@yankay: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kind-e2e-kubernetes-1-24 239bc7d link true /test pull-kind-e2e-kubernetes-1-24

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provider/docker Issues or PRs related to docker cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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.

5 participants