fix(setting): UX of updating danger zone settings

Introduce a new field `Applied`. It will be true when setting is applied. longhorn/longhorn#8070 Signed-off-by: James Lu <james.lu@suse.com>
c3y1huang · May 13, 2024 · 79b09b9 · 79b09b9
1 parent 8bdc615
commit 79b09b9
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 5 deletions.
diff --git a/controller/instance_manager_controller.go b/controller/instance_manager_controller.go
@@ -626,6 +626,12 @@ func (imc *InstanceManagerController) areDangerZoneSettingsSyncedToIMPod(im *lon
 		if !isSettingSynced {
 			return false, false, false, nil
 		}
+		if !setting.Applied {
+			setting.Applied = true
+			if _, err := imc.ds.UpdateSetting(setting); err != nil {
+				imc.logger.WithError(err).Debugf("Failed to update setting %v applied", setting.Name)
+			}
+		}
 	}
 
 	return true, false, false, nil

diff --git a/controller/setting_controller.go b/controller/setting_controller.go
@@ -219,15 +219,27 @@ func (sc *SettingController) handleErr(err error, key interface{}) {
 }
 
 func (sc *SettingController) syncSetting(key string) (err error) {
-	defer func() {
-		err = errors.Wrapf(err, "failed to sync setting for %v", key)
-	}()
-
 	_, name, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
 		return err
 	}
 
+	defer func() {
+		err = errors.Wrapf(err, "failed to sync setting for %v", key)
+
+		setting, dsErr := sc.ds.GetSettingExact(types.SettingName(name))
+		if dsErr != nil {
+			sc.logger.WithError(dsErr).Warnf("Failed to get setting: %v", name)
+			return
+		}
+		if setting.Applied != (err == nil) {
+			setting.Applied = err == nil
+			if _, dsErr := sc.ds.UpdateSetting(setting); dsErr != nil {
+				sc.logger.WithError(dsErr).Warnf("Failed to update setting: %v", name)
+			}
+		}
+	}()
+
 	if err := sc.syncNonDangerZoneSettingsForManagedComponents(types.SettingName(name)); err != nil {
 		return err
 	}
@@ -1166,13 +1178,16 @@ func (sc *SettingController) syncUpgradeChecker() error {
 
 	if !upgradeCheckerEnabled {
 		if latestLonghornVersion.Value != "" {
+
 			latestLonghornVersion.Value = ""
+			latestLonghornVersion.Applied = false
 			if _, err := sc.ds.UpdateSetting(latestLonghornVersion); err != nil {
 				return err
 			}
 		}
 		if stableLonghornVersions.Value != "" {
 			stableLonghornVersions.Value = ""
+			stableLonghornVersions.Applied = false
 			if _, err := sc.ds.UpdateSetting(stableLonghornVersions); err != nil {
 				return err
 			}
@@ -1200,6 +1215,7 @@ func (sc *SettingController) syncUpgradeChecker() error {
 	sc.lastUpgradeCheckedTimestamp = now
 
 	if latestLonghornVersion.Value != currentLatestVersion {
+		stableLonghornVersions.Applied = true
 		sc.logger.Infof("Latest Longhorn version is %v", latestLonghornVersion.Value)
 		if _, err := sc.ds.UpdateSetting(latestLonghornVersion); err != nil {
 			// non-critical error, don't retry
@@ -1208,6 +1224,7 @@ func (sc *SettingController) syncUpgradeChecker() error {
 		}
 	}
 	if stableLonghornVersions.Value != currentStableVersions {
+		stableLonghornVersions.Applied = true
 		sc.logger.Infof("The latest stable version of every minor release line: %v", stableLonghornVersions.Value)
 		if _, err := sc.ds.UpdateSetting(stableLonghornVersions); err != nil {
 			// non-critical error, don't retry

diff --git a/controller/system_rollout_controller.go b/controller/system_rollout_controller.go
@@ -1730,6 +1730,7 @@ func (c *SystemRolloutController) restoreSettings() (err error) {
 			if !ok {
 				return nil, fmt.Errorf(SystemRolloutErrFailedConvertToObjectFmt, exist.GetObjectKind(), types.LonghornKindSetting)
 			}
+			obj.Applied = true
 			return c.ds.UpdateSetting(obj)
 		}
 		_, err = c.rolloutResource(exist, fnUpdate, isSkipped, log, SystemRolloutMsgSkipIdentical)

diff --git a/datastore/longhorn.go b/datastore/longhorn.go
@@ -180,6 +180,7 @@ func (s *DataStore) createOrUpdateSetting(name types.SettingName, value, default
 	}
 	setting.Annotations[types.GetLonghornLabelKey(types.ConfigMapResourceVersionKey)] = defaultSettingCMResourceVersion
 	setting.Value = value
+	setting.Applied = true
 
 	_, err = s.UpdateSetting(setting)
 	return err

diff --git a/k8s/crds.yaml b/k8s/crds.yaml
@@ -2827,6 +2827,10 @@ spec:
       jsonPath: .value
       name: Value
       type: string
+    - description: The setting is applied
+      jsonPath: .applied
+      name: Applied
+      type: boolean
     - jsonPath: .metadata.creationTimestamp
       name: Age
       type: date
@@ -2838,6 +2842,9 @@ spec:
           apiVersion:
             description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
             type: string
+          applied:
+            description: The setting is applied.
+            type: boolean
           kind:
             description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
             type: string
@@ -2847,6 +2854,7 @@ spec:
             description: The value of the setting.
             type: string
         required:
+        - applied
         - value
         type: object
     served: true

diff --git a/k8s/pkg/apis/longhorn/v1beta2/setting.go b/k8s/pkg/apis/longhorn/v1beta2/setting.go
@@ -8,6 +8,7 @@ import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 // +kubebuilder:printcolumn:name="Value",type=string,JSONPath=`.value`,description="The value of the setting"
+// +kubebuilder:printcolumn:name="Applied",type=boolean,JSONPath=`.applied`,description="The setting is applied"
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
 
 // Setting is where Longhorn stores setting object.
@@ -17,6 +18,8 @@ type Setting struct {
 
 	// The value of the setting.
 	Value string `json:"value"`
+	// The setting is applied.
+	Applied bool `json:"applied"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

diff --git a/manager/setting.go b/manager/setting.go
@@ -45,6 +45,7 @@ func (m *VolumeManager) CreateOrUpdateSetting(s *longhorn.Setting) (*longhorn.Se
 	if err != nil {
 		return nil, err
 	}
+	s.Applied = true
 	setting, err := m.ds.UpdateSetting(s)
 	if err != nil {
 		if apierrors.IsNotFound(err) {

diff --git a/upgrade/v16xto170/upgrade.go b/upgrade/v16xto170/upgrade.go
@@ -28,7 +28,11 @@ func UpgradeResources(namespace string, lhClient *lhclientset.Clientset, kubeCli
 		return err
 	}
 
-	return upgradeNodes(namespace, lhClient, resourceMaps)
+	if err := upgradeNodes(namespace, lhClient, resourceMaps); err != nil {
+		return err
+	}
+
+	return upgradeSetting(namespace, lhClient, resourceMaps)
 }
 
 func UpgradeResourcesStatus(namespace string, lhClient *lhclientset.Clientset, kubeClient *clientset.Clientset, resourceMaps map[string]interface{}) error {
@@ -86,6 +90,26 @@ func upgradeReplicas(namespace string, lhClient *lhclientset.Clientset, resource
 	return nil
 }
 
+func upgradeSetting(namespace string, lhClient *lhclientset.Clientset, resourceMaps map[string]interface{}) (err error) {
+	defer func() {
+		err = errors.Wrapf(err, upgradeLogPrefix+"upgrade setting failed")
+	}()
+
+	settingMap, err := upgradeutil.ListAndUpdateSettingsInProvidedCache(namespace, lhClient, resourceMaps)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return errors.Wrapf(err, "failed to list all existing Longhorn settings during the setting upgrade")
+	}
+
+	for _, s := range settingMap {
+		s.Applied = true
+	}
+
+	return nil
+}
+
 func upgradeEngineStatus(namespace string, lhClient *lhclientset.Clientset, resourceMaps map[string]interface{}) (err error) {
 	defer func() {
 		err = errors.Wrapf(err, upgradeLogPrefix+"upgrade engines failed")