From 3269a84bf15316c08853653be5a86f0dce3e730a Mon Sep 17 00:00:00 2001 From: Terence Lim Date: Tue, 19 Jul 2022 16:26:03 +0800 Subject: [PATCH 1/2] Use background context rather than cancelled context --- api/turing/cluster/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/turing/cluster/controller.go b/api/turing/cluster/controller.go index ea3175c52..66de681d7 100644 --- a/api/turing/cluster/controller.go +++ b/api/turing/cluster/controller.go @@ -719,7 +719,7 @@ func (c *controller) waitKnativeServiceReady( for { select { case <-ctx.Done(): - terminationMessage := c.getKnativePodTerminationMessage(ctx, svcName, namespace) + terminationMessage := c.getKnativePodTerminationMessage(context.Background(), svcName, namespace) if terminationMessage == "" { // Pod was not created (as with invalid image names), get status messages from the knative service. svc, err := services.Get(ctx, svcName, metav1.GetOptions{}) From 13ac62dc119272a278bea8dc39b355e030b78533 Mon Sep 17 00:00:00 2001 From: Terence Lim Date: Tue, 19 Jul 2022 17:51:16 +0800 Subject: [PATCH 2/2] Standardize context usage --- api/turing/cluster/controller.go | 42 ++++--------------- api/turing/cluster/controller_test.go | 8 ++-- api/turing/cluster/mocks/controller.go | 42 +++++++++---------- .../service/router_deployment_service.go | 24 +++++------ .../service/router_deployment_service_test.go | 14 +++---- 5 files changed, 49 insertions(+), 81 deletions(-) diff --git a/api/turing/cluster/controller.go b/api/turing/cluster/controller.go index 66de681d7..ffbfeb86a 100644 --- a/api/turing/cluster/controller.go +++ b/api/turing/cluster/controller.go @@ -64,16 +64,13 @@ type clusterConfig struct { // Controller defines the operations supported by the cluster controller type Controller interface { DeployKnativeService(ctx context.Context, svc *KnativeService) error - DeleteKnativeService(ctx context.Context, svcName string, - namespace string, timeout time.Duration, ignoreNotFound bool) error + DeleteKnativeService(ctx context.Context, svcName string, namespace string, ignoreNotFound bool) error GetKnativeServiceURL(ctx context.Context, svcName string, namespace string) string ApplyIstioVirtualService(ctx context.Context, routerEndpoint *VirtualService) error - DeleteIstioVirtualService(ctx context.Context, svcName string, namespace string, timeout time.Duration) error + DeleteIstioVirtualService(ctx context.Context, svcName string, namespace string) error DeployKubernetesService(ctx context.Context, svc *KubernetesService) error - DeleteKubernetesDeployment(ctx context.Context, name string, - namespace string, timeout time.Duration, ignoreNotFound bool) error - DeleteKubernetesService(ctx context.Context, svcName string, - namespace string, timeout time.Duration, ignoreNotFound bool) error + DeleteKubernetesDeployment(ctx context.Context, name string, namespace string, ignoreNotFound bool) error + DeleteKubernetesService(ctx context.Context, svcName string, namespace string, ignoreNotFound bool) error CreateNamespace(ctx context.Context, name string) error ApplyConfigMap(ctx context.Context, namespace string, configMap *ConfigMap) error DeleteConfigMap(ctx context.Context, name string, namespace string, ignoreNotFound bool) error @@ -305,7 +302,6 @@ func (c *controller) DeleteKnativeService( ctx context.Context, svcName string, namespace string, - timeout time.Duration, ignoreNotFound bool, ) error { // Init knative ServicesGetter @@ -321,12 +317,7 @@ func (c *controller) DeleteKnativeService( } // Delete the service - gracePeriod := int64(timeout.Seconds()) - delOptions := metav1.DeleteOptions{ - GracePeriodSeconds: &gracePeriod, - } - - return services.Delete(ctx, svcName, delOptions) + return services.Delete(ctx, svcName, metav1.DeleteOptions{}) } // DeployKubernetesService deploys a kubernetes service and deployment @@ -396,14 +387,8 @@ func (c *controller) DeleteKubernetesDeployment( ctx context.Context, name string, namespace string, - timeout time.Duration, ignoreNotFound bool, ) error { - gracePeriod := int64(timeout.Seconds()) - delOptions := metav1.DeleteOptions{ - GracePeriodSeconds: &gracePeriod, - } - deployments := c.k8sAppsClient.Deployments(namespace) _, err := deployments.Get(ctx, name, metav1.GetOptions{}) if err != nil { @@ -412,7 +397,7 @@ func (c *controller) DeleteKubernetesDeployment( } return err } - return deployments.Delete(ctx, name, delOptions) + return deployments.Delete(ctx, name, metav1.DeleteOptions{}) } // DeleteKubernetesService deletes a kubernetes service @@ -420,14 +405,8 @@ func (c *controller) DeleteKubernetesService( ctx context.Context, svcName string, namespace string, - timeout time.Duration, ignoreNotFound bool, ) error { - gracePeriod := int64(timeout.Seconds()) - delOptions := metav1.DeleteOptions{ - GracePeriodSeconds: &gracePeriod, - } - services := c.k8sCoreClient.Services(namespace) _, err := services.Get(ctx, svcName, metav1.GetOptions{}) if err != nil { @@ -436,7 +415,7 @@ func (c *controller) DeleteKubernetesService( } return err } - return services.Delete(ctx, svcName, delOptions) + return services.Delete(ctx, svcName, metav1.DeleteOptions{}) } // ApplyIstioVirtualService creates a virtual service if not exists, if exists, updates the @@ -459,18 +438,13 @@ func (c *controller) DeleteIstioVirtualService( ctx context.Context, svcName string, namespace string, - timeout time.Duration, ) error { vservices := c.istioClient.VirtualServices(namespace) _, err := vservices.Get(ctx, svcName, metav1.GetOptions{}) if err != nil { return fmt.Errorf("unable to retrieve virtual service %s: %s", svcName, err.Error()) } - gracePeriod := int64(timeout.Seconds()) - delOptions := metav1.DeleteOptions{ - GracePeriodSeconds: &gracePeriod, - } - return vservices.Delete(ctx, svcName, delOptions) + return vservices.Delete(ctx, svcName, metav1.DeleteOptions{}) } // CreateSecret creates a secret. If the secret already exists, the existing secret will be updated. diff --git a/api/turing/cluster/controller_test.go b/api/turing/cluster/controller_test.go index 0f4684ee3..321230e42 100644 --- a/api/turing/cluster/controller_test.go +++ b/api/turing/cluster/controller_test.go @@ -475,7 +475,7 @@ func TestDeleteKnativeService(t *testing.T) { // Create test controller c := createTestKnController(cs, tc.reactors) // Run test - err := c.DeleteKnativeService(ctx, testName, testNamespace, time.Second*5, tc.ignoreNotFound) + err := c.DeleteKnativeService(ctx, testName, testNamespace, tc.ignoreNotFound) // Validate no error assert.Equal(t, err != nil, tc.hasErr) }) @@ -568,7 +568,7 @@ func TestDeleteKubernetesDeployment(t *testing.T) { // Create test controller c := createTestK8sController(cs, tc.reactors) // Run test - err := c.DeleteKubernetesDeployment(ctx, testName, testNamespace, time.Second*5, tc.ignoreNotFound) + err := c.DeleteKubernetesDeployment(ctx, testName, testNamespace, tc.ignoreNotFound) // Validate no error assert.Equal(t, err != nil, tc.hasErr) }) @@ -660,7 +660,7 @@ func TestDeleteKubernetesService(t *testing.T) { // Create test controller c := createTestK8sController(cs, tc.reactors) // Run test - err := c.DeleteKubernetesService(ctx, testName, testNamespace, time.Second*5, tc.ignoreNotFound) + err := c.DeleteKubernetesService(ctx, testName, testNamespace, tc.ignoreNotFound) // Validate no error assert.Equal(t, err != nil, tc.hasErr) }) @@ -1840,7 +1840,7 @@ func TestDeleteIstioVirtualService(t *testing.T) { defer cancel() // Run test - err := c.DeleteIstioVirtualService(ctx, vsConf.Name, testNamespace, time.Second*5) + err := c.DeleteIstioVirtualService(ctx, vsConf.Name, testNamespace) // Validate no error assert.NoError(t, err) } diff --git a/api/turing/cluster/mocks/controller.go b/api/turing/cluster/mocks/controller.go index 1d1f8e3c4..2d3fc0d04 100644 --- a/api/turing/cluster/mocks/controller.go +++ b/api/turing/cluster/mocks/controller.go @@ -15,8 +15,6 @@ import ( rbacv1 "k8s.io/api/rbac/v1" - time "time" - v1 "k8s.io/api/batch/v1" v1beta2 "github.com/GoogleCloudPlatform/spark-on-k8s-operator/pkg/apis/sparkoperator.k8s.io/v1beta2" @@ -226,13 +224,13 @@ func (_m *Controller) DeleteConfigMap(ctx context.Context, name string, namespac return r0 } -// DeleteIstioVirtualService provides a mock function with given fields: ctx, svcName, namespace, timeout -func (_m *Controller) DeleteIstioVirtualService(ctx context.Context, svcName string, namespace string, timeout time.Duration) error { - ret := _m.Called(ctx, svcName, namespace, timeout) +// DeleteIstioVirtualService provides a mock function with given fields: ctx, svcName, namespace +func (_m *Controller) DeleteIstioVirtualService(ctx context.Context, svcName string, namespace string) error { + ret := _m.Called(ctx, svcName, namespace) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, time.Duration) error); ok { - r0 = rf(ctx, svcName, namespace, timeout) + if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + r0 = rf(ctx, svcName, namespace) } else { r0 = ret.Error(0) } @@ -254,13 +252,13 @@ func (_m *Controller) DeleteJob(ctx context.Context, namespace string, jobName s return r0 } -// DeleteKnativeService provides a mock function with given fields: ctx, svcName, namespace, timeout, ignoreNotFound -func (_m *Controller) DeleteKnativeService(ctx context.Context, svcName string, namespace string, timeout time.Duration, ignoreNotFound bool) error { - ret := _m.Called(ctx, svcName, namespace, timeout, ignoreNotFound) +// DeleteKnativeService provides a mock function with given fields: ctx, svcName, namespace, ignoreNotFound +func (_m *Controller) DeleteKnativeService(ctx context.Context, svcName string, namespace string, ignoreNotFound bool) error { + ret := _m.Called(ctx, svcName, namespace, ignoreNotFound) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, time.Duration, bool) error); ok { - r0 = rf(ctx, svcName, namespace, timeout, ignoreNotFound) + if rf, ok := ret.Get(0).(func(context.Context, string, string, bool) error); ok { + r0 = rf(ctx, svcName, namespace, ignoreNotFound) } else { r0 = ret.Error(0) } @@ -268,13 +266,13 @@ func (_m *Controller) DeleteKnativeService(ctx context.Context, svcName string, return r0 } -// DeleteKubernetesDeployment provides a mock function with given fields: ctx, name, namespace, timeout, ignoreNotFound -func (_m *Controller) DeleteKubernetesDeployment(ctx context.Context, name string, namespace string, timeout time.Duration, ignoreNotFound bool) error { - ret := _m.Called(ctx, name, namespace, timeout, ignoreNotFound) +// DeleteKubernetesDeployment provides a mock function with given fields: ctx, name, namespace, ignoreNotFound +func (_m *Controller) DeleteKubernetesDeployment(ctx context.Context, name string, namespace string, ignoreNotFound bool) error { + ret := _m.Called(ctx, name, namespace, ignoreNotFound) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, time.Duration, bool) error); ok { - r0 = rf(ctx, name, namespace, timeout, ignoreNotFound) + if rf, ok := ret.Get(0).(func(context.Context, string, string, bool) error); ok { + r0 = rf(ctx, name, namespace, ignoreNotFound) } else { r0 = ret.Error(0) } @@ -282,13 +280,13 @@ func (_m *Controller) DeleteKubernetesDeployment(ctx context.Context, name strin return r0 } -// DeleteKubernetesService provides a mock function with given fields: ctx, svcName, namespace, timeout, ignoreNotFound -func (_m *Controller) DeleteKubernetesService(ctx context.Context, svcName string, namespace string, timeout time.Duration, ignoreNotFound bool) error { - ret := _m.Called(ctx, svcName, namespace, timeout, ignoreNotFound) +// DeleteKubernetesService provides a mock function with given fields: ctx, svcName, namespace, ignoreNotFound +func (_m *Controller) DeleteKubernetesService(ctx context.Context, svcName string, namespace string, ignoreNotFound bool) error { + ret := _m.Called(ctx, svcName, namespace, ignoreNotFound) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, time.Duration, bool) error); ok { - r0 = rf(ctx, svcName, namespace, timeout, ignoreNotFound) + if rf, ok := ret.Get(0).(func(context.Context, string, string, bool) error); ok { + r0 = rf(ctx, svcName, namespace, ignoreNotFound) } else { r0 = ret.Error(0) } diff --git a/api/turing/service/router_deployment_service.go b/api/turing/service/router_deployment_service.go index ee1b77845..14f03b964 100644 --- a/api/turing/service/router_deployment_service.go +++ b/api/turing/service/router_deployment_service.go @@ -234,7 +234,7 @@ func (ds *deploymentService) UndeployRouterVersion( eventsCh *EventChannel, isCleanUp bool, ) error { - ctx, cancel := context.WithTimeout(context.Background(), ds.deploymentTimeout) + ctx, cancel := context.WithTimeout(context.Background(), ds.deploymentDeletionTimeout) defer cancel() // Get the cluster controller controller, err := ds.getClusterControllerByEnvironment(environment.Name) @@ -266,7 +266,7 @@ func (ds *deploymentService) UndeployRouterVersion( if routerVersion.LogConfig.ResultLoggerType == models.BigQueryLogger { fluentdService := ds.svcBuilder.NewFluentdService(routerVersion, project, "", ds.routerDefaults.FluentdConfig) - err = deleteK8sService(controller, fluentdService, ds.deploymentTimeout, isCleanUp) + err = deleteK8sService(controller, fluentdService, isCleanUp) if err != nil { errs = append(errs, err.Error()) } @@ -286,7 +286,7 @@ func (ds *deploymentService) UndeployRouterVersion( // Delete experiment engine plugins server if routerVersion.ExperimentEngine.PluginConfig != nil { pluginsServerSvc := ds.svcBuilder.NewPluginsServerService(routerVersion, project) - err = deleteK8sService(controller, pluginsServerSvc, ds.deploymentTimeout, isCleanUp) + err = deleteK8sService(controller, pluginsServerSvc, isCleanUp) if err != nil { eventsCh.Write( models.NewErrorEvent( @@ -299,7 +299,7 @@ func (ds *deploymentService) UndeployRouterVersion( } // Delete all components - err = deleteKnServices(ctx, controller, services, ds.deploymentDeletionTimeout, eventsCh, isCleanUp) + err = deleteKnServices(ctx, controller, services, eventsCh, isCleanUp) if err != nil { errs = append(errs, err.Error()) } @@ -315,8 +315,6 @@ func (ds *deploymentService) DeleteRouterEndpoint( environment *merlin.Environment, routerVersion *models.RouterVersion, ) error { - ctx, cancel := context.WithTimeout(context.Background(), ds.deploymentTimeout) - defer cancel() // Get the cluster controller controller, err := ds.getClusterControllerByEnvironment(environment.Name) if err != nil { @@ -324,7 +322,7 @@ func (ds *deploymentService) DeleteRouterEndpoint( } routerEndpointName := fmt.Sprintf("%s-turing-router", routerVersion.Router.Name) - return controller.DeleteIstioVirtualService(ctx, routerEndpointName, project.Name, ds.deploymentDeletionTimeout) + return controller.DeleteIstioVirtualService(context.Background(), routerEndpointName, project.Name) } func (ds *deploymentService) getClusterControllerByEnvironment( @@ -482,14 +480,13 @@ func deployK8sService(ctx context.Context, controller cluster.Controller, servic func deleteK8sService( controller cluster.Controller, service *cluster.KubernetesService, - timeout time.Duration, isCleanUp bool, ) error { - err := controller.DeleteKubernetesDeployment(context.Background(), service.Name, service.Namespace, timeout, isCleanUp) + err := controller.DeleteKubernetesDeployment(context.Background(), service.Name, service.Namespace, isCleanUp) if err != nil { return err } - return controller.DeleteKubernetesService(context.Background(), service.Name, service.Namespace, timeout, isCleanUp) + return controller.DeleteKubernetesService(context.Background(), service.Name, service.Namespace, isCleanUp) } // createSecret creates a secret. @@ -554,7 +551,7 @@ func deployKnServices( eventsCh.Write(models.NewInfoEvent( models.EventStageDeployingServices, "deploying service %s", svc.Name)) if svc.ConfigMap != nil { - err := controller.ApplyConfigMap(context.Background(), svc.Namespace, svc.ConfigMap) + err := controller.ApplyConfigMap(ctx, svc.Namespace, svc.ConfigMap) if err != nil { err = errors.Wrapf(err, "Failed to apply config map %s", svc.ConfigMap.Name) eventsCh.Write(models.NewErrorEvent( @@ -590,7 +587,6 @@ func deleteKnServices( ctx context.Context, controller cluster.Controller, services []*cluster.KnativeService, - timeout time.Duration, eventsCh *EventChannel, isCleanUp bool, ) error { @@ -606,7 +602,7 @@ func deleteKnServices( eventsCh.Write(models.NewInfoEvent( models.EventStageUndeployingServices, "deleting service %s", svc.Name)) if svc.ConfigMap != nil { - err = controller.DeleteConfigMap(ctx, svc.ConfigMap.Name, svc.Namespace, isCleanUp) + err = controller.DeleteConfigMap(context.Background(), svc.ConfigMap.Name, svc.Namespace, isCleanUp) if err != nil { err = errors.Wrapf(err, "Failed to delete config map %s", svc.ConfigMap.Name) eventsCh.Write(models.NewErrorEvent( @@ -614,7 +610,7 @@ func deleteKnServices( errCh <- err } } - err = controller.DeleteKnativeService(ctx, svc.Name, svc.Namespace, timeout, isCleanUp) + err = controller.DeleteKnativeService(context.Background(), svc.Name, svc.Namespace, isCleanUp) if err != nil { err = errors.Wrapf(err, "Error when deleting %s", svc.Name) eventsCh.Write(models.NewErrorEvent( diff --git a/api/turing/service/router_deployment_service_test.go b/api/turing/service/router_deployment_service_test.go index 642feff46..d74338323 100644 --- a/api/turing/service/router_deployment_service_test.go +++ b/api/turing/service/router_deployment_service_test.go @@ -353,11 +353,11 @@ func TestDeleteEndpoint(t *testing.T) { // Create mock controller controller := &mocks.Controller{} controller.On("DeleteKnativeService", mock.Anything, mock.Anything, - mock.Anything, mock.Anything, false).Return(nil) + mock.Anything, false).Return(nil) controller.On("DeleteKubernetesDeployment", mock.Anything, mock.Anything, - mock.Anything, mock.Anything, false).Return(nil) + mock.Anything, false).Return(nil) controller.On("DeleteKubernetesService", mock.Anything, mock.Anything, - mock.Anything, mock.Anything, false).Return(nil) + mock.Anything, false).Return(nil) controller.On("DeleteSecret", mock.Anything, mock.Anything, mock.Anything, false).Return(nil) controller.On("DeleteConfigMap", mock.Anything, mock.Anything, mock.Anything, false).Return(nil) controller.On("DeletePersistentVolumeClaim", mock.Anything, mock.Anything, mock.Anything, false).Return(nil) @@ -410,11 +410,11 @@ func TestDeleteEndpoint(t *testing.T) { ) assert.NoError(t, err) controller.AssertCalled(t, "DeleteKubernetesService", - mock.Anything, "test-svc-fluentd-logger-1", testNs, timeout, false) + mock.Anything, "test-svc-fluentd-logger-1", testNs, false) controller.AssertCalled(t, "DeleteConfigMap", mock.Anything, "test-svc-fiber-config-1", testNs, false) - controller.AssertCalled(t, "DeleteKnativeService", mock.Anything, "test-svc-enricher-1", testNs, timeout, false) - controller.AssertCalled(t, "DeleteKnativeService", mock.Anything, "test-svc-ensembler-1", testNs, timeout, false) - controller.AssertCalled(t, "DeleteKnativeService", mock.Anything, "test-svc-router-1", testNs, timeout, false) + controller.AssertCalled(t, "DeleteKnativeService", mock.Anything, "test-svc-enricher-1", testNs, false) + controller.AssertCalled(t, "DeleteKnativeService", mock.Anything, "test-svc-ensembler-1", testNs, false) + controller.AssertCalled(t, "DeleteKnativeService", mock.Anything, "test-svc-router-1", testNs, false) controller.AssertCalled(t, "DeleteSecret", mock.Anything, "test-svc-svc-acct-secret-1", testNs, false) controller.AssertCalled(t, "DeletePersistentVolumeClaim", mock.Anything, "pvc", testNs, false) controller.AssertNumberOfCalls(t, "DeleteKnativeService", 3)