ephraimbuddy commented on code in PR #72909: URL: https://github.com/apache/airflow/pull/72909#discussion_r4004022606
########## airflow-core/docs/administration-and-deployment/dag-bundles.rst: ########## @@ -226,6 +226,39 @@ Starting Airflow 3.0.2 git is pre installed in the base image. However, if you a ENV GIT_PYTHON_REFRESH=quiet +Monitoring Dag bundles +---------------------- + +The **Dag Bundles** page under *Browse* shows, for each bundle, the version Airflow currently holds, +when a Dag processor last refreshed it, its import-error count, and whether it is still configured +-- so you can tell whether a commit has been picked up without reading the Dag source. The page +refreshes itself while open, and the same data is available at ``GET /api/v2/dagBundles`` for a +deployment pipeline to poll. + +``version`` is whatever the bundle reports; for a Git Dag bundle it is the commit SHA of the tracking +ref. Bundles that do not support versioning, such as ``LocalDagBundle``, report ``null``. + +``last_refreshed`` is the last *successful* refresh and advances whether or not the version changed. +The cadence is the bundle's :ref:`config:dag_processor__refresh_interval`, checked every +:ref:`config:dag_processor__bundle_refresh_check_interval`. + +Some caveats: + +* A failing refresh looks like one that is not due, since a refresh that raises is logged and leaves + ``last_refreshed`` at its last success. Check the Dag processor logs, and + ``/api/v2/jobs?job_type=DagProcessorJob`` for a live processor. +* A new version does not mean the new Dags are running, because bundles are refreshed before their + Dags are parsed -- read the import-error count alongside it. Review Comment: Reading the import-error count alongside the version still cannot tell us whether that commit parsed successfully. The version advances before parsing, error rows aren't versioned, and parser timeouts only update in-memory stats, so even an admin can keep seeing a fresh SHA with zero errors while parsing fails. Ordinary readers can also see zero when errors are filtered out by permissions. We can make the unknown/partial state explicit in the UI and avoid presenting this combination as a deployment-health check. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_bundles.py: ########## @@ -0,0 +1,214 @@ +# 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, Annotated + +from fastapi import Depends +from sqlalchemy import and_, func, select + +from airflow.api_fastapi.auth.managers.models.resource_details import AccessView, DagAccessEntity +from airflow.api_fastapi.common.db.common import SessionDep, paginated_select +from airflow.api_fastapi.common.parameters import QueryLimit, QueryOffset, SortParam +from airflow.api_fastapi.common.router import AirflowRouter +from airflow.api_fastapi.core_api.datamodels.dag_bundles import ( + DagBundleCollectionResponse, + DagBundleResponse, +) +from airflow.api_fastapi.core_api.security import ( + AuthManagerDep, + GetUserDep, + ReadableDagBundlesFilterDep, + requires_access_dag, +) +from airflow.configuration import conf +from airflow.models import DagModel +from airflow.models.dagbundle import DagBundleModel +from airflow.models.errors import ParseImportError + +if TYPE_CHECKING: + from collections.abc import Sequence + + from sqlalchemy.orm import Session + + from airflow.api_fastapi.auth.managers.base_auth_manager import BaseAuthManager + from airflow.api_fastapi.auth.managers.models.base_user import BaseUser + +dag_bundles_router = AirflowRouter(tags=["Dag Bundle"], prefix="/dagBundles") + + +def _import_error_counts( + *, + bundle_names: Sequence[str], + readable_dag_ids: set[str], + auth_manager: BaseAuthManager, + user: BaseUser, + session: Session, +) -> dict[str, int] | None: + """ + Count the import errors per bundle that this caller is allowed to know about. + + Reproduces the two-part authorization of ``GET /importErrors`` rather than counting every row + for the bundle: an error in a file the caller can read no Dag in stays hidden, and an error in + a file with no registered Dag needs the admin-by-default ``IMPORT_ERRORS_ALL``, since the + file's existence would otherwise leak. Returns ``None`` when the caller may not read import + errors at all. + """ + if not auth_manager.authorize_view(access_view=AccessView.IMPORT_ERRORS, user=user): + return None + if not bundle_names: + return {} + + # Files -- keyed ``(relative_fileloc, bundle_name)`` -- in which the caller can read a Dag. + readable_files = ( + select(DagModel.relative_fileloc, DagModel.bundle_name) + .where( + DagModel.dag_id.in_(readable_dag_ids), + DagModel.bundle_name.in_(bundle_names), + ) + .distinct() + .subquery() + ) + counts: dict[str, int] = { + bundle_name: count + for bundle_name, count in session.execute( + select(ParseImportError.bundle_name, func.count()) + .join( + readable_files, + and_( + ParseImportError.filename == readable_files.c.relative_fileloc, + ParseImportError.bundle_name == readable_files.c.bundle_name, + ), + ) + .group_by(ParseImportError.bundle_name) + ).all() + if bundle_name is not None + } + + # Errors for files that never registered a Dag, added only where the caller holds the + # admin-by-default view for that bundle's team. Narrowed to this page's bundles, unlike the + # equivalent in ``import_error.py``: this endpoint polls, so an unbounded ``SELECT DISTINCT`` + # over ``dag`` every few seconds would be a real cost on a large deployment. + files_with_any_dags = ( + select(DagModel.relative_fileloc, DagModel.bundle_name) + .where(DagModel.bundle_name.in_(bundle_names)) Review Comment: Filtering to this page's bundle names doesn't avoid the table scans here: `dag.bundle_name` and the import-error bundle/file lookup have no supporting indexes, and the query plan still scans both tables. Every open page repeats this work every ten seconds, including computing unregistered errors that authorization later discards. Maybe as a follow up, can we add the required indexes and skip that work for bundles whose unregistered errors the caller cannot read? ########## airflow-core/src/airflow/api_fastapi/core_api/security.py: ########## @@ -361,6 +336,21 @@ def to_orm(self, select: Select) -> Select: return select.where(Backfill.dag_id.in_(self.value or set())) +class PermittedDagBundleFilter(PermittedDagFilter): + """A parameter that filters Dag bundles to the ones holding a Dag the user may read.""" + + def to_orm(self, statement: Select) -> Select: + # A bundle carries no per-Dag key to authorize on, so it is scoped by the Dags inside it. + # Filtering in the query keeps unauthorized rows out of the count and pagination as well. + # A bundle from which no Dag has ever parsed is therefore invisible until one does, and one + # whose Dags have since been removed stays visible while a stale ``DagModel`` row names it. + return statement.where( + DagBundleModel.name.in_( + select(DagModel.bundle_name).where(DagModel.dag_id.in_(self.value or set())) + ) + ) Review Comment: This hides a new bundle even from an admin when its first parse fails, because there is no `DagModel` row yet. I don't think successful parsing should be a prerequisite for monitoring a bundle. We need a bundle/team or privileged authorization path that can show these rows without a registered Dag. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_bundles.py: ########## @@ -0,0 +1,214 @@ +# 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, Annotated + +from fastapi import Depends +from sqlalchemy import and_, func, select + +from airflow.api_fastapi.auth.managers.models.resource_details import AccessView, DagAccessEntity +from airflow.api_fastapi.common.db.common import SessionDep, paginated_select +from airflow.api_fastapi.common.parameters import QueryLimit, QueryOffset, SortParam +from airflow.api_fastapi.common.router import AirflowRouter +from airflow.api_fastapi.core_api.datamodels.dag_bundles import ( + DagBundleCollectionResponse, + DagBundleResponse, +) +from airflow.api_fastapi.core_api.security import ( + AuthManagerDep, + GetUserDep, + ReadableDagBundlesFilterDep, + requires_access_dag, +) +from airflow.configuration import conf +from airflow.models import DagModel +from airflow.models.dagbundle import DagBundleModel +from airflow.models.errors import ParseImportError + +if TYPE_CHECKING: + from collections.abc import Sequence + + from sqlalchemy.orm import Session + + from airflow.api_fastapi.auth.managers.base_auth_manager import BaseAuthManager + from airflow.api_fastapi.auth.managers.models.base_user import BaseUser + +dag_bundles_router = AirflowRouter(tags=["Dag Bundle"], prefix="/dagBundles") + + +def _import_error_counts( + *, + bundle_names: Sequence[str], + readable_dag_ids: set[str], + auth_manager: BaseAuthManager, + user: BaseUser, + session: Session, +) -> dict[str, int] | None: + """ + Count the import errors per bundle that this caller is allowed to know about. + + Reproduces the two-part authorization of ``GET /importErrors`` rather than counting every row + for the bundle: an error in a file the caller can read no Dag in stays hidden, and an error in + a file with no registered Dag needs the admin-by-default ``IMPORT_ERRORS_ALL``, since the + file's existence would otherwise leak. Returns ``None`` when the caller may not read import + errors at all. + """ + if not auth_manager.authorize_view(access_view=AccessView.IMPORT_ERRORS, user=user): + return None + if not bundle_names: + return {} + + # Files -- keyed ``(relative_fileloc, bundle_name)`` -- in which the caller can read a Dag. + readable_files = ( + select(DagModel.relative_fileloc, DagModel.bundle_name) + .where( + DagModel.dag_id.in_(readable_dag_ids), + DagModel.bundle_name.in_(bundle_names), + ) + .distinct() + .subquery() + ) + counts: dict[str, int] = { + bundle_name: count + for bundle_name, count in session.execute( + select(ParseImportError.bundle_name, func.count()) + .join( + readable_files, + and_( + ParseImportError.filename == readable_files.c.relative_fileloc, + ParseImportError.bundle_name == readable_files.c.bundle_name, + ), + ) + .group_by(ParseImportError.bundle_name) + ).all() + if bundle_name is not None + } + + # Errors for files that never registered a Dag, added only where the caller holds the + # admin-by-default view for that bundle's team. Narrowed to this page's bundles, unlike the + # equivalent in ``import_error.py``: this endpoint polls, so an unbounded ``SELECT DISTINCT`` + # over ``dag`` every few seconds would be a real cost on a large deployment. + files_with_any_dags = ( + select(DagModel.relative_fileloc, DagModel.bundle_name) + .where(DagModel.bundle_name.in_(bundle_names)) + .distinct() + .subquery() + ) + unregistered = session.execute( + select(ParseImportError.bundle_name, func.count()) + .outerjoin( + files_with_any_dags, + and_( + ParseImportError.filename == files_with_any_dags.c.relative_fileloc, + ParseImportError.bundle_name == files_with_any_dags.c.bundle_name, + ), + ) + .where( + files_with_any_dags.c.relative_fileloc.is_(None), + ParseImportError.bundle_name.in_(bundle_names), + ) + .group_by(ParseImportError.bundle_name) + ).all() + if unregistered: + team_names = DagBundleModel.get_team_names( + [bundle_name for bundle_name, _ in unregistered if bundle_name], session=session + ) + for bundle_name, count in unregistered: + if bundle_name is None: + continue + if auth_manager.authorize_view( + access_view=AccessView.IMPORT_ERRORS_ALL, + user=user, + team_name=team_names.get(bundle_name), + ): + counts[bundle_name] = counts.get(bundle_name, 0) + count + + return counts + + +@dag_bundles_router.get("", dependencies=[Depends(requires_access_dag(method="GET"))]) +def get_dag_bundles( + limit: QueryLimit, + offset: QueryOffset, + order_by: Annotated[ + SortParam, + Depends(SortParam(["name", "version", "last_refreshed", "active"], DagBundleModel).dynamic_depends()), + ], + readable_dag_bundles_filter: ReadableDagBundlesFilterDep, + auth_manager: AuthManagerDep, + session: SessionDep, + user: GetUserDep, +) -> DagBundleCollectionResponse: + """ + List the Dag bundles Airflow knows about. + + Reports the version of each bundle Airflow currently holds and when a Dag processor last + refreshed it, so whoever deployed the code can tell whether it has been picked up yet. + + A bundle is visible to a user who can read at least one Dag recorded against it, so one from + which no Dag has ever parsed successfully is not listed at all. + """ + bundles_select, total_entries = paginated_select( + statement=select(DagBundleModel), + filters=[readable_dag_bundles_filter], + order_by=order_by, + offset=offset, + limit=limit, + session=session, + ) + bundles = session.scalars(bundles_select).all() + bundle_names = [bundle.name for bundle in bundles] + + # ``DagBundleModel.team_name`` walks a lazily loaded relationship, so read them in one query. + team_names = ( + DagBundleModel.get_team_names(bundle_names, session=session) + if bundle_names and conf.getboolean("core", "multi_team") + else {} + ) + + import_error_counts = _import_error_counts( + bundle_names=bundle_names, + readable_dag_ids=readable_dag_bundles_filter.value or set(), + auth_manager=auth_manager, + user=user, + session=session, + ) + + # A rendered bundle url is otherwise only reachable through ``GET /dags/{dag_id}/dagVersions``, + # which additionally requires Dag *version* read, so withhold it from a role without that. + show_bundle_url = auth_manager.is_authorized_dag( + method="GET", access_entity=DagAccessEntity.VERSION, user=user + ) + + return DagBundleCollectionResponse( + dag_bundles=[ + DagBundleResponse( + name=bundle.name, + active=bundle.active, + version=bundle.version, + last_refreshed=bundle.last_refreshed, Review Comment: This can report an obsolete Git SHA as current after a Git bundle is replaced by a non-versioned bundle under the same name. Bundle sync reuses the row, and `update_bundle_state(version=None)` preserves the old version while advancing `last_refreshed`. We need to reconcile that stored state before exposing it as authoritative; otherwise this shows an old SHA with a fresh timestamp instead of “Not versioned.” -- 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]
