Fix two leftover bugs introduced with #256 take two

cmd/metal-api/internal/service/switch-service.go

@@ -222,22 +222,23 @@ func (r switchResource) updateSwitch(request *restful.Request, response *restful
 		return
 	}
 
-	oldSwitch, err := r.ds.FindSwitch(requestPayload.ID)
-	if checkError(request, response, utils.CurrentFuncName(), err) {
-		return
-	}
+	var newSwitch metal.Switch
+	err = retry.Do(
+		func() error {
+			oldSwitch, err := r.ds.FindSwitch(requestPayload.ID)
+			if err != nil {
+				return err
+			}
 
-	newSwitch := *oldSwitch
+			newSwitch = *oldSwitch
 
-	if requestPayload.Description != nil {
-		newSwitch.Description = *requestPayload.Description
-	}
+			if requestPayload.Description != nil {
+				newSwitch.Description = *requestPayload.Description
+			}
 
-	newSwitch.Mode = metal.SwitchModeFrom(requestPayload.Mode)
+			newSwitch.Mode = metal.SwitchModeFrom(requestPayload.Mode)
 
-	err = retry.Do(
-		func() error {
-			err := r.ds.UpdateSwitch(oldSwitch, &newSwitch)
+			err = r.ds.UpdateSwitch(oldSwitch, &newSwitch)
 			return err
 		},
 		retry.Attempts(10),
@@ -280,92 +281,96 @@ func (r switchResource) registerSwitch(request *restful.Request, response *restf
 	if checkError(request, response, utils.CurrentFuncName(), err) {
 		return
 	}
+	var s *metal.Switch
+	var returnCode int
+
+	err = retry.Do(
+		func() error {
+			s, returnCode, err = r.updateReplaceOrRegisterSwitch(requestPayload)
+			return err
+		},
+		retry.Attempts(10),
+		retry.RetryIf(func(err error) bool {
+			return strings.Contains(err.Error(), datastore.EntityAlreadyModifiedErrorMessage)
+		}),
+		retry.DelayType(retry.CombineDelay(retry.BackOffDelay, retry.RandomDelay)),
+		retry.LastErrorOnly(true),
+	)
+	if checkError(request, response, utils.CurrentFuncName(), err) {
+		return
+	}
+
+	resp, err := makeSwitchResponse(s, r.ds)
+	if checkError(request, response, utils.CurrentFuncName(), err) {
+		return
+	}
 
+	err = response.WriteHeaderAndEntity(returnCode, resp)
+	if err != nil {
+		zapup.MustRootLogger().Error("Failed to send response", zap.Error(err))
+		return
+	}
+}
+
+func (r switchResource) updateReplaceOrRegisterSwitch(requestPayload v1.SwitchRegisterRequest) (*metal.Switch, int, error) {
 	s, err := r.ds.FindSwitch(requestPayload.ID)
 	if err != nil && !metal.IsNotFound(err) {
-		if checkError(request, response, utils.CurrentFuncName(), err) {
-			return
-		}
+		return nil, http.StatusInternalServerError, err
 	}
 
-	returnCode := http.StatusOK
-
+	// Switch seen for th first time
 	if s == nil {
 		s = v1.NewSwitch(requestPayload)
-
 		if len(requestPayload.Nics) != len(s.Nics.ByMac()) {
-			if checkError(request, response, utils.CurrentFuncName(), errors.New("duplicate mac addresses found in nics")) {
-				return
-			}
+			return nil, http.StatusInternalServerError, errors.New("duplicate mac addresses found in nics")
 		}
-
 		err = r.ds.CreateSwitch(s)
-		if checkError(request, response, utils.CurrentFuncName(), err) {
-			return
-		}
-
-		returnCode = http.StatusCreated
-	} else if s.Mode == metal.SwitchReplace {
-		spec := v1.NewSwitch(requestPayload)
-		err = r.replaceSwitch(s, spec)
-		if checkError(request, response, utils.CurrentFuncName(), err) {
-			return
-		}
-		s = spec
-	} else {
-		old := *s
-		spec := v1.NewSwitch(requestPayload)
-		if len(requestPayload.Nics) != len(spec.Nics.ByMac()) {
-			if checkError(request, response, utils.CurrentFuncName(), errors.New("duplicate mac addresses found in nics")) {
-				return
-			}
-		}
-
-		nics, err := updateSwitchNics(old.Nics.ByMac(), spec.Nics.ByMac(), old.MachineConnections)
-		if checkError(request, response, utils.CurrentFuncName(), err) {
-			return
+		if err != nil {
+			return nil, http.StatusInternalServerError, err
 		}
+		return s, http.StatusCreated, nil
+	}
 
-		if requestPayload.Name != nil {
-			s.Name = *requestPayload.Name
-		}
-		if requestPayload.Description != nil {
-			s.Description = *requestPayload.Description
+	// Switch needs replacement because of hw failure for example
+	if s.Mode == metal.SwitchReplace {
+		newSwitch := v1.NewSwitch(requestPayload)
+		err = r.replaceSwitch(s, newSwitch)
+		if err != nil {
+			return nil, http.StatusInternalServerError, err
 		}
-		s.RackID = spec.RackID
-		s.PartitionID = spec.PartitionID
-
-		s.Nics = nics
-		// Do not replace connections here: We do not want to loose them!
+		return newSwitch, http.StatusOK, nil
+	}
 
-		err = retry.Do(
-			func() error {
-				err := r.ds.UpdateSwitch(&old, s)
-				return err
-			},
-			retry.Attempts(10),
-			retry.RetryIf(func(err error) bool {
-				return strings.Contains(err.Error(), datastore.EntityAlreadyModifiedErrorMessage)
-			}),
-			retry.DelayType(retry.CombineDelay(retry.BackOffDelay, retry.RandomDelay)),
-			retry.LastErrorOnly(true),
-		)
+	// Switch has new switchports configured, called on every metal-core restart
+	old := *s
+	spec := v1.NewSwitch(requestPayload)
+	if len(requestPayload.Nics) != len(spec.Nics.ByMac()) {
+		return nil, http.StatusInternalServerError, errors.New("duplicate mac addresses found in nics")
+	}
 
-		if checkError(request, response, utils.CurrentFuncName(), err) {
-			return
-		}
+	nics, err := updateSwitchNics(old.Nics.ByMac(), spec.Nics.ByMac(), old.MachineConnections)
+	if err != nil {
+		return nil, http.StatusInternalServerError, err
 	}
 
-	resp, err := makeSwitchResponse(s, r.ds)
-	if checkError(request, response, utils.CurrentFuncName(), err) {
-		return
+	if requestPayload.Name != nil {
+		s.Name = *requestPayload.Name
+	}
+	if requestPayload.Description != nil {
+		s.Description = *requestPayload.Description
 	}
+	s.RackID = spec.RackID
+	s.PartitionID = spec.PartitionID
 
-	err = response.WriteHeaderAndEntity(returnCode, resp)
+	s.Nics = nics
+	// Do not replace connections here: We do not want to loose them!
+
+	err = r.ds.UpdateSwitch(&old, s)
 	if err != nil {
-		zapup.MustRootLogger().Error("Failed to send response", zap.Error(err))
-		return
+		return nil, http.StatusInternalServerError, err
 	}
+
+	return s, http.StatusOK, nil
 }
 
 // replaceSwitch replaces a broken switch