Skip to content
This repository has been archived by the owner on Jun 21, 2024. It is now read-only.

Support for network types per bond, and for the new hybrid-bonded type #239

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions bond_ports.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
package packngo

import "fmt"

const NetworkTypeHybridBonded = "hybrid-bonded"

func (d *Device) GetBondNetworkType(portName string) string {
for _, p := range d.NetworkPorts {
if p.Name == portName {
return p.NetworkType
}
}
return ""
}

func (d *Device) GetEthPortsInBond(name string) []*Port {
ports := []*Port{}
for _, port := range d.NetworkPorts {
if port.Bond != nil && port.Bond.Name == name {
ports = append(ports, &port)
}
}
return ports
}

func BondStateTransitionNecessary(type1, type2 string) bool {
if type1 == type2 {
return false
}
if type1 == NetworkTypeHybridBonded && type2 == NetworkTypeL3 {
return false
}
if type2 == NetworkTypeHybridBonded && type1 == NetworkTypeL3 {
return false
}
return true
}

func (i *DevicePortServiceOp) BondToNetworkType(deviceID, bondPortName, targetType string) (*Device, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function builds to the list of helpers in #230

d, _, err := i.client.Devices.Get(deviceID, nil)
if err != nil {
return nil, err
}

curType := d.GetBondNetworkType(bondPortName)

if !BondStateTransitionNecessary(curType, targetType) {
return d, nil
}

err = i.ConvertDeviceBond(d, bondPortName, targetType)
if err != nil {
return nil, err
}

d, _, err = i.client.Devices.Get(deviceID, nil)

if err != nil {
return nil, err
}

finalType := d.GetNetworkType()

if BondStateTransitionNecessary(finalType, targetType) {
return nil, fmt.Errorf(
"Failed to convert %s on device %s from %s to %s. New type was %s",
bondPortName, deviceID, curType, targetType, finalType)

}
return d, err
}

func (i *DevicePortServiceOp) ConvertDeviceBond(d *Device, bondPortName, targetType string) error {
Copy link
Contributor

@displague displague Feb 5, 2021

Choose a reason for hiding this comment

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

I expect we would refactor ConvertDevice to call this function for each Bond.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you would prefer to still convert network type per device? Instead of to convert bonds?

I just noticed that it's not poss to put bond1 (in n2.xlarge) to L3 and hybrid type, and it reminded of a message you sent me a week ago:

I don’t think we are able to do L3 for bond1. And we were discussing to limit bond1 capabilities to only L2 bonded and unbonded.
Only bond0 should be used for L3 capabilities.

Since the configuration for n2.xlarge is so inflexible (or special), we could treat it as a special case and create a separate function for it. Also we could create a separate tf resource.

If we treat n2 as a special case, maybe we can rollback to use network type for a device (instead of for a bond as this PR suggests). I'm sorry I pushed for the network-type per bond, I thought it's the most versatile way to control the network type.

I don't understand why the bonds are now displayed and controlled separately in the web console, when the 2 bonds in n2.xlarge are so inflexible. Anyway, I figured what network types are possible for bonds of n2.xlarge.

  • bond0: L3, L2-bonded, L2-unbonded (no hybrid or hybrid-bonded)
  • bond1: L2-bonded, L2-unbonded (no L3, hybrid, hybrid-bonded)

Considering all this, I think we should define possible (useful) configurations for the n2.xlarge, and just treat it differently. I don't think that we should map the n2.xlarge confs to the one-bond types.

Copy link
Contributor

@displague displague Feb 8, 2021

Choose a reason for hiding this comment

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

So you would prefer to still convert network type per device? Instead of to convert bonds?

The thought here is that users (Terraform) will prefer ConvertDeviceBond. For backward compatibility, ConvertDevice can leverage ConvertDeviceBond to do the work. We can deprecate it independently of the refactor.

If we keep ConvertDevice it would have to result in the bonds and ports being in the same state they used to.
It may be easier to deprecate it (or remove it now).

What do you think?

Since the configuration for n2.xlarge is so inflexible (or special), we could treat it as a special case and create a separate function for it. Also we could create a separate tf resource.

I don't think users should have to choose different resource type based on device plan. I do think that they should consult documentation and use/choose attribute values that apply to the bond that they are configuring.

We run into problems when we try to bake logic into Terraform and Packngo to do more than what the API strictly offers.

If we remove the magic of device port name lookups and network type translation, a user could have a more powerful interface that shouldn't need much revisiting in the future.

resource "metal_port" "server-bond0" {
  port_id = metal_device.foo.network_ports.bond0.id
  bonded = true
  layer2 = false // this is default and represents either layer2 or layer3, leads to /port/id/convert-layer-X call on diff
  // network_type = "hybrid-bonded" // computed, read-only
  virtual_networks = [metal_vlan.foo.id]
  native_virtual_network = metal_vlan.native.id
  ip_address {
    type = "private_ipv4"
    cidr = 30
    // reservation_ids
    // cidr_notation from metal_ip_attachment
  }
}

resource "metal_port" "server-bond1" {
  port_id = metal_device.foo.network_ports.bond1.id
  bonded = true
  layer2 = true // representing the desired /port/id/convert-layer-X state
  // network_type = "layer2-bonded" // computed, read-only
  virtual_networks = [metal_vlan.foo.id]
}

The default of layer2=false would mean that layer3 is applied, which is the API default and is the most permissive (allowing for both IP addresses and VLANs in IBX facilities, thanks to hybrid-bonded).

The API endpoint has a hyphen (.../convert/layer-2, .../convert/layer-3), the facility Layer2 feature uses underscore (layer_2), and the network_type returned by bond devices uses no hyphens (layer3). I think we can follow the network_type naming pattern for this field without much confusion.

If the user sets invalid combinations, they will receive an API error and can adjust accordingly. Terraform doesn't have to validate the user input and does not need to make assumptions about what values can be used.

We can try adding validation, but I fear we will run into more problems than we avoid by doing so.

Considering all this, I think we should define possible (useful) configurations for the n2.xlarge, and just treat it differently. I don't think that we should map the n2.xlarge confs to the one-bond types.

I can add that in the future we may have 8 port resources, for example, that are neither "metal_device" nor "metal_hardware_reservation" resources. We'll benefit from treating ports as independent resources.

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 thought here is that users (Terraform) will prefer ConvertDeviceBond. For backward compatibility, ConvertDevice can leverage ConvertDeviceBond to do the work. We can deprecate it independently of the refactor.

sound good

If we remove the magic of device port name lookups and network type translation, a user could have a more powerful interface that shouldn't need much revisiting in the future.

powerful indeed, but not simple. A lot of docs will be needed to map it to the states from the UI. But it's probably a way to go if there are more resources with ports coming.

The API endpoint has a hyphen (.../convert/layer-2, .../convert/layer-3), the facility Layer2 feature uses underscore (layer_2), and the network_type returned by bond devices uses no hyphens (layer3). I think we can follow the network_type naming pattern for this field without much confusion.

Yes, "layer{2,3}" for naming is ok. Just it's not straightforward to find out the current l2/l3 state (for the sake of the /convert/ calls) from the network_type attr, but it can be done. I think it's

  • layer-3: layer3, hybrid, hybrid-bonded
  • layer-2: network type can be layer2-individual, layer2-bonded

This is sth that's not in the UI.

bondPort, err := d.GetPortByName(bondPortName)
if err != nil {
return err
}

if targetType == NetworkTypeL3 || targetType == NetworkTypeHybridBonded {
_, _, err := i.Bond(bondPort, false)
if err != nil {
return err
}
_, _, err = i.PortToLayerThree(d.ID, bondPortName)
if err != nil {
return err
}
// device needs to be refreshed, the bond and convert calls might bond eths
d, _, err := i.client.Devices.Get(d.ID, nil)
if err != nil {
return err
}
for _, p := range d.GetEthPortsInBond(bondPortName) {
_, _, err := i.Bond(p, false)
if err != nil {
return err
}
}
}
if targetType == NetworkTypeHybrid {
_, _, err := i.Bond(bondPort, false)
if err != nil {
return err
}
_, _, err = i.PortToLayerThree(d.ID, bondPortName)
if err != nil {
return err
}

// device needs to be refreshed, the bond and convert calls might bond eths
d, _, err := i.client.Devices.Get(d.ID, nil)
if err != nil {
return err
}
ethLatter := d.GetEthPortsInBond(bondPortName)[1]

_, _, err = i.Disbond(ethLatter, false)
if err != nil {
return err
}
}
if targetType == NetworkTypeL2Individual {
_, _, err := i.PortToLayerTwo(d.ID, bondPortName)
if err != nil {
return err
}
// device needs to be refreshed, the convert call might break the bond
d, _, err := i.client.Devices.Get(d.ID, nil)
if err != nil {
return err
}
bondPort, err := d.GetPortByName(bondPortName)
if err != nil {
return err
}
_, _, err = i.Disbond(bondPort, true)
if err != nil {
return err
}
}
if targetType == NetworkTypeL2Bonded {
_, _, err := i.PortToLayerTwo(d.ID, bondPortName)
if err != nil {
return err
}
// device needs to be refreshed, the convert call might break the bond
d, _, err := i.client.Devices.Get(d.ID, nil)
if err != nil {
return err
}
for _, p := range d.GetEthPortsInBond(bondPortName) {
_, _, err := i.Bond(p, false)
if err != nil {
return err
}
}
}
return nil
}
137 changes: 137 additions & 0 deletions bond_ports_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
package packngo

import (
"log"
"testing"
"time"
)

func deviceBondToNetworkType(t *testing.T, c *Client, deviceID, bondPortName, targetNetworkType string) {
d, _, err := c.Devices.Get(deviceID, nil)
if err != nil {
t.Fatal(err)
}
oldType := d.GetBondNetworkType(bondPortName)
log.Println("Converting", bondPortName, oldType, "=>", targetNetworkType, "...")
_, err = c.DevicePorts.BondToNetworkType(deviceID, bondPortName, targetNetworkType)
if err != nil {
t.Fatal(err)
}
log.Println(oldType, "=>", targetNetworkType, "OK")
// why sleep here??
time.Sleep(5 * time.Second)
}

func TestAccPortBondNetworkStateTransitions(t *testing.T) {
skipUnlessAcceptanceTestsAllowed(t)
t.Parallel()
c, projectID, teardown := setupWithProject(t)
defer teardown()

fac := testFacility()

cr := DeviceCreateRequest{
Hostname: "bondNetworkTypeTest",
Facility: []string{fac},
Plan: "c3.small.x86",
OS: testOS,
ProjectID: projectID,
BillingCycle: "hourly",
}
d, _, err := c.Devices.Create(&cr)
if err != nil {
t.Fatal(err)
}
defer deleteDevice(t, c, d.ID, false)
deviceID := d.ID

d = waitDeviceActive(t, c, deviceID)

bond := "bond0"

networkType := d.GetBondNetworkType(bond)
if networkType != NetworkTypeL3 {
t.Fatal("network_type should be 'layer3'")
}

if networkType != NetworkTypeL2Bonded {
deviceBondToNetworkType(t, c, deviceID, bond, NetworkTypeL2Bonded)
}

deviceBondToNetworkType(t, c, deviceID, bond, NetworkTypeL2Individual)
}

func TestAccPortBondNetworkStateHybridBonded(t *testing.T) {
skipUnlessAcceptanceTestsAllowed(t)
t.Parallel()
c, projectID, teardown := setupWithProject(t)
defer teardown()

fac := testFacility()

cr := DeviceCreateRequest{
Hostname: "NetworkTypeHybridBondedTest",
Facility: []string{fac},
Plan: "c3.small.x86",
OS: testOS,
ProjectID: projectID,
BillingCycle: "hourly",
}
d, _, err := c.Devices.Create(&cr)
if err != nil {
t.Fatal(err)
}
defer deleteDevice(t, c, d.ID, false)
deviceID := d.ID

d = waitDeviceActive(t, c, deviceID)

bond := "bond0"

networkType := d.GetBondNetworkType(bond)
if networkType != NetworkTypeL3 {
t.Fatal("network_type should be 'layer3'")
}

testDesc := "test_desc_" + randString8()

vncr := VirtualNetworkCreateRequest{
ProjectID: projectID,
Description: testDesc,
Facility: testFacility(),
}

vlan, _, err := c.ProjectVirtualNetworks.Create(&vncr)
if err != nil {
t.Fatal(err)
}
defer deleteProjectVirtualNetwork(t, c, vlan.ID)

bondPort, _ := d.GetPortByName(bond)

par := PortAssignRequest{
PortID: bondPort.ID,
VirtualNetworkID: vlan.ID}

p, _, err := c.DevicePorts.Assign(&par)
if err != nil {
t.Fatal(err)
}

log.Printf("%#v\n", p)

d, _, err = c.Devices.Get(d.ID, nil)
if err != nil {
t.Fatal(err)
}
newBondNetworkType := d.GetBondNetworkType(bond)
if newBondNetworkType != NetworkTypeHybridBonded {
t.Fatalf("After bond0 is in L3 and VLAN is assigned to bond0, it's network type should be %s. Was %s", NetworkTypeHybridBonded, newBondNetworkType)
}

_, _, err = c.DevicePorts.Unassign(&par)
if err != nil {
t.Fatal(err)
}

}
2 changes: 2 additions & 0 deletions ports.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ type DevicePortService interface {
GetOddEthPorts(*Device) (map[string]*Port, error)
GetAllEthPorts(*Device) (map[string]*Port, error)
ConvertDevice(*Device, string) error
BondToNetworkType(string, string, string) (*Device, error)
ConvertDeviceBond(*Device, string, string) error
}

type PortData struct {
Expand Down