diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d5acca3..9d4ab5e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ which could cause problems with GitOps tools (e.g. ArgoCD) reporting a diff in the custom resources. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#840]). - Task logs are served from `BASE_LOG_FOLDER` instead of the `task` handler's `base_log_folder` at the Vector agent log directory ([#834]). +- Avoid Python import race conditions by pre-cloning the git repo not only for Celery-based stacklets, but also for Kubernetes executor-based setups ([#844]). [#814]: https://github.com/stackabletech/airflow-operator/pull/814 [#821]: https://github.com/stackabletech/airflow-operator/pull/821 @@ -40,6 +41,7 @@ [#834]: https://github.com/stackabletech/airflow-operator/pull/834 [#835]: https://github.com/stackabletech/airflow-operator/pull/835 [#840]: https://github.com/stackabletech/airflow-operator/pull/840 +[#844]: https://github.com/stackabletech/airflow-operator/pull/844 ## [26.7.0] - 2026-07-21 diff --git a/docs/modules/airflow/pages/troubleshooting/index.adoc b/docs/modules/airflow/pages/troubleshooting/index.adoc index a2bec8fc..f5901b6d 100644 --- a/docs/modules/airflow/pages/troubleshooting/index.adoc +++ b/docs/modules/airflow/pages/troubleshooting/index.adoc @@ -218,16 +218,26 @@ NOTE: Generally speaking it is https://airflow.apache.org/docs/apache-airflow/st == GitSync race condition -Sometimes a race condition can arise when the long-running Python process started by the Airflow dag-processor caches python submodules before the gitsync fetch is complete. This is indeterminate but can be avoided by adding the following lines to the top of each DAG file that references submodules: +The operator runs a one-off `git-sync` process in an init container in every Pod, so the repository is guaranteed to be fully cloned before any Airflow process starts. +A `git-sync` sidecar container then keeps the cloned repository up to date while the Pod is running. +A race condition can still arise for content that is added to the repository *after* a Pod has started and is delivered by the sidecar: the long-running Python process started by the Airflow dag-processor has cached python submodules and a newly synced submodule may therefore not be found, even though it is already present on disk. +This is indeterminate but can be avoided by adding the following lines to the top of each DAG file that references submodules: [source,python] ---- import importlib import site -# invalidate cache due to race condition when using dag-processor +# invalidate import caches so that modules synced after process start are found importlib.reload(site) importlib.invalidate_caches() ---- -This will add some overhead that should be minimal in comparison to parsing the DAG as a whole. +These lines run whenever the DAG file is parsed by the dag-processor or re-parsed for task execution on workers, and add some overhead that should be minimal in comparison to parsing the DAG as a whole. + +[NOTE] +==== +This workaround does not cover the triggerer: the triggerer does not parse DAG files, but imports trigger classes and callables directly at runtime, so code placed in a DAG file never runs in that process. +If a deferred task references a module that was added to the repository after the triggerer Pod started and fails with `ModuleNotFoundError`, then restarting the triggerer may resolve this. +This starts a fresh Python process with no stale import caches, and the init container guarantees the repository is fully synced before it starts. +==== diff --git a/docs/modules/airflow/pages/usage-guide/mounting-dags.adoc b/docs/modules/airflow/pages/usage-guide/mounting-dags.adoc index d4cb0b3b..dfe167f7 100644 --- a/docs/modules/airflow/pages/usage-guide/mounting-dags.adoc +++ b/docs/modules/airflow/pages/usage-guide/mounting-dags.adoc @@ -42,7 +42,8 @@ For multiple DAGs, it is easier to expose them via `gitsync`, as shown below. == Via `git-sync` -{git-sync}[git-sync] is a command that pulls a git repository into a local directory and is supplied as a sidecar container for use within Kubernetes. +{git-sync}[git-sync] is a command that pulls a git repository into a local directory. +The operator runs it twice in each Pod: once in an init container, so that the repository is fully cloned before any Airflow process starts, and again as a sidecar container that keeps the clone up to date (Pods for the Kubernetes executor are short-lived and only get the init container). The Stackable Airflow images already ship with git-sync included, and the operator takes care of calling the tool and mounting volumes, so that only the repository and synchronization details are required: .git-sync usage example: https diff --git a/rust/operator-binary/src/controller/build/resource/executor.rs b/rust/operator-binary/src/controller/build/resource/executor.rs index 6ed1e617..9e8d23b9 100644 --- a/rust/operator-binary/src/controller/build/resource/executor.rs +++ b/rust/operator-binary/src/controller/build/resource/executor.rs @@ -29,8 +29,8 @@ use crate::{ object_meta, properties::env_vars::build_airflow_template_envs, resource::pod::{ - add_authentication_volumes_and_volume_mounts, add_git_sync_resources, - build_logging_container, + GitSyncSidecarsAddition, add_authentication_volumes_and_volume_mounts, + add_git_sync_resources, build_logging_container, }, volumes::{self, CONFIG_VOLUME_NAME, LOG_CONFIG_VOLUME_NAME, LOG_VOLUME_NAME}, }, @@ -147,8 +147,9 @@ pub fn build_executor_template_config_map( &mut pb, &mut airflow_container, git_sync_resources, - false, - true, + // We don't need a git-sync sidecar, an initial clone via the init-container is sufficient for + // Kubernetes executors, as they are short-lived. + &GitSyncSidecarsAddition::Skip, ) .context(PodSnafu)?; diff --git a/rust/operator-binary/src/controller/build/resource/pod.rs b/rust/operator-binary/src/controller/build/resource/pod.rs index 831e483a..3a735934 100644 --- a/rust/operator-binary/src/controller/build/resource/pod.rs +++ b/rust/operator-binary/src/controller/build/resource/pod.rs @@ -85,23 +85,38 @@ pub(crate) fn add_authentication_volumes_and_volume_mounts( Ok(()) } +#[derive(PartialEq, Eq)] +pub enum GitSyncSidecarsAddition { + Add, + Skip, +} + +/// Adds the needed git-sync init-container and (optionally) sidecar. +/// +/// If the DAG is modularized we may encounter a timing issue whereby the main process +/// has started *before* all modules referenced by the DAG have been fetched by gitsync +/// and registered. This will result in ModuleNotFoundError errors. This can be avoided +/// by running a one-off git-sync process in an init-container so that all DAG +/// dependencies are fully loaded. The sidecar git-sync is then used for regular updates. +/// +/// For that reason, we always add an init-container that clones the repo initially. All Pods +/// (except the Kubernetes executors) additionally use a sidecar to keep the git contents +/// up-to-date. pub(crate) fn add_git_sync_resources( pb: &mut PodBuilder, cb: &mut ContainerBuilder, git_sync_resources: &git_sync::v1alpha2::GitSyncResources, - add_sidecar_containers: bool, - add_init_containers: bool, + add_sidecar_containers: &GitSyncSidecarsAddition, ) -> Result<()> { - if add_sidecar_containers { + if add_sidecar_containers == &GitSyncSidecarsAddition::Add { for container in git_sync_resources.git_sync_containers.iter().cloned() { pb.add_container(container); } } - if add_init_containers { - for container in git_sync_resources.git_sync_init_containers.iter().cloned() { - pb.add_init_container(container); - } + for container in git_sync_resources.git_sync_init_containers.iter().cloned() { + pb.add_init_container(container); } + pb.add_volumes(git_sync_resources.git_content_volumes.to_owned()) .context(AddVolumeSnafu)?; pb.add_volumes(git_sync_resources.git_ssh_volumes.to_owned()) diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 81915cab..48aaac50 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -37,8 +37,8 @@ use crate::{ properties::env_vars, resource::{ pod::{ - add_authentication_volumes_and_volume_mounts, add_git_sync_resources, - build_logging_container, + GitSyncSidecarsAddition, add_authentication_volumes_and_volume_mounts, + add_git_sync_resources, build_logging_container, }, service::stateful_set_service_name, }, @@ -253,18 +253,12 @@ pub fn build_server_rolegroup_statefulset( .context(AddVolumeMountSnafu)?; } - // If the DAG is modularized we may encounter a timing issue whereby the celery worker - // has started *before* all modules referenced by the DAG have been fetched by gitsync - // and registered. This will result in ModuleNotFoundError errors. This can be avoided - // by running a one-off git-sync process in an init-container so that all DAG - // dependencies are fully loaded. The sidecar git-sync is then used for regular updates. - let use_git_sync_init_containers = matches!(executor, AirflowExecutor::CeleryExecutors { .. }); add_git_sync_resources( &mut pb, &mut airflow_container, git_sync_resources, - true, - use_git_sync_init_containers, + // We need a git-sync sidecar to keep the git contents up-to-date + &GitSyncSidecarsAddition::Add, ) .context(PodSnafu)?; diff --git a/tests/templates/kuttl/ca-cert/25-assert.yaml b/tests/templates/kuttl/ca-cert/25-assert.yaml index bed7d7fb..5405bc97 100644 --- a/tests/templates/kuttl/ca-cert/25-assert.yaml +++ b/tests/templates/kuttl/ca-cert/25-assert.yaml @@ -8,7 +8,10 @@ commands: sleep 10 POD="airflow-wrong-cert-dagprocessor-default-0" - CONTAINER="git-sync-0" + # All Pods clone the repo in an init container, so the git-sync sidecar never gets + # started when the CA cert is wrong. Because of this we need to check the init + # container instead. + CONTAINER="git-sync-0-init" kubectl logs -n "$NAMESPACE" "$POD" -c "$CONTAINER" 2>/dev/null \ | grep -q "SSL certificate problem: unable to get local issuer certificate" && exit 0 diff --git a/tests/templates/kuttl/mount-dags-gitsync/30-assert.yaml.j2 b/tests/templates/kuttl/mount-dags-gitsync/30-assert.yaml.j2 index 6ffaabb2..5fd6afb2 100644 --- a/tests/templates/kuttl/mount-dags-gitsync/30-assert.yaml.j2 +++ b/tests/templates/kuttl/mount-dags-gitsync/30-assert.yaml.j2 @@ -9,6 +9,11 @@ apiVersion: apps/v1 kind: StatefulSet metadata: name: airflow-webserver-default +spec: + template: + spec: + initContainers: + - name: git-sync-0-init status: readyReplicas: 1 replicas: 1 @@ -18,6 +23,11 @@ apiVersion: apps/v1 kind: StatefulSet metadata: name: airflow-worker-default +spec: + template: + spec: + initContainers: + - name: git-sync-0-init status: readyReplicas: 1 replicas: 1 @@ -27,6 +37,37 @@ apiVersion: apps/v1 kind: StatefulSet metadata: name: airflow-scheduler-default +spec: + template: + spec: + initContainers: + - name: git-sync-0-init +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: airflow-dagprocessor-default +spec: + template: + spec: + initContainers: + - name: git-sync-0-init +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: airflow-triggerer-default +spec: + template: + spec: + initContainers: + - name: git-sync-0-init status: readyReplicas: 1 replicas: 1 diff --git a/tests/templates/kuttl/mount-dags-gitsync/30-install-airflow-cluster.yaml.j2 b/tests/templates/kuttl/mount-dags-gitsync/30-install-airflow-cluster.yaml.j2 index 380b61f9..12e11ef6 100644 --- a/tests/templates/kuttl/mount-dags-gitsync/30-install-airflow-cluster.yaml.j2 +++ b/tests/templates/kuttl/mount-dags-gitsync/30-install-airflow-cluster.yaml.j2 @@ -181,3 +181,13 @@ spec: envOverrides: AIRFLOW_CONN_KUBERNETES_IN_CLUSTER: "kubernetes://?__extra__=%7B%22extra__kubernetes__in_cluster%22%3A+true%2C+%22extra__kubernetes__kube_config%22%3A+%22%22%2C+%22extra__kubernetes__kube_config_path%22%3A+%22%22%2C+%22extra__kubernetes__namespace%22%3A+%22%22%7D" replicas: 1 + triggerers: + config: + gracefulShutdownTimeout: 10s + logging: + enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} + roleGroups: + default: + envOverrides: + AIRFLOW_CONN_KUBERNETES_IN_CLUSTER: "kubernetes://?__extra__=%7B%22extra__kubernetes__in_cluster%22%3A+true%2C+%22extra__kubernetes__kube_config%22%3A+%22%22%2C+%22extra__kubernetes__kube_config_path%22%3A+%22%22%2C+%22extra__kubernetes__namespace%22%3A+%22%22%7D" + replicas: 1