[controller] Harden update-store workflow #1091

nisargthakkar · 2024-07-31T19:05:47Z

Harden update-store workflow

This PR unifies the UpdateStore logic between child controller and parent controllers. We've faced many issues due to the structure of the code (especially VeniceParentHelixAdmin) where a set of store-update operations puts it into a non-healthy state. These are all the changes in this PR:

Add a new class UpdateStoreUtils that is called by both parent and child controllers to apply the store update and perform the necessary validations.
Perform store config validation after applying all configs. This helps check for config compatibility. The following validations are performed currently.
1. Batch-only:
  1. Inc push is not allowed
  2. WC is not allowed
  3. A/A is not allowed
2. Hybrid:
  1. Rewind time in seconds cannot be negative
  2. Both offset lag threshold and producer time lag threshold cannot be negative
  3. In multi-region mode, inc push is not allowed for NON_AGGREGATE data replication policy
  4. ACTIVE_ACTIVE data replication policy is only supported when Active-Active replication is enabled
  5. Partition count must be greater than 0
3. In single-region mode, incremental push allowed when the store is hybrid
4. In multi-region mode, incremental push is allowed when the store is hybrid and either of the following are true:
  1. Store has active-active replication enabled
  2. Store doesn't have active-active replication, but DataReplicationPolicy is AGGREGATE
5. Storage quota cannot be less than 0
6. Read quota cannot be less than 0
7. Read quota cannot be larger than "router count * max_per_router_quota"
8. Store cannot be active-active replication enabled if it is not also native-replication enabled
9. Active-Active is not supported when amplification factor is more than 1
10. Write-compute is not supported when amplification factor is more than 1
11. Store must have a partitioner-config set
12. Validate if the partitioner can be built with the specified partitioner configs
13. Latest superset value schema id must map to a valid value schema id
14. Max compaction lag < min compaction lag
15. ETL Proxy user must be set. (We might want to deprecate this and remove the corresponding tests, but we have plans to get KIF-based ETL that could benefit from the ETLConfig in store configs)
Added a new concept of "primary controller". This is the controller that is allowed to perform inferred updates. In a multi-region mode, this is the parent controller. In a single-region mode, this is a child controller.
Previously each "feature" update incurred an update to the store metadata on Zk - and the corresponding watchers would get invoked. This change replaces all those updates with a single Zk update, which is done after the resulting configs are validated.
The ordinal from BackupStrategy enum was being used to write to the admin channel. This is problematic when the enums evolve. Added a getValue function and used that instead.
A new config: controller.external.superset.schema.generation.enabled has been added to replace controller.parent.external.superset.schema.generation.enabled because external superset schema generation must be allowed in single-region mode also. controller.parent.external.superset.schema.generation.enabled has been marked deprecated, but it has not been completely removed yet for backward compatibility reasons.
Due to the changes to inferred configs, a large chunk of tests started failing as they had incorrect setups. Fixed most of such test setups.

Some side-effects of this change are:

Controller now checks if the store schema can support WC at the time of enabling the WC config. There were a few tests that also used unsupported value schemas for WC.
Persona validation was only present in parent controller in multi-region mode. This now works in single-region mode as well
SupersetSchemaGeneratorWithCustomProp had a bug where if the first schema has a custom prop, the future superset schema generation would fail as Avro doesn't allow overriding custom props. This got caught as the update store logic now also tries creating superset schema if a store enabled RC or WC, or if it previously had a superset schema.

There are a few other changes that we should do, but are not done in this PR:
All major operations should only be allowed on the parent controller - Create a store, delete a store, add schemas. We should exclude some system stores from this check like we have for the check allowing batch push to admin in child

Recommendation for Reviewers

I recommend going through the changes in at least two passes. In the first pass, look through all the stuff that has been purely deleted. (Skip ParentControllerConfigUpdateUtils as that has been renamed to PrimaryControllerConfigUpdateUtils and it's contents partially moved to UpdateStoreUtils. So, GitHub doesn't detect is as a renaming). While doing this pass, you can also glance through various small changes that do not need much pondering over.

In the second pass, follow this review order:

VeniceControllerClusterConfig, VeniceControllerConfig, VeniceControllerMultiClusterConfig
VeniceControllerService
Admin
UpdateStoreUtils
PrimaryControllerConfigUpdateUtils
AdminUtils
UpdateStoreWrapper
VeniceHelixAdmin
VeniceParentHelixAdmin
SupersetSchemaGeneratorWithCustomProp
All the test modifications
All newly added tests

How was this PR tested?

Added new tests. Modified existing tests. GH CI

Does this PR introduce any user-facing changes?

No. You can skip the rest of this section.
Yes. Make sure to explain your proposed changes and call out the behavior change.

FelixGV

Thanks! This looks like a good change! I didn't have time to look at it fully yet, but I left one minor comment so far.

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

mynameborat

I did a pass. Would recommend having another pair of eyes especially on the following files due to my limited context in the code base

VeniceParentHelixAdmin
VeniceHelixAdmin
UpdateStoreUtils

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java

internal/venice-common/src/main/java/com/linkedin/venice/helix/StoragePersonaRepository.java

internal/venice-common/src/main/java/com/linkedin/venice/meta/HybridStoreConfig.java

internal/venice-common/src/main/java/com/linkedin/venice/meta/PartitionerConfigImpl.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/Admin.java

...oller/src/main/java/com/linkedin/venice/controller/init/SystemStoreInitializationHelper.java

...ntroller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

mynameborat

Looks good. I mostly made sure that the checks and description in the summary matches w/ the code changes.

Final comment son strategy of releasing the changes

Do we have branch based releases for complex changes/features?
It feels merging this w/ master and finding issues during certification can be potentially difficult to deal/mitigate as surface area of the changes is large and reverting such large commit accurately (assuming other changes pile in) might be tricky.

How do plan to certify this w/ our current certification flow?

mynameborat · 2024-10-01T17:21:49Z

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

+    if (params.getRegionsFilter().isPresent()) {
+      Set<String> regionsFilter = parseRegionsFilterList(params.getRegionsFilter().get());


Can we get rid of this conditional to make this more readable? Perhaps something like

Set<String>regionsFilter = params.getRegionsFilter().map(this::parseRegionsFilterList).orElse(Collections.emptySet());

This way you have a top level early exit in case of regions filter not containing region name.

mynameborat · 2024-10-01T17:22:57Z

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

+            clusterName,
+            regionsFilter,
+            multiClusterConfigs.getRegionName());
+        return null;


We expect caller to handle null is it?

mynameborat · 2024-10-01T17:27:14Z

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

+    addToUpdatedConfigs(
+        updatedConfigs,
+        BACKUP_VERSION_RETENTION_MS,
+        backupVersionRetentionMs,
+        updatedStore::setBackupVersionRetentionMs);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        NATIVE_REPLICATION_SOURCE_FABRIC,
+        nativeReplicationSourceFabric,
+        updatedStore::setNativeReplicationSourceFabric);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        LATEST_SUPERSET_SCHEMA_ID,
+        latestSupersetSchemaId,
+        updatedStore::setLatestSuperSetValueSchemaId);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        MIN_COMPACTION_LAG_SECONDS,
+        minCompactionLagSeconds,
+        updatedStore::setMinCompactionLagSeconds);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        MAX_COMPACTION_LAG_SECONDS,
+        maxCompactionLagSeconds,
+        updatedStore::setMaxCompactionLagSeconds);
+    addToUpdatedConfigs(updatedConfigs, MAX_RECORD_SIZE_BYTES, maxRecordSizeBytes, updatedStore::setMaxRecordSizeBytes);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        MAX_NEARLINE_RECORD_SIZE_BYTES,
+        maxNearlineRecordSizeBytes,
+        updatedStore::setMaxNearlineRecordSizeBytes);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        UNUSED_SCHEMA_DELETION_ENABLED,
+        unusedSchemaDeletionEnabled,
+        updatedStore::setUnusedSchemaDeletionEnabled);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        BLOB_TRANSFER_ENABLED,
+        blobTransferEnabled,
+        updatedStore::setBlobTransferEnabled);
+    addToUpdatedConfigs(
+        updatedConfigs,
+        STORAGE_NODE_READ_QUOTA_ENABLED,
+        storageNodeReadQuotaEnabled,
+        updatedStore::setStorageNodeReadQuotaEnabled);
+    addToUpdatedConfigs(updatedConfigs, REGULAR_VERSION_ETL_ENABLED, regularVersionETLEnabled, regularVersionETL -> {


Is there a way to group this configurations into some sort of logical category? Reason being, we can break this method into smaller methods where each of those smaller methods take care of updating their logical group of configuration.

It definitely makes it more readable and enables developers to make changes within a group of configuration to reason about potential implication and validations.

mynameborat · 2024-10-01T17:39:24Z

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

+      throw new VeniceHttpException(HttpStatus.SC_BAD_REQUEST, errorMessage, ErrorType.INVALID_CONFIG);
+    }
+
+    if (updatedStore.isHybrid() && newPartitionCount == 0) {


Is it okay for non-hybrid stores to have newPartitionCount = 0 is it?

mynameborat · 2024-10-01T17:40:50Z

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

+    if (newPartitionCount < minPartitionNum && newPartitionCount != 0) {
+      throw new VeniceHttpException(
+          HttpStatus.SC_BAD_REQUEST,
+          "Partition count must be at least " + minPartitionNum + " for store: " + storeName
+              + ". If a specific partition count is not required, set it to 0.",
+          ErrorType.INVALID_CONFIG);
+    }


can we be consistent w/ rest and log the error before throwing the exception?

mynameborat · 2024-10-01T17:47:22Z

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java

+    if (oldStore.getPartitionerConfig() == null) {
+      originalPartitionerConfig = new PartitionerConfigImpl();
+    } else {
+      originalPartitionerConfig = oldStore.getPartitionerConfig();
+    }


Minor:
Some of the places below starts with initialization and then checks for null and reinitializes with the default in the event of null. e.g., ln: 1186, ln: 1197

Although I personally prefer
Optional.ofNullable(...).orElse(defaultInstance)

Feel free to make them consistent w/ the rest at the very least.

mynameborat · 2024-10-01T18:02:04Z

services/venice-controller/src/main/java/com/linkedin/venice/controller/util/AdminUtils.java

+  public static boolean isIncrementalPushSupported(
+      boolean multiRegion,
+      boolean activeActiveReplicationEnabled,
+      HybridStoreConfig hybridStoreConfig) {
+    // Only hybrid stores can support incremental push
+    if (!AdminUtils.isHybrid(hybridStoreConfig)) {
+      return false;
+    }
+
+    // If the system is running in multi-region mode, we need to validate the data replication policies
+    if (!multiRegion) {
+      return true;
+    }
+
+    // A/A can always support incremental push
+    if (activeActiveReplicationEnabled) {
+      return true;
+    }
+
+    DataReplicationPolicy dataReplicationPolicy = hybridStoreConfig.getDataReplicationPolicy();
+    return dataReplicationPolicy == DataReplicationPolicy.AGGREGATE
+        || dataReplicationPolicy == DataReplicationPolicy.NONE;
+  }


Should we leverage this in the UpdateStoreUtils for validation of scenarios for incremental push allowed? Seems redudant checks

mynameborat · 2024-10-01T18:06:55Z

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

+
+      sendAdminMessageAndWaitForConsumed(clusterName, storeName, message);
+    } finally {
+      releaseAdminMessageLock(clusterName, storeName);


Can we guarantee that we don't run into scenario where the admin is stuck and this never executes? Is there some sort of timed lock releases?

nisargthakkar force-pushed the adminSingleUpdateZk2 branch 4 times, most recently from 9edf55a to a019bc8 Compare August 1, 2024 17:15

nisargthakkar changed the title ~~[WIP] Harden update-store workflow~~ [controller] Harden update-store workflow Aug 1, 2024

nisargthakkar marked this pull request as ready for review August 1, 2024 19:27

FelixGV reviewed Aug 1, 2024

View reviewed changes

...es/venice-controller/src/main/java/com/linkedin/venice/controller/util/UpdateStoreUtils.java Outdated Show resolved Hide resolved

adamxchen mentioned this pull request Aug 8, 2024

[controller][admin-tool] Streamline controller configs and harden update-store workflow #946

Closed

2 tasks

nisargthakkar force-pushed the adminSingleUpdateZk2 branch 3 times, most recently from e42ddde to a81e40e Compare August 14, 2024 03:29

mynameborat reviewed Aug 22, 2024

View reviewed changes

nisargthakkar force-pushed the adminSingleUpdateZk2 branch from 0d3e2c1 to 916b14d Compare September 5, 2024 16:28

nisargthakkar force-pushed the adminSingleUpdateZk2 branch 3 times, most recently from a296585 to 33d1620 Compare September 19, 2024 21:11

nisargthakkar force-pushed the adminSingleUpdateZk2 branch 5 times, most recently from 3fa383a to e4ebdbe Compare October 1, 2024 02:04

[controller] Harden update-store workflow

408d463

nisargthakkar force-pushed the adminSingleUpdateZk2 branch from e4ebdbe to 408d463 Compare October 1, 2024 02:37

mynameborat reviewed Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller] Harden update-store workflow #1091

[controller] Harden update-store workflow #1091

nisargthakkar commented Jul 31, 2024 •

edited

Loading

FelixGV left a comment

mynameborat left a comment

mynameborat left a comment

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

mynameborat Oct 1, 2024

		if (params.getRegionsFilter().isPresent()) {
		Set<String> regionsFilter = parseRegionsFilterList(params.getRegionsFilter().get());

[controller] Harden update-store workflow #1091

Are you sure you want to change the base?

[controller] Harden update-store workflow #1091

Conversation

nisargthakkar commented Jul 31, 2024 • edited Loading

Harden update-store workflow

Recommendation for Reviewers

How was this PR tested?

Does this PR introduce any user-facing changes?

FelixGV left a comment

Choose a reason for hiding this comment

mynameborat left a comment

Choose a reason for hiding this comment

mynameborat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisargthakkar commented Jul 31, 2024 •

edited

Loading