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.

Sign up for GitHub

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

Jump to bottom

kubenet: disable DAD in the container. #55247

Merged
merged 1 commit into from
Nov 9, 2017
Merged

kubenet: disable DAD in the container. #55247

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 45 additions & 0 deletions pkg/kubelet/network/kubenet/kubenet_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,11 @@ func (plugin *kubenetNetworkPlugin) Capabilities() utilsets.Int {
// TODO: Don't pass the pod to this method, it only needs it for bandwidth
// shaping and hostport management.
func (plugin *kubenetNetworkPlugin) setup(namespace string, name string, id kubecontainer.ContainerID, pod *v1.Pod, annotations map[string]string) error {
// Disable DAD so we skip the kernel delay on bringing up new interfaces.
if err := plugin.disableContainerDAD(id); err != nil {
glog.V(3).Infof("Failed to disable DAD in container: %v", err)
Copy link

Choose a reason for hiding this comment

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

It's my understanding that a pod will not get an IPv6 address if hairpin and dad are enabled. If that is the case, should a check be implemented and have setup return an error if disableContainerDAD returns an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CNI bridge plugin correctly handles this case - but it's a bit more subtle. It tries to use enhanced_dad, which all recent kernels have. That includes a nonce so hairpin is correctly detected.

Copy link

Choose a reason for hiding this comment

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

Understood. I looked at the bridge plugin code and saw exactly what you describe. I am fine with kubenet not supporting enhanced_dad. However, I still think it makes sense to throw an error instead of a log message if hairpin is enabled and disableContainerDAD returns an error. Otherwise, the pod will not get an IP address. Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether or not this particular line succeeds or fails won't affect the plugin, which will raise errors appropriately. So, I don't think this needs to fail here, which is a minor performance tweak.

Copy link

Choose a reason for hiding this comment

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

Ah... now it makes sense. kubenet is essentially backed by the bridge/local-ipam plugins. I got it.

}

// Bring up container loopback interface
if _, err := plugin.addContainerToNetwork(plugin.loConfig, "lo", namespace, name, id); err != nil {
return err
Expand Down Expand Up @@ -820,3 +825,43 @@ func (plugin *kubenetNetworkPlugin) syncEbtablesDedupRules(macAddr net.HardwareA
return
}
}

// disableContainerDAD disables duplicate address detection in the container.
// DAD has a negative affect on pod creation latency, since we have to wait
Copy link
Member

Choose a reason for hiding this comment

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

I'd link to the issue here.

// a second or more for the addresses to leave the "tentative" state. Since
// we're sure there won't be an address conflict (since we manage them manually),
// this is safe. See issue 54651.
//
// This sets net.ipv6.conf.default.dad_transmits to 0. It must be run *before*
// the CNI plugins are run.
func (plugin *kubenetNetworkPlugin) disableContainerDAD(id kubecontainer.ContainerID) error {
key := "net/ipv6/conf/default/dad_transmits"

sysctlBin, err := plugin.execer.LookPath("sysctl")
if err != nil {
return fmt.Errorf("Could not find sysctl binary: %s", err)
}

netnsPath, err := plugin.host.GetNetNS(id.ID)
if err != nil {
return fmt.Errorf("Failed to get netns: %v", err)
}
if netnsPath == "" {
return fmt.Errorf("Pod has no network namespace")
}

// If the sysctl doesn't exist, it means ipv6 is disabled; log and move on
if _, err := plugin.sysctl.GetSysctl(key); err != nil {
return fmt.Errorf("Ipv6 not enabled: %v", err)
}

output, err := plugin.execer.Command(plugin.nsenterPath,
fmt.Sprintf("--net=%s", netnsPath), "-F", "--",
sysctlBin, "-w", fmt.Sprintf("%s=%s", key, "0"),
).CombinedOutput()
if err != nil {
return fmt.Errorf("Failed to write sysctl: output: %s error: %s",
output, err)
}
return nil
}