shahar1 commented on code in PR #68930: URL: https://github.com/apache/airflow/pull/68930#discussion_r3692213062
########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/handlers/__init__.py: ########## @@ -0,0 +1,53 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from airflow_google_provider_resource_cleanup.handlers._base import BaseDeleteHandler + + +def get_delete_handlers() -> dict[str, type[BaseDeleteHandler]]: + from airflow_google_provider_resource_cleanup.handlers.ai import AIPlatformDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.bq import BigQueryDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.composer import ComposerDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.dataform import DataformDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.dataplex import DataplexDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.dataproc import DataprocDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.dlp import DLPDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.logging import LoggingDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.ray import RayClusterOnVertexAIDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.sqladmin import CloudSQLDeleteHandler + from airflow_google_provider_resource_cleanup.handlers.storage import StorageDeleteHandler + + return { Review Comment: **Blocking:** `get_delete_handlers()` never registers `ComputeDeleteHandler`, `DataflowDeleteHandler`, `DataprocMetastoreDeleteHandler`, `ManagedKafkaDeleteHandler`, `GKEDeleteHandler`, or `SpannerDeleteHandler` — those six modules are dead code, and e.g. `delete --asset-type compute` prints "no delete handler implemented" despite `compute.py` existing. Several of them would also break if registered (see the anchored comments in those files). Please either register + fix them, or drop the unfinished modules from this PR and add them in follow-ups. ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/handlers/compute.py: ########## @@ -0,0 +1,51 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from airflow_google_provider_resource_cleanup.handlers._base import BaseDeleteHandler +from airflow_google_provider_resource_cleanup.helpers import get_resource_path, run_command_async + + +async def delete_instance_group_manager(resource): Review Comment: These deleters take only `resource`, but the `DELETERS` contract (`BaseDeleteHandler.delete_single_resource`) calls `deleter(resource, prefix)` — this would raise `TypeError` the moment the handler is registered. ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/handlers/spanner.py: ########## @@ -0,0 +1,74 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from airflow_google_provider_resource_cleanup.handlers._base import BaseDeleteHandler +from airflow_google_provider_resource_cleanup.helpers import get_resource_path, run_command_async + + +async def _delete_database(resource: dict, log_prefix: str): + path = get_resource_path(resource) + instance, database = path.split("/") # TODO implement Review Comment: All five deleters here carry `# TODO implement`, and the unpacking is wrong: `path.split("/")` on `projects/<p>/instances/<i>/databases/<d>` yields 6 items unpacked into 2 (`ValueError`), and `_delete_instance` passes the whole list into the command string. Please finish or remove this module. ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/handlers/dataproc_metastore.py: ########## @@ -0,0 +1,47 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from airflow_google_provider_resource_cleanup.handlers._base import BaseDeleteHandler +from airflow_google_provider_resource_cleanup.helpers import get_resource_path, run_command_async + + +async def _delete_dataproc_metastore_service(resource: dict, log_prefix: str): + location = resource.get("location") + name = get_resource_path(resource) + cmd = f"gcloud metastore services delete {name} --location={location} --quiet" + await run_command_async(cmd, log_prefix) + + +async def _delete_dataproc_metastore_backup(resource: dict, log_prefix: str): + location = resource.get("location") + name = get_resource_path(resource) + cmd = f"gcloud metastore services delete {name} --location={location} --quiet" Review Comment: Copy-paste bug: the *backup* deleter runs `gcloud metastore services delete` — it deletes the service, not the backup. ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/commands/cmd_delete.py: ########## @@ -0,0 +1,209 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import argparse +import datetime +import sys +from collections import defaultdict +from collections.abc import Mapping, Sequence +from typing import Any + +import airflow_google_provider_resource_cleanup.constants as constants +from airflow_google_provider_resource_cleanup.handlers import get_delete_handlers +from airflow_google_provider_resource_cleanup.helpers import ( + GCPProjectConfig, + check_white_list, + get_auxiliary_asset_data_item, + get_resources_file, + load_json, + provide_auxiliary_asset_resource_file, +) + +RESOURCE_CREATE_TIME_FIELDS = ("createTime", "creationTimestamp") + + +def _parse_datetime(value: str) -> datetime.datetime: + dt = datetime.datetime.fromisoformat(value) Review Comment: `datetime.fromisoformat()` rejects the `Z` suffix on Python 3.10 (the provider still supports it: `requires-python = ">=3.10"`), and Cloud Asset Inventory timestamps are `Z`-suffixed. Here the resulting `ValueError` is swallowed by `_parse_create_time`, so on 3.10 `--min-age-days` silently skips **every** resource; in `composer.py:31` the same call crashes. Normalize with `value.replace("Z", "+00:00")` before parsing. ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/__main__.py: ########## @@ -0,0 +1,134 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import argparse +import asyncio +import time +from collections.abc import Callable + +from airflow_google_provider_resource_cleanup import constants as c +from airflow_google_provider_resource_cleanup.commands.cmd_delete import handle_delete +from airflow_google_provider_resource_cleanup.commands.cmd_list import handle_list +from airflow_google_provider_resource_cleanup.commands.cmd_list_asset_types import handle_list_asset_types +from airflow_google_provider_resource_cleanup.commands.cmd_tree import handler_tree + +HANDLERS = { + "list": handle_list, + "list-asset-types": handle_list_asset_types, + "tree": handler_tree, + "delete": handle_delete, +} + + +def _init_argparse() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + prog="airflow-google-system-test-cleanup", + description="CLI to manage resources for a GCP project", + ) + + subparsers = parser.add_subparsers(dest="command", required=True) + + # Global options + parser.add_argument("--config-path", help="Direct path to a project config JSON file") + parser.add_argument("--resources-file-path", help="Direct path to the resources.json file") + + # command: list + parser_list = subparsers.add_parser( + "list", + help="Retrieve the GCP resources for the given GCP project", + ) + parser_list.add_argument("--project-id", help="", required=True) + parser_list.add_argument("--asset-type", help="", choices=c.ASSET_TYPE_OPTIONS) + parser_list.add_argument( + "--sync", + action="store_true", + default=False, + ) + + # command: list-asset-types + parser_list_asset_types = subparsers.add_parser( + "list-asset-types", + help="List all the unique asset types hierarchically in the GCP project", + ) + parser_list_asset_types.add_argument("--project-id", help="", required=True) + parser_list_asset_types.add_argument("--asset-type", help="", choices=c.ASSET_TYPE_OPTIONS) + + # command: tree + parser_tree = subparsers.add_parser( + "tree", + help="Show the resources hierarchically as an HTML file", + ) + parser_tree.add_argument("--project-id", help="", required=True) + parser_tree.add_argument("--asset-type", help="", choices=c.ASSET_TYPE_OPTIONS) + + # command: cleanup + parser_delete = subparsers.add_parser( + "delete", + help="Delete the resources for the given GCP project", + ) + parser_delete.add_argument("--project-id", help="", required=True) + parser_delete.add_argument("--asset-type", help="", choices=c.ASSET_TYPE_OPTIONS) + + parser_delete.add_argument( + "--min-age-days", + type=int, + help="Delete only resources created at least this many days ago", + ) + parser_delete.add_argument( + "--skip-asset-type", + action="append", + default=[], + choices=c.ASSET_TYPE_OPTIONS, + help="Asset type group to skip during deletion. Can be used multiple times.", + ) + + return parser + + +def main(): + _parser = _init_argparse() + _args: argparse.Namespace = _parser.parse_args() + handler: Callable[[argparse.ArgumentParser, argparse.Namespace], None] = HANDLERS[_args.command] + start_time = time.monotonic() + + try: + if asyncio.iscoroutinefunction(handler): + asyncio.run(handler(_parser, _args)) + else: + handler(_parser, _args) + except Exception as e: Review Comment: The bare `except Exception` swallows all errors and the process exits 0 — cron/automation running this cleanup can't detect failure. Re-raise or `sys.exit(1)` after printing. (Also `hours > 1` / `minutes > 1` at line 125 drop the unit exactly at 1 — a 90-minute run prints "30 minutes …"; should be `>= 1`.) ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/constants.py: ########## @@ -0,0 +1,94 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from pathlib import Path + +AUXILIARY_ASSET_TYPES = {"vertex_ai_raycluster": "vertex_ai_raycluster.*"} + +ASSET_TYPES = { + "ai": "aiplatform.googleapis.com.*", + "alloydb": "alloydb.googleapis.com.*", + "artifact": "artifactregistry.googleapis.com.*", + "batch": "batch.googleapis.com.*", + "bq": "bigquery.googleapis.com.*", + "bqtransfer": "bigquerydatatransfer.googleapis.com.*", + "bigtable": "bigtableadmin.googleapis.com.*", + "cloudbuild": "cloudbuild.googleapis.com.*", + "cloudtasks": "cloudtasks.googleapis.com.*", + "composer": "composer.googleapis.com.*", + "compute": "compute.googleapis.com.*", + "dataflow": "dataflow.googleapis.com.*", + "dataform": "dataform.googleapis.com.*", + "datafusion": "datafusion.googleapis.com.*", + "dataplex": "dataplex.googleapis.com.*", + "dataproc": "dataproc.googleapis.com.*", + "dataproc_metastore": "metastore.googleapis.com.*", + "dlp": "dlp.googleapis.com.*", + "firestore": "firestore.googleapis.com.*", + "gke": "container.googleapis.com.*", + "logging": "logging.googleapis.com.*", + "memcache": "memcache.googleapis.com.*", + "metastore": "metastore.googleapis.com.*", + "monitoring": "monitoring.googleapis.com.*", + "pubsub": "pubsub.googleapis.com.*", + "run": "run.googleapis.com.*", + "sqladmin": "sqladmin.googleapis.com.*", + "storage": "storage.googleapis.com.*", + "storagetransfer": "storagetransfer.googleapis.com.*", + "kafka": "managedkafka.googleapis.com.*", + "workflows": "workflows.googleapis.com.*", + "firebase": "firebaserules.googleapis.com.*", + "container": "container.googleapis.com.*", Review Comment: `container` duplicates `gke` (same `container.googleapis.com.*` pattern), and `metastore` duplicates `dataproc_metastore` — in delete-all mode the same resources get categorized twice and deletion-attempted twice. (Also `DO_NOT_DELETE_ASSET_TYPES` repeats `serviceusage.googleapis.com/Service` and `apikeys.googleapis.com/Key` at lines 91–92.) ########## providers/google/tests/system/google/resources_cleanup/airflow_google_provider_resource_cleanup/handlers/ray.py: ########## @@ -0,0 +1,61 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import asyncio + +import vertex_ray Review Comment: Top-level `import vertex_ray` requires the aiplatform `[ray]` extra, which the provider deliberately does not install (see the note at `providers/google/pyproject.toml:84` about aiplatform not installing ray on Python 3.12). Since `get_delete_handlers()` imports this module unconditionally, **every `delete` invocation fails with `ImportError`** in the documented setup. Please make the import lazy inside `delete_ray_cluster` with a helpful message (or gate registration on availability). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
