[PSM interop] Introduce isort/pylint to the GKE framework #29476

lidizheng · 2022-04-21T21:38:49Z

This PR introduces the PSM GKE framework to:

isort
pylint

This PR also tries to fix the sanity complains if possible, but there were lots of places where the best fix is ambiguous: https://paste.googleplex.com/5988734773231616 (pylint has ~200 complains). In those ambiguous cases, this PR disables pylint, folks, please lift the disable comments during daily coding (boy-scout rule). Here are a few note-worthy ones:

"arguments-differ"/"signature-differs": The child class overriding the parent class but not respecting parent's signature. Several cases under the "framework" folder.
"abstract-method": We have abstract class depends on abstract class, where abstract methods weren't necessarily being implemented.
"unsubscriptable-object": See [Python 3.9] Value 'Optional' is unsubscriptable (unsubscriptable-object) (also Union) pylint-dev/pylint#3882

Our pytype sanity tests might need more work. Currently, we are using a 2019 release of pytype, which don't support Python > 3.7. On the other hand, the latest pytype don't support Python < 3.7. It requires more delicate testing to sort out what version of pytype + Python suits our infra.

Also, the latest pytype release found more issues with our existing code, the upgrade won't be easy.

lidizheng · 2022-04-21T23:14:02Z

sergiitk

This is great, without any exaggeration. Thank you so much for the hard work.

I left a few comments/notes, but this is almost good to go.

sergiitk · 2022-04-29T16:56:14Z

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/iam.py

@@ -54,7 +53,7 @@ def _replace_binding(policy: 'Policy', binding: 'Policy.Binding',
    new_bindings = set(policy.bindings)
    new_bindings.discard(binding)
    new_bindings.add(new_binding)
-    return dataclasses.replace(policy, bindings=frozenset(new_bindings))
+    return dataclasses.replace(policy, bindings=frozenset(new_bindings))  # pylint: disable=too-many-function-args


Wait, what's wrong with this?

Good catch. This is still a mystery to me. Here is the raw pylint complaint:

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/iam.py:56:11: E1121: Too many positional arguments for function call (too-many-function-args)

Our usage complies with official doc: https://docs.python.org/3/library/dataclasses.html#dataclasses.replace

I suspect it might be a PyLint issue with dataclasses library.

Yea, I have the same suspicion. Probably the same as pylint-dev/pylint#3492

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/network_security.py

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/network_services.py

tools/run_tests/xds_k8s_test_driver/framework/xds_url_map_test_resources.py

sergiitk · 2022-04-29T17:27:19Z

tools/run_tests/xds_k8s_test_driver/framework/xds_url_map_testcase.py

@@ -102,7 +102,7 @@ def __init__(self, xds_json: JsonType):
                    for endpoint in xds_config['endpointConfig'][
                            'dynamicEndpointConfigs']:
                        self.eds.append(endpoint['endpointConfig'])
-            except Exception as e:
+            except Exception as e:  # pylint: disable=broad-except


Here and below:
Do we really need catching Exception here? Looking at the code, it can be replaced with LookupError. But I may be wrong.

Added as TODO. Let's do it in another PR, in case the narrowing of the catch scope causing tests to fail.

sergiitk · 2022-04-29T17:39:25Z

tools/run_tests/xds_k8s_test_driver/tests/failover_test.py

+        default_test_servers: List[_XdsTestServer] = []
+        alternate_test_servers: List[_XdsTestServer] = []


I don't think it's required to assign the value, you should be able to just specify the type.

Suggested change

default_test_servers: List[_XdsTestServer] = []

alternate_test_servers: List[_XdsTestServer] = []

default_test_servers: List[_XdsTestServer]

alternate_test_servers: List[_XdsTestServer]

I was afraid that in case of non-failfast run, we might carry a non-List value, then cause later tests to fail (since they assumed this value to be List, but seen None). To make the typing work, we will need to wrap it with Optional.

I found giving it an initial value saves a few lines of code (don't need to do null checks).

From what I see, declaring the type doesn't necessary assign it to None:

Python 3.10.4 (main, Mar 25 2022, 04:37:13) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from typing import List # not defined - error >>> print(default_test_servers) Traceback (most recent call last): File "<stdin>", line 1, in <module> NameError: name 'default_test_servers' is not defined # only type declared - same as not defined - error >>> default_test_servers: List[str] >>> print(default_test_servers) Traceback (most recent call last): File "<stdin>", line 1, in <module> NameError: name 'default_test_servers' is not defined # type and assignment - all fine >>> default_test_servers: List[str] = [] >>> print(default_test_servers) []

Confirming py3.6 behaves the same as above.

However, if mypy complains about something and it's easier to assign the value - it's fine by me.

TIL it will end up with "not defined" instead of giving it a value... This is very different than other languages. I will update all test cases to use the suggested pattern.

tools/run_tests/xds_k8s_test_driver/tests/remove_neg_test.py

tools/run_tests/xds_k8s_test_driver/tests/url_map/fault_injection_test.py

tools/run_tests/xds_k8s_test_driver/tests/url_map/retry_test.py

lidizheng

Thanks for reviewing this PR! Most comments are resolved, except the styling of variable annotations in test case files. Let's discuss that in the thread in failover_test.py.

lidizheng · 2022-04-29T19:17:32Z

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/iam.py

@@ -54,7 +53,7 @@ def _replace_binding(policy: 'Policy', binding: 'Policy.Binding',
    new_bindings = set(policy.bindings)
    new_bindings.discard(binding)
    new_bindings.add(new_binding)
-    return dataclasses.replace(policy, bindings=frozenset(new_bindings))
+    return dataclasses.replace(policy, bindings=frozenset(new_bindings))  # pylint: disable=too-many-function-args


Good catch. This is still a mystery to me. Here is the raw pylint complaint:

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/iam.py:56:11: E1121: Too many positional arguments for function call (too-many-function-args)

Our usage complies with official doc: https://docs.python.org/3/library/dataclasses.html#dataclasses.replace

I suspect it might be a PyLint issue with dataclasses library.

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/network_security.py

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/network_services.py

lidizheng · 2022-04-29T20:40:44Z

tools/run_tests/xds_k8s_test_driver/framework/xds_url_map_testcase.py

@@ -102,7 +102,7 @@ def __init__(self, xds_json: JsonType):
                    for endpoint in xds_config['endpointConfig'][
                            'dynamicEndpointConfigs']:
                        self.eds.append(endpoint['endpointConfig'])
-            except Exception as e:
+            except Exception as e:  # pylint: disable=broad-except


Added as TODO. Let's do it in another PR, in case the narrowing of the catch scope causing tests to fail.

lidizheng · 2022-04-29T20:45:05Z

tools/run_tests/xds_k8s_test_driver/framework/xds_url_map_testcase.py

@@ -362,13 +360,13 @@ def tearDownClass(cls):

    def _fetch_and_check_xds_config(self):
        # Cleanup state for this attempt
-        self._xds_json_config = None
+        self._xds_json_config = None  # pylint: disable=attribute-defined-outside-init


Todo added.

I attempted that approach, the problem is unittest.TestCase contains too much magic. Theoretically, the variable declaration should happen in the constructor, but the constructor is invoked by unittest's internal code. To keep magic working, we might make the __init__ function overly complex. There might be an elegant way to pass the last-seen config to caller across the retry-layer. Feel free to suggest.

tools/run_tests/xds_k8s_test_driver/framework/xds_url_map_testcase.py

lidizheng · 2022-04-29T20:52:09Z

tools/run_tests/xds_k8s_test_driver/tests/failover_test.py

+        default_test_servers: List[_XdsTestServer] = []
+        alternate_test_servers: List[_XdsTestServer] = []


I was afraid that in case of non-failfast run, we might carry a non-List value, then cause later tests to fail (since they assumed this value to be List, but seen None). To make the typing work, we will need to wrap it with Optional.

I found giving it an initial value saves a few lines of code (don't need to do null checks).

tools/run_tests/xds_k8s_test_driver/tests/remove_neg_test.py

tools/run_tests/xds_k8s_test_driver/tests/url_map/fault_injection_test.py

sergiitk

All good! Thank you for getting this done! This will go a long way - from making reviews simpler, to stability improvements.

sergiitk · 2022-04-29T21:47:30Z

Let's re-run tests before merging this. Also, better to postpone the merge so we don't break anything over the weekend.

lidizheng · 2022-05-02T22:29:46Z

lidizheng · 2022-05-02T22:55:18Z

Interestingly, Google served Golang binaries are unavailable now causing Kokoro jobs to fail:

WARNING: Download from https://dl.google.com/go/go1.18.linux-amd64.tar.gz failed: class com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException GET returned 502 Bad Gateway
ERROR: An error occurred during the fetch of repository 'go_sdk':
   Traceback (most recent call last):
	File "/root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/io_bazel_rules_go/go/private/sdk.bzl", line 100, column 16, in _go_download_sdk_impl
		_remote_sdk(ctx, [url.format(filename) for url in ctx.attr.urls], ctx.attr.strip_prefix, sha256)
	File "/root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/io_bazel_rules_go/go/private/sdk.bzl", line 205, column 21, in _remote_sdk
		ctx.download(
Error in download: java.io.IOException: Error downloading [https://dl.google.com/go/go1.18.linux-amd64.tar.gz] to /root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/go_sdk/go_sdk.tar.gz: GET returned 502 Bad Gateway
ERROR: Error fetching repository: Traceback (most recent call last):
	File "/root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/io_bazel_rules_go/go/private/sdk.bzl", line 100, column 16, in _go_download_sdk_impl
		_remote_sdk(ctx, [url.format(filename) for url in ctx.attr.urls], ctx.attr.strip_prefix, sha256)
	File "/root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/io_bazel_rules_go/go/private/sdk.bzl", line 205, column 21, in _remote_sdk
		ctx.download(
Error in download: java.io.IOException: Error downloading [https://dl.google.com/go/go1.18.linux-amd64.tar.gz] to /root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/go_sdk/go_sdk.tar.gz: GET returned 502 Bad Gateway
ERROR: Analysis of target '//src/python/grpcio/grpc/experimental:aio' failed; build aborted: java.io.IOException: Error downloading [https://dl.google.com/go/go1.18.linux-amd64.tar.gz] to /root/.cache/bazel/_bazel_root/954bb7512d44d20015390af6e76121c6/external/go_sdk/go_sdk.tar.gz: GET returned 502 Bad Gateway

lidizheng · 2022-05-03T17:33:18Z

All xDS interop test passed, except AffinityTest failed due to known flake:

framework.rpc.grpc.GrpcApp.NotFound: Client has no READY channel with the server

* [PSM interop] Introduce isort/pylint to the GKE framework * Update the rpc_types * Update variable type annotation style in tests/*.py

lidizheng added lang/Python kind/internal cleanup release notes: no Indicates if PR should not be in release notes labels Apr 21, 2022

grpc-checks bot added per-call-memory/neutral bloat/none labels Apr 21, 2022

lidizheng force-pushed the psm-sanity branch from 8ce713a to 8c72263 Compare April 21, 2022 23:00

lidizheng changed the title ~~[PSM interop] Introduce sanity to the GKE framework~~ [PSM interop] Introduce isort/pylint to the GKE framework Apr 21, 2022

lidizheng force-pushed the psm-sanity branch 2 times, most recently from 78a8608 to 42297f7 Compare April 22, 2022 20:21

lidizheng marked this pull request as ready for review April 22, 2022 22:18

lidizheng requested a review from sergiitk April 23, 2022 00:25

lidizheng assigned sergiitk Apr 23, 2022

sergiitk reviewed Apr 29, 2022

View reviewed changes

[PSM interop] Introduce isort/pylint to the GKE framework

e176c7d

lidizheng force-pushed the psm-sanity branch from a46f387 to e176c7d Compare April 29, 2022 21:02

Update the rpc_types

2910667

lidizheng commented Apr 29, 2022

View reviewed changes

sergiitk approved these changes Apr 29, 2022

View reviewed changes

Update variable type annotation style in tests/*.py

b9defae

lidizheng added the kokoro:force-run label May 2, 2022

grpc-kokoro removed the kokoro:force-run label May 2, 2022

lidizheng added the kokoro:force-run label May 3, 2022

grpc-kokoro removed the kokoro:force-run label May 3, 2022

lidizheng enabled auto-merge (squash) May 3, 2022 17:33

lidizheng merged commit 697c438 into grpc:master May 3, 2022

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 4, 2022

lidizheng mentioned this pull request May 5, 2022

xds-k8s: Fix incorrect type hint in the baseline test #29577

Merged

anicr7 pushed a commit that referenced this pull request May 6, 2022

[PSM interop] Introduce isort/pylint to the GKE framework (#29476)

c29decc

* [PSM interop] Introduce isort/pylint to the GKE framework * Update the rpc_types * Update variable type annotation style in tests/*.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PSM interop] Introduce isort/pylint to the GKE framework #29476

[PSM interop] Introduce isort/pylint to the GKE framework #29476

lidizheng commented Apr 21, 2022 •

edited

lidizheng commented Apr 21, 2022 •

edited

sergiitk left a comment

sergiitk Apr 29, 2022

lidizheng Apr 29, 2022

sergiitk Apr 29, 2022

sergiitk Apr 29, 2022

lidizheng Apr 29, 2022

sergiitk Apr 29, 2022

lidizheng Apr 29, 2022

sergiitk Apr 29, 2022

sergiitk Apr 29, 2022

lidizheng Apr 29, 2022

lidizheng left a comment

lidizheng Apr 29, 2022

lidizheng Apr 29, 2022

lidizheng Apr 29, 2022

lidizheng Apr 29, 2022

sergiitk left a comment

sergiitk commented Apr 29, 2022

lidizheng commented May 2, 2022 •

edited

lidizheng commented May 2, 2022

lidizheng commented May 3, 2022

		default_test_servers: List[_XdsTestServer] = []
		alternate_test_servers: List[_XdsTestServer] = []

[PSM interop] Introduce isort/pylint to the GKE framework #29476

[PSM interop] Introduce isort/pylint to the GKE framework #29476

Conversation

lidizheng commented Apr 21, 2022 • edited

lidizheng commented Apr 21, 2022 • edited

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

lidizheng 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 left a comment

Choose a reason for hiding this comment

sergiitk commented Apr 29, 2022

lidizheng commented May 2, 2022 • edited

lidizheng commented May 2, 2022

lidizheng commented May 3, 2022

lidizheng commented Apr 21, 2022 •

edited

lidizheng commented Apr 21, 2022 •

edited

lidizheng commented May 2, 2022 •

edited