From 91b3624ad883db4815af3436bda60671aad5f3fb Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Tue, 13 Jun 2023 14:00:43 -0400 Subject: [PATCH 01/13] Add port forwarder closed channel and remove the service uuid entry from the service to local connection map when the connections closes. --- .../kurtosis_gateway/connection/connection.go | 16 +++++++++++++++- .../api_container_gateway_service_server.go | 6 ++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/cli/cli/kurtosis_gateway/connection/connection.go b/cli/cli/kurtosis_gateway/connection/connection.go index 833eac7009..6dc47abd1e 100644 --- a/cli/cli/kurtosis_gateway/connection/connection.go +++ b/cli/cli/kurtosis_gateway/connection/connection.go @@ -27,6 +27,7 @@ type GatewayConnectionToKurtosis interface { // GetLocalPorts returns a map keyed with an identifier string describing local ports being forwarded GetLocalPorts() map[string]*port_spec.PortSpec GetGrpcClientConn() (*grpc.ClientConn, error) + GetClosedChannel() chan(struct{}) Stop() } @@ -42,6 +43,8 @@ type gatewayConnectionToKurtosisImpl struct { portforwarderStopChannel chan struct{} portforwarderReadyChannel chan struct{} + // Connection to the pod closes + portforwarderClosedChannel chan struct{} // RemotePort -> port-spec ID remotePortNumberToPortSpecIdMap map[uint16]string @@ -56,6 +59,7 @@ func newLocalPortToPodPortConnection(kubernetesRestConfig *k8s_rest.Config, podP var portforwardStdErr bytes.Buffer portforwardStopChannel := make(chan struct{}, 1) portforwardReadyChannel := make(chan struct{}, 1) + portforwardClosedChannel := make(chan struct{}, 1) portForwardAddresses := []string{localHostIpStr} remotePortNumberToPortSpecIdMapping := map[uint16]string{} @@ -97,7 +101,12 @@ func newLocalPortToPodPortConnection(kubernetesRestConfig *k8s_rest.Config, podP // Start forwarding ports asynchronously go func() { if err := portForwarder.ForwardPorts(); err != nil { - logrus.Warnf("Expected to be able to start forwarding local ports to remote ports, instead our portforwarder has returned a non-nil err:\n%v", err) + if err == portforward.ErrLostConnectionToPod { + logrus.Infof("Lost connection to pod:\n%v", portForwarder) + close(portforwardClosedChannel) + } else { + logrus.Errorf("Expected to be able to start forwarding local ports to remote ports, instead our portforwarder has returned a non-nil err:\n%v", err) + } } }() // Wait for the portforwarder to be ready with timeout @@ -139,6 +148,7 @@ func newLocalPortToPodPortConnection(kubernetesRestConfig *k8s_rest.Config, podP portforwarderStdErr: portforwardStdErr, portforwarderStopChannel: portforwardStopChannel, portforwarderReadyChannel: portforwardReadyChannel, + portforwarderClosedChannel: portforwardClosedChannel, remotePortNumberToPortSpecIdMap: remotePortNumberToPortSpecIdMapping, urlString: podProxyEndpointUrl.String(), } @@ -154,6 +164,10 @@ func (connection *gatewayConnectionToKurtosisImpl) GetLocalPorts() map[string]*p return connection.localPorts } +func (connection *gatewayConnectionToKurtosisImpl) GetClosedChannel() chan(struct{}) { + return connection.portforwarderClosedChannel +} + // GetGrpcClientConn returns a client conn dialed in to the local port // It is the caller's responsibility to call resultClientConn.close() func (connection *gatewayConnectionToKurtosisImpl) GetGrpcClientConn() (resultClientConn *grpc.ClientConn, resultErr error) { diff --git a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go index 887b9c1976..24337ea850 100644 --- a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go +++ b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go @@ -413,6 +413,12 @@ func (service *ApiContainerGatewayServiceServer) startRunningConnectionForKurtos delete(service.userServiceGuidToLocalConnectionMap, serviceUuid) } }() + + go func() { + // Wait on closed connection and remove the service entry local connection from the user service to local connection map + <- runingLocalServiceConnection.kurtosisConnection.GetClosedChannel() + delete(service.userServiceGuidToLocalConnectionMap, serviceUuid) + }() cleanUpMapEntry = false cleanUpConnection = false From baa79dd03f4208183359cdbaa3a761267bc351a7 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Tue, 13 Jun 2023 14:01:46 -0400 Subject: [PATCH 02/13] Do not set the k8s service selector to nil when we stop the user service. --- .../user_services_functions/stop_user_services.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go index 6bde645a6a..c4c731ed17 100644 --- a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go +++ b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go @@ -7,7 +7,7 @@ import ( "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/enclave" "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/service" "github.com/kurtosis-tech/stacktrace" - applyconfigurationsv1 "k8s.io/client-go/applyconfigurations/core/v1" + //applyconfigurationsv1 "k8s.io/client-go/applyconfigurations/core/v1" ) func StopUserServices( @@ -46,7 +46,7 @@ func StopUserServices( } } - kubernetesService := resources.Service + /*kubernetesService := resources.Service serviceName := kubernetesService.Name updateConfigurator := func(updatesToApply *applyconfigurationsv1.ServiceApplyConfiguration) { specUpdates := applyconfigurationsv1.ServiceSpec().WithSelector(nil) @@ -60,7 +60,7 @@ func StopUserServices( namespaceName, ) continue - } + }*/ successfulUuids[serviceUuid] = true } From 4194f12b4b22b1aad08dbbbe565aec05407a9c95 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Tue, 13 Jun 2023 16:49:03 -0400 Subject: [PATCH 03/13] Do not set the k8s service selector to nil when the user service stops so we can easily re-create a pod to re-start the user service. --- .../stop_user_services.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go index c4c731ed17..dca225022e 100644 --- a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go +++ b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go @@ -46,22 +46,6 @@ func StopUserServices( } } - /*kubernetesService := resources.Service - serviceName := kubernetesService.Name - updateConfigurator := func(updatesToApply *applyconfigurationsv1.ServiceApplyConfiguration) { - specUpdates := applyconfigurationsv1.ServiceSpec().WithSelector(nil) - updatesToApply.WithSpec(specUpdates) - } - if _, err := kubernetesManager.UpdateService(ctx, namespaceName, serviceName, updateConfigurator); err != nil { - erroredUuids[serviceUuid] = stacktrace.Propagate( - err, - "An error occurred updating service '%v' in namespace '%v' to reflect that it's no longer running", - serviceName, - namespaceName, - ) - continue - }*/ - successfulUuids[serviceUuid] = true } return successfulUuids, erroredUuids, nil From 1ad5d1056d3accf0a342700f6807fd1cdbac64d6 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Tue, 13 Jun 2023 16:50:10 -0400 Subject: [PATCH 04/13] Enable start and stop service tests for k8s. --- .../startosis_start_service_test.go | 5 ----- .../startosis_stop_service_test.go | 5 ----- internal_testsuites/typescript/scripts/test.sh | 3 +-- 3 files changed, 1 insertion(+), 12 deletions(-) diff --git a/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go b/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go index df4f2711db..3a7a83d626 100644 --- a/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go +++ b/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go @@ -1,8 +1,3 @@ -//go:build !minikube -// +build !minikube - -// We don't run this test in Kubernetes because the gateway does not support start and stop services - package startosis_stop_service_test import ( diff --git a/internal_testsuites/golang/testsuite/startosis_stop_service_test/startosis_stop_service_test.go b/internal_testsuites/golang/testsuite/startosis_stop_service_test/startosis_stop_service_test.go index 06e18ef74a..8c3a005382 100644 --- a/internal_testsuites/golang/testsuite/startosis_stop_service_test/startosis_stop_service_test.go +++ b/internal_testsuites/golang/testsuite/startosis_stop_service_test/startosis_stop_service_test.go @@ -1,8 +1,3 @@ -//go:build !minikube -// +build !minikube - -// We don't run this test in Kubernetes because the gateway does not support start and stop services - package startosis_stop_service_test import ( diff --git a/internal_testsuites/typescript/scripts/test.sh b/internal_testsuites/typescript/scripts/test.sh index 39d2a45f58..e5bfa66686 100755 --- a/internal_testsuites/typescript/scripts/test.sh +++ b/internal_testsuites/typescript/scripts/test.sh @@ -20,8 +20,7 @@ DEFAULT_TESTSUITE_CLUSTER_BACKEND="${TESTSUITE_CLUSTER_BACKEND_DOCKER}" # network_partition_starlark,network_partition_test,network_soft_partition_test - Networking partitioning is not implemented in kubernetes # service_pause_test - Service pausing not implemented in Kubernetes # stream_log_test,search_log_test - The centralized logs feature is not implemented in Kubernetes yet -# startosis_start_service_test,startosis_stop_service_test - The start and stop service feature does not work in Kubernetes due to a required update in the gateway -KUBERNETES_TEST_IGNORE_PATTERNS="/build/testsuite/(network_partition_starlark|network_partition_test|network_soft_partition_test|service_pause_test|stream_log_test|search_logs_test|startosis_start_service_test|startosis_stop_service_test)" +KUBERNETES_TEST_IGNORE_PATTERNS="/build/testsuite/(network_partition_starlark|network_partition_test|network_soft_partition_test|service_pause_test|stream_log_test|search_logs_test)" # ================================================================================================== # Arg Parsing & Validation From 75b631c6393e492c9f57f89883e826cab24dfc72 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Tue, 13 Jun 2023 16:50:30 -0400 Subject: [PATCH 05/13] Add log msg. --- cli/cli/kurtosis_gateway/connection/connection.go | 2 +- .../api_container_gateway_service_server.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cli/cli/kurtosis_gateway/connection/connection.go b/cli/cli/kurtosis_gateway/connection/connection.go index 6dc47abd1e..d08125562f 100644 --- a/cli/cli/kurtosis_gateway/connection/connection.go +++ b/cli/cli/kurtosis_gateway/connection/connection.go @@ -102,7 +102,7 @@ func newLocalPortToPodPortConnection(kubernetesRestConfig *k8s_rest.Config, podP go func() { if err := portForwarder.ForwardPorts(); err != nil { if err == portforward.ErrLostConnectionToPod { - logrus.Infof("Lost connection to pod:\n%v", portForwarder) + logrus.Infof("Lost connection to pod: %s", podProxyEndpointUrl.String()) close(portforwardClosedChannel) } else { logrus.Errorf("Expected to be able to start forwarding local ports to remote ports, instead our portforwarder has returned a non-nil err:\n%v", err) diff --git a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go index 24337ea850..a65d331c1a 100644 --- a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go +++ b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go @@ -415,9 +415,10 @@ func (service *ApiContainerGatewayServiceServer) startRunningConnectionForKurtos }() go func() { - // Wait on closed connection and remove the service entry local connection from the user service to local connection map + // Wait on closed connection and remove the service entry from the user service to local connection map <- runingLocalServiceConnection.kurtosisConnection.GetClosedChannel() delete(service.userServiceGuidToLocalConnectionMap, serviceUuid) + logrus.Debugf("Remove service with guid '%v' from the user service to local connection map", serviceUuid) }() cleanUpMapEntry = false From 76b264ee8be08600a58903decc1ba3da589c7e68 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Tue, 13 Jun 2023 17:05:29 -0400 Subject: [PATCH 06/13] Remove caution in docs now that the start/stop service instruction works for both backends. --- docs/docs/starlark-reference/plan.md | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/docs/docs/starlark-reference/plan.md b/docs/docs/starlark-reference/plan.md index 7edabede6b..6b612ee988 100644 --- a/docs/docs/starlark-reference/plan.md +++ b/docs/docs/starlark-reference/plan.md @@ -444,13 +444,6 @@ plan.start_service( ) ``` -:::caution - -`start_service` is only available with the Docker backend. - -::: - - stop_service ------------ @@ -464,12 +457,6 @@ plan.stop_service( ) ``` -:::caution - -`stop_service` is only available with the Docker backend. - -::: - store_service_files ------------------- From 1e5cd1e99ec81e4d671a6605daed6051eb185b4d Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Wed, 14 Jun 2023 10:26:34 -0400 Subject: [PATCH 07/13] Update start test to run stop and start in the same script. --- .../startosis_start_service_test.go | 41 +++++-------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go b/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go index 3a7a83d626..9a4f3a74e1 100644 --- a/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go +++ b/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go @@ -39,17 +39,19 @@ def run(plan): plan.add_service(name = DATASTORE_SERVICE_NAME, config = config) plan.print("Service " + DATASTORE_SERVICE_NAME + " deployed successfully.") ` - // We start the service we created through the script above with a different script - startScript = ` + + // We stop and restart the service we created through the script above with a different script + stopAndStartScript = ` DATASTORE_SERVICE_NAME = "` + serviceName + `" def run(plan): + plan.stop_service(DATASTORE_SERVICE_NAME) plan.start_service(DATASTORE_SERVICE_NAME) ` - // We stop the service we created through the script above with a different script - stopScript = ` + // We start the service we created through the script above with a different script + startScript = ` DATASTORE_SERVICE_NAME = "` + serviceName + `" def run(plan): - plan.stop_service(DATASTORE_SERVICE_NAME) + plan.start_service(DATASTORE_SERVICE_NAME) ` ) @@ -93,7 +95,6 @@ Service example-datastore-server-1 deployed successfully. "Error validating datastore server '%s' is healthy", serviceName, ) - logrus.Infof("Validated that all services are healthy") // we run the start script and validate that an error is returned since the service is already started. @@ -104,8 +105,8 @@ Service example-datastore-server-1 deployed successfully. expectedErrorStr := fmt.Sprintf("Service '%v' is already started", serviceName) require.Contains(t, runResult.ExecutionError.ErrorMessage, expectedErrorStr) - // we run the stop script and validate that the service is unreachable. - runResult, err = test_helpers.RunScriptWithDefaultConfig(ctx, enclaveCtx, stopScript) + // we run the stop and restart script and validate that the service is unreachable. + runResult, err = test_helpers.RunScriptWithDefaultConfig(ctx, enclaveCtx, stopAndStartScript) require.NoError(t, err, "Unexpected error executing stop script") require.Nil(t, runResult.InterpretationError, "Unexpected interpretation error") @@ -116,35 +117,13 @@ Service example-datastore-server-1 deployed successfully. ` require.Regexp(t, expectedScriptOutput, string(runResult.RunOutput)) - require.Error( - t, - test_helpers.ValidateDatastoreServiceHealthy(context.Background(), enclaveCtx, serviceName, portId), - "Error validating datastore server '%s' is not healthy", - serviceName, - ) - - logrus.Infof("Validated that the service is stopped") - - // we run the start script and validate that the service is ready - runResult, err = test_helpers.RunScriptWithDefaultConfig(ctx, enclaveCtx, startScript) - require.NoError(t, err, "Unexpected error executing start script") - - require.Nil(t, runResult.InterpretationError, "Unexpected interpretation error") - require.Empty(t, runResult.ValidationErrors, "Unexpected validation error") - require.Nil(t, runResult.ExecutionError, "Unexpected execution error") - - expectedScriptOutput = `Service 'example-datastore-server-1' started -` - require.Regexp(t, expectedScriptOutput, string(runResult.RunOutput)) - require.NoError( t, test_helpers.ValidateDatastoreServiceHealthy(context.Background(), enclaveCtx, serviceName, portId), "Error validating datastore server '%s' is healthy", serviceName, ) - - logrus.Infof("Validated that the service is started") + logrus.Infof("Validated that the service is restarted") // we run the start script and validate that an error is returned since the service is already started. runResult, _ = test_helpers.RunScriptWithDefaultConfig(ctx, enclaveCtx, startScript) From 8023cf637332013abc7abc6cc5f078062d630bae Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Wed, 21 Jun 2023 16:10:24 +0200 Subject: [PATCH 08/13] Revert some uneeded changes. --- cli/cli/kurtosis_gateway/connection/connection.go | 1 - .../api_container_gateway_service_server.go | 7 ------- .../kubernetes/kubernetes_manager/kubernetes_manager.go | 2 +- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/cli/cli/kurtosis_gateway/connection/connection.go b/cli/cli/kurtosis_gateway/connection/connection.go index d08125562f..5fc2986735 100644 --- a/cli/cli/kurtosis_gateway/connection/connection.go +++ b/cli/cli/kurtosis_gateway/connection/connection.go @@ -27,7 +27,6 @@ type GatewayConnectionToKurtosis interface { // GetLocalPorts returns a map keyed with an identifier string describing local ports being forwarded GetLocalPorts() map[string]*port_spec.PortSpec GetGrpcClientConn() (*grpc.ClientConn, error) - GetClosedChannel() chan(struct{}) Stop() } diff --git a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go index df35513898..edcfe065a8 100644 --- a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go +++ b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go @@ -389,13 +389,6 @@ func (service *ApiContainerGatewayServiceServer) startRunningConnectionForKurtos delete(service.userServiceGuidToLocalConnectionMap, serviceUuid) } }() - - go func() { - // Wait on closed connection and remove the service entry from the user service to local connection map - <- runingLocalServiceConnection.kurtosisConnection.GetClosedChannel() - delete(service.userServiceGuidToLocalConnectionMap, serviceUuid) - logrus.Debugf("Remove service with guid '%v' from the user service to local connection map", serviceUuid) - }() cleanUpMapEntry = false cleanUpConnection = false diff --git a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go index 7646a0c942..c274f2351e 100644 --- a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go +++ b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go @@ -1051,7 +1051,7 @@ func (manager *KubernetesManager) CreatePod( Containers: podContainers, EphemeralContainers: nil, // We don't want Kubernetes auto-magically restarting our containers if they fail - RestartPolicy: apiv1.RestartPolicyNever, + RestartPolicy: apiv1.RestartPolicyAlways, TerminationGracePeriodSeconds: nil, ActiveDeadlineSeconds: nil, DNSPolicy: "", From ee019fbbbc24a9163810f77180f31e7625a8b156 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Wed, 21 Jun 2023 16:30:26 +0200 Subject: [PATCH 09/13] Revert some uneeded changes. --- .../user_services_functions/stop_user_services.go | 1 - .../kubernetes/kubernetes_manager/kubernetes_manager.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go index dca225022e..15434f5ece 100644 --- a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go +++ b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_kurtosis_backend/user_services_functions/stop_user_services.go @@ -7,7 +7,6 @@ import ( "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/enclave" "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/service" "github.com/kurtosis-tech/stacktrace" - //applyconfigurationsv1 "k8s.io/client-go/applyconfigurations/core/v1" ) func StopUserServices( diff --git a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go index afb3fdbc80..3599fbf29d 100644 --- a/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go +++ b/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager/kubernetes_manager.go @@ -1064,7 +1064,7 @@ func (manager *KubernetesManager) CreatePod( Containers: podContainers, EphemeralContainers: nil, // We don't want Kubernetes auto-magically restarting our containers if they fail - RestartPolicy: apiv1.RestartPolicyAlways, + RestartPolicy: apiv1.RestartPolicyNever, TerminationGracePeriodSeconds: nil, ActiveDeadlineSeconds: nil, DNSPolicy: "", From 5389b0d25cba942c0ebfd65a6a950feb71e8b15f Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Thu, 22 Jun 2023 10:49:22 +0200 Subject: [PATCH 10/13] Do not try to create a port forward connection to stopped services. --- .../kurtosis_gateway/connection/provider.go | 13 +++++++--- .../api_container_gateway_service_server.go | 26 ++++++++++++------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/cli/cli/kurtosis_gateway/connection/provider.go b/cli/cli/kurtosis_gateway/connection/provider.go index baae3b5967..3d33c98699 100644 --- a/cli/cli/kurtosis_gateway/connection/provider.go +++ b/cli/cli/kurtosis_gateway/connection/provider.go @@ -93,10 +93,12 @@ func (provider *GatewayConnectionProvider) ForEnclaveApiContainer(enclaveInfo *k return apiContainerConnection, nil } -func (provider *GatewayConnectionProvider) ForUserService(enclaveId string, serviceUuid string, servicePortSpecs map[string]*port_spec.PortSpec) (GatewayConnectionToKurtosis, error) { - podPortforwardEndpoint, err := provider.getUserServicePodPortforwardEndpoint(enclaveId, serviceUuid) +func (provider *GatewayConnectionProvider) ForMaybeUserService(enclaveId string, serviceUuid string, servicePortSpecs map[string]*port_spec.PortSpec) (GatewayConnectionToKurtosis, error) { + podPortforwardEndpoint, err := provider.getMaybeUserServicePodPortforwardEndpoint(enclaveId, serviceUuid) if err != nil { return nil, stacktrace.Propagate(err, "Expected to be able to find an api endpoint for Kubernetes portforward to a Kurtosis user service with id '%v' in enclave '%v', instead a non-nil error was returned", enclaveId, serviceUuid) + } else if podPortforwardEndpoint == nil { + return nil, nil } userServiceConnection, err := newLocalPortToPodPortConnection(provider.config, podPortforwardEndpoint, servicePortSpecs) if err != nil { @@ -167,7 +169,7 @@ func (provider *GatewayConnectionProvider) getApiContainerPodPortforwardEndpoint return provider.kubernetesManager.GetPodPortforwardEndpointUrl(enclaveNamespaceName, apiContainerPodName), nil } -func (provider *GatewayConnectionProvider) getUserServicePodPortforwardEndpoint(enclaveId string, serviceUuid string) (*url.URL, error) { +func (provider *GatewayConnectionProvider) getMaybeUserServicePodPortforwardEndpoint(enclaveId string, serviceUuid string) (*url.URL, error) { userServiceNamespaceName, err := provider.getEnclaveNamespaceNameForEnclaveId(enclaveId) if err != nil { return nil, stacktrace.Propagate(err, "Expected to be able to get a Kubernetes namespace corresponding to a Kurtosis enclave with id '%v', instead a non-nil error was returned", enclaveId) @@ -181,7 +183,10 @@ func (provider *GatewayConnectionProvider) getUserServicePodPortforwardEndpoint( if err != nil { return nil, stacktrace.Propagate(err, "Expected to be able to get running user service pods with labels '%+v' in namespace '%v', instead a non nil error was returned", runningUserServicePodNames, userServiceNamespaceName) } - if len(runningUserServicePodNames) != 1 { + if len(runningUserServicePodNames) == 0 { + // A service with no pod running is a stopped service + return nil, nil + } else if len(runningUserServicePodNames) != 1 { return nil, stacktrace.NewError("Expected to find exactly 1 running user service pod with guid '%v' in enclave '%v', instead found '%v'", serviceUuid, enclaveId, len(runningUserServicePodNames)) } userServicePodName := runningUserServicePodNames[0] diff --git a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go index aea6da1ada..f8e9ea1f43 100644 --- a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go +++ b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go @@ -2,6 +2,9 @@ package api_container_gateway import ( "context" + "io" + "sync" + "github.com/kurtosis-tech/kurtosis/api/golang/core/kurtosis_core_rpc_api_bindings" "github.com/kurtosis-tech/kurtosis/api/golang/core/lib/binding_constructors" "github.com/kurtosis-tech/kurtosis/cli/cli/kurtosis_gateway/connection" @@ -10,8 +13,6 @@ import ( "github.com/sirupsen/logrus" "google.golang.org/grpc" "google.golang.org/protobuf/types/known/emptypb" - "io" - "sync" ) const ( @@ -123,7 +124,7 @@ func (service *ApiContainerGatewayServiceServer) AddServices(ctx context.Context // Write over the PublicIp and Public Ports fields so the service can be accessed through local port forwarding for serviceNameStr, serviceInfo := range remoteApiContainerResponse.GetSuccessfulServiceNameToServiceInfo() { - if err := service.writeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo); err != nil { + if err := service.writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo); err != nil { err = stacktrace.Propagate(err, "Expected to be able to write over service info fields for service '%v', instead a non-nil error was returned", serviceNameStr) failedServicesPool[serviceNameStr] = err.Error() } @@ -291,9 +292,10 @@ func (service *ApiContainerGatewayServiceServer) UploadStarlarkPackage(server ku // // ==================================================================================================== -// writeOverServiceInfoFieldsWithLocalConnectionInformation overwites the `MaybePublicPorts` and `MaybePublicIpAdrr` fields to connect to local ports forwarding requests to private ports in Kubernetes +// writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation overwites the `MaybePublicPorts` and `MaybePublicIpAdrr` fields to connect to local ports forwarding requests to private ports in Kubernetes // Only TCP Private Ports are forwarded -func (service *ApiContainerGatewayServiceServer) writeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo *kurtosis_core_rpc_api_bindings.ServiceInfo) error { +// Does nothing if the service is stopped (no pod running) +func (service *ApiContainerGatewayServiceServer) writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo *kurtosis_core_rpc_api_bindings.ServiceInfo) error { // If the service has no private ports, then don't overwrite any of the service info fields if len(serviceInfo.PrivatePorts) == 0 { return nil @@ -305,9 +307,11 @@ func (service *ApiContainerGatewayServiceServer) writeOverServiceInfoFieldsWithL cleanUpConnection := true runningLocalConnection, isFound := service.userServiceGuidToLocalConnectionMap[serviceUuid] if !isFound { - runningLocalConnection, localConnErr = service.startRunningConnectionForKurtosisService(serviceUuid, serviceInfo.PrivatePorts) + runningLocalConnection, localConnErr = service.startMaybeRunningConnectionForKurtosisService(serviceUuid, serviceInfo.PrivatePorts) if localConnErr != nil { return stacktrace.Propagate(localConnErr, "Expected to be able to start a local connection to Kurtosis service '%v', instead a non-nil error was returned", serviceUuid) + } else if runningLocalConnection == nil { + return nil } defer func() { if cleanUpConnection { @@ -322,9 +326,9 @@ func (service *ApiContainerGatewayServiceServer) writeOverServiceInfoFieldsWithL return nil } -// startRunningConnectionForKurtosisService starts a port forwarding process from kernel assigned local ports to the remote service ports specified +// startMaybeRunningConnectionForKurtosisService starts a port forwarding process from kernel assigned local ports to the remote service ports specified // If privatePortsFromApi is empty, an error is thrown -func (service *ApiContainerGatewayServiceServer) startRunningConnectionForKurtosisService(serviceUuid string, privatePortsFromApi map[string]*kurtosis_core_rpc_api_bindings.Port) (*runningLocalServiceConnection, error) { +func (service *ApiContainerGatewayServiceServer) startMaybeRunningConnectionForKurtosisService(serviceUuid string, privatePortsFromApi map[string]*kurtosis_core_rpc_api_bindings.Port) (*runningLocalServiceConnection, error) { if len(privatePortsFromApi) == 0 { return nil, stacktrace.NewError("Expected Kurtosis service to have private ports specified for port forwarding, instead no ports were provided") } @@ -351,9 +355,11 @@ func (service *ApiContainerGatewayServiceServer) startRunningConnectionForKurtos } // Start listening - serviceConnection, err := service.connectionProvider.ForUserService(service.enclaveId, serviceUuid, remotePrivatePortSpecs) + serviceConnection, err := service.connectionProvider.ForMaybeUserService(service.enclaveId, serviceUuid, remotePrivatePortSpecs) if err != nil { return nil, stacktrace.Propagate(err, "Expected to be able to start a local connection service with guid '%v' in enclave '%v', instead a non-nil error was returned", serviceUuid, service.enclaveId) + } else if serviceConnection == nil { + return nil, nil } cleanUpConnection := true defer func() { @@ -414,7 +420,7 @@ func (service *ApiContainerGatewayServiceServer) updateServicesLocalConnection(s serviceUuids := map[string]bool{} for serviceId, serviceInfo := range serviceInfos { - if err := service.writeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo); err != nil { + if err := service.writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo); err != nil { return stacktrace.Propagate(err, "Expected to be able to write over service info fields for service '%v', instead a non-nil error was returned", serviceId) } serviceUuids[serviceInfo.GetServiceUuid()] = true From 1b2ccd497046ab75fd0942319a885ac0058aaea8 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Thu, 22 Jun 2023 11:46:09 +0200 Subject: [PATCH 11/13] Clarify comments. --- cli/cli/kurtosis_gateway/connection/provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/cli/kurtosis_gateway/connection/provider.go b/cli/cli/kurtosis_gateway/connection/provider.go index 3d33c98699..9967adf361 100644 --- a/cli/cli/kurtosis_gateway/connection/provider.go +++ b/cli/cli/kurtosis_gateway/connection/provider.go @@ -184,7 +184,7 @@ func (provider *GatewayConnectionProvider) getMaybeUserServicePodPortforwardEndp return nil, stacktrace.Propagate(err, "Expected to be able to get running user service pods with labels '%+v' in namespace '%v', instead a non nil error was returned", runningUserServicePodNames, userServiceNamespaceName) } if len(runningUserServicePodNames) == 0 { - // A service with no pod running is a stopped service + // A stopped service has no pod running and no port forward endpoint return nil, nil } else if len(runningUserServicePodNames) != 1 { return nil, stacktrace.NewError("Expected to find exactly 1 running user service pod with guid '%v' in enclave '%v', instead found '%v'", serviceUuid, enclaveId, len(runningUserServicePodNames)) From 30bbd2f8349f8de649183b85de5d636936e73749 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Thu, 22 Jun 2023 11:55:00 +0200 Subject: [PATCH 12/13] Linting. --- .../startosis_start_service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go b/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go index 7ba212754f..2132e3dff5 100644 --- a/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go +++ b/internal_testsuites/golang/testsuite/startosis_start_service_test/startosis_start_service_test.go @@ -37,7 +37,7 @@ def run(plan): plan.add_service(name = DATASTORE_SERVICE_NAME, config = config) plan.print("Service " + DATASTORE_SERVICE_NAME + " deployed successfully.") ` - + // We stop and restart the service we created through the script above with a different script stopAndStartScript = ` DATASTORE_SERVICE_NAME = "` + serviceName + `" From e6de4be07ec628ba533c1b18e4981cb9864da2c8 Mon Sep 17 00:00:00 2001 From: Laurent Luce Date: Fri, 23 Jun 2023 13:14:08 +0200 Subject: [PATCH 13/13] Rename functions to use an IfRunning suffix instead of the confusing Maybe. --- cli/cli/kurtosis_gateway/connection/provider.go | 5 +++-- .../api_container_gateway_service_server.go | 16 ++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/cli/cli/kurtosis_gateway/connection/provider.go b/cli/cli/kurtosis_gateway/connection/provider.go index 9967adf361..da2744b412 100644 --- a/cli/cli/kurtosis_gateway/connection/provider.go +++ b/cli/cli/kurtosis_gateway/connection/provider.go @@ -2,6 +2,8 @@ package connection import ( "context" + "net/url" + "github.com/kurtosis-tech/kurtosis/api/golang/engine/kurtosis_engine_rpc_api_bindings" "github.com/kurtosis-tech/kurtosis/api/golang/engine/lib/kurtosis_context" "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_impls/kubernetes/kubernetes_manager" @@ -15,7 +17,6 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" restclient "k8s.io/client-go/rest" - "net/url" ) const ( @@ -93,7 +94,7 @@ func (provider *GatewayConnectionProvider) ForEnclaveApiContainer(enclaveInfo *k return apiContainerConnection, nil } -func (provider *GatewayConnectionProvider) ForMaybeUserService(enclaveId string, serviceUuid string, servicePortSpecs map[string]*port_spec.PortSpec) (GatewayConnectionToKurtosis, error) { +func (provider *GatewayConnectionProvider) ForUserServiceIfRunning(enclaveId string, serviceUuid string, servicePortSpecs map[string]*port_spec.PortSpec) (GatewayConnectionToKurtosis, error) { podPortforwardEndpoint, err := provider.getMaybeUserServicePodPortforwardEndpoint(enclaveId, serviceUuid) if err != nil { return nil, stacktrace.Propagate(err, "Expected to be able to find an api endpoint for Kubernetes portforward to a Kurtosis user service with id '%v' in enclave '%v', instead a non-nil error was returned", enclaveId, serviceUuid) diff --git a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go index f8e9ea1f43..587aaf99dc 100644 --- a/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go +++ b/cli/cli/kurtosis_gateway/server/api_container_gateway/api_container_gateway_service_server.go @@ -124,7 +124,7 @@ func (service *ApiContainerGatewayServiceServer) AddServices(ctx context.Context // Write over the PublicIp and Public Ports fields so the service can be accessed through local port forwarding for serviceNameStr, serviceInfo := range remoteApiContainerResponse.GetSuccessfulServiceNameToServiceInfo() { - if err := service.writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo); err != nil { + if err := service.writeOverServiceInfoFieldsWithLocalConnectionInformationIfServiceRunning(serviceInfo); err != nil { err = stacktrace.Propagate(err, "Expected to be able to write over service info fields for service '%v', instead a non-nil error was returned", serviceNameStr) failedServicesPool[serviceNameStr] = err.Error() } @@ -292,10 +292,10 @@ func (service *ApiContainerGatewayServiceServer) UploadStarlarkPackage(server ku // // ==================================================================================================== -// writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation overwites the `MaybePublicPorts` and `MaybePublicIpAdrr` fields to connect to local ports forwarding requests to private ports in Kubernetes +// writeOverServiceInfoFieldsWithLocalConnectionInformationIfServiceRunning overwites the `MaybePublicPorts` and `MaybePublicIpAdrr` fields to connect to local ports forwarding requests to private ports in Kubernetes // Only TCP Private Ports are forwarded // Does nothing if the service is stopped (no pod running) -func (service *ApiContainerGatewayServiceServer) writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo *kurtosis_core_rpc_api_bindings.ServiceInfo) error { +func (service *ApiContainerGatewayServiceServer) writeOverServiceInfoFieldsWithLocalConnectionInformationIfServiceRunning(serviceInfo *kurtosis_core_rpc_api_bindings.ServiceInfo) error { // If the service has no private ports, then don't overwrite any of the service info fields if len(serviceInfo.PrivatePorts) == 0 { return nil @@ -307,7 +307,7 @@ func (service *ApiContainerGatewayServiceServer) writeMaybeOverServiceInfoFields cleanUpConnection := true runningLocalConnection, isFound := service.userServiceGuidToLocalConnectionMap[serviceUuid] if !isFound { - runningLocalConnection, localConnErr = service.startMaybeRunningConnectionForKurtosisService(serviceUuid, serviceInfo.PrivatePorts) + runningLocalConnection, localConnErr = service.startRunningConnectionForKurtosisServiceIfRunning(serviceUuid, serviceInfo.PrivatePorts) if localConnErr != nil { return stacktrace.Propagate(localConnErr, "Expected to be able to start a local connection to Kurtosis service '%v', instead a non-nil error was returned", serviceUuid) } else if runningLocalConnection == nil { @@ -326,9 +326,9 @@ func (service *ApiContainerGatewayServiceServer) writeMaybeOverServiceInfoFields return nil } -// startMaybeRunningConnectionForKurtosisService starts a port forwarding process from kernel assigned local ports to the remote service ports specified +// startRunningConnectionForKurtosisServiceIfRunning starts a port forwarding process from kernel assigned local ports to the remote service ports specified // If privatePortsFromApi is empty, an error is thrown -func (service *ApiContainerGatewayServiceServer) startMaybeRunningConnectionForKurtosisService(serviceUuid string, privatePortsFromApi map[string]*kurtosis_core_rpc_api_bindings.Port) (*runningLocalServiceConnection, error) { +func (service *ApiContainerGatewayServiceServer) startRunningConnectionForKurtosisServiceIfRunning(serviceUuid string, privatePortsFromApi map[string]*kurtosis_core_rpc_api_bindings.Port) (*runningLocalServiceConnection, error) { if len(privatePortsFromApi) == 0 { return nil, stacktrace.NewError("Expected Kurtosis service to have private ports specified for port forwarding, instead no ports were provided") } @@ -355,7 +355,7 @@ func (service *ApiContainerGatewayServiceServer) startMaybeRunningConnectionForK } // Start listening - serviceConnection, err := service.connectionProvider.ForMaybeUserService(service.enclaveId, serviceUuid, remotePrivatePortSpecs) + serviceConnection, err := service.connectionProvider.ForUserServiceIfRunning(service.enclaveId, serviceUuid, remotePrivatePortSpecs) if err != nil { return nil, stacktrace.Propagate(err, "Expected to be able to start a local connection service with guid '%v' in enclave '%v', instead a non-nil error was returned", serviceUuid, service.enclaveId) } else if serviceConnection == nil { @@ -420,7 +420,7 @@ func (service *ApiContainerGatewayServiceServer) updateServicesLocalConnection(s serviceUuids := map[string]bool{} for serviceId, serviceInfo := range serviceInfos { - if err := service.writeMaybeOverServiceInfoFieldsWithLocalConnectionInformation(serviceInfo); err != nil { + if err := service.writeOverServiceInfoFieldsWithLocalConnectionInformationIfServiceRunning(serviceInfo); err != nil { return stacktrace.Propagate(err, "Expected to be able to write over service info fields for service '%v', instead a non-nil error was returned", serviceId) } serviceUuids[serviceInfo.GetServiceUuid()] = true