From 69ea8dcf0030ae29c90b385ccbb394912da3ba0d Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Thu, 9 Jul 2020 17:59:56 -0400 Subject: [PATCH 1/4] experimental/backpressure: wrap code in a feature flag Wraps the experimental backpressure code using a feature flag. This is in preparation for the merge from experimental -> main. --- charts/osm/templates/osm-deployment.yaml | 3 +++ charts/osm/values.yaml | 1 + cmd/ads/ads.go | 3 +++ cmd/cli/install.go | 3 +++ cmd/cli/install_test.go | 4 ++++ go.sum | 2 ++ pkg/featureflags/featureflags.go | 6 ++++++ pkg/smi/client.go | 24 ++++++++++++++++++++---- pkg/smi/types.go | 1 + 9 files changed, 43 insertions(+), 4 deletions(-) diff --git a/charts/osm/templates/osm-deployment.yaml b/charts/osm/templates/osm-deployment.yaml index 478e260467..71e44c33e0 100644 --- a/charts/osm/templates/osm-deployment.yaml +++ b/charts/osm/templates/osm-deployment.yaml @@ -42,6 +42,9 @@ spec: {{- if .Values.OpenServiceMesh.enableDebugServer }} "--enableDebugServer", {{- end }} + {{- if .Values.OpenServiceMesh.enableBackpressureExperimental }} + "--enable-backpressure-experimental", + {{- end }} ] resources: limits: diff --git a/charts/osm/values.yaml b/charts/osm/values.yaml index 01812f8f74..c847be4d13 100644 --- a/charts/osm/values.yaml +++ b/charts/osm/values.yaml @@ -27,4 +27,5 @@ OpenServiceMesh: port: 9411 enableDebugServer: false enablePermissiveTrafficPolicy: false + enableBackpressureExperimental: false meshName: osm diff --git a/cmd/ads/ads.go b/cmd/ads/ads.go index 44682b0b3d..02bf4ea650 100644 --- a/cmd/ads/ads.go +++ b/cmd/ads/ads.go @@ -96,6 +96,9 @@ func init() { flags.IntVar(&injectorConfig.ListenPort, "webhook-port", constants.InjectorWebhookPort, "Webhook port for sidecar-injector") flags.StringVar(&injectorConfig.InitContainerImage, "init-container-image", "", "InitContainer image") flags.StringVar(&injectorConfig.SidecarImage, "sidecar-image", "", "Sidecar proxy Container image") + + // feature flags + flags.BoolVar(&optionalFeatures.BackpressureEnabled, "enable-backpressure-experimental", false, "Enable experimental backpressure feature") } func main() { diff --git a/cmd/cli/install.go b/cmd/cli/install.go index 7ae2d249ff..3b8976d67e 100644 --- a/cmd/cli/install.go +++ b/cmd/cli/install.go @@ -75,6 +75,7 @@ type installCmd struct { prometheusRetentionTime string enableDebugServer bool enablePermissiveTrafficPolicy bool + enableBackpressureExperimental bool meshName string // checker runs checks before any installation is attempted. Its type is @@ -113,6 +114,7 @@ func newInstallCmd(config *helm.Configuration, out io.Writer) *cobra.Command { f.StringVar(&inst.prometheusRetentionTime, "prometheus-retention-time", constants.PrometheusDefaultRetentionTime, "Duration for which data will be retained in prometheus") f.BoolVar(&inst.enableDebugServer, "enable-debug-server", false, "Enable the debug HTTP server") f.BoolVar(&inst.enablePermissiveTrafficPolicy, "enable-permissive-traffic-policy", false, "Enable permissive traffic policy mode") + f.BoolVar(&inst.enableBackpressureExperimental, "enable-backpressure-experimental", false, "Enable experimental backpressure feature") f.StringVar(&inst.meshName, "mesh-name", defaultMeshName, "Name of the service mesh") return cmd @@ -206,6 +208,7 @@ func (i *installCmd) resolveValues() (map[string]interface{}, error) { fmt.Sprintf("OpenServiceMesh.prometheus.retention.time=%s", i.prometheusRetentionTime), fmt.Sprintf("OpenServiceMesh.enableDebugServer=%t", i.enableDebugServer), fmt.Sprintf("OpenServiceMesh.enablePermissiveTrafficPolicy=%t", i.enablePermissiveTrafficPolicy), + fmt.Sprintf("OpenServiceMesh.enableBackpressureExperimental=%t", i.enableBackpressureExperimental), fmt.Sprintf("OpenServiceMesh.meshName=%s", i.meshName), } diff --git a/cmd/cli/install_test.go b/cmd/cli/install_test.go index c5d99f7dbb..18ad2d46d7 100644 --- a/cmd/cli/install_test.go +++ b/cmd/cli/install_test.go @@ -130,6 +130,7 @@ var _ = Describe("Running the install command", func() { }}, "enableDebugServer": false, "enablePermissiveTrafficPolicy": false, + "enableBackpressureExperimental": false, }})) }) @@ -227,6 +228,7 @@ var _ = Describe("Running the install command", func() { }}, "enableDebugServer": false, "enablePermissiveTrafficPolicy": false, + "enableBackpressureExperimental": false, }})) }) @@ -329,6 +331,7 @@ var _ = Describe("Running the install command", func() { }, "enableDebugServer": false, "enablePermissiveTrafficPolicy": false, + "enableBackpressureExperimental": false, }})) }) @@ -588,6 +591,7 @@ var _ = Describe("Resolving values for install command with vault parameters", f }, "enableDebugServer": false, "enablePermissiveTrafficPolicy": false, + "enableBackpressureExperimental": false, }})) }) }) diff --git a/go.sum b/go.sum index 51949ce31d..0a94ad3211 100644 --- a/go.sum +++ b/go.sum @@ -914,6 +914,8 @@ k8s.io/cli-runtime v0.18.0/go.mod h1:1eXfmBsIJosjn9LjEBUd2WVPoPAY9XGTqTFcPMIBsUQ k8s.io/client-go v0.17.4/go.mod h1:ouF6o5pz3is8qU0/qYL2RnoxOPqgfuidYLowytyLJmc= k8s.io/client-go v0.18.0 h1:yqKw4cTUQraZK3fcVCMeSa+lqKwcjZ5wtcOIPnxQno4= k8s.io/client-go v0.18.0/go.mod h1:uQSYDYs4WhVZ9i6AIoEZuwUggLVEF64HOD37boKAtF8= +k8s.io/client-go v1.5.1 h1:XaX/lo2/u3/pmFau8HN+sB5C/b4dc4Dmm2eXjBH4p1E= +k8s.io/client-go v11.0.0+incompatible h1:LBbX2+lOwY9flffWlJM7f1Ct8V2SRNiMRDFeiwnJo9o= k8s.io/code-generator v0.17.4/go.mod h1:l8BLVwASXQZTo2xamW5mQNFCe1XPiAesVq7Y1t7PiQQ= k8s.io/code-generator v0.18.0/go.mod h1:+UHX5rSbxmR8kzS+FAv7um6dtYrZokQvjHpDSYRVkTc= k8s.io/code-generator v0.18.3 h1:5H57pYEbkMMXCLKD16YQH3yDPAbVLweUsB1M3m70D1c= diff --git a/pkg/featureflags/featureflags.go b/pkg/featureflags/featureflags.go index a1f9519f4e..8074060777 100644 --- a/pkg/featureflags/featureflags.go +++ b/pkg/featureflags/featureflags.go @@ -9,6 +9,7 @@ import ( // OptionalFeatures is a struct to enable/disable optional features type OptionalFeatures struct { // FeatureName bool + BackpressureEnabled bool } var ( @@ -32,3 +33,8 @@ func IsFeatureNameEnabled() bool { return Features.FeatureName } */ + +// IsBackpressureEnabled returns a boolean indicating if the experimental backpressure feature is enabled +func IsBackpressureEnabled() bool { + return Features.BackpressureEnabled +} diff --git a/pkg/smi/client.go b/pkg/smi/client.go index d0fa9b07fd..73263afb9b 100644 --- a/pkg/smi/client.go +++ b/pkg/smi/client.go @@ -22,6 +22,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" + "github.com/open-service-mesh/osm/pkg/featureflags" k8s "github.com/open-service-mesh/osm/pkg/kubernetes" "github.com/open-service-mesh/osm/pkg/namespace" "github.com/open-service-mesh/osm/pkg/service" @@ -35,7 +36,10 @@ func NewMeshSpecClient(smiKubeConfig *rest.Config, kubeClient kubernetes.Interfa smiTrafficSplitClientSet := smiTrafficSplitClient.NewForConfigOrDie(smiKubeConfig) smiTrafficSpecClientSet := smiTrafficSpecClient.NewForConfigOrDie(smiKubeConfig) smiTrafficTargetClientSet := smiTrafficTargetClient.NewForConfigOrDie(smiKubeConfig) - backpressureClientSet := backpressureClient.NewForConfigOrDie(smiKubeConfig) + var backpressureClientSet *backpressureClient.Clientset + if featureflags.IsBackpressureEnabled() { + backpressureClientSet = backpressureClient.NewForConfigOrDie(smiKubeConfig) + } client := newSMIClient(kubeClient, smiTrafficSplitClientSet, smiTrafficSpecClientSet, smiTrafficTargetClientSet, backpressureClientSet, osmNamespace, namespaceController, kubernetesClientName) @@ -111,7 +115,6 @@ func newSMIClient(kubeClient kubernetes.Interface, smiTrafficSplitClient *smiTra TrafficSplit: smiTrafficSplitInformerFactory.Split().V1alpha2().TrafficSplits().Informer(), TrafficSpec: smiTrafficSpecInformerFactory.Specs().V1alpha2().HTTPRouteGroups().Informer(), TrafficTarget: smiTrafficTargetInformerFactory.Access().V1alpha1().TrafficTargets().Informer(), - Backpressure: backPressureInformerFactory.Policy().V1alpha1().Backpressures().Informer(), } cacheCollection := CacheCollection{ @@ -119,7 +122,11 @@ func newSMIClient(kubeClient kubernetes.Interface, smiTrafficSplitClient *smiTra TrafficSplit: informerCollection.TrafficSplit.GetStore(), TrafficSpec: informerCollection.TrafficSpec.GetStore(), TrafficTarget: informerCollection.TrafficTarget.GetStore(), - Backpressure: informerCollection.Backpressure.GetStore(), + } + + if featureflags.IsBackpressureEnabled() { + informerCollection.Backpressure = backPressureInformerFactory.Policy().V1alpha1().Backpressures().Informer() + cacheCollection.Backpressure = informerCollection.Backpressure.GetStore() } client := Client{ @@ -130,6 +137,7 @@ func newSMIClient(kubeClient kubernetes.Interface, smiTrafficSplitClient *smiTra announcements: make(chan interface{}), osmNamespace: osmNamespace, namespaceController: namespaceController, + backpressureEnabled: featureflags.IsBackpressureEnabled(), } shouldObserve := func(obj interface{}) bool { @@ -140,7 +148,9 @@ func newSMIClient(kubeClient kubernetes.Interface, smiTrafficSplitClient *smiTra informerCollection.TrafficSplit.AddEventHandler(k8s.GetKubernetesEventHandlers("TrafficSplit", "SMI", client.announcements, shouldObserve)) informerCollection.TrafficSpec.AddEventHandler(k8s.GetKubernetesEventHandlers("TrafficSpec", "SMI", client.announcements, shouldObserve)) informerCollection.TrafficTarget.AddEventHandler(k8s.GetKubernetesEventHandlers("TrafficTarget", "SMI", client.announcements, shouldObserve)) - informerCollection.Backpressure.AddEventHandler(k8s.GetKubernetesEventHandlers("Backpressure", "SMI", client.announcements, shouldObserve)) + if featureflags.IsBackpressureEnabled() { + informerCollection.Backpressure.AddEventHandler(k8s.GetKubernetesEventHandlers("Backpressure", "SMI", client.announcements, shouldObserve)) + } return &client } @@ -199,6 +209,12 @@ func (c *Client) ListTrafficTargets() []*target.TrafficTarget { // ListBackpressures implements smi.MeshSpec by returning the list of backpressures func (c *Client) ListBackpressures() []*backpressure.Backpressure { var backpressureList []*backpressure.Backpressure + + if c.backpressureEnabled { + log.Info().Msgf("Backpressure turned off!") + return backpressureList + } + for _, pressureIface := range c.caches.Backpressure.List() { backpressure := pressureIface.(*backpressure.Backpressure) // TODO: Add type assertion check using renamed variable when merging with main diff --git a/pkg/smi/types.go b/pkg/smi/types.go index 98c0834295..e729e18c58 100644 --- a/pkg/smi/types.go +++ b/pkg/smi/types.go @@ -45,6 +45,7 @@ type Client struct { announcements chan interface{} osmNamespace string namespaceController namespace.Controller + backpressureEnabled bool } // ClientIdentity is the identity of an Envoy proxy connected to the Open Service Mesh. From fa5baa0992356658e863ce980bd3a6a173dda01a Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Mon, 13 Jul 2020 16:59:17 -0400 Subject: [PATCH 2/4] Fixed format errors --- cmd/cli/install.go | 30 +++++++++++++++--------------- cmd/cli/install_test.go | 16 ++++++++-------- pkg/featureflags/featureflags.go | 2 +- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/cmd/cli/install.go b/cmd/cli/install.go index 3b8976d67e..238918d80d 100644 --- a/cmd/cli/install.go +++ b/cmd/cli/install.go @@ -61,22 +61,22 @@ const ( var chartTGZSource string type installCmd struct { - out io.Writer - containerRegistry string - containerRegistrySecret string - chartPath string - osmImageTag string - certManager string - vaultHost string - vaultProtocol string - vaultToken string - vaultRole string - serviceCertValidityMinutes int - prometheusRetentionTime string - enableDebugServer bool - enablePermissiveTrafficPolicy bool + out io.Writer + containerRegistry string + containerRegistrySecret string + chartPath string + osmImageTag string + certManager string + vaultHost string + vaultProtocol string + vaultToken string + vaultRole string + serviceCertValidityMinutes int + prometheusRetentionTime string + enableDebugServer bool + enablePermissiveTrafficPolicy bool enableBackpressureExperimental bool - meshName string + meshName string // checker runs checks before any installation is attempted. Its type is // abstract here to make testing easy. diff --git a/cmd/cli/install_test.go b/cmd/cli/install_test.go index 18ad2d46d7..491b30e9f7 100644 --- a/cmd/cli/install_test.go +++ b/cmd/cli/install_test.go @@ -128,8 +128,8 @@ var _ = Describe("Running the install command", func() { "retention": map[string]interface{}{ "time": "5d", }}, - "enableDebugServer": false, - "enablePermissiveTrafficPolicy": false, + "enableDebugServer": false, + "enablePermissiveTrafficPolicy": false, "enableBackpressureExperimental": false, }})) }) @@ -226,8 +226,8 @@ var _ = Describe("Running the install command", func() { "retention": map[string]interface{}{ "time": "5d", }}, - "enableDebugServer": false, - "enablePermissiveTrafficPolicy": false, + "enableDebugServer": false, + "enablePermissiveTrafficPolicy": false, "enableBackpressureExperimental": false, }})) }) @@ -329,8 +329,8 @@ var _ = Describe("Running the install command", func() { "time": "5d", }, }, - "enableDebugServer": false, - "enablePermissiveTrafficPolicy": false, + "enableDebugServer": false, + "enablePermissiveTrafficPolicy": false, "enableBackpressureExperimental": false, }})) }) @@ -589,8 +589,8 @@ var _ = Describe("Resolving values for install command with vault parameters", f "time": "5d", }, }, - "enableDebugServer": false, - "enablePermissiveTrafficPolicy": false, + "enableDebugServer": false, + "enablePermissiveTrafficPolicy": false, "enableBackpressureExperimental": false, }})) }) diff --git a/pkg/featureflags/featureflags.go b/pkg/featureflags/featureflags.go index 8074060777..e955657437 100644 --- a/pkg/featureflags/featureflags.go +++ b/pkg/featureflags/featureflags.go @@ -9,7 +9,7 @@ import ( // OptionalFeatures is a struct to enable/disable optional features type OptionalFeatures struct { // FeatureName bool - BackpressureEnabled bool + BackpressureEnabled bool } var ( From 31ad3e58aeb09102d2fe55b61d38977210ce56fc Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Tue, 14 Jul 2020 13:31:44 -0400 Subject: [PATCH 3/4] Removed backpressure field from SMI client --- pkg/smi/client.go | 5 ++--- pkg/smi/types.go | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/smi/client.go b/pkg/smi/client.go index 73263afb9b..9af08ddb08 100644 --- a/pkg/smi/client.go +++ b/pkg/smi/client.go @@ -137,7 +137,6 @@ func newSMIClient(kubeClient kubernetes.Interface, smiTrafficSplitClient *smiTra announcements: make(chan interface{}), osmNamespace: osmNamespace, namespaceController: namespaceController, - backpressureEnabled: featureflags.IsBackpressureEnabled(), } shouldObserve := func(obj interface{}) bool { @@ -210,9 +209,9 @@ func (c *Client) ListTrafficTargets() []*target.TrafficTarget { func (c *Client) ListBackpressures() []*backpressure.Backpressure { var backpressureList []*backpressure.Backpressure - if c.backpressureEnabled { + if !featureflags.IsBackpressureEnabled() { log.Info().Msgf("Backpressure turned off!") - return backpressureList + return nil } for _, pressureIface := range c.caches.Backpressure.List() { diff --git a/pkg/smi/types.go b/pkg/smi/types.go index e729e18c58..98c0834295 100644 --- a/pkg/smi/types.go +++ b/pkg/smi/types.go @@ -45,7 +45,6 @@ type Client struct { announcements chan interface{} osmNamespace string namespaceController namespace.Controller - backpressureEnabled bool } // ClientIdentity is the identity of an Envoy proxy connected to the Open Service Mesh. From 1ba1934e08cdd01e470c8af9c3e1ebfc9e7a6c85 Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Tue, 14 Jul 2020 14:15:57 -0400 Subject: [PATCH 4/4] Renamed optional feature from BackpressureEnabled to Backpressure --- cmd/ads/ads.go | 2 +- pkg/featureflags/featureflags.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/ads/ads.go b/cmd/ads/ads.go index 02bf4ea650..2515572b6c 100644 --- a/cmd/ads/ads.go +++ b/cmd/ads/ads.go @@ -98,7 +98,7 @@ func init() { flags.StringVar(&injectorConfig.SidecarImage, "sidecar-image", "", "Sidecar proxy Container image") // feature flags - flags.BoolVar(&optionalFeatures.BackpressureEnabled, "enable-backpressure-experimental", false, "Enable experimental backpressure feature") + flags.BoolVar(&optionalFeatures.Backpressure, "enable-backpressure-experimental", false, "Enable experimental backpressure feature") } func main() { diff --git a/pkg/featureflags/featureflags.go b/pkg/featureflags/featureflags.go index e955657437..e74be940f4 100644 --- a/pkg/featureflags/featureflags.go +++ b/pkg/featureflags/featureflags.go @@ -9,7 +9,7 @@ import ( // OptionalFeatures is a struct to enable/disable optional features type OptionalFeatures struct { // FeatureName bool - BackpressureEnabled bool + Backpressure bool } var ( @@ -36,5 +36,5 @@ func IsFeatureNameEnabled() bool { // IsBackpressureEnabled returns a boolean indicating if the experimental backpressure feature is enabled func IsBackpressureEnabled() bool { - return Features.BackpressureEnabled + return Features.Backpressure }