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.

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

Implement hybrid-bonded network type #231

Open
displague opened this issue Dec 16, 2020 · 16 comments
Open

Implement hybrid-bonded network type #231

displague opened this issue Dec 16, 2020 · 16 comments

Comments

@displague
Copy link
Member

The feature initially described at https://feedback.equinixmetal.com/networking-features/p/mixed-mode-layer-23-for-hybrid-cloud is coming to the Equinix Metal API.

This allows for Layer3 addresses and Layer 2 VLANs features to be simultaneously configured on each bonded port.

This feature allows for the default Layer3 bonded network type to immediately attach Layer2 VLANs without explicitly changing Network Type first. When in effect, the network_type will report hybrid-bonded.

Simply put, you will be able to POST /ports/{id}/assign {"vnid":"1234"} on any newly created device.

Attached VLANs can be found through existing API endpoints (device network_ports[*].virtual_networks[*].vxlan).

Only after all VLANs are removed from a port (using POST /ports/{id}/unassign {"vnid": "1234"}), the network type and bonding for that port may be changed to one that does not support Layer2.

This feature is only available for devices in ibx facilities (facilities with ibx included in the API's facility features property). Users in other facilities will receive the error: {"errors":["Can't assign vlan, the port is configured for Layer 3"]}

@displague
Copy link
Member Author

displague commented Dec 16, 2020

The context provided here is to support the implementation and downstream implementations.

Validation should be left to the API service, packngo should not try to verify if the current state meets any level of criteria.

@t0mk
Copy link
Contributor

t0mk commented Dec 18, 2020

@displague can you please check out the code in the portal javascript where the network type transitions are happening, and paste it here?
So that we don't have to guess the api calls for the new network type transition.

@displague
Copy link
Member Author

displague commented Dec 19, 2020

@t0mk That had not crossed my mind. We did benefit from console.equinix.com front-end Javascript in #185 and it would be great to sync that again here.

The UI code doesn't exist yet.

I don't know if we can continue to think of a device as being in a single-mode. This is something I'll check into.
(what would the device's single computed network type be, when ports 1/3 are hybrid-bonded, and ports 2/4 in layer2-bonded? If all ports must be hybrid-bonded, then this is just one extra mode to compute and we could detect it as "all ports have type=hybrid-bonded".)

@displague
Copy link
Member Author

I checked with another internal composition of this field.
The new hybrid-bonded computed value has these requirements:

  • all ports are bonded
  • it has any ip management addresses
  • it has virtual_networks.

This is the same standard used to evaluate hybrid mode, with the added criteria that there must be virtual_networks. We'll need a HasVirtualNetworks like

packngo/devices.go

Lines 176 to 183 in a38fa6e

func (d *Device) HasManagementIPs() bool {
for _, ip := range d.Network {
if ip.Management {
return true
}
}
return false
}

So in GetNetworkType,

	if bonds.allBonded() {
		if phys.allBonded() {
			if !d.HasManagementIPs() {
				return NetworkTypeL2Bonded
			}
			// <---- This condition is new
			if d.HasVirtualNetworks() {
				return NetworkTypeHybridBonded 
			} 
			// ---->
			return NetworkTypeL3
		}
		return NetworkTypeHybrid
	}
	return NetworkTypeL2Individual

@displague
Copy link
Member Author

Transitioning to Hybrid-Bonded, would not be possible without a VLAN to attach, but it would otherwise be the same as transitioning to Layer3 Bonded.

@displague
Copy link
Member Author

displague commented Dec 19, 2020

Thinking about how this might work in Terraform, I think if someone asks for "hybrid-bonded", we may have to ignore "layer3-bonded", because it is the same but they don't have any virtual networks attached (which they would have to add as a manage through a separate resource).

Part of the reason that this is more complicated in Terraform is that we are repurposing these computed terms, we are trying to apply them to the machine instead of just offering a direct terraform interface to the ports and bonds:

resource "metal_device" "" {
  network_ports:
    bonds: 
      ...
    ports
      ...
    ip_addresses: [] 

Functions like IP attachments would be detected through changes to the device state. I haven't really thought this through and I'm sure there are more complexities.

@t0mk
Copy link
Contributor

t0mk commented Dec 20, 2020

@displague from what you've wrote it looks that this is more of a API internal functionality. IIUC, from the API client point of view, the hybrid-bonded network type is only a label for a layer3 network type with VLANs attached. Do we really need to do anything but alias hybrid-bonded to layer3? Or maybe just use layer3 and attach VLANs. VLAN assignment can already be done from Packngo, and also from the TF provider.

I don't see the benefit of having additional network type (with a parameter!) just to indicate if VLANs are assigned. I'd rather do only the HasVirtualNetworks as you suggested, and expose it via read-only attr in metal_device in the TF provider.

Will there be some special API call to transition bond port to hybrid-bonded (akin to /ports/<id>/convert/layer2)?

@displague
Copy link
Member Author

Or maybe just use layer3 and attach VLANs. VLAN assignment can already be done from Packngo, and also from the TF provider.

In this TF case we would have to treat a computed value of hybrid-bonded as matching type = "layer3", but then what happens when a TF user wants to change to layer3 from hybrid-bonded, we wouldn't know that the user wants us to remove the attached vlans. And if we do remove the vlans, and the TF config has vlans defined, we'll end up in a weird diff jitter (you want layer3, but you have vlan attachments, one of these will win and the other will lose on each apply).

Do we really need to do anything but alias hybrid-bonded to layer3?

For https://github.com/packethost/packngo/blob/master/ports.go#L225 (ConvertDevice),
we should consider what should or could be done with a request to transition to this hybrid-bonded. Should we add new parameters to this function or should we create a new function that takes vlan parameters? Do we auto-create a vlan? Do we not expose this type as a conversion target (and respond with an error)?

Will there be some special API call to transition bond port to hybrid-bonded (akin to /ports//convert/layer2)?

There are no additional API endpoints being exposed.

@alice-sowerby
Copy link

alice-sowerby commented Jan 25, 2021

This has been identified in the Jira as DEVREL-957

@t0mk
Copy link
Contributor

t0mk commented Jan 26, 2021

I have looked at the new L2 network UI and I noticed a couple of things that @displague didn't mention in the first comment #231 (comment)

  • The network type is now defined per bond, and not per device. This is a good thing, considering how we struggled with defining type for n2.xlarge. But it will need a fundamental change in packngo and the provider. Especially for the TF provider there will have to be a period when the old network type handling will be deprecating (but should still work). Is this even feasible when the API will change? Is there some action we could make in the provider release now, to make the transition smoother. Maybe make a release deprecating the network type per device.
  • Layer2 modes are now "bonded" and "unbonded". The label of the L2 individual mode has changed to "Unbonded".

Last new thing I noticed is the hybrid-bonded mode which needs a VLAN, described in this PR.

@displague
Copy link
Member Author

displague commented Jan 26, 2021

Is this even feasible when the API will change?

AFAIK, the API is not changing other than the new reported mode "hybrid-bonded", which is effectively a Layer3 port (as determined today) with a VLAN attachment.

Is there some action we could make in the provider release now, to make the transition smoother. Maybe make a release deprecating the network type per device.

Maybe we should create new ConvertPortPair helpers (maybe helpers/port/port.ConvertGroup) and deprecate the ConvertDevice helpers (which would stay very close to what they are today, with hybrid-bonded awareness added).

Or perhaps, ConvertDevice((d *Device, targetType ...string) where the targetTypes map to each port group. For a device with multiple port groups, we could keep the current behavior when only one targetType was provided.

Layer2 modes are now "bonded" and "unbonded". The label of the L2 individual mode has changed to "Unbonded".

The API should still be reporting the same keywords it was returning before. The UI has changed the names, but the API has not. We don't have to change this in Terraform or packngo (but we could note the alternate name in the field descriptions).

@displague
Copy link
Member Author

@t0mk supporting documentation for this feature is now available at https://metal.equinix.com/developers/docs/layer2-networking/hybrid-bonded-mode/

@t0mk
Copy link
Contributor

t0mk commented Jan 30, 2021

I spent a day last week looking on how to start wtih the hybrid-bonded state. First thing was that I had to create a struct for the network state:

type NetworkTypeName string

const (
       NetworkTypeHybrid       NetworkTypeName = "hybrid"
       NetworkTypeL2Bonded     NetworkTypeName = "layer2-bonded"
       NetworkTypeL2Individual NetworkTypeName = "layer2-individual"
       NetworkTypeL3           NetworkTypeName = "layer3"
)

type NetworkType struct {
       Name             NetworkTypeName
       VirtualNetworkID *string
}

.. and the functions handling the state should be changed according to that.

@thebsdbox
Copy link
Contributor

Excellent work @t0mk! Are there any ETAs for when work will commence on this?

@t0mk
Copy link
Contributor

t0mk commented Feb 1, 2021

Hey @thebsdbox, we still need to agree with out with @displague. We struggle with how to represent the network modes here in packngo and then in the provider. It's a bit complex, so it would first be good to sketch how we will do it.

Since there might be changes in the network type specification for the TF device resource, we also need to agree if there's anything we can do right now in the TF provider to make it smooth to transition.

@displague
Copy link
Member Author

@t0mk Perhaps we should take this one-line https://github.com/packethost/packngo/pull/239/files#diff-e9a0f6f206a7d5bf4e4fb40ffcdf7089c3223878283915d8d03f2175727515dcR5 (adding the hybrid-bonded state) and make an enum of it for type PortNetworkType string.

Here's a snippet of the packngo JSON serialization for reference:

network_ports:
- data:
    bonded: true
  disbond_operation_supported: true
  id: 5d4df7e8-2b10-4919-addf-93a3514bbaf9
  name: bond0
  network_type: hybrid-bonded
  type: NetworkBondPort
  virtual_networks:
  - href: /metal/v1/virtual-networks/f414c93a-51eb-461b-978a-f6fb4952580a
    id: ""
- bond:
    id: 5d4df7e8-2b10-4919-addf-93a3514bbaf9
    name: bond0
  data:
    bonded: true
    mac: 0c:42:a1:7e:a2:20
  disbond_operation_supported: true
  id: a0e0150f-ca63-484e-8112-5337c94273ed
  name: eth0
  type: NetworkPort
- bond:
    id: 5d4df7e8-2b10-4919-addf-93a3514bbaf9
    name: bond0
  data:
    bonded: true
    mac: 0c:42:a1:7e:a2:21
  disbond_operation_supported: true
  id: 9a70a714-2ed8-44c6-86c4-820bd811ccab
  name: eth1
  type: NetworkPort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants