Skip to content

Commit

Permalink
Refactor out CreateEndpoint method.
Browse files Browse the repository at this point in the history 
CreateEndpoint touches many different parts of the codebase, so the
refactors are surprisingly extensive.

* Create a facade for Controller to try and shrink the Controller port
interface. It's still WIP (needs to be done gradually when refactoring
other handlers), but I belive it's a step in the right direction.
* Refactor CreateEndpoint by simplifying the way it calls its
collaborators. Move it to driver_core package.
* Fixed fake controller implementation by using
contrail-go-api Interceptors to mimic the actual API server behaviour of
assigning Default GW, MAC and IPs. This accidentally fixed two
controlelr_test.go tests. Oops.

Change-Id: I8a4f9dbdd390f7167409689cd2ae6c204b0a3edb
Partial-Bug: #1778671
  • Loading branch information
Michal Kostrzewa committed Jul 16, 2018
1 parent ab523a6 commit ad77e13
Show file tree
Hide file tree
Showing 16 changed files with 585 additions and 194 deletions.
10 changes: 8 additions & 2 deletions Gopkg.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

118 changes: 7 additions & 111 deletions adapters/primary/docker_libnetwork_plugin/plugin.go
View file
Expand Up @@ -32,7 +32,6 @@ import (
"github.com/Juniper/contrail-windows-docker-driver/common"
"github.com/Juniper/contrail-windows-docker-driver/core/driver_core"
winio "github.com/Microsoft/go-winio"
"github.com/Microsoft/hcsshim"
dockerTypes "github.com/docker/docker/api/types"
dockerClient "github.com/docker/docker/client"
"github.com/docker/go-connections/sockets"
Expand All @@ -41,8 +40,6 @@ import (
log "github.com/sirupsen/logrus"
)

const hnsEndpointWaitingTime = 5

type DockerPluginServer struct {
// TODO: for now, Core field is public, because we need to access its fields, like controller.
// This should be made private when making the Controller port smaller.
Expand Down Expand Up @@ -224,12 +221,12 @@ func (d *DockerPluginServer) CreateNetwork(req *network.CreateNetworkRequest) er
if len(req.IPv4Data) == 0 {
return errors.New("Docker subnet IPv4 data missing")
}
ipPool := req.IPv4Data[0].Pool
subnetCIDR := req.IPv4Data[0].Pool

tenantName := tenant.(string)
networkName := netName.(string)

return d.Core.CreateNetwork(tenantName, networkName, ipPool)
return d.Core.CreateNetwork(tenantName, networkName, subnetCIDR)
}

func (d *DockerPluginServer) AllocateNetwork(req *network.AllocateNetworkRequest) (
Expand Down Expand Up @@ -312,102 +309,16 @@ func (d *DockerPluginServer) CreateEndpoint(req *network.CreateEndpointRequest)
return nil, err
}

contrailNetwork, err := d.Core.Controller.GetNetwork(meta.tenant, meta.network)
if err != nil {
return nil, err
}
log.Infoln("Retrieved Contrail network:", contrailNetwork.GetUuid())

// TODO JW-187.
// We need to retreive Container ID here and use it instead of EndpointID as
// argument to GetOrCreateInstance().
// EndpointID is equiv to interface, but in Contrail, we have a "VirtualMachine" in
// data model.
// A single VM can be connected to two or more overlay networks, but when we use
// EndpointID, this won't work.
// We need something like:
// containerID := req.Options["vmname"]
containerID := req.EndpointID

contrailIpam, err := d.Core.Controller.GetIpamSubnet(contrailNetwork, meta.subnetCIDR)
if err != nil {
return nil, err
}
contrailSubnetCIDR := d.Core.GetContrailSubnetCIDR(contrailIpam)

contrailVif, err := d.Core.Controller.GetOrCreateInterface(contrailNetwork, meta.tenant,
containerID)
if err != nil {
return nil, err
}

contrailVM, err := d.Core.Controller.GetOrCreateInstance(contrailVif, containerID)
if err != nil {
return nil, err
}

contrailIP, err := d.Core.Controller.GetOrCreateInstanceIp(contrailNetwork, contrailVif, contrailIpam.SubnetUuid)
container, err := d.Core.CreateEndpoint(meta.tenant, meta.network, meta.subnetCIDR, req.EndpointID)
if err != nil {
return nil, err
}
instanceIP := contrailIP.GetInstanceIpAddress()
log.Infoln("Retrieved instance IP:", instanceIP)

contrailGateway := contrailIpam.DefaultGateway
log.Infoln("Retrieved GW address:", contrailGateway)
if contrailGateway == "" {
return nil, errors.New("Default GW is empty")
}

contrailMac, err := d.Core.Controller.GetInterfaceMac(contrailVif)
log.Infoln("Retrieved MAC:", contrailMac)
if err != nil {
return nil, err
}
// contrail MACs are like 11:22:aa:bb:cc:dd
// HNS needs MACs like 11-22-AA-BB-CC-DD
formattedMac := strings.Replace(strings.ToUpper(contrailMac), ":", "-", -1)

hnsNet, err := d.Core.LocalContrailNetworksRepo.GetNetwork(meta.tenant, meta.network, contrailSubnetCIDR)
if err != nil {
return nil, err
}

hnsEndpointConfig := &hcsshim.HNSEndpoint{
VirtualNetworkName: hnsNet.Name,
Name: req.EndpointID,
IPAddress: net.ParseIP(instanceIP),
MacAddress: formattedMac,
GatewayAddress: contrailGateway,
}

hnsEndpointID, err := d.Core.LocalContrailEndpointsRepo.CreateEndpoint(hnsEndpointConfig)
if err != nil {
return nil, err
}

// TODO: test this when Agent is ready
ifName := d.generateFriendlyName(hnsEndpointID)

go func() {
// Temporary workaround for HNS issue.
// Due to the bug in Microsoft HNS, Docker Driver has to wait for some time until endpoint
// is ready to handle ARP requests. Unfortunately it seems that HNS doesn't have api
// to verify if endpoint setup is done.

time.Sleep(hnsEndpointWaitingTime * time.Second)
err := d.Core.Agent.AddPort(contrailVM.GetUuid(), contrailVif.GetUuid(), ifName, contrailMac, containerID,
contrailIP.GetInstanceIpAddress(), contrailNetwork.GetUuid())
if err != nil {
log.Error(err.Error())
}
}()

epAddressCIDR := fmt.Sprintf("%s/%v", instanceIP, contrailIpam.Subnet.IpPrefixLen)
ipCIDR := fmt.Sprintf("%s/%v", container.IP, container.PrefixLen)
r := &network.CreateEndpointResponse{
Interface: &network.EndpointInterface{
Address: epAddressCIDR,
MacAddress: contrailMac,
Address: ipCIDR,
MacAddress: container.Mac,
},
}
return r, nil
Expand Down Expand Up @@ -442,6 +353,7 @@ func (d *DockerPluginServer) DeleteEndpoint(req *network.DeleteEndpointRequest)
// Temporary workaround for HNS issue, see 'CreateEndpoint' method.
// This sleep is added to ensure that DeletePort request is called after AddPort.
// Value of waiting time has to be equal or greater than the one in 'CreateEndpoint'.
const hnsEndpointWaitingTime = 5
time.Sleep(hnsEndpointWaitingTime * time.Second)
err := d.Core.Agent.DeletePort(contrailVif.GetUuid())
if err != nil {
Expand Down Expand Up @@ -701,19 +613,3 @@ func (d *DockerPluginServer) hnsNetworksMeta() ([]NetworkMeta, error) {
}
return meta, nil
}

func (d *DockerPluginServer) generateFriendlyName(hnsEndpointID string) string {
// Here's how the Forwarding Extension (kernel) can identify interfaces based on their
// friendly names.
// Windows Containers have NIC names like "NIC ID abcdef", where abcdef are the first 6 chars
// of their HNS endpoint ID.
// Hyper-V Containers have NIC names consisting of two uuids, probably representing utitlity
// VM's interface and endpoint's interface:
// "227301f6-bee9-4ae2-8a93-5e900cde3f47--910c5490-bff8-45e3-a2a0-0114ed9903e0"
// The second UUID (after the "--") is the HNS endpoints ID.

// For now, we will always send the name in the Windows Containers format, because it probably
// has enough information to recognize it in kernel (6 first chars of UUID should be enough):
containerNicID := strings.Split(hnsEndpointID, "-")[0]
return fmt.Sprintf("Container NIC %s", containerNicID)
}
4 changes: 3 additions & 1 deletion adapters/primary/docker_libnetwork_plugin/plugin_test.go
View file
Expand Up @@ -824,7 +824,9 @@ func newIntegrationModulesUnderTest() (vr ports.VRouter, d *docker_libnetwork_pl
serverUrl, _ := url.Parse("http://127.0.0.1:9091")
a := agent.NewAgentRestAPI(http.DefaultClient, serverUrl)

driverCore, err := driver_core.NewContrailDriverCore(vr, c, a, netRepo, epRepo)
hnsWorkaroundSleepTime := time.Second * 0
driverCore, err := driver_core.NewContrailDriverCore(vr, c, a, netRepo, epRepo,
hnsWorkaroundSleepTime)
Expect(err).ToNot(HaveOccurred())
d = docker_libnetwork_plugin.NewDockerPluginServer(driverCore)

Expand Down
4 changes: 4 additions & 0 deletions adapters/secondary/controller_REST/api/api_client.go
View file
Expand Up @@ -23,6 +23,10 @@ import (
func NewFakeApiClient() *mocks.ApiClient {
mockedApiClient := new(mocks.ApiClient)
mockedApiClient.Init()
// Add interceptors to mimick the behaviour of actual API server.
mockedApiClient.AddInterceptor("virtual-network", &vnInterceptor{})
mockedApiClient.AddInterceptor("virtual-machine-interface", &vmiInterceptor{})
mockedApiClient.AddInterceptor("instance-ip", &iipInterceptor{})
return mockedApiClient
}

Expand Down
57 changes: 40 additions & 17 deletions adapters/secondary/controller_REST/controller.go
View file
Expand Up @@ -31,25 +31,48 @@ import (
type Info struct {
}

type ControllerAdapter struct {
type ControllerAdapterImpl struct {
ApiClient contrail.ApiClient
}

func newControllerAdapter(apiClient contrail.ApiClient) *ControllerAdapter {
client := &ControllerAdapter{ApiClient: apiClient}
// TODO: this factory function is public only because we have a bunch of tests for
// ControllerAdapterImpl methods. We should instead test via the Facade - ControllerAdapter.
// I decided against rewriting all the tests for now, but it would be an improvement.
// Right now, ControllerAdapterImpl test the implementation, and not the behaviour.
func NewControllerAdapterImpl(apiClient contrail.ApiClient) *ControllerAdapterImpl {
client := &ControllerAdapterImpl{ApiClient: apiClient}
return client
}

func (c *ControllerAdapter) NewProject(domain, tenant string) (*types.Project, error) {
// TODO: this method is only used by tests - it can probably be removed from ControllerAdapterImpl
// entirely and moved to helpers.
func (c *ControllerAdapterImpl) NewProject(domain, tenant string) (*types.Project, error) {
project := new(types.Project)
project.SetFQName("domain", []string{domain, tenant})
if err := c.ApiClient.Create(project); err != nil {
return nil, err
}

// Create security group as soon as project is created. This mimics contrail API server
// behaviuor. We can do it here, because NewProject method is used only in tests (see
// method comment).
if _, err := c.createSecurityGroup(domain, tenant); err != nil {
return nil, err
}

return project, nil
}

func (c *ControllerAdapter) CreateNetworkWithSubnet(tenantName, networkName, subnetCIDR string) (*types.VirtualNetwork, error) {
func (c *ControllerAdapterImpl) createSecurityGroup(domain, tenant string) (*types.SecurityGroup, error) {
secgroup := new(types.SecurityGroup)
secgroup.SetFQName("project", []string{domain, tenant, "default"})
if err := c.ApiClient.Create(secgroup); err != nil {
return nil, err
}
return secgroup, nil
}

func (c *ControllerAdapterImpl) CreateNetworkWithSubnet(tenantName, networkName, subnetCIDR string) (*types.VirtualNetwork, error) {
net, err := c.GetNetwork(tenantName, networkName)
if err == nil {
showDetails := true
Expand Down Expand Up @@ -82,7 +105,7 @@ func (c *ControllerAdapter) CreateNetworkWithSubnet(tenantName, networkName, sub
return net, nil
}

func (c *ControllerAdapter) GetNetwork(tenantName, networkName string) (*types.VirtualNetwork,
func (c *ControllerAdapterImpl) GetNetwork(tenantName, networkName string) (*types.VirtualNetwork,
error) {
name := fmt.Sprintf("%s:%s:%s", common.DomainName, tenantName, networkName)
net, err := types.VirtualNetworkByName(c.ApiClient, name)
Expand All @@ -95,7 +118,7 @@ func (c *ControllerAdapter) GetNetwork(tenantName, networkName string) (*types.V

// GetIpamSubnet returns IPAM subnet of specified virtual network with specified CIDR.
// If virtual network has only one subnet, CIDR is ignored.
func (c *ControllerAdapter) GetIpamSubnet(net *types.VirtualNetwork, CIDR string) (
func (c *ControllerAdapterImpl) GetIpamSubnet(net *types.VirtualNetwork, CIDR string) (
*types.IpamSubnetType, error) {

if strings.HasPrefix(CIDR, "0.0.0.0") {
Expand Down Expand Up @@ -150,7 +173,7 @@ func (c *ControllerAdapter) GetIpamSubnet(net *types.VirtualNetwork, CIDR string
return nil, err
}

func (c *ControllerAdapter) GetDefaultGatewayIp(subnet *types.IpamSubnetType) (string, error) {
func (c *ControllerAdapterImpl) GetDefaultGatewayIp(subnet *types.IpamSubnetType) (string, error) {
gw := subnet.DefaultGateway
if gw == "" {
err := errors.New("Default GW is empty")
Expand All @@ -160,7 +183,7 @@ func (c *ControllerAdapter) GetDefaultGatewayIp(subnet *types.IpamSubnetType) (s
return gw, nil
}

func (c *ControllerAdapter) GetOrCreateInstance(vif *types.VirtualMachineInterface, containerId string) (
func (c *ControllerAdapterImpl) GetOrCreateInstance(vif *types.VirtualMachineInterface, containerId string) (
*types.VirtualMachine, error) {
instance, err := c.GetInstance(containerId)
if err == nil {
Expand Down Expand Up @@ -199,12 +222,12 @@ func (c *ControllerAdapter) GetOrCreateInstance(vif *types.VirtualMachineInterfa
return createdInstance, nil
}

func (c *ControllerAdapter) GetInstance(containerId string) (
func (c *ControllerAdapterImpl) GetInstance(containerId string) (
*types.VirtualMachine, error) {
return types.VirtualMachineByName(c.ApiClient, containerId)
}

func (c *ControllerAdapter) GetExistingInterface(net *types.VirtualNetwork, tenantName,
func (c *ControllerAdapterImpl) GetExistingInterface(net *types.VirtualNetwork, tenantName,
containerId string) (*types.VirtualMachineInterface, error) {

fqName := fmt.Sprintf("%s:%s:%s", common.DomainName, tenantName, containerId)
Expand All @@ -220,7 +243,7 @@ func (c *ControllerAdapter) GetExistingInterface(net *types.VirtualNetwork, tena
return nil, errors.New("Interface does not exist")
}

func (c *ControllerAdapter) GetOrCreateInterface(net *types.VirtualNetwork, tenantName,
func (c *ControllerAdapterImpl) GetOrCreateInterface(net *types.VirtualNetwork, tenantName,
containerId string) (*types.VirtualMachineInterface, error) {

fqName := fmt.Sprintf("%s:%s:%s", common.DomainName, tenantName, containerId)
Expand Down Expand Up @@ -259,7 +282,7 @@ func (c *ControllerAdapter) GetOrCreateInterface(net *types.VirtualNetwork, tena
return createdIface, nil
}

func (c *ControllerAdapter) assignDefaultSecurityGroup(iface *types.VirtualMachineInterface, tenantName string) error {
func (c *ControllerAdapterImpl) assignDefaultSecurityGroup(iface *types.VirtualMachineInterface, tenantName string) error {
secGroupFqName := fmt.Sprintf("%s:%s:default", common.DomainName, tenantName)
secGroup, err := types.SecurityGroupByName(c.ApiClient, secGroupFqName)
if err != nil || secGroup == nil {
Expand All @@ -269,7 +292,7 @@ func (c *ControllerAdapter) assignDefaultSecurityGroup(iface *types.VirtualMachi
return iface.AddSecurityGroup(secGroup)
}

func (c *ControllerAdapter) GetInterfaceMac(iface *types.VirtualMachineInterface) (string, error) {
func (c *ControllerAdapterImpl) GetInterfaceMac(iface *types.VirtualMachineInterface) (string, error) {
macs := iface.GetVirtualMachineInterfaceMacAddresses()
if len(macs.MacAddress) == 0 {
err := errors.New("Empty MAC list")
Expand All @@ -279,7 +302,7 @@ func (c *ControllerAdapter) GetInterfaceMac(iface *types.VirtualMachineInterface
return macs.MacAddress[0], nil
}

func (c *ControllerAdapter) GetOrCreateInstanceIp(net *types.VirtualNetwork,
func (c *ControllerAdapterImpl) GetOrCreateInstanceIp(net *types.VirtualNetwork,
iface *types.VirtualMachineInterface, subnetUuid string) (*types.InstanceIp, error) {
instIp, err := types.InstanceIpByName(c.ApiClient, iface.GetName())
if err == nil && instIp != nil {
Expand Down Expand Up @@ -314,7 +337,7 @@ func (c *ControllerAdapter) GetOrCreateInstanceIp(net *types.VirtualNetwork,
return allocatedIP, nil
}

func (c *ControllerAdapter) DeleteElementRecursive(parent contrail.IObject) error {
func (c *ControllerAdapterImpl) DeleteElementRecursive(parent contrail.IObject) error {
log.Debugln("Deleting", parent.GetType(), parent.GetUuid())
for err := c.ApiClient.Delete(parent); err != nil; err = c.ApiClient.Delete(parent) {
// TODO: when fixing this method, consider using c.is404() method.
Expand Down Expand Up @@ -359,6 +382,6 @@ func (c *ControllerAdapter) DeleteElementRecursive(parent contrail.IObject) erro
return nil
}

func (c *ControllerAdapter) is404(err error) bool {
func (c *ControllerAdapterImpl) is404(err error) bool {
return strings.HasPrefix(err.Error(), "404")
}

0 comments on commit ad77e13

Please sign in to comment.