From a3e59e4981c4c78c608f7c2f056547dd7bbb4763 Mon Sep 17 00:00:00 2001 From: Stefan Majer Date: Wed, 23 Mar 2022 14:51:14 +0100 Subject: [PATCH 1/4] factor out updateOrCreateSwitch --- .../internal/service/switch-service.go | 114 +++++++++--------- 1 file changed, 59 insertions(+), 55 deletions(-) diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go index 3cbd60d02..c84e3881f 100644 --- a/cmd/metal-api/internal/service/switch-service.go +++ b/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,50 +281,74 @@ 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.updateOrRegisterSwitch(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) updateOrRegisterSwitch(requestPayload v1.SwitchRegisterRequest) (*metal.Switch, int, error) { + returnCode := http.StatusInternalServerError s, err := r.ds.FindSwitch(requestPayload.ID) if err != nil && !metal.IsNotFound(err) { - if checkError(request, response, utils.CurrentFuncName(), err) { - return - } + return nil, http.StatusNotFound, err } - returnCode := http.StatusOK - 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, returnCode, errors.New("duplicate mac addresses found in nics") } err = r.ds.CreateSwitch(s) - if checkError(request, response, utils.CurrentFuncName(), err) { - return + if err != nil { + return nil, returnCode, err } 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 + if err != nil { + return nil, returnCode, err } 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 - } + return nil, returnCode, errors.New("duplicate mac addresses found in nics") } nics, err := updateSwitchNics(old.Nics.ByMac(), spec.Nics.ByMac(), old.MachineConnections) - if checkError(request, response, utils.CurrentFuncName(), err) { - return + if err != nil { + return nil, returnCode, err } if requestPayload.Name != nil { @@ -338,34 +363,13 @@ func (r switchResource) registerSwitch(request *restful.Request, response *restf s.Nics = nics // Do not replace connections here: We do not want to loose them! - 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), - ) - - if checkError(request, response, utils.CurrentFuncName(), err) { - return + err = r.ds.UpdateSwitch(&old, s) + if err != nil { + return nil, returnCode, err } + returnCode = http.StatusOK } - - 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 - } + return s, returnCode, nil } // replaceSwitch replaces a broken switch From c483ae79d2323df15e8d7609338461983ba79678 Mon Sep 17 00:00:00 2001 From: Stefan Majer Date: Wed, 23 Mar 2022 15:08:19 +0100 Subject: [PATCH 2/4] fix --- cmd/metal-api/internal/service/switch-service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go index c84e3881f..9ebe1dbc8 100644 --- a/cmd/metal-api/internal/service/switch-service.go +++ b/cmd/metal-api/internal/service/switch-service.go @@ -316,7 +316,7 @@ func (r switchResource) updateOrRegisterSwitch(requestPayload v1.SwitchRegisterR returnCode := http.StatusInternalServerError s, err := r.ds.FindSwitch(requestPayload.ID) if err != nil && !metal.IsNotFound(err) { - return nil, http.StatusNotFound, err + return nil, returnCode, err } if s == nil { From fbb070e467113ccbff494d80d8e37659d2186552 Mon Sep 17 00:00:00 2001 From: Stefan Majer Date: Wed, 23 Mar 2022 15:09:09 +0100 Subject: [PATCH 3/4] fix --- cmd/metal-api/internal/service/switch-service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go index 9ebe1dbc8..e0905ac94 100644 --- a/cmd/metal-api/internal/service/switch-service.go +++ b/cmd/metal-api/internal/service/switch-service.go @@ -286,7 +286,7 @@ func (r switchResource) registerSwitch(request *restful.Request, response *restf err = retry.Do( func() error { - s, returnCode, err = r.updateOrRegisterSwitch(requestPayload) + s, returnCode, err = r.updateReplaceOrRegisterSwitch(requestPayload) return err }, retry.Attempts(10), @@ -312,7 +312,7 @@ func (r switchResource) registerSwitch(request *restful.Request, response *restf } } -func (r switchResource) updateOrRegisterSwitch(requestPayload v1.SwitchRegisterRequest) (*metal.Switch, int, error) { +func (r switchResource) updateReplaceOrRegisterSwitch(requestPayload v1.SwitchRegisterRequest) (*metal.Switch, int, error) { returnCode := http.StatusInternalServerError s, err := r.ds.FindSwitch(requestPayload.ID) if err != nil && !metal.IsNotFound(err) { From c15d4e6dbef31976370d1bfcc8ee0c1e4838a9ab Mon Sep 17 00:00:00 2001 From: Stefan Majer Date: Thu, 24 Mar 2022 08:21:42 +0100 Subject: [PATCH 4/4] Better readability --- .../internal/service/switch-service.go | 77 ++++++++++--------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go index e0905ac94..29f38abbb 100644 --- a/cmd/metal-api/internal/service/switch-service.go +++ b/cmd/metal-api/internal/service/switch-service.go @@ -313,63 +313,64 @@ func (r switchResource) registerSwitch(request *restful.Request, response *restf } func (r switchResource) updateReplaceOrRegisterSwitch(requestPayload v1.SwitchRegisterRequest) (*metal.Switch, int, error) { - returnCode := http.StatusInternalServerError s, err := r.ds.FindSwitch(requestPayload.ID) if err != nil && !metal.IsNotFound(err) { - return nil, returnCode, err + return nil, http.StatusInternalServerError, err } + // Switch seen for th first time if s == nil { s = v1.NewSwitch(requestPayload) - if len(requestPayload.Nics) != len(s.Nics.ByMac()) { - return nil, returnCode, errors.New("duplicate mac addresses found in nics") + return nil, http.StatusInternalServerError, errors.New("duplicate mac addresses found in nics") } - err = r.ds.CreateSwitch(s) if err != nil { - return nil, returnCode, err + return nil, http.StatusInternalServerError, err } + return s, http.StatusCreated, nil + } - returnCode = http.StatusCreated - } else if s.Mode == metal.SwitchReplace { - spec := v1.NewSwitch(requestPayload) - err = r.replaceSwitch(s, spec) + // 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, returnCode, err - } - s = spec - } else { - old := *s - spec := v1.NewSwitch(requestPayload) - if len(requestPayload.Nics) != len(spec.Nics.ByMac()) { - return nil, returnCode, errors.New("duplicate mac addresses found in nics") + return nil, http.StatusInternalServerError, err } + return newSwitch, http.StatusOK, nil + } - nics, err := updateSwitchNics(old.Nics.ByMac(), spec.Nics.ByMac(), old.MachineConnections) - if err != nil { - return nil, returnCode, err - } + // 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 requestPayload.Name != nil { - s.Name = *requestPayload.Name - } - if requestPayload.Description != nil { - s.Description = *requestPayload.Description - } - s.RackID = spec.RackID - s.PartitionID = spec.PartitionID + nics, err := updateSwitchNics(old.Nics.ByMac(), spec.Nics.ByMac(), old.MachineConnections) + if err != nil { + return nil, http.StatusInternalServerError, err + } - s.Nics = nics - // Do not replace connections here: We do not want to loose them! + 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 = r.ds.UpdateSwitch(&old, s) - if err != nil { - return nil, returnCode, err - } - returnCode = http.StatusOK + s.Nics = nics + // Do not replace connections here: We do not want to loose them! + + err = r.ds.UpdateSwitch(&old, s) + if err != nil { + return nil, http.StatusInternalServerError, err } - return s, returnCode, nil + + return s, http.StatusOK, nil } // replaceSwitch replaces a broken switch