Bugfix: Fix pyfunc ensembler undeployment bug (#331)

deadlycoconuts · web-flow · commit 6c7b8be5a587 · 2023-04-18T11:43:13.000+08:00
* Remove redundant error handling

* Remove unused envType argument from NewRouterEndpoint

* Remove unsused secretName argument in buildRouterEnvs

* Remove unused envType argument from NewEnricherService

* Remove unused envType argument from NewEnsemblerService

* Rename unused request argument in experiments api methods

* Rename unused arguments in routers api

* Rename unused arguments in all remaining api files

* Rename unused arguments in router deployment service test file

* Remove obsolete labels from mocked and expected responses

* Remove redundant fields

* Align sdk e2e test configs with those in api tests

* Add additional steps to update enricher and ensemblers separately

* Remove obsolete todo comment

* Add an additional step to sdk e2e test to ensure that the router status is pending

* Add additional checks before saving enrichers and ensemblers to db
diff --git a/api/e2e/test/config/config.go b/api/e2e/test/config/config.go
@@ -92,10 +92,7 @@ func (k *K8sConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
 		return err
 	}
 	// use sigyaml.Unmarshal to convert to json object then unmarshal
-	if err := sigyaml.Unmarshal(byteForm, k); err != nil {
-		return err
-	}
-	return nil
+	return sigyaml.Unmarshal(byteForm, k)
 }
 
 type ClusterConfig struct {
diff --git a/api/e2e/test/router_e2e_test.go b/api/e2e/test/router_e2e_test.go
@@ -184,7 +184,6 @@ var _ = DeployedRouterContext("testdata/create_router_nop_logger_proprietary_exp
 
 							Eventually(func(g Gomega) {
 								router = api.GetRouter(apiE, routerCtx.ProjectID, routerCtx.ID)
-								// TODO: Why is it pending? - g.Expect(router.Value("status").Raw()).To(Equal(api.Status.Pending))
 								g.Expect(router.Value("status").Raw()).To(Equal(api.Status.Undeployed))
 							}, arbitraryUpdateIntervals...).Should(Succeed())
 						})
diff --git a/api/turing/api/alerts_api.go b/api/turing/api/alerts_api.go
@@ -59,7 +59,7 @@ func (c AlertsController) CreateAlert(r *http.Request, vars RequestVars, body in
 	return Ok(created)
 }
 
-func (c AlertsController) ListAlerts(r *http.Request, vars RequestVars, _ interface{}) *Response {
+func (c AlertsController) ListAlerts(_ *http.Request, vars RequestVars, _ interface{}) *Response {
 	if c.AlertService == nil {
 		return BadRequest(ErrAlertDisabled.Error(), "")
 	}
@@ -79,7 +79,7 @@ func (c AlertsController) ListAlerts(r *http.Request, vars RequestVars, _ interf
 	return Ok(alerts)
 }
 
-func (c AlertsController) GetAlert(r *http.Request, vars RequestVars, _ interface{}) *Response {
+func (c AlertsController) GetAlert(_ *http.Request, vars RequestVars, _ interface{}) *Response {
 	if c.AlertService == nil {
 		return BadRequest(ErrAlertDisabled.Error(), "")
 	}
diff --git a/api/turing/api/ensemblers_api.go b/api/turing/api/ensemblers_api.go
@@ -16,7 +16,7 @@ type EnsemblersController struct {
 }
 
 func (c EnsemblersController) ListEnsemblers(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
diff --git a/api/turing/api/ensembling_job_api.go b/api/turing/api/ensembling_job_api.go
@@ -18,7 +18,7 @@ type EnsemblingJobController struct {
 // Create is a HTTP Post Method that creates an Ensembling job.
 // This method will only return the acceptance/rejection of the job rather than the actual result of the job.
 func (c EnsemblingJobController) Create(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	body interface{},
 ) *Response {
diff --git a/api/turing/api/experiments_api.go b/api/turing/api/experiments_api.go
@@ -22,7 +22,7 @@ func (c ExperimentsController) ListExperimentEngines(
 
 // ListExperimentEngineClients returns a list of clients on the given experiment engine
 func (c ExperimentsController) ListExperimentEngineClients(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
@@ -42,7 +42,7 @@ func (c ExperimentsController) ListExperimentEngineClients(
 // ListExperimentEngineExperiments returns a list of experiments on the given experiment engine,
 // optionally tied to the given client id
 func (c ExperimentsController) ListExperimentEngineExperiments(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
@@ -64,7 +64,7 @@ func (c ExperimentsController) ListExperimentEngineExperiments(
 
 // ListExperimentEngineVariables returns a list of variables for the given client and/or experiments
 func (c ExperimentsController) ListExperimentEngineVariables(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
diff --git a/api/turing/api/router_versions_api.go b/api/turing/api/router_versions_api.go
@@ -16,7 +16,7 @@ type RouterVersionsController struct {
 
 // ListRouterVersions lists the router versions of the provided router.
 func (c RouterVersionsController) ListRouterVersions(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars, _ interface{},
 ) *Response {
 	// Parse request vars
@@ -38,7 +38,7 @@ func (c RouterVersionsController) ListRouterVersions(
 // CreateRouterVersion creates a router version from the provided configuration. If no router exists
 // within the provided project with the provided id, this method will throw an error.
 // If the update is valid, a new RouterVersion will be created but NOT deployed.
-func (c RouterVersionsController) CreateRouterVersion(r *http.Request, vars RequestVars, body interface{}) *Response {
+func (c RouterVersionsController) CreateRouterVersion(_ *http.Request, vars RequestVars, body interface{}) *Response {
 	// Parse request vars
 	var errResp *Response
 	var router *models.Router
@@ -81,7 +81,7 @@ func (c RouterVersionsController) CreateRouterVersion(r *http.Request, vars Requ
 
 // GetRouterVersion gets the router version for the provided router id and version number.
 func (c RouterVersionsController) GetRouterVersion(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars, _ interface{},
 ) *Response {
 	// Parse request vars
@@ -97,7 +97,7 @@ func (c RouterVersionsController) GetRouterVersion(
 
 // DeleteRouterVersion deletes the config for the given version number.
 func (c RouterVersionsController) DeleteRouterVersion(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
@@ -132,9 +132,9 @@ func (c RouterVersionsController) DeleteRouterVersion(
 
 // DeployRouterVersion deploys the given router version into the associated kubernetes cluster
 func (c RouterVersionsController) DeployRouterVersion(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
-	body interface{},
+	_ interface{},
 ) *Response {
 	// Parse request vars
 	var errResp *Response
diff --git a/api/turing/api/routers_api.go b/api/turing/api/routers_api.go
@@ -20,7 +20,7 @@ type RoutersController struct {
 // ListRouters lists all routers configured in the provided project.
 // If none are found, an error will be thrown.
 func (c RoutersController) ListRouters(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars, _ interface{},
 ) *Response {
 	// Parse input
@@ -41,7 +41,7 @@ func (c RoutersController) ListRouters(
 
 // GetRouter gets a router matching the provided routerID.
 func (c RoutersController) GetRouter(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars, _ interface{},
 ) *Response {
 	// Parse input
@@ -59,7 +59,7 @@ func (c RoutersController) GetRouter(
 // a router within the provided project with the same name, this method will throw an error.
 // If not, a new Router and associated RouterVersion will be created and deployed.
 func (c RoutersController) CreateRouter(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	body interface{},
 ) *Response {
@@ -134,7 +134,7 @@ func (c RoutersController) CreateRouter(
 // UpdateRouter updates a router from the provided configuration. If no router exists
 // within the provided project with the provided id, this method will throw an error.
 // If the update is valid, a new RouterVersion will be created and deployed.
-func (c RoutersController) UpdateRouter(r *http.Request, vars RequestVars, body interface{}) *Response {
+func (c RoutersController) UpdateRouter(_ *http.Request, vars RequestVars, body interface{}) *Response {
 	// Parse request vars
 	var errResp *Response
 	var project *mlp.Project
@@ -196,7 +196,7 @@ func (c RoutersController) UpdateRouter(r *http.Request, vars RequestVars, body
 
 // DeleteRouter deletes a router and all its associated versions.
 func (c RoutersController) DeleteRouter(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
@@ -231,9 +231,9 @@ func (c RoutersController) DeleteRouter(
 // DeployRouter deploys the current version of the given router into the associated
 // kubernetes cluster. If there is no current version, an error is returned.
 func (c RoutersController) DeployRouter(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
-	body interface{},
+	_ interface{},
 ) *Response {
 	// Parse request vars
 	var errResp *Response
@@ -284,9 +284,9 @@ func (c RoutersController) DeployRouter(
 
 // UndeployRouter deletes the given router specs from the associated kubernetes cluster
 func (c RoutersController) UndeployRouter(
-	r *http.Request,
+	_ *http.Request,
 	vars RequestVars,
-	body interface{},
+	_ interface{},
 ) *Response {
 	// Parse request vars
 	var errResp *Response
@@ -308,9 +308,9 @@ func (c RoutersController) UndeployRouter(
 	return Ok(map[string]int{"router_id": int(router.ID)})
 }
 
-func (c RoutersController) ListRouterEvents(r *http.Request,
+func (c RoutersController) ListRouterEvents(_ *http.Request,
 	vars RequestVars,
-	body interface{},
+	_ interface{},
 ) *Response {
 	// Parse request vars
 	var errResp *Response
diff --git a/api/turing/cluster/servicebuilder/router.go b/api/turing/cluster/servicebuilder/router.go
@@ -123,7 +123,7 @@ func (sb *clusterSvcBuilder) NewRouterService(
 
 	// Build env vars
 	envs, err := sb.buildRouterEnvs(namespace, envType, routerDefaults,
-		sentryEnabled, sentryDSN, secretName, routerVersion)
+		sentryEnabled, sentryDSN, routerVersion)
 	if err != nil {
 		return nil, err
 	}
@@ -166,7 +166,6 @@ func (sb *clusterSvcBuilder) NewRouterService(
 func (sb *clusterSvcBuilder) NewRouterEndpoint(
 	routerVersion *models.RouterVersion,
 	project *mlp.Project,
-	envType string,
 	versionEndpoint string,
 ) (*cluster.VirtualService, error) {
 	labels := buildLabels(project, routerVersion.Router)
@@ -208,7 +207,6 @@ func (sb *clusterSvcBuilder) buildRouterEnvs(
 	routerDefaults *config.RouterDefaults,
 	sentryEnabled bool,
 	sentryDSN string,
-	secretName string,
 	ver *models.RouterVersion,
 ) ([]corev1.EnvVar, error) {
 	// Add app name, router timeout, jaeger collector
diff --git a/api/turing/cluster/servicebuilder/router_test.go b/api/turing/cluster/servicebuilder/router_test.go
@@ -1058,7 +1058,7 @@ func TestNewRouterEndpoint(t *testing.T) {
 		MatchURIPrefixes: defaultMatchURIPrefixes,
 	}
 
-	got, err := sb.NewRouterEndpoint(&routerVersion, project, "test-env", versionEndpoint)
+	got, err := sb.NewRouterEndpoint(&routerVersion, project, versionEndpoint)
 	assert.NoError(t, err)
 	assert.Equal(t, expected, got)
 }
@@ -1193,7 +1193,6 @@ func TestBuildRouterEnvsResultLogger(t *testing.T) {
 				tt.args.routerDefaults,
 				tt.args.sentryEnabled,
 				tt.args.sentryDSN,
-				tt.args.secretName,
 				tt.args.ver)
 			assert.Equal(t, tt.want, got)
 		})
diff --git a/api/turing/cluster/servicebuilder/service_builder.go b/api/turing/cluster/servicebuilder/service_builder.go
@@ -60,15 +60,13 @@ type ClusterServiceBuilder interface {
 	NewEnricherService(
 		ver *models.RouterVersion,
 		project *mlp.Project,
-		envType string,
 		secretName string,
 		knativeQueueProxyResourcePercentage int,
 		userContainerLimitRequestFactor float64,
 	) (*cluster.KnativeService, error)
 	NewEnsemblerService(
 		ver *models.RouterVersion,
 		project *mlp.Project,
-		envType string,
 		secretName string,
 		knativeQueueProxyResourcePercentage int,
 		userContainerLimitRequestFactor float64,
@@ -94,7 +92,6 @@ type ClusterServiceBuilder interface {
 	NewRouterEndpoint(
 		routerVersion *models.RouterVersion,
 		project *mlp.Project,
-		envType string,
 		versionEndpoint string,
 	) (*cluster.VirtualService, error)
 	NewSecret(
@@ -136,7 +133,6 @@ func NewClusterServiceBuilder(
 func (sb *clusterSvcBuilder) NewEnricherService(
 	routerVersion *models.RouterVersion,
 	project *mlp.Project,
-	envType string,
 	secretName string,
 	knativeQueueProxyResourcePercentage int,
 	userContainerLimitRequestFactor float64,
@@ -221,7 +217,6 @@ func (sb *clusterSvcBuilder) NewEnricherService(
 func (sb *clusterSvcBuilder) NewEnsemblerService(
 	routerVersion *models.RouterVersion,
 	project *mlp.Project,
-	envType string,
 	secretName string,
 	knativeQueueProxyResourcePercentage int,
 	userContainerLimitRequestFactor float64,
diff --git a/api/turing/cluster/servicebuilder/service_builder_test.go b/api/turing/cluster/servicebuilder/service_builder_test.go
@@ -104,7 +104,7 @@ func TestNewEnricherService(t *testing.T) {
 				Stream: "test-stream",
 				Team:   "test-team",
 			}
-			svc, err := sb.NewEnricherService(routerVersion, project, "test-env", "secret", 10, 1.5)
+			svc, err := sb.NewEnricherService(routerVersion, project, "secret", 10, 1.5)
 			if data.err == "" {
 				assert.NoError(t, err)
 				assert.Equal(t, data.expected, svc)
@@ -220,7 +220,7 @@ func TestNewEnsemblerService(t *testing.T) {
 				Stream: "test-stream",
 				Team:   "test-team",
 			}
-			svc, err := sb.NewEnsemblerService(routerVersion, project, "test-env", "secret", 20, 1.5)
+			svc, err := sb.NewEnsemblerService(routerVersion, project, "secret", 20, 1.5)
 			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
@@ -206,7 +206,7 @@ func (ds *deploymentService) DeployRouterVersion(
 
 	// Deploy or update the virtual service
 	eventsCh.Write(models.NewInfoEvent(models.EventStageUpdatingEndpoint, "updating router endpoint"))
-	routerEndpoint, err := ds.svcBuilder.NewRouterEndpoint(routerVersion, project, ds.environmentType, endpoint)
+	routerEndpoint, err := ds.svcBuilder.NewRouterEndpoint(routerVersion, project, endpoint)
 	if err != nil {
 		eventsCh.Write(models.NewErrorEvent(
 			models.EventStageUpdatingEndpoint, "failed to update router endpoint: %s", err.Error()))
@@ -352,7 +352,6 @@ func (ds *deploymentService) createServices(
 		enricherSvc, err := ds.svcBuilder.NewEnricherService(
 			routerVersion,
 			project,
-			envType,
 			secretName,
 			knativeQueueProxyResourcePercentage,
 			userContainerLimitRequestFactor,
@@ -368,7 +367,6 @@ func (ds *deploymentService) createServices(
 		ensemblerSvc, err := ds.svcBuilder.NewEnsemblerService(
 			routerVersion,
 			project,
-			envType,
 			secretName,
 			knativeQueueProxyResourcePercentage,
 			userContainerLimitRequestFactor,
@@ -455,7 +453,6 @@ func (ds *deploymentService) buildEnsemblerServiceImage(
 		Endpoint:          PyFuncEnsemblerServiceEndpoint,
 		Port:              PyFuncEnsemblerServicePort,
 		Env:               routerVersion.Ensembler.PyfuncConfig.Env,
-		ServiceAccount:    "",
 	}
 
 	return nil
diff --git a/api/turing/service/router_deployment_service_test.go b/api/turing/service/router_deployment_service_test.go
diff --git a/api/turing/service/router_version_service.go b/api/turing/service/router_version_service.go
diff --git a/sdk/e2e/01_create_router_test.py b/sdk/e2e/01_create_router_test.py
diff --git a/sdk/e2e/06_deploy_valid_config_test.py b/sdk/e2e/06_deploy_valid_config_test.py

Original file line number	Diff line number	Diff line change
`@@ -92,10 +92,7 @@ func (k *K8sConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {`
`92`	`92`	`return err`
`93`	`93`	`}`
`94`	`94`	`// use sigyaml.Unmarshal to convert to json object then unmarshal`
`95`		`- if err := sigyaml.Unmarshal(byteForm, k); err != nil {`
`96`		`- return err`
`97`		`- }`
`98`		`- return nil`
	`95`	`+ return sigyaml.Unmarshal(byteForm, k)`
`99`	`96`	`}`
`100`	`97`
`101`	`98`	`type ClusterConfig struct {`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ func (c AlertsController) CreateAlert(r *http.Request, vars RequestVars, body in`
`59`	`59`	`return Ok(created)`
`60`	`60`	`}`
`61`	`61`
`62`		`-func (c AlertsController) ListAlerts(r http.Request, vars RequestVars, _ interface{}) Response {`
	`62`	`+func (c AlertsController) ListAlerts(_ http.Request, vars RequestVars, _ interface{}) Response {`
`63`	`63`	`if c.AlertService == nil {`
`64`	`64`	`return BadRequest(ErrAlertDisabled.Error(), "")`
`65`	`65`	`}`
`@@ -79,7 +79,7 @@ func (c AlertsController) ListAlerts(r *http.Request, vars RequestVars, _ interf`
`79`	`79`	`return Ok(alerts)`
`80`	`80`	`}`
`81`	`81`
`82`		`-func (c AlertsController) GetAlert(r http.Request, vars RequestVars, _ interface{}) Response {`
	`82`	`+func (c AlertsController) GetAlert(_ http.Request, vars RequestVars, _ interface{}) Response {`
`83`	`83`	`if c.AlertService == nil {`
`84`	`84`	`return BadRequest(ErrAlertDisabled.Error(), "")`
`85`	`85`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ type EnsemblersController struct {`
`16`	`16`	`}`
`17`	`17`
`18`	`18`	`func (c EnsemblersController) ListEnsemblers(`
`19`		`- r *http.Request,`
	`19`	`+ _ *http.Request,`
`20`	`20`	`vars RequestVars,`
`21`	`21`	`_ interface{},`
`22`	`22`	`) *Response {`