From b9a2193b3a20eca364c8aa5b411bcbcb3ff91a5e Mon Sep 17 00:00:00 2001 From: Onur Filiz Date: Wed, 16 Aug 2017 14:13:46 -0700 Subject: [PATCH] Improve cleanup on network failure paths --- network/endpoint.go | 30 ++++++++++---------- network/endpoint_linux.go | 58 +++++++++++++++++++-------------------- network/network.go | 30 +++++++++++--------- network/network_linux.go | 34 ++++++++++++----------- 4 files changed, 80 insertions(+), 72 deletions(-) diff --git a/network/endpoint.go b/network/endpoint.go index 9707f3c65c..07ac9eb7ee 100644 --- a/network/endpoint.go +++ b/network/endpoint.go @@ -45,16 +45,21 @@ func (nw *network) newEndpoint(epInfo *EndpointInfo) (*endpoint, error) { var err error log.Printf("[net] Creating endpoint %+v in network %v.", epInfo, nw.Id) + defer func() { + if err != nil { + log.Printf("[net] Failed to create endpoint %v, err:%v.", epInfo.Id, err) + } + }() if nw.Endpoints[epInfo.Id] != nil { err = errEndpointExists - goto fail + return nil, err } // Call the platform implementation. ep, err = nw.newEndpointImpl(epInfo) if err != nil { - goto fail + return nil, err } nw.Endpoints[epInfo.Id] = ep @@ -62,27 +67,29 @@ func (nw *network) newEndpoint(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Created endpoint %+v.", ep) return ep, nil - -fail: - log.Printf("[net] Creating endpoint %v failed, err:%v.", epInfo.Id, err) - - return nil, err } // DeleteEndpoint deletes an existing endpoint from the network. func (nw *network) deleteEndpoint(endpointId string) error { + var err error + log.Printf("[net] Deleting endpoint %v from network %v.", endpointId, nw.Id) + defer func() { + if err != nil { + log.Printf("[net] Failed to delete endpoint %v, err:%v.", endpointId, err) + } + }() // Look up the endpoint. ep, err := nw.getEndpoint(endpointId) if err != nil { - goto fail + return err } // Call the platform implementation. err = nw.deleteEndpointImpl(ep) if err != nil { - goto fail + return err } // Remove the endpoint object. @@ -91,11 +98,6 @@ func (nw *network) deleteEndpoint(endpointId string) error { log.Printf("[net] Deleted endpoint %+v.", ep) return nil - -fail: - log.Printf("[net] Deleting endpoint %v failed, err:%v.", endpointId, err) - - return err } // GetEndpoint returns the endpoint with the given ID. diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go index 6ccc96c168..618cbae5de 100644 --- a/network/endpoint_linux.go +++ b/network/endpoint_linux.go @@ -52,6 +52,13 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { return nil, err } + // On failure, delete the veth pair. + defer func() { + if err != nil { + netlink.DeleteLink(contIfName) + } + }() + // // Host network interface setup. // @@ -60,14 +67,14 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Setting link %v state up.", hostIfName) err = netlink.SetLinkState(hostIfName, true) if err != nil { - goto cleanup + return nil, err } // Connect host interface to the bridge. log.Printf("[net] Setting link %v master %v.", hostIfName, nw.extIf.BridgeName) err = netlink.SetLinkMaster(hostIfName, nw.extIf.BridgeName) if err != nil { - goto cleanup + return nil, err } // @@ -77,7 +84,7 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { // Query container network interface info. containerIf, err = net.InterfaceByName(contIfName) if err != nil { - goto cleanup + return nil, err } // Setup rules for IP addresses on the container interface. @@ -86,14 +93,14 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Adding ARP reply rule for IP address %v on %v.", ipAddr.String(), contIfName) err = ebtables.SetArpReply(ipAddr.IP, nw.getArpReplyAddress(containerIf.HardwareAddr), ebtables.Append) if err != nil { - goto cleanup + return nil, err } // Add MAC address translation rule. log.Printf("[net] Adding MAC DNAT rule for IP address %v on %v.", ipAddr.String(), contIfName) err = ebtables.SetDnatForIPAddress(nw.extIf.Name, ipAddr.IP, containerIf.HardwareAddr, ebtables.Append) if err != nil { - goto cleanup + return nil, err } } @@ -103,7 +110,7 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Opening netns %v.", epInfo.NetNsPath) ns, err = OpenNamespace(epInfo.NetNsPath) if err != nil { - goto cleanup + return nil, err } defer ns.Close() @@ -111,15 +118,24 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Setting link %v netns %v.", contIfName, epInfo.NetNsPath) err = netlink.SetLinkNetNs(contIfName, ns.GetFd()) if err != nil { - goto cleanup + return nil, err } // Enter the container network namespace. log.Printf("[net] Entering netns %v.", epInfo.NetNsPath) err = ns.Enter() if err != nil { - goto cleanup + return nil, err } + + // Return to host network namespace. + defer func() { + log.Printf("[net] Exiting netns %v.", epInfo.NetNsPath) + err = ns.Exit() + if err != nil { + log.Printf("[net] Failed to exit netns, err:%v.", err) + } + }() } // If a name for the container interface is specified... @@ -128,14 +144,14 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Setting link %v state down.", contIfName) err = netlink.SetLinkState(contIfName, false) if err != nil { - goto cleanup + return nil, err } // Rename the container interface. log.Printf("[net] Setting link %v name %v.", contIfName, epInfo.IfName) err = netlink.SetLinkName(contIfName, epInfo.IfName) if err != nil { - goto cleanup + return nil, err } contIfName = epInfo.IfName @@ -143,7 +159,7 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Setting link %v state up.", contIfName) err = netlink.SetLinkState(contIfName, true) if err != nil { - goto cleanup + return nil, err } } @@ -152,7 +168,7 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { log.Printf("[net] Adding IP address %v to link %v.", ipAddr.String(), contIfName) err = netlink.AddIpAddress(contIfName, ipAddr.IP, &ipAddr) if err != nil { - goto cleanup + return nil, err } } @@ -169,17 +185,7 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { err = netlink.AddIpRoute(nlRoute) if err != nil { - goto cleanup - } - } - - // If inside the container network namespace... - if ns != nil { - // Return to host network namespace. - log.Printf("[net] Exiting netns %v.", epInfo.NetNsPath) - err = ns.Exit() - if err != nil { - goto cleanup + return nil, err } } @@ -194,12 +200,6 @@ func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { } return ep, nil - -cleanup: - // Roll back the changes for the endpoint. - netlink.DeleteLink(contIfName) - - return nil, err } // deleteEndpointImpl deletes an existing endpoint from the network. diff --git a/network/network.go b/network/network.go index d09ad1c106..3340dcf9ce 100644 --- a/network/network.go +++ b/network/network.go @@ -121,6 +121,11 @@ func (nm *networkManager) newNetwork(nwInfo *NetworkInfo) (*network, error) { var err error log.Printf("[net] Creating network %+v.", nwInfo) + defer func() { + if err != nil { + log.Printf("[net] Failed to create network %v, err:%v.", nwInfo.Id, err) + } + }() // Set defaults. if nwInfo.Mode == "" { @@ -131,19 +136,19 @@ func (nm *networkManager) newNetwork(nwInfo *NetworkInfo) (*network, error) { extIf := nm.findExternalInterfaceBySubnet(nwInfo.Subnets[0].Prefix.String()) if extIf == nil { err = errSubnetNotFound - goto fail + return nil, err } // Make sure this network does not already exist. if extIf.Networks[nwInfo.Id] != nil { err = errNetworkExists - goto fail + return nil, err } // Call the OS-specific implementation. nw, err = nm.newNetworkImpl(nwInfo, extIf) if err != nil { - goto fail + return nil, err } // Add the network object. @@ -152,26 +157,29 @@ func (nm *networkManager) newNetwork(nwInfo *NetworkInfo) (*network, error) { log.Printf("[net] Created network %v on interface %v.", nwInfo.Id, extIf.Name) return nw, nil - -fail: - log.Printf("[net] Failed to create network %v, err:%v.", nwInfo.Id, err) - return nil, err } // DeleteNetwork deletes an existing container network. func (nm *networkManager) deleteNetwork(networkId string) error { + var err error + log.Printf("[net] Deleting network %v.", networkId) + defer func() { + if err != nil { + log.Printf("[net] Failed to delete network %v, err:%v.", networkId, err) + } + }() // Find the network. nw, err := nm.getNetwork(networkId) if err != nil { - goto fail + return err } // Call the OS-specific implementation. err = nm.deleteNetworkImpl(nw) if err != nil { - goto fail + return err } // Remove the network object. @@ -179,10 +187,6 @@ func (nm *networkManager) deleteNetwork(networkId string) error { log.Printf("[net] Deleted network %+v.", nw) return nil - -fail: - log.Printf("[net] Failed to delete network %v, err:%v.", networkId, err) - return err } // GetNetwork returns the network with the given ID. diff --git a/network/network_linux.go b/network/network_linux.go index 64d48bb111..16e0e6907d 100644 --- a/network/network_linux.go +++ b/network/network_linux.go @@ -188,7 +188,10 @@ func (nm *networkManager) deleteBridgeRules(extIf *externalInterface) { // ConnectExternalInterface connects the given host interface to a bridge. func (nm *networkManager) connectExternalInterface(extIf *externalInterface, nwInfo *NetworkInfo) error { + var err error + log.Printf("[net] Connecting interface %v.", extIf.Name) + defer func() { log.Printf("[net] Connecting interface %v completed with err:%v.", extIf.Name, err) }() // Check whether this interface is already connected. if extIf.BridgeName != "" { @@ -226,9 +229,16 @@ func (nm *networkManager) connectExternalInterface(extIf *externalInterface, nwI return err } + // On failure, delete the bridge. + defer func() { + if err != nil { + netlink.DeleteLink(bridgeName) + } + }() + bridge, err = net.InterfaceByName(bridgeName) if err != nil { - goto cleanup + return err } } else { // Use the existing bridge. @@ -244,42 +254,42 @@ func (nm *networkManager) connectExternalInterface(extIf *externalInterface, nwI // Add the bridge rules. err = nm.addBridgeRules(extIf, hostIf, bridgeName, nwInfo.Mode) if err != nil { - goto cleanup + return err } // External interface down. log.Printf("[net] Setting link %v state down.", hostIf.Name) err = netlink.SetLinkState(hostIf.Name, false) if err != nil { - goto cleanup + return err } // Connect the external interface to the bridge. log.Printf("[net] Setting link %v master %v.", hostIf.Name, bridgeName) err = netlink.SetLinkMaster(hostIf.Name, bridgeName) if err != nil { - goto cleanup + return err } // External interface up. log.Printf("[net] Setting link %v state up.", hostIf.Name) err = netlink.SetLinkState(hostIf.Name, true) if err != nil { - goto cleanup + return err } // External interface hairpin on. log.Printf("[net] Setting link %v hairpin on.", hostIf.Name) err = netlink.SetLinkHairpin(hostIf.Name, true) if err != nil { - goto cleanup + return err } // Bridge up. log.Printf("[net] Setting link %v state up.", bridgeName) err = netlink.SetLinkState(bridgeName, true) if err != nil { - goto cleanup + return err } // Apply IP configuration to the bridge for host traffic. @@ -289,19 +299,11 @@ func (nm *networkManager) connectExternalInterface(extIf *externalInterface, nwI } extIf.BridgeName = bridgeName + err = nil log.Printf("[net] Connected interface %v to bridge %v.", extIf.Name, extIf.BridgeName) return nil - -cleanup: - log.Printf("[net] Connecting interface %v failed, err:%v.", extIf.Name, err) - - // Roll back the changes for the network. - nm.deleteBridgeRules(extIf) - netlink.DeleteLink(bridgeName) - - return err } // DisconnectExternalInterface disconnects a host interface from its bridge.