-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add support to set loopback to up #10238
Add support to set loopback to up #10238
Conversation
Skipping CI for Draft Pull Request. |
98c78a9
to
b8d85c8
Compare
yay 👏 |
0e74629
to
996733d
Compare
I had this on my queue for so long, figured I would just get it done. |
b8f2dea
to
218f6c7
Compare
I'm not familiar with the conventions of containerd for new APIs and config fields or the introduction of deprecations, so I can not judge that, but technically this LGTM
I still have question though that I think is worth to test it, what happens if an user uses this new option but still has the loopback cni plugin, if both loopback are configured are there any bad interactions or is a noop when a pod is created and runs both of them? |
The community CNI plugin just sets the lo interface to up. Cilium and Calico for example set the lo to up as well. I believe only flannel doesn't set it to up. This should not be impactful. Hopefully the users follow the upgrade instructions as well. However since the field defaults to the legacy behavior we should not see any issues. |
/test pull-containerd-k8s-e2e-ec2 checking if "cmd: failed to assign an IP address to container" was a flake.. |
/test pull-containerd-k8s-e2e-ec2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a minor nit on spacing :)
218f6c7
to
efca09b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on migration test in integration...
A nice to have here would be a test bucket for the config switch enabled vs disabled.. maybe an exec into busybox that uses localhost.. checks the result.
Signed-off-by: Michael Zappa <michael.zappa@gmail.com>
efca09b
to
332caf1
Compare
/test pull-containerd-k8s-e2e-ec2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The CNI maintainers intend on deprecating the loopback plugin. This approach where we set the lo interface to up is similar to what CRI-O does. We (@mikebrow ) decided to have a flag to disable/enable and eventually we will go with removing the flags and just always setting the lo interface to up.