From 8226882be08bfc2826b7ccd7417985ac9e497f48 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Wed, 15 Jan 2025 16:40:34 -0800 Subject: [PATCH] enable warm duration through runtime param (#377) enable warm duration through runtime param Signed-off-by: Shriram Sharma --- admiral/cmd/admiral/cmd/root.go | 1 + .../pkg/clusters/virtualservice_routing.go | 7 +- .../clusters/virtualservice_routing_test.go | 59 ++++++++++++++-- admiral/pkg/controller/common/config.go | 17 +++++ admiral/pkg/controller/common/config_test.go | 70 +++++++++++++++++++ admiral/pkg/controller/common/types.go | 11 +-- 6 files changed, 154 insertions(+), 11 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 75fac2f1..2f94e03a 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -259,6 +259,7 @@ func GetRootCmd(args []string) *cobra.Command { // Flags pertaining to VS based routing rootCmd.PersistentFlags().BoolVar(¶ms.EnableVSRouting, "enable_vs_routing", false, "Enable/Disable VS Based Routing") rootCmd.PersistentFlags().StringSliceVar(¶ms.VSRoutingEnabledClusters, "vs_routing_enabled_clusters", []string{}, "The source clusters to enable VS based routing on") + rootCmd.PersistentFlags().StringSliceVar(¶ms.VSRoutingSlowStartEnabledClusters, "vs_routing_slow_start_enabled_clusters", []string{}, "The source clusters to where VS routing is enabled and require slow start") rootCmd.PersistentFlags().StringSliceVar(¶ms.VSRoutingGateways, "vs_routing_gateways", []string{}, "The PASSTHROUGH gateways to use for VS based routing") rootCmd.PersistentFlags().StringSliceVar(¶ms.IngressVSExportToNamespaces, "ingress_vs_export_to_namespaces", []string{"istio-system"}, "List of namespaces where the ingress VS should be exported") rootCmd.PersistentFlags().StringVar(¶ms.IngressLBPolicy, "ingress_lb_policy", "round_robin", "loadbalancer policy for ingress destination rule (round_robin/random/passthrough/least_request)") diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go index cd0e81bf..5a1825f9 100644 --- a/admiral/pkg/clusters/virtualservice_routing.go +++ b/admiral/pkg/clusters/virtualservice_routing.go @@ -776,13 +776,18 @@ func addUpdateDestinationRuleForSourceIngress( LocalityLbSetting: &networkingV1Alpha3.LocalityLoadBalancerSetting{ Enabled: &wrappers.BoolValue{Value: false}, }, - WarmupDurationSecs: &duration.Duration{Seconds: common.GetDefaultWarmupDurationSecs()}, }, Tls: &networkingV1Alpha3.ClientTLSSettings{ SubjectAltNames: []string{san}, }, }, } + + if common.IsSlowStartEnabledForCluster(sourceCluster) { + drObj.TrafficPolicy.LoadBalancer.WarmupDurationSecs = + &duration.Duration{Seconds: common.GetDefaultWarmupDurationSecs()} + } + drName := fmt.Sprintf("%s-routing-dr", name) newDR := createDestinationRuleSkeleton(drObj, drName, util.IstioSystemNamespace) diff --git a/admiral/pkg/clusters/virtualservice_routing_test.go b/admiral/pkg/clusters/virtualservice_routing_test.go index 2a65e45c..33052582 100644 --- a/admiral/pkg/clusters/virtualservice_routing_test.go +++ b/admiral/pkg/clusters/virtualservice_routing_test.go @@ -2378,10 +2378,11 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { } admiralParams := common.AdmiralParams{ - SANPrefix: "test-san-prefix", - IngressVSExportToNamespaces: []string{"istio-system"}, - EnableVSRouting: true, - VSRoutingEnabledClusters: []string{"cluster-1"}, + SANPrefix: "test-san-prefix", + IngressVSExportToNamespaces: []string{"istio-system"}, + EnableVSRouting: true, + VSRoutingEnabledClusters: []string{"cluster-1", "cluster-2"}, + VSRoutingSlowStartEnabledClusters: []string{"cluster-1"}, } common.ResetSync() common.InitializeConfig(admiralParams) @@ -2401,6 +2402,7 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { testCases := []struct { name string istioClient *istioFake.Clientset + drName string sourceClusterToDRHosts map[string]map[string]string sourceIdentity string expectedError error @@ -2410,6 +2412,7 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { name: "Given a empty sourceIdentity " + "When addUpdateDestinationRuleForSourceIngress is invoked, " + "Then it should return an error", + drName: "test-ns.svc.cluster.local-routing-dr", sourceClusterToDRHosts: map[string]map[string]string{ "cluster-1": { "test-ns.svc.cluster.local": "*.test-ns.svc.cluster.local", @@ -2422,6 +2425,7 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { name: "Given a valid sourceClusterToDRHosts " + "When addUpdateDestinationRuleForSourceIngress is invoked, " + "Then it should create the destination rules", + drName: "test-ns.svc.cluster.local-routing-dr", sourceIdentity: "test-identity", sourceClusterToDRHosts: map[string]map[string]string{ "cluster-1": { @@ -2459,6 +2463,7 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { name: "Given a valid sourceClusterToDRHosts " + "When addUpdateDestinationRuleForSourceIngress is invoked, " + "Then it should create the destination rules", + drName: "test-ns.svc.cluster.local-routing-dr", sourceIdentity: "test-identity", sourceClusterToDRHosts: map[string]map[string]string{ "cluster-1": { @@ -2492,6 +2497,44 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { }, }, }, + { + name: "Given a valid sourceClusterToDRHosts " + + "When addUpdateDestinationRuleForSourceIngress is invoked," + + "And the cluster is not enabled with slow start " + + "Then it should create the destination rules without warmupDurationSecs", + drName: "test-ns2.svc.cluster.local-routing-dr", + sourceIdentity: "test-identity", + sourceClusterToDRHosts: map[string]map[string]string{ + "cluster-2": { + "test-ns2.svc.cluster.local": "*.test-ns2.svc.cluster.local", + }, + }, + istioClient: istioClientWithExistingDR, + expectedError: nil, + expectedDestinationRules: &apiNetworkingV1Alpha3.DestinationRule{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "test-ns2.svc.cluster.local-routing-dr", + Namespace: util.IstioSystemNamespace, + }, + Spec: networkingV1Alpha3.DestinationRule{ + Host: "*.test-ns2.svc.cluster.local", + ExportTo: []string{util.IstioSystemNamespace}, + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{ + LbPolicy: &networkingV1Alpha3.LoadBalancerSettings_Simple{ + Simple: networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN, + }, + LocalityLbSetting: &networkingV1Alpha3.LocalityLoadBalancerSetting{ + Enabled: &wrappers.BoolValue{Value: false}, + }, + }, + Tls: &networkingV1Alpha3.ClientTLSSettings{ + SubjectAltNames: []string{"spiffe://test-san-prefix/test-identity"}, + }, + }, + }, + }, + }, } for _, tc := range testCases { @@ -2500,8 +2543,14 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { ClusterID: "cluster-1", DestinationRuleController: &istio.DestinationRuleController{}, } + rc2 := &RemoteController{ + ClusterID: "cluster-2", + DestinationRuleController: &istio.DestinationRuleController{}, + } rc.DestinationRuleController.IstioClient = tc.istioClient + rc2.DestinationRuleController.IstioClient = tc.istioClient rr.PutRemoteController("cluster-1", rc) + rr.PutRemoteController("cluster-2", rc2) err := addUpdateDestinationRuleForSourceIngress( context.Background(), @@ -2514,7 +2563,7 @@ func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { require.Equal(t, tc.expectedError.Error(), err.Error()) } else { actualDR, err := tc.istioClient.NetworkingV1alpha3().DestinationRules(util.IstioSystemNamespace). - Get(context.Background(), "test-ns.svc.cluster.local-routing-dr", metaV1.GetOptions{}) + Get(context.Background(), tc.drName, metaV1.GetOptions{}) require.Nil(t, err) require.Equal(t, tc.expectedDestinationRules.Spec.Host, actualDR.Spec.Host) require.Equal(t, tc.expectedDestinationRules.Spec.TrafficPolicy, actualDR.Spec.TrafficPolicy) diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index 807ea03f..194a3622 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -461,6 +461,23 @@ func DoVSRoutingForCluster(cluster string) bool { return false } +func IsSlowStartEnabledForCluster(cluster string) bool { + wrapper.RLock() + defer wrapper.RUnlock() + if !wrapper.params.EnableVSRouting { + return false + } + for _, c := range wrapper.params.VSRoutingSlowStartEnabledClusters { + if c == "*" { + return true + } + if c == cluster { + return true + } + } + return false +} + func DoRoutingPolicyForCluster(cluster string) bool { wrapper.RLock() defer wrapper.RUnlock() diff --git a/admiral/pkg/controller/common/config_test.go b/admiral/pkg/controller/common/config_test.go index 981fd09f..207be6b8 100644 --- a/admiral/pkg/controller/common/config_test.go +++ b/admiral/pkg/controller/common/config_test.go @@ -238,6 +238,76 @@ func TestDoVSRoutingForCluster(t *testing.T) { } +func TestIsSlowStartEnabledForCluster(t *testing.T) { + p := AdmiralParams{} + + testCases := []struct { + name string + cluster string + enableVSRouting bool + enableVSRoutingSlowStartForCluster []string + expected bool + }{ + { + name: "Given enableVSRouting is false, enableVSRoutingSlowStartForCluster is empty" + + "When IsSlowStartEnabledForCluster is called" + + "Then it should return false", + cluster: "cluster1", + enableVSRouting: false, + enableVSRoutingSlowStartForCluster: []string{}, + expected: false, + }, + { + name: "Given enableVSRouting is true, enableVSRoutingSlowStartForCluster is empty" + + "When IsSlowStartEnabledForCluster is called" + + "Then it should return false", + cluster: "cluster1", + enableVSRouting: true, + enableVSRoutingSlowStartForCluster: []string{}, + expected: false, + }, + { + name: "Given enableVSRouting is true, and given cluster doesn't exists in the list" + + "When IsSlowStartEnabledForCluster is called" + + "Then it should return false", + cluster: "cluster2", + enableVSRouting: true, + enableVSRoutingSlowStartForCluster: []string{"cluster1"}, + expected: false, + }, + { + name: "Given enableVSRouting is true, and given cluster does exists in the list" + + "When IsSlowStartEnabledForCluster is called" + + "Then it should return true", + cluster: "cluster1", + enableVSRouting: true, + enableVSRoutingSlowStartForCluster: []string{"cluster1"}, + expected: true, + }, + { + name: "Given enableVSRouting is true, and all slow start is enabled in all clusters using '*'" + + "When IsSlowStartEnabledForCluster is called" + + "Then it should return false", + cluster: "cluster1", + enableVSRouting: true, + enableVSRoutingSlowStartForCluster: []string{"*"}, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + p.EnableVSRouting = tc.enableVSRouting + p.VSRoutingSlowStartEnabledClusters = tc.enableVSRoutingSlowStartForCluster + ResetSync() + InitializeConfig(p) + + assert.Equal(t, tc.expected, IsSlowStartEnabledForCluster(tc.cluster)) + }) + } + +} + func TestConfigManagement(t *testing.T) { setupForConfigTests() diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index 1159fbfe..32a1e68c 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -131,11 +131,12 @@ type AdmiralParams struct { OperatorSecretFilterTags string // VS Based Routing - EnableVSRouting bool - VSRoutingGateways []string - IngressVSExportToNamespaces []string - IngressLBPolicy string - VSRoutingEnabledClusters []string + EnableVSRouting bool + VSRoutingGateways []string + IngressVSExportToNamespaces []string + IngressLBPolicy string + VSRoutingEnabledClusters []string + VSRoutingSlowStartEnabledClusters []string //Client discovery (types requiring mesh egress only) EnableClientDiscovery bool