From fbba2579713afed2c6bd97984fa51207fd7145cd Mon Sep 17 00:00:00 2001 From: Koorous Vargha Date: Mon, 23 Sep 2024 16:06:48 -0700 Subject: [PATCH 1/6] [controller] Make setting instance group tags for controller cluster resources configurable --- .../java/com/linkedin/venice/ConfigKeys.java | 2 ++ .../venice/controller/TestHAASController.java | 21 +++++++++++++++++++ .../VeniceControllerClusterConfig.java | 7 +++++++ .../VeniceControllerMultiClusterConfig.java | 4 ++++ .../venice/controller/ZkHelixAdminClient.java | 6 ++++++ 5 files changed, 40 insertions(+) diff --git a/internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java b/internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java index 33d6b312ee..f9a8f40caf 100644 --- a/internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java +++ b/internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java @@ -214,6 +214,8 @@ private ConfigKeys() { public static final String CONTROLLER_CLUSTER_ZK_ADDRESSS = "controller.cluster.zk.address"; // Name of the Helix cluster for controllers public static final String CONTROLLER_CLUSTER = "controller.cluster.name"; + // What instance group tag to assign to a cluster resource + public static final String CONTROLLER_RESOURCE_INSTANCE_GROUP_TAG = "controller.resource.instance.group.tag"; // What tags to assign to a controller instance public static final String CONTROLLER_INSTANCE_TAG_LIST = "controller.instance.tag.list"; diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java index f26b6bb515..5538d7d0c7 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java @@ -35,6 +35,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import org.apache.helix.HelixAdmin; import org.apache.helix.manager.zk.ZKHelixManager; import org.apache.helix.model.LiveInstance; import org.testng.annotations.BeforeClass; @@ -44,6 +45,7 @@ public class TestHAASController { private Properties enableControllerClusterHAASProperties; private Properties enableControllerAndStorageClusterHAASProperties; + private final String instanceTag = "GENERAL"; @BeforeClass public void setUp() { @@ -51,11 +53,30 @@ public void setUp() { enableControllerClusterHAASProperties.put(ConfigKeys.CONTROLLER_CLUSTER_LEADER_HAAS, String.valueOf(true)); enableControllerClusterHAASProperties .put(ConfigKeys.CONTROLLER_HAAS_SUPER_CLUSTER_NAME, HelixAsAServiceWrapper.HELIX_SUPER_CLUSTER_NAME); + enableControllerClusterHAASProperties.put(ConfigKeys.CONTROLLER_RESOURCE_INSTANCE_GROUP_TAG, instanceTag); + enableControllerClusterHAASProperties.put(ConfigKeys.CONTROLLER_INSTANCE_TAG_LIST, instanceTag); + enableControllerAndStorageClusterHAASProperties = (Properties) enableControllerClusterHAASProperties.clone(); enableControllerAndStorageClusterHAASProperties .put(ConfigKeys.VENICE_STORAGE_CLUSTER_LEADER_HAAS, String.valueOf(true)); } + @Test(timeOut = 60 * Time.MS_PER_SECOND) + public void testClusterResourceInstanceTag() { + try (VeniceClusterWrapper venice = ServiceFactory.getVeniceCluster(0, 0, 0, 1); + HelixAsAServiceWrapper helixAsAServiceWrapper = startAndWaitForHAASToBeAvailable(venice.getZk().getAddress())) { + VeniceControllerWrapper controllerWrapper = + venice.addVeniceController(enableControllerAndStorageClusterHAASProperties); + + String controllerClusterName = "venice-controllers"; + HelixAdmin helixAdmin = controllerWrapper.getVeniceHelixAdmin().getHelixAdmin(); + List resources = helixAdmin.getResourcesInClusterWithTag(controllerClusterName, instanceTag); + assertEquals(resources.size(), 1); + List instances = helixAdmin.getInstancesInClusterWithTag(controllerClusterName, instanceTag); + assertEquals(instances.size(), 1); + } + } + @Test(timeOut = 60 * Time.MS_PER_SECOND) public void testStartHAASHelixControllerAsControllerClusterLeader() { try (VeniceClusterWrapper venice = ServiceFactory.getVeniceCluster(0, 0, 0, 1); diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java index 1e0ed716ff..bd8f2aeaf2 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java @@ -59,6 +59,7 @@ import static com.linkedin.venice.ConfigKeys.CONTROLLER_PARENT_SYSTEM_STORE_REPAIR_CHECK_INTERVAL_SECONDS; import static com.linkedin.venice.ConfigKeys.CONTROLLER_PARENT_SYSTEM_STORE_REPAIR_RETRY_COUNT; import static com.linkedin.venice.ConfigKeys.CONTROLLER_PARENT_SYSTEM_STORE_REPAIR_SERVICE_ENABLED; +import static com.linkedin.venice.ConfigKeys.CONTROLLER_RESOURCE_INSTANCE_GROUP_TAG; import static com.linkedin.venice.ConfigKeys.CONTROLLER_SCHEMA_VALIDATION_ENABLED; import static com.linkedin.venice.ConfigKeys.CONTROLLER_SSL_ENABLED; import static com.linkedin.venice.ConfigKeys.CONTROLLER_STORE_GRAVEYARD_CLEANUP_DELAY_MINUTES; @@ -224,6 +225,7 @@ public class VeniceControllerClusterConfig { // Name of the Helix cluster for controllers private final String controllerClusterName; private final String controllerClusterZkAddress; + private final String controllerResourceInstanceGroupTag; private final List controllerInstanceTagList; private final boolean multiRegion; private final boolean parent; @@ -637,6 +639,7 @@ public VeniceControllerClusterConfig(VeniceProperties props) { */ this.adminCheckReadMethodForKafka = props.getBoolean(ADMIN_CHECK_READ_METHOD_FOR_KAFKA, true); this.controllerClusterName = props.getString(CONTROLLER_CLUSTER, "venice-controllers"); + this.controllerResourceInstanceGroupTag = props.getString(CONTROLLER_RESOURCE_INSTANCE_GROUP_TAG, ""); this.controllerInstanceTagList = props.getList(CONTROLLER_INSTANCE_TAG_LIST, Collections.emptyList()); this.controllerClusterReplica = props.getInt(CONTROLLER_CLUSTER_REPLICA, 3); this.controllerClusterZkAddress = props.getString(CONTROLLER_CLUSTER_ZK_ADDRESSS, getZkAddress()); @@ -1164,6 +1167,10 @@ public String getControllerClusterName() { return controllerClusterName; } + public String getControllerResourceInstanceGroupTag() { + return controllerResourceInstanceGroupTag; + } + public List getControllerInstanceTagList() { return controllerInstanceTagList; } diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java index 2f9ea6d8a5..8db04e4d01 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java @@ -283,6 +283,10 @@ public long getServiceDiscoveryRegistrationRetryMS() { return getCommonConfig().getServiceDiscoveryRegistrationRetryMS(); } + public String getControllerResourceInstanceGroupTag() { + return getCommonConfig().getControllerResourceInstanceGroupTag(); + } + public List getControllerInstanceTagList() { return getCommonConfig().getControllerInstanceTagList(); } diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java index cad3dfd036..e0751b80d7 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java @@ -168,6 +168,12 @@ public void addVeniceStorageClusterToControllerCluster(String clusterName) { idealState.setMinActiveReplicas(controllerClusterReplicaCount); idealState.setRebalancerClassName(DelayedAutoRebalancer.class.getName()); idealState.setRebalanceStrategy(CrushRebalanceStrategy.class.getName()); + + String instanceGroupTag = multiClusterConfigs.getControllerResourceInstanceGroupTag(); + if (!instanceGroupTag.isEmpty()) { + idealState.setInstanceGroupTag(instanceGroupTag); + } + helixAdmin.setResourceIdealState(controllerClusterName, clusterName, idealState); helixAdmin.rebalance(controllerClusterName, clusterName, controllerClusterReplicaCount); } catch (Exception e) { From 831e3881be08d8364bff882c2b72dbd6c520bbf5 Mon Sep 17 00:00:00 2001 From: Koorous Vargha Date: Mon, 23 Sep 2024 16:19:34 -0700 Subject: [PATCH 2/6] Fetch from controller config, not common config --- .../venice/controller/VeniceControllerMultiClusterConfig.java | 4 ---- .../com/linkedin/venice/controller/ZkHelixAdminClient.java | 3 ++- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java index 8db04e4d01..2f9ea6d8a5 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerMultiClusterConfig.java @@ -283,10 +283,6 @@ public long getServiceDiscoveryRegistrationRetryMS() { return getCommonConfig().getServiceDiscoveryRegistrationRetryMS(); } - public String getControllerResourceInstanceGroupTag() { - return getCommonConfig().getControllerResourceInstanceGroupTag(); - } - public List getControllerInstanceTagList() { return getCommonConfig().getControllerInstanceTagList(); } diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java index e0751b80d7..38afcaecd5 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/ZkHelixAdminClient.java @@ -164,12 +164,13 @@ public void addVeniceStorageClusterToControllerCluster(String clusterName) { LeaderStandbySMD.name, IdealState.RebalanceMode.FULL_AUTO.toString(), AutoRebalanceStrategy.class.getName()); + VeniceControllerClusterConfig config = multiClusterConfigs.getControllerConfig(clusterName); IdealState idealState = helixAdmin.getResourceIdealState(controllerClusterName, clusterName); idealState.setMinActiveReplicas(controllerClusterReplicaCount); idealState.setRebalancerClassName(DelayedAutoRebalancer.class.getName()); idealState.setRebalanceStrategy(CrushRebalanceStrategy.class.getName()); - String instanceGroupTag = multiClusterConfigs.getControllerResourceInstanceGroupTag(); + String instanceGroupTag = config.getControllerResourceInstanceGroupTag(); if (!instanceGroupTag.isEmpty()) { idealState.setInstanceGroupTag(instanceGroupTag); } From 5208b0ce4cbfb048b9d8db8c8cfbfa8256f69ed0 Mon Sep 17 00:00:00 2001 From: Koorous Vargha Date: Mon, 23 Sep 2024 16:26:24 -0700 Subject: [PATCH 3/6] Make instanceTag static --- .../java/com/linkedin/venice/controller/TestHAASController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java index 5538d7d0c7..4a5797cba2 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java @@ -45,7 +45,7 @@ public class TestHAASController { private Properties enableControllerClusterHAASProperties; private Properties enableControllerAndStorageClusterHAASProperties; - private final String instanceTag = "GENERAL"; + private final static String instanceTag = "GENERAL"; @BeforeClass public void setUp() { From b798b651f8824cd729e8ba6eee928c38c13419fe Mon Sep 17 00:00:00 2001 From: Koorous Vargha Date: Mon, 23 Sep 2024 17:10:08 -0700 Subject: [PATCH 4/6] Refactor test to use local cluster properties --- .../venice/controller/TestHAASController.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java index 4a5797cba2..5e28ef2cbf 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java @@ -53,9 +53,6 @@ public void setUp() { enableControllerClusterHAASProperties.put(ConfigKeys.CONTROLLER_CLUSTER_LEADER_HAAS, String.valueOf(true)); enableControllerClusterHAASProperties .put(ConfigKeys.CONTROLLER_HAAS_SUPER_CLUSTER_NAME, HelixAsAServiceWrapper.HELIX_SUPER_CLUSTER_NAME); - enableControllerClusterHAASProperties.put(ConfigKeys.CONTROLLER_RESOURCE_INSTANCE_GROUP_TAG, instanceTag); - enableControllerClusterHAASProperties.put(ConfigKeys.CONTROLLER_INSTANCE_TAG_LIST, instanceTag); - enableControllerAndStorageClusterHAASProperties = (Properties) enableControllerClusterHAASProperties.clone(); enableControllerAndStorageClusterHAASProperties .put(ConfigKeys.VENICE_STORAGE_CLUSTER_LEADER_HAAS, String.valueOf(true)); @@ -65,10 +62,15 @@ public void setUp() { public void testClusterResourceInstanceTag() { try (VeniceClusterWrapper venice = ServiceFactory.getVeniceCluster(0, 0, 0, 1); HelixAsAServiceWrapper helixAsAServiceWrapper = startAndWaitForHAASToBeAvailable(venice.getZk().getAddress())) { - VeniceControllerWrapper controllerWrapper = - venice.addVeniceController(enableControllerAndStorageClusterHAASProperties); - + String instanceTag = "GENERAL"; String controllerClusterName = "venice-controllers"; + + Properties clusterProperties = (Properties) enableControllerAndStorageClusterHAASProperties.clone(); + clusterProperties.put(ConfigKeys.CONTROLLER_RESOURCE_INSTANCE_GROUP_TAG, instanceTag); + clusterProperties.put(ConfigKeys.CONTROLLER_INSTANCE_TAG_LIST, instanceTag); + + VeniceControllerWrapper controllerWrapper = venice.addVeniceController(clusterProperties); + HelixAdmin helixAdmin = controllerWrapper.getVeniceHelixAdmin().getHelixAdmin(); List resources = helixAdmin.getResourcesInClusterWithTag(controllerClusterName, instanceTag); assertEquals(resources.size(), 1); From 4618083e2022959d2f8cc707609f48a844e28baa Mon Sep 17 00:00:00 2001 From: Koorous Vargha Date: Tue, 24 Sep 2024 11:49:18 -0700 Subject: [PATCH 5/6] Create unit test for instance group tag --- .../venice/controller/TestHAASController.java | 1 - .../controller/TestZkHelixAdminClient.java | 39 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java index 5e28ef2cbf..796edb2aa2 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestHAASController.java @@ -45,7 +45,6 @@ public class TestHAASController { private Properties enableControllerClusterHAASProperties; private Properties enableControllerAndStorageClusterHAASProperties; - private final static String instanceTag = "GENERAL"; @BeforeClass public void setUp() { diff --git a/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java b/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java new file mode 100644 index 0000000000..a40cc278b7 --- /dev/null +++ b/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java @@ -0,0 +1,39 @@ +package com.linkedin.venice.controller; + +import static org.mockito.Mockito.*; + +import java.lang.reflect.Field; +import org.apache.helix.HelixAdmin; +import org.apache.helix.model.IdealState; +import org.testng.annotations.Test; + + +public class TestZkHelixAdminClient { + @Test + public void testInstanceGroupTag() throws NoSuchFieldException, IllegalAccessException { + ZkHelixAdminClient zkHelixAdminClient = mock(ZkHelixAdminClient.class); + HelixAdmin mockHelixAdmin = mock(HelixAdmin.class); + VeniceControllerMultiClusterConfig mockMultiClusterConfigs = mock(VeniceControllerMultiClusterConfig.class); + VeniceControllerClusterConfig mockClusterConfig = mock(VeniceControllerClusterConfig.class); + IdealState mockIdealState = mock(IdealState.class); + + when(mockClusterConfig.getControllerResourceInstanceGroupTag()).thenReturn("GENERAL"); + when(mockMultiClusterConfigs.getControllerConfig(anyString())).thenReturn(mockClusterConfig); + when(mockHelixAdmin.getResourceIdealState(any(), any())).thenReturn(mockIdealState); + + doCallRealMethod().when(zkHelixAdminClient).addVeniceStorageClusterToControllerCluster(anyString()); + + Field multiClusterConfigsField = ZkHelixAdminClient.class.getDeclaredField("multiClusterConfigs"); + multiClusterConfigsField.setAccessible(true); + multiClusterConfigsField.set(zkHelixAdminClient, mockMultiClusterConfigs); + + Field helixAdminField = ZkHelixAdminClient.class.getDeclaredField("helixAdmin"); + helixAdminField.setAccessible(true); + helixAdminField.set(zkHelixAdminClient, mockHelixAdmin); + + String clusterName = "test-cluster"; + zkHelixAdminClient.addVeniceStorageClusterToControllerCluster(clusterName); + + verify(mockIdealState, times(1)).setInstanceGroupTag("GENERAL"); + } +} From 2894c387009fc9b82378d6c1d711a231b302b01d Mon Sep 17 00:00:00 2001 From: Koorous Vargha Date: Tue, 24 Sep 2024 11:51:49 -0700 Subject: [PATCH 6/6] Simplify verify --- .../com/linkedin/venice/controller/TestZkHelixAdminClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java b/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java index a40cc278b7..361a307c67 100644 --- a/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java +++ b/services/venice-controller/src/test/java/com/linkedin/venice/controller/TestZkHelixAdminClient.java @@ -34,6 +34,6 @@ public void testInstanceGroupTag() throws NoSuchFieldException, IllegalAccessExc String clusterName = "test-cluster"; zkHelixAdminClient.addVeniceStorageClusterToControllerCluster(clusterName); - verify(mockIdealState, times(1)).setInstanceGroupTag("GENERAL"); + verify(mockIdealState).setInstanceGroupTag("GENERAL"); } }