Roll forward "Enable ipv6, nftables, NAT, and nf_raw for Firecracker amd64 guests"#11585
Roll forward "Enable ipv6, nftables, NAT, and nf_raw for Firecracker amd64 guests"#11585
Conversation
…amd64 guests" This reverts commit 740ec46.
bduffany
force-pushed
the
rollfwd-firecracker-guest-fixes
branch
4 times, most recently
from
f291b6b to
c776ff1
Compare
| if runtime.GOARCH != "amd64" { | ||
| // Note: despite the big scary INSECURE env var name, dockerd is completely sandboxed inside a VM, so it's secure for our usage. Once we upgrade our guest kernels to support nf tables, we can remove this. | ||
| cmd.Env = append(os.Environ(), "DOCKER_INSECURE_NO_IPTABLES_RAW=1") | ||
| } |
There was a problem hiding this comment.
Should this be controlled by the platform prop?
There was a problem hiding this comment.
The platform prop controls IPv6, but we're enabling NF_RAW unconditionally on amd64 (NF_RAW not being enabled is the thing that this env var works around). Enabling NF_RAW felt less risky than IPv6, so I didn't put it behind a prop.
| cat /proc/net/if_inet6 >&2 || true | ||
| exit 1 | ||
| fi | ||
| echo ipv4_ipv6_enabled |
There was a problem hiding this comment.
Did you want this test to actually test any network connectivity? Or is it redundant with other tests
There was a problem hiding this comment.
Good call, checking connectivity is better than just checking procfs. Done. (I'm only checking VM-internal connectivity for ipv6, because it's less likely that the host executor will support ipv6 and be able to route ipv6 packets properly)
| c, err := firecracker.NewContainer(ctx, env, &repb.ExecutionTask{}, opts) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| err := c.Remove(ctx) |
There was a problem hiding this comment.
Saw this was added in the diff. Why do we need this when c.Run below includes Remove?
There was a problem hiding this comment.
ah I forgot that we don't need this for firecracker (removed). I'm used to ociruntime where we have to do this because we don't do the cleanup in Run and instead defer the cleanup until after we send the execution reply to bazel. Would like to do this for firecracker at some point, but not today
bduffany
force-pushed
the
rollfwd-firecracker-guest-fixes
branch
from
c776ff1 to
276b726
Compare
bduffany
force-pushed
the
rollfwd-firecracker-guest-fixes
branch
from
276b726 to
f907c0e
Compare
3 participants
This PR contains two commits - an unrevert, then a fix. The diff from the fix commit on its own is here: rollfwd-firecracker-guest-fixes~1...rollfwd-firecracker-guest-fixes
The fix for now is to put ipv6 behind a platform prop. The ipv6 configuration causes some packets to fly around during network interface setup, which caused some of our networking metrics tests to fail. Putting it behind a prop will let us / customers experiment for a bit, and then we can turn it on by default once we're more certain it won't cause other breakages.