Skip to content

Commit

Permalink
feat: better link update handling for vxlan
Browse files Browse the repository at this point in the history
  • Loading branch information
@carlmontanari
carlmontanari committed Mar 28, 2024
1 parent b91c0c1 commit 6e49f07
Show file tree
Hide file tree
Showing 9 changed files with 413 additions and 88 deletions.
21 changes: 12 additions & 9 deletions controllers/topology/deployment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1023,24 +1023,19 @@ func (r *DeploymentReconciler) Conforms( //nolint: gocyclo
// rendered sub-topologies) and updates the reconcile data NodesNeedingReboot set with each node
// that needs restarting due to configuration changes.
func (r *DeploymentReconciler) DetermineNodesNeedingRestart(
owningTopology *clabernetesapisv1alpha1.Topology,
reconcileData *ReconcileData,
) {
for nodeName, nodeConfig := range reconcileData.ResolvedConfigs {
for nodeName := range reconcileData.ResolvedConfigs {
_, nodeExistedBefore := reconcileData.PreviousConfigs[nodeName]
if !nodeExistedBefore {
continue
}

if owningTopology.Spec.Connectivity == clabernetesconstants.ConnectivitySlurpeeth {
determineNodeNeedsRestartSlurpeeth(reconcileData, nodeName)
} else if !reflect.DeepEqual(nodeConfig, reconcileData.PreviousConfigs[nodeName]) {
reconcileData.NodesNeedingReboot.Add(nodeName)
}
determineNodeNeedsRestart(reconcileData, nodeName)
}
}

func determineNodeNeedsRestartSlurpeeth(
func determineNodeNeedsRestart(
reconcileData *ReconcileData,
nodeName string,
) {
Expand Down Expand Up @@ -1089,14 +1084,22 @@ func determineNodeNeedsRestartSlurpeeth(
return
}

if len(previousConfig.Topology.Links) != len(currentConfig.Topology.Links) {
// dont bother checking links since they cant be same/same, node needs rebooted to restart
// clab bits
reconcileData.NodesNeedingReboot.Add(nodeName)

return
}

// we know (because we set this) that topology will never be nil and links will always be slices
// that are len 2... so we are a little risky here but its probably ok :)
for idx := range previousConfig.Topology.Links {
previousASide := previousConfig.Topology.Links[idx].Endpoints[0]
currentASide := currentConfig.Topology.Links[idx].Endpoints[0]

if previousASide == currentASide {
// as long as "a" side is the same, slurpeeth will auto update itself since launcher is
// as long as "a" side is the same, things will auto update itself since launcher is
// watching the connectivity cr
continue
}
Expand Down
1 change: 0 additions & 1 deletion controllers/topology/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,6 @@ func (r *Reconciler) reconcileDeploymentsHandleRestarts(
r.Log.Debug("determining nodes needing restart")

r.DeploymentReconciler.DetermineNodesNeedingRestart(
owningTopology,
reconcileData,
)

Expand Down
54 changes: 54 additions & 0 deletions controllers/topology/test-fixtures/golden/tunnels/allocate-tunnel-ids/updating-tunnels.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
{
"srl1": [
{
"tunnelID": 1,
"destination": "topo-1-srl2.clabernetes.svc.cluster.local",
"localNode": "srl1",
"localInterface": "e1-1",
"remoteNode": "srl2",
"remoteInterface": "e1-3"
},
{
"tunnelID": 2,
"destination": "topo-1-srl2.clabernetes.svc.cluster.local",
"localNode": "srl1",
"localInterface": "e1-2",
"remoteNode": "srl2",
"remoteInterface": "e1-1"
},
{
"tunnelID": 3,
"destination": "topo-1-srl2.clabernetes.svc.cluster.local",
"localNode": "srl1",
"localInterface": "e1-3",
"remoteNode": "srl2",
"remoteInterface": "e1-2"
}
],
"srl2": [
{
"tunnelID": 1,
"destination": "topo-1-srl1.clabernetes.svc.cluster.local",
"localNode": "srl2",
"localInterface": "e1-3",
"remoteNode": "srl1",
"remoteInterface": "e1-1"
},
{
"tunnelID": 2,
"destination": "topo-1-srl1.clabernetes.svc.cluster.local",
"localNode": "srl2",
"localInterface": "e1-1",
"remoteNode": "srl1",
"remoteInterface": "e1-2"
},
{
"tunnelID": 3,
"destination": "topo-1-srl1.clabernetes.svc.cluster.local",
"localNode": "srl2",
"localInterface": "e1-2",
"remoteNode": "srl1",
"remoteInterface": "e1-3"
}
]
}
5 changes: 3 additions & 2 deletions controllers/topology/tunnels.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
// given status object by iterating over the freshly processed tunnels (as processed during a
// reconciliation) and assigning any tunnels in the status without a vnid the next valid vnid.
func AllocateTunnelIDs(
statusTunnels map[string][]*clabernetesapisv1alpha1.PointToPointTunnel,
previousTunnels map[string][]*clabernetesapisv1alpha1.PointToPointTunnel,
processedTunnels map[string][]*clabernetesapisv1alpha1.PointToPointTunnel,
) {
// we want to allocate ids deterministically, so lets iterate over the maps in *order* by
Expand All @@ -33,14 +33,15 @@ func AllocateTunnelIDs(
allocatedTunnelIDs := make(map[int]bool)

for nodeName, nodeTunnels := range processedTunnels {
existingNodeTunnels, ok := statusTunnels[nodeName]
existingNodeTunnels, ok := previousTunnels[nodeName]
if !ok {
continue
}

for _, newTunnel := range nodeTunnels {
for _, existingTunnel := range existingNodeTunnels {
if newTunnel.LocalInterface == existingTunnel.LocalInterface &&
newTunnel.RemoteInterface == existingTunnel.RemoteInterface &&
newTunnel.RemoteNode == existingTunnel.RemoteNode {
newTunnel.TunnelID = existingTunnel.TunnelID

Expand Down
134 changes: 121 additions & 13 deletions controllers/topology/tunnels_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package topology_test
import (
"encoding/json"
"fmt"
"reflect"
"testing"

clabernetesapisv1alpha1 "github.com/srl-labs/clabernetes/apis/v1alpha1"
Expand All @@ -20,12 +19,12 @@ const testAllocateTunnelIDsTestName = "tunnels/allocate-tunnel-ids"
func TestAllocateTunnelIds(t *testing.T) {
cases := []struct {
name string
statusTunnels map[string][]*clabernetesapisv1alpha1.PointToPointTunnel
previousTunnels map[string][]*clabernetesapisv1alpha1.PointToPointTunnel
processedTunnels map[string][]*clabernetesapisv1alpha1.PointToPointTunnel
}{
{
name: "simple",
statusTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{},
name: "simple",
previousTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{},
processedTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
Expand All @@ -51,7 +50,7 @@ func TestAllocateTunnelIds(t *testing.T) {
},
{
name: "simple-existing-status",
statusTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
previousTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
TunnelID: 0,
Expand Down Expand Up @@ -98,7 +97,7 @@ func TestAllocateTunnelIds(t *testing.T) {
},
{
name: "simple-already-allocated-ids",
statusTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
previousTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
TunnelID: 1,
Expand Down Expand Up @@ -145,7 +144,7 @@ func TestAllocateTunnelIds(t *testing.T) {
},
{
name: "simple-weirdly-allocated-ids",
statusTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
previousTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
TunnelID: 0,
Expand Down Expand Up @@ -191,8 +190,8 @@ func TestAllocateTunnelIds(t *testing.T) {
},
},
{
name: "meshy-links",
statusTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{},
name: "meshy-links",
previousTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{},
processedTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
Expand Down Expand Up @@ -284,14 +283,125 @@ func TestAllocateTunnelIds(t *testing.T) {
},
},
},
{
name: "updating-tunnels",
previousTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
TunnelID: 1,
LocalNode: "srl1",
Destination: "topo-1-srl2.clabernetes.svc.cluster.local",
RemoteNode: "srl2",
LocalInterface: "e1-1",
RemoteInterface: "e1-1",
},
{
TunnelID: 2,
LocalNode: "srl1",
Destination: "topo-1-srl2.clabernetes.svc.cluster.local",
RemoteNode: "srl2",
LocalInterface: "e1-2",
RemoteInterface: "e1-2",
},
{
TunnelID: 3,
LocalNode: "srl1",
Destination: "topo-1-srl2.clabernetes.svc.cluster.local",
RemoteNode: "srl2",
LocalInterface: "e1-3",
RemoteInterface: "e1-3",
},
},
"srl2": {
{
TunnelID: 1,
LocalNode: "srl2",
Destination: "topo-1-srl1.clabernetes.svc.cluster.local",
RemoteNode: "srl1",
LocalInterface: "e1-1",
RemoteInterface: "e1-1",
},
{
TunnelID: 2,
LocalNode: "srl2",
Destination: "topo-1-srl1.clabernetes.svc.cluster.local",
RemoteNode: "srl1",
LocalInterface: "e1-2",
RemoteInterface: "e1-2",
},
{
TunnelID: 3,
LocalNode: "srl2",
Destination: "topo-1-srl1.clabernetes.svc.cluster.local",
RemoteNode: "srl1",
LocalInterface: "e1-3",
RemoteInterface: "e1-3",
},
},
},
processedTunnels: map[string][]*clabernetesapisv1alpha1.PointToPointTunnel{
"srl1": {
{
TunnelID: 0,
LocalNode: "srl1",
Destination: "topo-1-srl2.clabernetes.svc.cluster.local",
RemoteNode: "srl2",
LocalInterface: "e1-1",
RemoteInterface: "e1-3",
},
{
TunnelID: 0,
LocalNode: "srl1",
Destination: "topo-1-srl2.clabernetes.svc.cluster.local",
RemoteNode: "srl2",
LocalInterface: "e1-2",
RemoteInterface: "e1-1",
},
{
TunnelID: 0,
LocalNode: "srl1",
Destination: "topo-1-srl2.clabernetes.svc.cluster.local",
RemoteNode: "srl2",
LocalInterface: "e1-3",
RemoteInterface: "e1-2",
},
},
"srl2": {
{
TunnelID: 0,
LocalNode: "srl2",
Destination: "topo-1-srl1.clabernetes.svc.cluster.local",
RemoteNode: "srl1",
LocalInterface: "e1-3",
RemoteInterface: "e1-1",
},
{
TunnelID: 0,
LocalNode: "srl2",
Destination: "topo-1-srl1.clabernetes.svc.cluster.local",
RemoteNode: "srl1",
LocalInterface: "e1-1",
RemoteInterface: "e1-2",
},
{
TunnelID: 0,
LocalNode: "srl2",
Destination: "topo-1-srl1.clabernetes.svc.cluster.local",
RemoteNode: "srl1",
LocalInterface: "e1-2",
RemoteInterface: "e1-3",
},
},
},
},
}

for _, testCase := range cases {
t.Run(
testCase.name,
func(t *testing.T) {
clabernetescontrollerstopology.AllocateTunnelIDs(
testCase.statusTunnels,
testCase.previousTunnels,
testCase.processedTunnels,
)

Expand Down Expand Up @@ -326,9 +436,7 @@ func TestAllocateTunnelIds(t *testing.T) {
t.Fatal(err)
}

if !reflect.DeepEqual(got, want) {
clabernetestesthelper.FailOutput(t, got, want)
}
clabernetestesthelper.MarshaledEqual(t, got, want)
},
)
}
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
github.com/carlmontanari/difflibgo v0.0.0-20231101235608-20f26fe20f37 h1:1cclCPimXRdSnaB1TaRf0N6cTyqqBxHa5K/CUpegsWY=
github.com/carlmontanari/difflibgo v0.0.0-20231101235608-20f26fe20f37/go.mod h1:+3MuSIeC3qmdSesR12cTLeb47R/Vvo+bHdB6hC5HShk=
github.com/carlmontanari/difflibgo v0.0.0-20240227210139-93685b1c22ae h1:h4sxL/AXg3FRPf+sT2Y4daEQQE/UAkNAM3U0t4Cgha8=
github.com/carlmontanari/difflibgo v0.0.0-20240227210139-93685b1c22ae/go.mod h1:+3MuSIeC3qmdSesR12cTLeb47R/Vvo+bHdB6hC5HShk=
github.com/carlmontanari/slurpeeth v0.0.0-20240209224827-246fa87e31f3 h1:+XFqJFGVs1EVP5GajTTDB91tTKG+mF0jlN1U1jT90tI=
Expand Down
Loading

0 comments on commit 6e49f07

Please sign in to comment.