When the new instance operation APIs are used, we need to consider th…

…at an older version of helix could have been used to modify the legacy fields of HELIX_ENABLED, HELIX_DISABLED_TYPE, and HELIX_DISABLED_REASON. When this happens, we need to write those legacy fields to the USER source instance operation to preserve the history when another source sets the instance operation. When the other source re-enables the instance, we should restore the old legacy fields with what was previously set.

helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java

@@ -54,7 +54,9 @@ public int getPriority() {
      *
      * @param disabledType InstanceDisabledType
      * @return InstanceOperationTrigger
+     * @deprecated The concept of InstanceDisabledType mapping directly to an InstanceOperationSource is no longer used.
      */
+    @Deprecated
     public static InstanceOperationSource instanceDisabledTypeToInstanceOperationSource(
         InstanceDisabledType disabledType) {
       switch (disabledType) {

helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java

@@ -51,14 +51,9 @@ public void disableInstance(HelixManager manager, Object eventInfo) {
     LOG.info("DefaultCloudEventCallbackImpl disable Instance {}", manager.getInstanceName());
     if (InstanceValidationUtil
         .isEnabled(manager.getHelixDataAccessor(), manager.getInstanceName())) {
-      InstanceUtil.setInstanceOperation(manager.getConfigAccessor(),
-          manager.getHelixDataAccessor().getBaseDataAccessor(), manager.getClusterName(),
-          manager.getInstanceName(),
-              new InstanceConfig.InstanceOperation.Builder().setOperation(
-                      InstanceConstants.InstanceOperation.DISABLE)
-                  .setSource(InstanceConstants.InstanceOperationSource.AUTOMATION)
-                  .setReason(message)
-                  .build());
+      manager.getClusterManagmentTool()
+          .enableInstance(manager.getClusterName(), manager.getInstanceName(), false,
+              InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
     }
     HelixEventHandlingUtil.updateCloudEventOperationInClusterConfig(manager.getClusterName(),
         manager.getInstanceName(), manager.getHelixDataAccessor().getBaseDataAccessor(), false,
@@ -79,13 +74,10 @@ public void enableInstance(HelixManager manager, Object eventInfo) {
     HelixEventHandlingUtil
         .updateCloudEventOperationInClusterConfig(manager.getClusterName(), instanceName,
             manager.getHelixDataAccessor().getBaseDataAccessor(), true, message);
-    InstanceUtil.setInstanceOperation(manager.getConfigAccessor(),
-        manager.getHelixDataAccessor().getBaseDataAccessor(), manager.getClusterName(),
-        manager.getInstanceName(),
-            new InstanceConfig.InstanceOperation.Builder().setOperation(
-                    InstanceConstants.InstanceOperation.ENABLE)
-                .setSource(InstanceConstants.InstanceOperationSource.AUTOMATION).setReason(message)
-                .build());
+    if (HelixEventHandlingUtil.isInstanceDisabledForCloudEvent(instanceName, accessor)) {
+      manager.getClusterManagmentTool().enableInstance(manager.getClusterName(), instanceName, true,
+          InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
+    }
   }
 
   /**

helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java

@@ -747,7 +747,7 @@ public boolean forceKillInstance(String clusterName, String instanceName, String
 
     InstanceConfig.InstanceOperation instanceOperationObj = new InstanceConfig.InstanceOperation.Builder()
         .setOperation(InstanceConstants.InstanceOperation.UNKNOWN).setReason(reason)
-        .setSource(operationSource != null ? operationSource : InstanceConstants.InstanceOperationSource.USER).build();
+        .setSource(operationSource).build();
     InstanceConfig instanceConfig = getInstanceConfig(clusterName, instanceName);
     instanceConfig.setInstanceOperation(instanceOperationObj);
 
@@ -2363,12 +2363,20 @@ public ZNRecord update(ZNRecord currentData) {
         }
 
         InstanceConfig config = new InstanceConfig(currentData);
-        config.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setOperation(
-            enabled ? InstanceConstants.InstanceOperation.ENABLE
-                : InstanceConstants.InstanceOperation.DISABLE).setReason(reason).setSource(
-            disabledType != null
-                ? InstanceConstants.InstanceOperationSource.instanceDisabledTypeToInstanceOperationSource(
-                disabledType) : null).build());
+        config.setInstanceEnabled(enabled);
+        if (!enabled) {
+          // new disabled type and reason will overwrite existing ones.
+          config.resetInstanceDisabledTypeAndReason();
+          if (reason != null) {
+            config.setInstanceDisabledReason(reason);
+          }
+          if (disabledType != null) {
+            config.setInstanceDisabledType(disabledType);
+          }
+        }
+//        config.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setOperation(
+//            enabled ? InstanceConstants.InstanceOperation.ENABLE
+//                : InstanceConstants.InstanceOperation.DISABLE).setReason(reason).build());
         return config.getRecord();
       }
     }, AccessOption.PERSISTENT);

helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java

@@ -67,14 +67,20 @@ public enum InstanceConfigProperty {
     DELAY_REBALANCE_ENABLED,
     MAX_CONCURRENT_TASK,
     INSTANCE_INFO_MAP,
-    INSTANCE_CAPACITY_MAP, TARGET_TASK_THREAD_POOL_SIZE, HELIX_INSTANCE_OPERATIONS
+    INSTANCE_CAPACITY_MAP,
+    TARGET_TASK_THREAD_POOL_SIZE,
+    HELIX_INSTANCE_OPERATIONS
   }
 
   public static class InstanceOperation {
     private final Map<String, String> _properties;
 
     private enum InstanceOperationProperties {
-      OPERATION, REASON, SOURCE, TIMESTAMP
+      OPERATION,
+      REASON,
+      SOURCE,
+      TIMESTAMP,
+      LEGACY_DISABLED_TYPE
     }
 
     private InstanceOperation(@Nullable Map<String, String> properties) {
@@ -91,6 +97,7 @@ public static class Builder {
 
       /**
        * Set the operation type for this instance operation.
+       *
        * @param operationType InstanceOperation type of this instance operation.
        */
       public Builder setOperation(@Nullable InstanceConstants.InstanceOperation operationType) {
@@ -102,7 +109,8 @@ public Builder setOperation(@Nullable InstanceConstants.InstanceOperation operat
 
       /**
        * Set the reason for this instance operation.
-       * @param reason
+       *
+       * @param reason the reason for this instance operation.
        */
       public Builder setReason(String reason) {
         _properties.put(InstanceOperationProperties.REASON.name(), reason != null ? reason : "");
@@ -111,13 +119,29 @@ public Builder setReason(String reason) {
 
       /**
        * Set the source for this instance operation.
-       * @param source InstanceOperationSource
-       *              that caused this instance operation to be triggered.
+       *
+       * @param source InstanceOperationSource that caused this instance operation to be triggered.
+       * @return Builder
        */
       public Builder setSource(InstanceConstants.InstanceOperationSource source) {
         _properties.put(InstanceOperationProperties.SOURCE.name(),
-            source == null ? InstanceConstants.InstanceOperationSource.USER.name()
-                : source.name());
+            source == null ? InstanceConstants.InstanceOperationSource.USER.name() : source.name());
+        return this;
+      }
+
+      /**
+       * Set the HELIX_DISABLED_TYPE which is a legacy field that must be stored, so we can write it
+       * back when we write back to the legacy fields.
+       *
+       * @param disabledType InstanceDisabledType
+       * @return Builder
+       */
+      private Builder setLegacyDisabledType(InstanceConstants.InstanceDisabledType disabledType) {
+        if (disabledType == null) {
+          return this;
+        }
+        _properties.put(InstanceOperationProperties.LEGACY_DISABLED_TYPE.name(),
+            disabledType.name());
         return this;
       }
 
@@ -126,6 +150,8 @@ public InstanceOperation build() throws IllegalArgumentException {
           throw new IllegalArgumentException(
               "Instance operation type is not set, this is a required field.");
         }
+        _properties.putIfAbsent(InstanceOperationProperties.SOURCE.name(),
+            InstanceConstants.InstanceOperationSource.USER.name());
         _properties.put(InstanceOperationProperties.TIMESTAMP.name(),
             String.valueOf(System.currentTimeMillis()));
         return new InstanceOperation(_properties);
@@ -134,6 +160,7 @@ public InstanceOperation build() throws IllegalArgumentException {
 
     /**
      * Get the operation type of this instance operation.
+     *
      * @return the InstanceOperation type
      */
     public InstanceConstants.InstanceOperation getOperation() throws IllegalArgumentException {
@@ -142,8 +169,8 @@ public InstanceConstants.InstanceOperation getOperation() throws IllegalArgument
     }
 
     /**
-     * Get the reason for this instance operation.
-     * If the reason is not set, it will default to an empty string.
+     * Get the reason for this instance operation. If the reason is not set, it will default to an
+     * empty string.
      *
      * @return the reason for this instance operation.
      */
@@ -152,19 +179,23 @@ public String getReason() {
     }
 
     /**
-     * Get the InstanceOperationSource
-     * that caused this instance operation to be triggered.
-     * If the source is not set, it will default to DEFAULT.
+     * Get the InstanceOperationSource that caused this instance operation to be triggered. If the
+     * source is not set, it will default to DEFAULT.
      *
-     * @return the InstanceOperationSource
-     *that caused this instance operation to be triggered.
+     * @return the InstanceOperationSource that caused this instance operation to be triggered.
      */
     public InstanceConstants.InstanceOperationSource getSource() {
       return InstanceConstants.InstanceOperationSource.valueOf(
           _properties.getOrDefault(InstanceOperationProperties.SOURCE.name(),
               InstanceConstants.InstanceOperationSource.USER.name()));
     }
 
+    private InstanceConstants.InstanceDisabledType getLegacyDisabledType() {
+      return InstanceConstants.InstanceDisabledType.valueOf(
+          _properties.getOrDefault(InstanceOperationProperties.LEGACY_DISABLED_TYPE.name(),
+              InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name()));
+    }
+
     /**
      * Get the timestamp (milliseconds from epoch) when this instance operation was triggered.
      *
@@ -442,7 +473,8 @@ public void setInstanceDisabledReason(String disabledReason) {
    */
   @Deprecated
   public void setInstanceDisabledType(InstanceConstants.InstanceDisabledType disabledType) {
-    if (getInstanceOperation().getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) {
+    if (getInstanceOperation().getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)
+        && disabledType != InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE) {
       _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
           disabledType.name());
     }
@@ -501,14 +533,7 @@ private List<InstanceOperation> getInstanceOperations() {
     return _deserializedInstanceOperations;
   }
 
-  /**
-   * Set the instance operation for this instance.
-   * This method also sets the HELIX_ENABLED, HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields
-   * for backwards compatibility.
-   *
-   * @param operation the instance operation
-   */
-  public void setInstanceOperation(InstanceOperation operation) {
+  private void setInstanceOperationHelper(InstanceOperation operation) {
     List<InstanceOperation> deserializedInstanceOperations = getInstanceOperations();
 
     if (operation.getSource() == InstanceConstants.InstanceOperationSource.ADMIN) {
@@ -518,6 +543,7 @@ public void setInstanceOperation(InstanceOperation operation) {
       deserializedInstanceOperations.removeIf(
           instanceOperation -> instanceOperation.getSource() == operation.getSource());
     }
+
     if (operation.getOperation() == InstanceConstants.InstanceOperation.ENABLE) {
       // Insert the operation after the last ENABLE or at the beginning if there isn't ENABLE in the list.
       int insertIndex = 0;
@@ -532,6 +558,7 @@ public void setInstanceOperation(InstanceOperation operation) {
     } else {
       deserializedInstanceOperations.add(operation);
     }
+
     // Set the actual field in the ZnRecord
     _record.setListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name(),
         deserializedInstanceOperations.stream().map(instanceOperation -> {
@@ -555,13 +582,13 @@ public void setInstanceOperation(InstanceOperation operation) {
         setInstanceEnabledHelper(false, operation.getTimestamp());
       }
 
-      _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
-          operation.getReason());
-      _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
-          InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
+      resetInstanceDisabledTypeAndReason();
+      setInstanceDisabledReason(operation.getReason());
+      setInstanceDisabledType(operation.getLegacyDisabledType());
     } else if (operation.getOperation() == InstanceConstants.InstanceOperation.ENABLE) {
       // If any of the other InstanceOperations are of type DISABLE, set that in the HELIX_ENABLED,
       // HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields.
+
       InstanceOperation latestDisableInstanceOperation = null;
       for (InstanceOperation instanceOperation : getInstanceOperations()) {
         if (instanceOperation.getOperation() == InstanceConstants.InstanceOperation.DISABLE && (
@@ -571,17 +598,42 @@ public void setInstanceOperation(InstanceOperation operation) {
         }
       }
 
-      if (latestDisableInstanceOperation != null) {
-        _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
-            latestDisableInstanceOperation.getReason());
-        _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
-            InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
-      } else {
-        setInstanceEnabledHelper(true, operation.getTimestamp());
+      if (!_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
+          HELIX_ENABLED_DEFAULT_VALUE)) {
+        // Only set the disable reason and disable type if the HELIX_ENABLED is false because HELIX_ENABLED
+        // being true takes precedence over an existing latest disable operation existing.
+        if (latestDisableInstanceOperation != null) {
+          setInstanceDisabledReason(latestDisableInstanceOperation.getReason());
+          setInstanceDisabledType(latestDisableInstanceOperation.getLegacyDisabledType());
+        } else {
+          setInstanceEnabledHelper(true, operation.getTimestamp());
+        }
       }
     }
   }
 
+  /**
+   * Set the instance operation for this instance. This method also sets the HELIX_ENABLED,
+   * HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields for backwards compatibility.
+   *
+   * @param operation the instance operation
+   */
+  public void setInstanceOperation(InstanceOperation operation) {
+    // TODO: This can be removed when HELIX_ENABLED is removed.
+    // This is used for backwards compatibility. getInstanceOperation will respect the old
+    // legacy field of HELIX_ENABLED which could have been set by older versions of Helix.
+    // If the HELIX_ENABLED is set by an older version of Helix, we need to sync that value to the
+    // new InstanceOperation field when any new instance operation is set.
+    // We only need to do this if there is already an instance operation set. If there isn't, then
+    // the default is ENABLE with a source of DEFAULT.
+    if (!getInstanceOperations().isEmpty()) {
+      setInstanceOperationHelper(getInstanceOperation());
+    }
+
+    // Set the instance operation passed in.
+    setInstanceOperationHelper(operation);
+  }
+
   /**
    * Set the instance operation for this instance. Provide the InstanceOperation enum and the reason
    * and source will be set to default values.
@@ -648,12 +700,19 @@ public InstanceOperation getInstanceOperation() {
         HELIX_ENABLED_DEFAULT_VALUE)
         && (InstanceConstants.INSTANCE_DISABLED_OVERRIDABLE_OPERATIONS.contains(
         activeInstanceOperation.getOperation()))) {
-      return new InstanceOperation.Builder().setOperation(
-              InstanceConstants.InstanceOperation.DISABLE).setReason(getInstanceDisabledReason())
-          .setSource(
-              InstanceConstants.InstanceOperationSource.instanceDisabledTypeToInstanceOperationSource(
-                  InstanceConstants.InstanceDisabledType.valueOf(getInstanceDisabledType())))
-          .build();
+      InstanceOperation.Builder instanceOperationBuilder =
+          new InstanceOperation.Builder().setOperation(InstanceConstants.InstanceOperation.DISABLE)
+              .setReason(getInstanceDisabledReason());
+
+      try {
+        instanceOperationBuilder.setLegacyDisabledType(
+            InstanceConstants.InstanceDisabledType.valueOf(getInstanceDisabledType()));
+      } catch (IllegalArgumentException e) {
+        _logger.error("Invalid instance disabled type for instance: " + _record.getId()
+            + ". Defaulting to DEFAULT_INSTANCE_DISABLE_TYPE.");
+      }
+
+      return instanceOperationBuilder.build();
     }
 
     // if instance operation is DISABLE, we override it to ENABLE if HELIX_ENABLED set to true
@@ -662,8 +721,10 @@ public InstanceOperation getInstanceOperation() {
       // we always set HELIX_ENABLED to false
       // If instance is enabled by old version helix (not having instance operation), the instance config
       // will have HELIX_ENABLED set to true. In this case, we should override the instance operation to ENABLE
-      if ("true".equals(_record.getSimpleField(InstanceConfigProperty.HELIX_ENABLED.name()))) {
-        return new InstanceOperation.Builder().setOperation(InstanceConstants.InstanceOperation.ENABLE).build();
+      if (_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
+          HELIX_ENABLED_DEFAULT_VALUE)) {
+        return new InstanceOperation.Builder().setOperation(
+            InstanceConstants.InstanceOperation.ENABLE).build();
       }
     }
 
@@ -1098,7 +1159,6 @@ public static InstanceConfig toInstanceConfig(String instanceId) {
     if (host != null && port > 0) {
       config.setHostName(host);
       config.setPort(String.valueOf(port));
-
     }
 
     if (config.getHostName() == null) {

helix-core/src/test/java/org/apache/helix/cloud/event/TestDefaultCloudEventCallbackImpl.java

@@ -53,7 +53,7 @@ public void testDisableInstance() {
         .isEnabled(_manager.getHelixDataAccessor(), _instanceManager.getInstanceName()));
     Assert.assertEquals(_manager.getConfigAccessor()
         .getInstanceConfig(CLUSTER_NAME, _instanceManager.getInstanceName()).getInstanceOperation()
-        .getSource(), InstanceConstants.InstanceOperationSource.AUTOMATION);
+        .getSource(), InstanceConstants.InstanceOperationSource.USER);
 
     _admin.enableInstance(CLUSTER_NAME, _instanceManager.getInstanceName(), false);
     _impl.disableInstance(_instanceManager, null);

helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java

@@ -287,12 +287,16 @@ public void enableInstance(String clusterName, String instanceName, boolean enab
 
     ZNRecord record = (ZNRecord) _baseDataAccessor.get(instanceConfigPath, null, 0);
     InstanceConfig instanceConfig = new InstanceConfig(record);
-    instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setOperation(
-            enabled ? InstanceConstants.InstanceOperation.ENABLE
-                : InstanceConstants.InstanceOperation.DISABLE).setReason(reason).build());
+    instanceConfig.setInstanceEnabled(enabled);
     if (!enabled) {
       // TODO: Replace this when the HELIX_ENABLED and HELIX_DISABLED fields are removed.
       instanceConfig.resetInstanceDisabledTypeAndReason();
+      if (reason != null) {
+        instanceConfig.setInstanceDisabledReason(reason);
+      }
+      if (disabledType != null) {
+        instanceConfig.setInstanceDisabledType(disabledType);
+      }
     }
     _baseDataAccessor.set(instanceConfigPath, instanceConfig.getRecord(), 0);
   }