From 380c0c3ed653f26efac6785a778a5fcb2975a7ea Mon Sep 17 00:00:00 2001 From: James Munson Date: Tue, 17 Oct 2023 15:34:49 -0600 Subject: [PATCH] Suppress nagging error logging. Remove debug logging. Incorporate PR feedback. Change error name per review. Signed-off-by: James Munson --- controller/setting_controller.go | 2 +- controller/utils.go | 4 ++++ datastore/longhorn.go | 41 +++++++++++++++++++------------- types/types.go | 13 ++++++++++ 4 files changed, 42 insertions(+), 18 deletions(-) diff --git a/controller/setting_controller.go b/controller/setting_controller.go index a8dfbf4d85..5cca7e0a2b 100644 --- a/controller/setting_controller.go +++ b/controller/setting_controller.go @@ -665,7 +665,7 @@ func (sc *SettingController) updateCNI() error { } if !volumesDetached { - return errors.Errorf("failed to apply %v setting to Longhorn workloads when there are attached volumes", types.SettingNameStorageNetwork) + return &types.ErrorInvalidState{Reason: fmt.Sprintf("failed to apply %v setting to Longhorn workloads when there are attached volumes", types.SettingNameStorageNetwork)} } nadAnnot := string(types.CNIAnnotationNetworks) diff --git a/controller/utils.go b/controller/utils.go index 622589eafa..b16bc26a71 100644 --- a/controller/utils.go +++ b/controller/utils.go @@ -61,6 +61,10 @@ func createOrUpdateAttachmentTicket(va *longhorn.VolumeAttachment, ticketID, nod } func handleReconcileErrorLogging(logger logrus.FieldLogger, err error, mesg string) { + if types.ErrorIsInvalidState(err) { + return + } + if apierrors.IsConflict(err) { logger.WithError(err).Debug(mesg) } else { diff --git a/datastore/longhorn.go b/datastore/longhorn.go index 194877298b..c5a38c3bbb 100644 --- a/datastore/longhorn.go +++ b/datastore/longhorn.go @@ -305,24 +305,20 @@ func (s *DataStore) ValidateSetting(name, value string) (err error) { } } case types.SettingNameTaintToleration: - list, err := s.ListVolumesRO() + volumesDetached, err := s.AreAllVolumesDetachedState() if err != nil { return errors.Wrapf(err, "failed to list volumes before modifying toleration setting") } - for _, v := range list { - if v.Status.State != longhorn.VolumeStateDetached { - return fmt.Errorf("cannot modify toleration setting before all volumes are detached") - } + if !volumesDetached { + return &types.ErrorInvalidState{Reason: fmt.Sprintf("cannot modify toleration setting before all volumes are detached")} } case types.SettingNameSystemManagedComponentsNodeSelector: - list, err := s.ListVolumesRO() + volumesDetached, err := s.AreAllVolumesDetachedState() if err != nil { return errors.Wrapf(err, "failed to list volumes before modifying node selector for managed components setting") } - for _, v := range list { - if v.Status.State != longhorn.VolumeStateDetached { - return fmt.Errorf("cannot modify node selector for managed components setting before all volumes are detached") - } + if !volumesDetached { + return &types.ErrorInvalidState{Reason: fmt.Sprintf("cannot modify node selector for managed components setting before all volumes are detached")} } case types.SettingNamePriorityClass: if value != "" { @@ -330,14 +326,12 @@ func (s *DataStore) ValidateSetting(name, value string) (err error) { return errors.Wrapf(err, "failed to get priority class %v before modifying priority class setting", value) } } - list, err := s.ListVolumesRO() + volumesDetached, err := s.AreAllVolumesDetachedState() if err != nil { return errors.Wrapf(err, "failed to list volumes before modifying priority class setting") } - for _, v := range list { - if v.Status.State != longhorn.VolumeStateDetached { - return fmt.Errorf("cannot modify priority class setting before all volumes are detached") - } + if !volumesDetached { + return &types.ErrorInvalidState{Reason: fmt.Sprintf("cannot modify priority class setting before all volumes are detached")} } case types.SettingNameGuaranteedInstanceManagerCPU: guaranteedInstanceManagerCPU, err := s.GetSetting(types.SettingNameGuaranteedInstanceManagerCPU) @@ -356,7 +350,7 @@ func (s *DataStore) ValidateSetting(name, value string) (err error) { return errors.Wrapf(err, "failed to check volume detachment for %v setting update", name) } if !volumesDetached { - return errors.Errorf("cannot apply %v setting to Longhorn workloads when there are attached volumes", name) + return &types.ErrorInvalidState{Reason: fmt.Sprintf("cannot apply %v setting to Longhorn workloads when there are attached volumes", name)} } case types.SettingNameV2DataEngine: old, err := s.GetSetting(types.SettingNameV2DataEngine) @@ -383,7 +377,7 @@ func (s *DataStore) ValidateV2DataEngine(v2DataEngineEnabled bool) error { return errors.Wrapf(err, "failed to check volume detachment for %v setting update", types.SettingNameV2DataEngine) } if !volumesDetached { - return errors.Errorf("cannot apply %v setting to Longhorn workloads when there are attached volumes", types.SettingNameV2DataEngine) + return &types.ErrorInvalidState{Reason: fmt.Sprintf("cannot apply %v setting to Longhorn workloads when there are attached volumes", types.SettingNameV2DataEngine)} } // Check if there is enough hugepages-2Mi capacity for all nodes @@ -463,6 +457,19 @@ func (s *DataStore) AreAllVolumesDetached() (bool, error) { return true, nil } +func (s *DataStore) AreAllVolumesDetachedState() (bool, error) { + list, err := s.ListVolumesRO() + if err != nil { + return false, errors.Wrapf(err, "failed to list volumes") + } + for _, v := range list { + if v.Status.State != longhorn.VolumeStateDetached { + return false, nil + } + } + return true, nil +} + func (s *DataStore) getSettingRO(name string) (*longhorn.Setting, error) { return s.settingLister.Settings(s.namespace).Get(name) } diff --git a/types/types.go b/types/types.go index e2d0cba0f9..ab3bd97dab 100644 --- a/types/types.go +++ b/types/types.go @@ -273,6 +273,14 @@ func (e *NotFoundError) Error() string { return fmt.Sprintf("cannot find %v", e.Name) } +type ErrorInvalidState struct { + Reason string +} + +func (e *ErrorInvalidState) Error() string { + return fmt.Sprintf("current state prevents this: %v", e.Reason) +} + const ( engineSuffix = "-e" replicaSuffix = "-r" @@ -706,6 +714,11 @@ func ErrorAlreadyExists(err error) bool { return strings.Contains(err.Error(), "already exists") } +func ErrorIsInvalidState(err error) bool { + var dummy *ErrorInvalidState + return errors.As(err, &dummy) +} + func ValidateReplicaCount(count int) error { if count < 1 || count > 20 { return fmt.Errorf("replica count value must between 1 to 20")