Refactor knativeServiceConfig and make it an attribute of the service…

… builder
caraml-dev · May 21, 2024 · 70da2bc · 70da2bc
1 parent 40398cb
commit 70da2bc
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 87 deletions.
diff --git a/api/turing/cluster/servicebuilder/router.go b/api/turing/cluster/servicebuilder/router.go
@@ -104,9 +104,6 @@ func (sb *clusterSvcBuilder) NewRouterService(
 	routerDefaults *config.RouterDefaults,
 	sentryEnabled bool,
 	sentryDSN string,
-	knativeQueueProxyResourcePercentage int,
-	userContainerCPULimitRequestFactor float64,
-	userContainerMemoryLimitRequestFactor float64,
 	initialScale *int,
 ) (*cluster.KnativeService, error) {
 	// Create service name
@@ -160,9 +157,9 @@ func (sb *clusterSvcBuilder) NewRouterService(
 		AutoscalingMetric:                     string(routerVersion.AutoscalingPolicy.Metric),
 		AutoscalingTarget:                     routerVersion.AutoscalingPolicy.Target,
 		TopologySpreadConstraints:             topologySpreadConstraints,
-		QueueProxyResourcePercentage:          knativeQueueProxyResourcePercentage,
-		UserContainerCPULimitRequestFactor:    userContainerCPULimitRequestFactor,
-		UserContainerMemoryLimitRequestFactor: userContainerMemoryLimitRequestFactor,
+		QueueProxyResourcePercentage:          sb.knativeServiceConfig.QueueProxyResourcePercentage,
+		UserContainerCPULimitRequestFactor:    sb.knativeServiceConfig.UserContainerCPULimitRequestFactor,
+		UserContainerMemoryLimitRequestFactor: sb.knativeServiceConfig.UserContainerMemoryLimitRequestFactor,
 	}
 	return sb.validateKnativeService(svc)
 }

diff --git a/api/turing/cluster/servicebuilder/router_test.go b/api/turing/cluster/servicebuilder/router_test.go
@@ -20,7 +20,17 @@ import (
 )
 
 func TestNewRouterService(t *testing.T) {
-	sb := NewClusterServiceBuilder(resource.MustParse("2"), resource.MustParse("2Gi"), 30, testTopologySpreadConstraints)
+	sb := NewClusterServiceBuilder(
+		resource.MustParse("2"),
+		resource.MustParse("2Gi"),
+		30,
+		testTopologySpreadConstraints,
+		&config.KnativeServiceDefaults{
+			QueueProxyResourcePercentage:          20,
+			UserContainerCPULimitRequestFactor:    0,
+			UserContainerMemoryLimitRequestFactor: 1.5,
+		},
+	)
 	testDataBasePath := filepath.Join("..", "..", "testdata", "cluster", "servicebuilder")
 	enrEndpoint := "http://test-svc-turing-enricher-1.test-project.svc.cluster.local/echo?delay=10ms"
 	ensEndpoint := "http://test-svc-turing-ensembler-1.test-project.svc.cluster.local/echo?delay=20ms"
@@ -1025,7 +1035,7 @@ func TestNewRouterService(t *testing.T) {
 				},
 				true,
 				"sentry-dsn",
-				20, 0, 1.5, data.initialScale,
+				data.initialScale,
 			)
 
 			if data.err == "" {
@@ -1040,7 +1050,17 @@ func TestNewRouterService(t *testing.T) {
 
 func TestNewRouterEndpoint(t *testing.T) {
 	// Get router version
-	sb := NewClusterServiceBuilder(resource.MustParse("2"), resource.MustParse("2Gi"), 30, testTopologySpreadConstraints)
+	sb := NewClusterServiceBuilder(
+		resource.MustParse("2"),
+		resource.MustParse("2Gi"),
+		30,
+		testTopologySpreadConstraints,
+		&config.KnativeServiceDefaults{
+			QueueProxyResourcePercentage:          10,
+			UserContainerCPULimitRequestFactor:    0,
+			UserContainerMemoryLimitRequestFactor: 1.5,
+		},
+	)
 	testDataBasePath := filepath.Join("..", "..", "testdata", "cluster", "servicebuilder")
 	fileBytes, err := tu.ReadFile(filepath.Join(testDataBasePath, "router_version_success.json"))
 	require.NoError(t, err)

diff --git a/api/turing/cluster/servicebuilder/service_builder.go b/api/turing/cluster/servicebuilder/service_builder.go
@@ -66,18 +66,12 @@ type ClusterServiceBuilder interface {
 		ver *models.RouterVersion,
 		project *mlp.Project,
 		secretName string,
-		knativeQueueProxyResourcePercentage int,
-		userContainerCPULimitRequestFactor float64,
-		userContainerMemoryLimitRequestFactor float64,
 		initialScale *int,
 	) (*cluster.KnativeService, error)
 	NewEnsemblerService(
 		ver *models.RouterVersion,
 		project *mlp.Project,
 		secretName string,
-		knativeQueueProxyResourcePercentage int,
-		userContainerCPULimitRequestFactor float64,
-		userContainerMemoryLimitRequestFactor float64,
 		initialScale *int,
 	) (*cluster.KnativeService, error)
 	NewRouterService(
@@ -89,9 +83,6 @@ type ClusterServiceBuilder interface {
 		routerDefaults *config.RouterDefaults,
 		sentryEnabled bool,
 		sentryDSN string,
-		knativeQueueProxyResourcePercentage int,
-		userContainerCPULimitRequestFactor float64,
-		userContainerMemoryLimitRequestFactor float64,
 		initialScale *int,
 	) (*cluster.KnativeService, error)
 	NewFluentdService(
@@ -128,6 +119,9 @@ type clusterSvcBuilder struct {
 	MaxMemory                 resource.Quantity
 	MaxAllowedReplica         int
 	TopologySpreadConstraints []corev1.TopologySpreadConstraint
+
+	// Knative service configs
+	knativeServiceConfig *config.KnativeServiceDefaults
 }
 
 // NewClusterServiceBuilder creates a new service builder with the supplied configs for defaults
@@ -136,12 +130,14 @@ func NewClusterServiceBuilder(
 	memoryLimit resource.Quantity,
 	maxAllowedReplica int,
 	topologySpreadConstraints []corev1.TopologySpreadConstraint,
+	knativeServiceConfig *config.KnativeServiceDefaults,
 ) ClusterServiceBuilder {
 	return &clusterSvcBuilder{
 		MaxCPU:                    cpuLimit,
 		MaxMemory:                 memoryLimit,
 		MaxAllowedReplica:         maxAllowedReplica,
 		TopologySpreadConstraints: topologySpreadConstraints,
+		knativeServiceConfig:      knativeServiceConfig,
 	}
 }
 
@@ -151,9 +147,6 @@ func (sb *clusterSvcBuilder) NewEnricherService(
 	routerVersion *models.RouterVersion,
 	project *mlp.Project,
 	secretName string,
-	knativeQueueProxyResourcePercentage int,
-	userContainerCPULimitRequestFactor float64,
-	userContainerMemoryLimitRequestFactor float64,
 	initialScale *int,
 ) (*cluster.KnativeService, error) {
 	// Get the enricher reference
@@ -227,9 +220,9 @@ func (sb *clusterSvcBuilder) NewEnricherService(
 		AutoscalingMetric:                     string(enricher.AutoscalingPolicy.Metric),
 		AutoscalingTarget:                     enricher.AutoscalingPolicy.Target,
 		TopologySpreadConstraints:             topologySpreadConstraints,
-		QueueProxyResourcePercentage:          knativeQueueProxyResourcePercentage,
-		UserContainerCPULimitRequestFactor:    userContainerCPULimitRequestFactor,
-		UserContainerMemoryLimitRequestFactor: userContainerMemoryLimitRequestFactor,
+		QueueProxyResourcePercentage:          sb.knativeServiceConfig.QueueProxyResourcePercentage,
+		UserContainerCPULimitRequestFactor:    sb.knativeServiceConfig.UserContainerCPULimitRequestFactor,
+		UserContainerMemoryLimitRequestFactor: sb.knativeServiceConfig.UserContainerMemoryLimitRequestFactor,
 	})
 }
 
@@ -239,9 +232,6 @@ func (sb *clusterSvcBuilder) NewEnsemblerService(
 	routerVersion *models.RouterVersion,
 	project *mlp.Project,
 	secretName string,
-	knativeQueueProxyResourcePercentage int,
-	userContainerCPULimitRequestFactor float64,
-	userContainerMemoryLimitRequestFactor float64,
 	initialScale *int,
 ) (*cluster.KnativeService, error) {
 	// Get the ensembler reference
@@ -316,9 +306,9 @@ func (sb *clusterSvcBuilder) NewEnsemblerService(
 		AutoscalingMetric:                     string(docker.AutoscalingPolicy.Metric),
 		AutoscalingTarget:                     docker.AutoscalingPolicy.Target,
 		TopologySpreadConstraints:             topologySpreadConstraints,
-		QueueProxyResourcePercentage:          knativeQueueProxyResourcePercentage,
-		UserContainerCPULimitRequestFactor:    userContainerCPULimitRequestFactor,
-		UserContainerMemoryLimitRequestFactor: userContainerMemoryLimitRequestFactor,
+		QueueProxyResourcePercentage:          sb.knativeServiceConfig.QueueProxyResourcePercentage,
+		UserContainerCPULimitRequestFactor:    sb.knativeServiceConfig.UserContainerCPULimitRequestFactor,
+		UserContainerMemoryLimitRequestFactor: sb.knativeServiceConfig.UserContainerMemoryLimitRequestFactor,
 	})
 }
 

diff --git a/api/turing/cluster/servicebuilder/service_builder_test.go b/api/turing/cluster/servicebuilder/service_builder_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	mlp "github.com/caraml-dev/mlp/api/client"
+	"github.com/caraml-dev/turing/api/turing/config"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -42,7 +43,17 @@ type testSuiteNewService struct {
 }
 
 func TestNewEnricherService(t *testing.T) {
-	sb := NewClusterServiceBuilder(resource.MustParse("2"), resource.MustParse("2Gi"), 30, testTopologySpreadConstraints)
+	sb := NewClusterServiceBuilder(
+		resource.MustParse("2"),
+		resource.MustParse("2Gi"),
+		30,
+		testTopologySpreadConstraints,
+		&config.KnativeServiceDefaults{
+			QueueProxyResourcePercentage:          10,
+			UserContainerCPULimitRequestFactor:    0,
+			UserContainerMemoryLimitRequestFactor: 1.5,
+		},
+	)
 	testDataBasePath := filepath.Join("..", "..", "testdata", "cluster", "servicebuilder")
 	testInitialScale := 5
 
@@ -111,7 +122,7 @@ func TestNewEnricherService(t *testing.T) {
 				Team:   "test-team",
 				Labels: []mlp.Label{{Key: "custom-label-key", Value: "value-1"}},
 			}
-			svc, err := sb.NewEnricherService(routerVersion, project, "secret", 10, 0, 1.5, data.initialScale)
+			svc, err := sb.NewEnricherService(routerVersion, project, "secret", data.initialScale)
 			if data.err == "" {
 				assert.NoError(t, err)
 				assert.Equal(t, data.expected, svc)
@@ -123,7 +134,17 @@ func TestNewEnricherService(t *testing.T) {
 }
 
 func TestNewEnsemblerService(t *testing.T) {
-	sb := NewClusterServiceBuilder(resource.MustParse("2"), resource.MustParse("2Gi"), 30, testTopologySpreadConstraints)
+	sb := NewClusterServiceBuilder(
+		resource.MustParse("2"),
+		resource.MustParse("2Gi"),
+		30,
+		testTopologySpreadConstraints,
+		&config.KnativeServiceDefaults{
+			QueueProxyResourcePercentage:          20,
+			UserContainerCPULimitRequestFactor:    0,
+			UserContainerMemoryLimitRequestFactor: 1.5,
+		},
+	)
 	testDataBasePath := filepath.Join("..", "..", "testdata", "cluster", "servicebuilder")
 	testInitialScale := 5
 
@@ -233,7 +254,7 @@ func TestNewEnsemblerService(t *testing.T) {
 				Stream: "test-stream",
 				Team:   "test-team",
 			}
-			svc, err := sb.NewEnsemblerService(routerVersion, project, "secret", 20, 0, 1.5, data.initialScale)
+			svc, err := sb.NewEnsemblerService(routerVersion, project, "secret", data.initialScale)
 			if data.err == "" {
 				assert.NoError(t, err)
 				assert.Equal(t, data.expected, svc)

diff --git a/api/turing/service/router_deployment_service.go b/api/turing/service/router_deployment_service.go
@@ -66,9 +66,6 @@ type deploymentService struct {
 	sentryDSN      string
 	routerDefaults *config.RouterDefaults
 
-	// Knative service configs
-	knativeServiceConfig *config.KnativeServiceDefaults
-
 	// Ensembler service image builder for real time ensemblers
 	ensemblerServiceImageBuilder imagebuilder.ImageBuilder
 
@@ -94,14 +91,14 @@ func NewDeploymentService(
 		resource.Quantity(cfg.DeployConfig.MaxMemory),
 		cfg.DeployConfig.MaxAllowedReplica,
 		cfg.DeployConfig.TopologySpreadConstraints,
+		cfg.KnativeServiceDefaults,
 	)
 
 	return &deploymentService{
 		deploymentTimeout:            cfg.DeployConfig.Timeout,
 		deploymentDeletionTimeout:    cfg.DeployConfig.DeletionTimeout,
 		environmentType:              cfg.DeployConfig.EnvironmentType,
 		routerDefaults:               cfg.RouterDefaults,
-		knativeServiceConfig:         cfg.KnativeServiceDefaults,
 		ensemblerServiceImageBuilder: ensemblerServiceImageBuilder,
 		sentryEnabled:                cfg.Sentry.Enabled,
 		sentryDSN:                    cfg.Sentry.DSN,
@@ -176,9 +173,6 @@ func (ds *deploymentService) DeployRouterVersion(
 		routerVersion, currRouterVersion, project, ds.environmentType,
 		secretName, experimentConfig,
 		ds.routerDefaults, ds.sentryEnabled, ds.sentryDSN,
-		ds.knativeServiceConfig.QueueProxyResourcePercentage,
-		ds.knativeServiceConfig.UserContainerCPULimitRequestFactor,
-		ds.knativeServiceConfig.UserContainerMemoryLimitRequestFactor,
 	)
 	if err != nil {
 		return endpoint, err
@@ -277,9 +271,6 @@ func (ds *deploymentService) UndeployRouterVersion(
 		ctx, controller,
 		routerVersion, nil, project, ds.environmentType, "", nil,
 		ds.routerDefaults, ds.sentryEnabled, ds.sentryDSN,
-		ds.knativeServiceConfig.QueueProxyResourcePercentage,
-		ds.knativeServiceConfig.UserContainerCPULimitRequestFactor,
-		ds.knativeServiceConfig.UserContainerMemoryLimitRequestFactor,
 	)
 	if err != nil {
 		return err
@@ -367,9 +358,6 @@ func (ds *deploymentService) createServices(
 	routerDefaults *config.RouterDefaults,
 	sentryEnabled bool,
 	sentryDSN string,
-	knativeQueueProxyResourcePercentage int,
-	userContainerCPULimitRequestFactor float64,
-	userContainerMemoryLimitRequestFactor float64,
 ) ([]*cluster.KnativeService, error) {
 	services := []*cluster.KnativeService{}
 	namespace := servicebuilder.GetNamespace(project)
@@ -387,9 +375,7 @@ func (ds *deploymentService) createServices(
 			}
 		}
 		enricherSvc, err := ds.svcBuilder.NewEnricherService(
-			routerVersion, project, secretName,
-			knativeQueueProxyResourcePercentage, userContainerCPULimitRequestFactor,
-			userContainerMemoryLimitRequestFactor, currEnricherReplicas,
+			routerVersion, project, secretName, currEnricherReplicas,
 		)
 		if err != nil {
 			return services, err
@@ -410,9 +396,7 @@ func (ds *deploymentService) createServices(
 			}
 		}
 		ensemblerSvc, err := ds.svcBuilder.NewEnsemblerService(
-			routerVersion, project, secretName,
-			knativeQueueProxyResourcePercentage, userContainerCPULimitRequestFactor,
-			userContainerMemoryLimitRequestFactor, currEnsemblerReplicas,
+			routerVersion, project, secretName, currEnsemblerReplicas,
 		)
 		if err != nil {
 			return services, err
@@ -431,9 +415,7 @@ func (ds *deploymentService) createServices(
 	}
 	routerService, err := ds.svcBuilder.NewRouterService(
 		routerVersion, project, envType, secretName, experimentConfig,
-		routerDefaults, sentryEnabled, sentryDSN,
-		knativeQueueProxyResourcePercentage, userContainerCPULimitRequestFactor,
-		userContainerMemoryLimitRequestFactor, currRouterReplicas,
+		routerDefaults, sentryEnabled, sentryDSN, currRouterReplicas,
 	)
 	if err != nil {
 		return services, err