[DRAFT] Handle failing reloader fixed crash

cmd/internal/core/reconfigure-switch.go

@@ -45,7 +45,11 @@ func (c *Core) ReconfigureSwitch() {
 		}
 
 		// fill the port states of the switch
-		for _, n := range s.Nics {
+		var nics []*models.V1SwitchNic
+		if s != nil {
+			nics = s.Nics
+		}
+		for _, n := range nics {
 			if n == nil || n.Name == nil {
 				// lets log the whole nic because the name could be empty; lets hope there is some useful information
 				// in the nic

cmd/internal/switcher/cumulus/applier.go

@@ -6,41 +6,37 @@ import (
 )
 
 const (
-	frr                  = "/etc/frr/frr.conf"
-	frrTmp               = "/etc/frr/frr.tmp"
+	frrConfFile          = "/etc/frr/frr.conf"
 	frrReloadService     = "frr.service"
 	frrValidationService = "frr-validation"
 
-	interfaces                  = "/etc/network/interfaces"
-	interfacesTmp               = "/etc/network/interfaces.tmp"
+	interfacesConfFile          = "/etc/network/interfaces"
 	interfacesReloadService     = "ifreload.service"
 	interfacesValidationService = "interfaces-validation"
 )
 
 func NewInterfacesApplier(tplPath string) *templates.Applier {
 	return &templates.Applier{
-		Dest:              interfaces,
+		Dest:              interfacesConfFile,
 		Reloader:          reloadInterfaces,
-		Tmp:               interfacesTmp,
 		Tpl:               templates.InterfacesTemplate(tplPath),
 		ValidationService: interfacesValidationService,
 	}
 }
 
 func NewFrrApplier(tplPath string) *templates.Applier {
 	return &templates.Applier{
-		Dest:              frr,
+		Dest:              frrConfFile,
 		Reloader:          reloadFrr,
-		Tmp:               frrTmp,
 		Tpl:               templates.CumulusFrrTemplate(tplPath),
 		ValidationService: frrValidationService,
 	}
 }
 
-func reloadInterfaces() error {
+func reloadInterfaces(_ string) error {
 	return dbus.Start(interfacesReloadService)
 }
 
-func reloadFrr() error {
+func reloadFrr(_ string) error {
 	return dbus.Reload(frrReloadService)
 }

cmd/internal/switcher/sonic/frr.go

@@ -1,27 +1,48 @@
 package sonic
 
 import (
+	"errors"
+	"fmt"
+	"os"
+
 	"github.com/metal-stack/metal-core/cmd/internal/dbus"
 	"github.com/metal-stack/metal-core/cmd/internal/switcher/templates"
 )
 
 const (
-	frr                  = "/etc/sonic/frr/frr.conf"
-	frrTmp               = "/etc/sonic/frr/frr.tmp"
+	frrConfFile          = "/etc/sonic/frr/frr.conf"
 	frrReloadService     = "frr-reload.service"
 	frrValidationService = "bgp-validation"
 )
 
 func NewFrrApplier(tplPath string) *templates.Applier {
 	return &templates.Applier{
-		Dest:              frr,
+		Dest:              frrConfFile,
 		Reloader:          reloadFrr,
-		Tmp:               frrTmp,
 		Tpl:               templates.SonicFrrTemplate(tplPath),
 		ValidationService: frrValidationService,
 	}
 }
 
-func reloadFrr() error {
-	return dbus.Start(frrReloadService)
+func reloadFrr(previousConf string) error {
+	err := dbus.Start(frrReloadService)
+	if err == nil {
+		return nil
+	}
+
+	errs := []error{fmt.Errorf("reloading %s failed: %w", frrReloadService, err)}
+
+	if previousConf != "" {
+		err = os.Rename(previousConf, frrConfFile)
+		if err == nil {
+			return errors.Join(errs...)
+		}
+		errs = append(errs, fmt.Errorf("could not restore %s from %s: %w", frrConfFile, previousConf, err))
+	}
+
+	err = os.Remove(frrConfFile)
+	if err != nil {
+		errs = append(errs, fmt.Errorf("failed to remove %s: %w", frrConfFile, err))
+	}
+	return errors.Join(errs...)
 }

cmd/internal/switcher/templates/applier.go

@@ -3,6 +3,7 @@ package templates
 import (
 	"bytes"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -14,40 +15,40 @@ import (
 	"github.com/metal-stack/metal-core/cmd/internal/switcher/types"
 )
 
-type Reloader func() error
+type Reloader func(previousConf string) error
 
 type Applier struct {
 	Dest              string
 	Reloader          Reloader
-	Tmp               string
 	Tpl               *template.Template
 	ValidationService string
 }
 
 func (a *Applier) Apply(c *types.Conf) error {
-	err := write(c, a.Tpl, a.Tmp)
+	tmp := fmt.Sprintf("%s.tmp", a.Dest)
+	err := write(c, a.Tpl, tmp)
 	if err != nil {
 		return err
 	}
 
-	equal, err := areEqual(a.Tmp, a.Dest)
+	equal, err := areEqual(tmp, a.Dest)
 	if err != nil {
 		return err
 	}
 	if equal {
-		return os.Remove(a.Tmp)
+		return os.Remove(tmp)
 	}
 
-	err = validate(a.ValidationService, a.Tmp)
+	err = validate(a.ValidationService, tmp)
 	if err != nil {
 		return err
 	}
 
-	err = os.Rename(a.Tmp, a.Dest)
+	previousConf, err := backupAndRename(tmp, a.Dest)
 	if err != nil {
 		return err
 	}
-	return a.Reloader()
+	return a.Reloader(previousConf)
 }
 
 func write(c *types.Conf, tpl *template.Template, tmpPath string) error {
@@ -107,3 +108,22 @@ func checksum(path string) ([]byte, error) {
 
 	return h.Sum(nil), nil
 }
+
+func backupAndRename(src, dest string) (backup string, err error) {
+	destStat, err := os.Stat(dest)
+
+	if errors.Is(err, os.ErrNotExist) {
+		backup = ""
+	} else if err != nil {
+		return "", fmt.Errorf("could not obtain file info %s: %w", dest, err)
+	} else if destStat.Mode().IsRegular() {
+		backup = fmt.Sprintf("%s.bak", dest)
+		if err := os.Rename(dest, backup); err != nil {
+			return "", fmt.Errorf("could not backup file %s: %w", dest, err)
+		}
+	} else {
+		return "", fmt.Errorf("path %s is not a regular file", dest)
+	}
+
+	return backup, os.Rename(src, dest)
+}