cloudrun server test #150

sourabhsinghs · 2025-01-28T16:24:42Z

No description provided.

sergiitk

A quick review, please address. Then I'll follow up with a more thorough review.

sergiitk · 2025-01-28T17:25:47Z

framework/test_app/runners/c6n/c6n_base_runner.py

+from typing import Optional
+
+import framework
+from framework.infrastructure import c6n


Is c6n this a common abbreviation? If no, we should call it cloud_run.

sergiitk · 2025-01-28T17:26:17Z

bin/run_test_server_c6n.py

@@ -0,0 +1,66 @@
+# Copyright 2020 gRPC authors.


please bump copyrights of the new files to 2025

sergiitk · 2025-01-28T17:26:48Z

framework/infrastructure/gcp/compute.py

+                #                "balancingMode": "CONNECTION",
+                #                "maxRatePerEndpoint": max_rate_per_endpoint,


please parametrize this, so it doesn't affect current tests

sergiitk · 2025-01-28T17:27:00Z

framework/infrastructure/gcp/compute.py

@@ -158,7 +158,7 @@ def create_backend_service_traffic_director(
        body = {
            "name": name,
            "loadBalancingScheme": "INTERNAL_SELF_MANAGED",  # Traffic Director
-            "healthChecks": [health_check.url],
+            # "healthChecks": [health_check.url],


please parametrize this, so it doesn't affect current tests

sergiitk · 2025-01-28T17:29:08Z

framework/infrastructure/gcp/compute.py

+            print(neg)
+            return neg
+
+        except Exception as e:


Too broad for a create_* function.

sergiitk · 2025-01-28T17:29:15Z

framework/infrastructure/gcp/compute.py

+                .execute()
+            )
+            neg = self.get_serverless_network_endpoint_group(name, region)
+            print(neg)


sergiitk · 2025-01-28T17:29:38Z

kubernetes-manifests/client.cloudrun.deployment.yaml

+        - name: grpc-td-conf
+          emptyDir:
+            medium: Memory
+...


add nl at EOF

sergiitk · 2025-01-28T17:29:57Z

kubernetes-manifests/client.deployment.yaml

@@ -36,7 +36,7 @@ spec:
            ## driver waiting for the container to start.
            failureThreshold: 1000
          args:
-            - "--server=${server_target}"
+            - "--server=psm-grpc-server:8080"


this will break existing tests

sergiitk · 2025-01-28T17:31:53Z

tests/bootstrap_generator_test.py

-GCR_TESTING: Final[
-    str
-] = "us-docker.pkg.dev/grpc-testing/trafficdirector/td-grpc-bootstrap"
+GCR_TESTING: Final[str] = (


You're probably using more a more recent version of black. Please reinstall packages with pip install -r requirements-dev.txt, and revert unrelated code changes

sergiitk

Added more comments.

sergiitk · 2025-01-28T21:01:16Z

framework/infrastructure/traffic_director.py

@@ -1282,3 +1285,125 @@ def _get_certificate_provider(cls):
                "pluginInstance": cls.CERTIFICATE_PROVIDER_INSTANCE,
            },
        }
+
+
+class TrafficDirectorCloudRunManager(TrafficDirectorAppNetManager):


Please move to a separate file, similar to traffic_director_gamma.py

sergiitk · 2025-01-28T21:04:40Z

kubernetes-manifests/client.cloudrun.deployment.yaml

+    spec:
+      containers:
+      - name: psm-grpc-client
+        image: docker.io/grpc/xds-example-cpp-client:v1.69.x


Seem like you removed most variables from the template (image, bootstrap image, deployment name, etc). Is this template supposed to be temporary? I'd highly prefer if we parametrized regular client.deployment.yaml instead,

sergiitk · 2025-01-28T21:05:37Z

tests/baseline_test_c6n_server.py

+class BaselineTest(xds_k8s_testcase.CloudRunXdsKubernetesTestCase):
+    def test_traffic_director_grpc_setup(self):


Please choose appropriate names for the test class and test case

sergiitk · 2025-01-28T21:06:31Z

framework/xds_flags.py

@@ -199,6 +199,12 @@
    help="Enable support for Dual Stack resources to the framework.",
 )

+REGION = flags.DEFINE_string(
+    "region",
+    default="us-central1",


Suggested change

default="us-central1",

default=None,

sergiitk · 2025-01-28T21:07:23Z

framework/xds_k8s_flags.py

@@ -33,6 +33,18 @@
    help="Traffic Director gRPC Bootstrap Docker image",
 )

+CLOUDRUN_SERVER_IMAGE = flags.DEFINE_string(


Why do we need separate images? I thought we'll be using our regular test server / test client.

sergiitk · 2025-01-28T21:10:58Z

framework/infrastructure/traffic_director.py

+
+    def create_grpc_route(self, src_host: str, src_port: int):
+        hostname = src_host
+        route_name = f"{self.resource_prefix}-grpc-route-{self.resource_suffix}"


is this different from TrafficDirectorAppNetManager.create_grpc_route? If so, please add a note explaining how

sergiitk · 2025-01-28T21:12:02Z

framework/test_app/runners/c6n/c6n_base_runner.py

+
+        https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity
+        """
+        return (


this looks the same as KubernetesBaseRunner._get_workload_identity_member_name

We don't run the cloud run service as a workload in the CSM tests (and I don't see this method called from anywhere). Only for SPIFFE tests the calling cloud run service will join the workload identity pool.

Makes sense. Looks like there are other methods in this class that are copy-pasted from KubernetesBaseRunner, but never called

sergiitk · 2025-01-28T21:13:28Z

framework/infrastructure/c6n.py

please move to gcp/

sergiitk · 2025-01-28T21:13:48Z

framework/infrastructure/c6n.py

+logger = logging.getLogger(__name__)
+
+
+class CloudRunApiManager:


consider extending GcpStandardCloudApiResource

sergiitk · 2025-01-28T21:15:45Z

framework/infrastructure/c6n.py

+
+        try:
+            operation = self._client.create_service(request=request)
+            self._service = operation.result(timeout=300)


Note that operation is not guaranteed to succeed on the first try. Instead, use GcpProjectApiResource.wait_for_operation

kannanjgithub · 2025-01-29T07:28:37Z

framework/test_app/runners/c6n/c6n_xds_server_runner.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""
+Run xDS Test Client on Kubernetes.


Fix the comment.

kannanjgithub · 2025-01-29T07:47:40Z

framework/test_app/runners/c6n/c6n_base_runner.py

+        self.run_history = collections.deque()
+
+        # Persistent across many runs.
+        self.run_history = collections.deque()


Repeated twice.

kannanjgithub · 2025-01-29T07:52:40Z

framework/test_app/runners/c6n/c6n_base_runner.py

+            self.service_name, self.image_name
+        )
+
+    def _start_completed(self):


This method is not called from anywhere.

Good catch. @sourabhsinghs - this should be called from CloudRunServerRunner, once you finished the deployment.

Note that you also want to call _stop from at the end of cleanup

sergiitk · 2025-01-30T03:03:57Z

framework/test_app/runners/c6n/c6n_base_runner.py

+            )
+
+        self._reset_state()
+        self.time_start_requested = dt.datetime.now(tz=dt.timezone.utc)


Why did you add tz=dt.timezone.utc everywhere when copying from KubernetesBaseRunner? We intentionally assume local timezone elsewhere. We even log it at the beginning

logger.info("Logs timezone: %s", time.localtime().tm_zone)

kannanjgithub · 2025-01-30T11:45:13Z

framework/test_app/runners/c6n/c6n_xds_server_runner.py

+        """Deploys and manages the xDS Test Server on Cloud Run."""
+        logger.info(self.service_name)
+        logger.info(self.image_name)
+        deployed_service_url = self.cloudrun_api_manager.deploy_service(


Did you intend to use the baseclass' run method which also has the cloudrun_api_manager.deploy_service call?

kannanjgithub · 2025-01-30T12:14:27Z

framework/infrastructure/c6n.py

+                        ports=[
+                            run_v2.ContainerPort(
+                                name="http1",
+                                container_port=50051,


I think this should be getting the value from self.server_xds_port?

kannanjgithub · 2025-01-30T12:36:23Z

framework/xds_k8s_testcase.py

+        return self._start_test_client(test_server.xds_uri, **kwargs)
+
+    def backend_service_add_serverless_neg_backends(self):
+        logger.info("Creating serverless NEG")


The cloud run service name needs to be added to the NEG

gcloud compute network-endpoint-groups create destination-neg --region=us-central1 --network-endpoint-type=serverless --cloud-run-service=$DEST_SERVICE

kannanjgithub · 2025-01-30T12:45:17Z

tests/url_map/affinity_test.py

-        ] = GcpResourceManager().affinity_backend_service()
+        path_matcher["defaultService"] = (
+            GcpResourceManager().affinity_backend_service()
+        )
        return host_rule, path_matcher


nit: Only whitespace diffs in this file, revert.

cloudrun server test

ad3d5ca

sourabhsinghs requested a review from a team as a code owner January 28, 2025 16:24

sourabhsinghs requested a review from sergiitk January 28, 2025 16:24

Merge branch 'main' into feature/cloudrun/server-test

45f84dc

sergiitk requested changes Jan 28, 2025

View reviewed changes

sergiitk reviewed Jan 28, 2025

View reviewed changes

kannanjgithub reviewed Jan 29, 2025

View reviewed changes

sergiitk reviewed Jan 30, 2025

View reviewed changes

kannanjgithub reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloudrun server test #150

cloudrun server test #150

sourabhsinghs commented Jan 28, 2025

sergiitk left a comment

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025 •

edited

Loading

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk left a comment

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

kannanjgithub Jan 29, 2025

sergiitk Jan 30, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

sergiitk Jan 28, 2025

kannanjgithub Jan 29, 2025

kannanjgithub Jan 29, 2025

kannanjgithub Jan 29, 2025

sergiitk Jan 30, 2025

sergiitk Jan 30, 2025

sergiitk Jan 30, 2025

kannanjgithub Jan 30, 2025 •

edited

Loading

kannanjgithub Jan 30, 2025

kannanjgithub Jan 30, 2025

kannanjgithub Jan 30, 2025

		# "balancingMode": "CONNECTION",
		# "maxRatePerEndpoint": max_rate_per_endpoint,

		class BaselineTest(xds_k8s_testcase.CloudRunXdsKubernetesTestCase):
		def test_traffic_director_grpc_setup(self):

		logger = logging.getLogger(__name__)


		class CloudRunApiManager:

cloudrun server test #150

Are you sure you want to change the base?

cloudrun server test #150

Conversation

sourabhsinghs commented Jan 28, 2025

sergiitk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiitk Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiitk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannanjgithub Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiitk Jan 28, 2025 •

edited

Loading

kannanjgithub Jan 30, 2025 •

edited

Loading