Use the network luke #1508
Use the network luke #1508
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder 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 |
k8s-ci-robot
added
approved
|
this almost definitely:
on docker node provider and ipv4 nodes though this should work great both should be resolvable in a future iteration, but |
| @@ -19,6 +19,8 @@ package config | |||
|
|
|||
| import ( | |||
| "bytes" | |||
| "fmt" | |||
| "net" | |||
There was a problem hiding this comment.
this whole commit can be dropped but I'm not fighting with git at 2am and it's not going to show up in the git history anyhow.
|
|
||
| # fixup IPs in manifests ... | ||
| # TODO: dual-stack LOL :this-is-fine: | ||
| curr_ip=$(hostname --ip-address) |
There was a problem hiding this comment.
we'll want to swap this out with something else. this was sufficient for bootstrapping it but we'll need to actually change this to look at /kind/old-ip and get an IP of the same family.
not going to be too hard, but wasn't relevant to the bigger problems. bash wrangling for ipv6 singlestack can be a follow up, but should happen prior to releasing any official kind updates.
| // network because the default bridge is actually special versus any other | ||
| // docker network and lacks the emebdded DNS | ||
| // | ||
| // for now this also makes it easier for apps to join the same network |
There was a problem hiding this comment.
I.E. you can do docker run --network=kind --name=metallb-test-cluster-sidecar .... when doing fun test stuff.
Having a predictable default network makes that saner.
We could do one per cluster but not only is that more complex, it's going to cause IP allocation issues more readily when you have lots of small clusters (which is much moreso the usage pattern than one cluster with a large number of nodes).
In the future this can be configurable and this can just be the default, but it's roughly equivilant to the current state and keeps the PR focused on the necessary changes.
There was a problem hiding this comment.
also it will modify current behaviour where multiple cluster are in the same network.
IIRC istio was using it
|
hehe surprise surprise we did actually break ipv6 |
k8s-ci-robot
added
the
do-not-merge/hold
|
will fix ipv6 in the morning 😴 |
| if string(out) == name+"\n" { | ||
| return nil | ||
| } | ||
| return exec.Command("docker", "network", "create", "-d=bridge", name).Run() |
There was a problem hiding this comment.
TODO
if ipv6 {
exec.Command("docker", "network", "create", "--ipv6","-d=bridge", name).Run()There was a problem hiding this comment.
I'll probably actually fix it 😅
There was a problem hiding this comment.
ohhhh ugh, if we want to support simultaneous ipv4 and ipv6 we're going to have to either:
- always enable ipv6 on the kind bridge
- require that users ensure the bridge exists w/ ipv6 before kind
- require that users create an ipv6 cluster first before any ipv4
- use a special network for ipv6 clusters
requiring that the ipv6 one is created first seems sanest?
that or having a kind-ipv6 network ...
always enabling ipv6 won't work because docker OOTB will cause:
Error response from daemon: could not find an available, non-overlapping IPv6 address pool among the defaults to assign to the network
due to ipv6 not being configured on the daemon.
There was a problem hiding this comment.
i implemented:
require that users create an ipv6 cluster first before any ipv4
just to see how far CI gets.
There was a problem hiding this comment.
can we just retry and fallback to an ipv4 only bridge?
There was a problem hiding this comment.
not sure what you mean.
ipv6 has to be enabled at network creation time.
the old default one has it IF the daemon has ipv6 configured. but no embedded DNS.
if the user creates a kind cluster with ipv4 then creates ipv6 we'll have created a v4 only bridge.
we could try to check if the daemon has v6 configured when creating v4 clusters and opportunistically enable v6 maybe.
let's see how CI does first.
There was a problem hiding this comment.
if err:= exec.Command("docker", "network", "create", "--ipv6","-d=bridge", name).Run(): err != nil {
return exec.Command("docker", "network", "create", "-d=bridge", name).Run()
}
return nilThere was a problem hiding this comment.
this will report odd errors when the user was actually trying to do ipv6.
I will think on this some more and follow up
|
I tossed in the subnet from wrapper.sh for now, technically it's customizable if you just create the network before using kind ;-) edit: adjusted to not overlap 🙃 |
| if ipFamily == config.IPv6Family { | ||
| return exec.Command("docker", "network", "create", "-d=bridge", "--ipv6", name).Run() | ||
| return exec.Command("docker", "network", "create", "-d=bridge", "--ipv6", "--subnet=fc00:db8:1::/64", name).Run() |
There was a problem hiding this comment.
we can use a /104 , it means 255 addresses and we have a low risk of collisions based on this kubernetes/kubernetes#90115 (comment)
There was a problem hiding this comment.
i'm using a /104 just above this now. i'm not sure this is the value we want to settle on.
There was a problem hiding this comment.
BenTheElder
force-pushed
the
use-the-network-luke
branch
from
ad77b29 to
31e8939
Compare
|
Subpath flake |
|
IPv6 green. |
|
that flake is similar to the ones we were worried about in the pause pr ... |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
FYI i tested podman ipv6 and it wasn't working before this PR as well, so no regression there. |
|
this does appear to fix #1508, i tested with a host using iptables_nft one outstanding issue I found that @amwat confirmed is this: $ kubectl get po -A
NAMESPACE NAME READY STATUS RESTARTS AGE
kube-system coredns-66bff467f8-fxzqz 1/1 Running 1 14m
kube-system coredns-66bff467f8-kg9sf 1/1 Running 1 14m
kube-system etcd-kind-control-plane 1/1 Running 1 14m
kube-system kindnet-mr62p 1/1 Running 1 14m
kube-system kube-apiserver-kind-control-plane 1/1 Running 1 14m
kube-system kube-controller-manager-kind-control-plane 1/1 Running 1 14m
kube-system kube-proxy-gpp6v 1/1 Running 1 14m
kube-system kube-scheduler-kind-control-plane 1/1 Running 1 14m
local-path-storage local-path-provisioner-774f7f8fdb-j8mmd 0/1 NodeAffinity 0 14m
local-path-storage local-path-provisioner-774f7f8fdb-wxp7w 1/1 Running 0 9s... a stale haven't debugged it further yet. this is after one physical host reboot. |
|
/test all |
| return "", errors.Wrap(err, "failed to get apiserver endpoint") | ||
| } | ||
| // TODO: check cluster IP family and return the correct IP | ||
| // This means IPv6 singlestack is broken on podman |
There was a problem hiding this comment.
noting that per @amwat this never worked, leaving the TODO for the future though.
| @@ -87,7 +87,7 @@ spec: | |||
| app: local-path-provisioner | |||
| spec: | |||
| nodeSelector: | |||
| node-role.kubernetes.io/master: '' | |||
There was a problem hiding this comment.
this label is deprecated, however we do want to only run on linux nodes for our friends doing windows join hijinks.
|
I think the local-path-storage issue should be resoolllved now. |
|
|
|
/lgtm |
k8s-ci-robot
added
the
lgtm
implement #148
NOTE: breaking change for docker backend, requires new node images, switches nodes from the default bridge to a
kindnetwork. Also causes clusters to start back up on reboot...