feat: Airflow Listener integration #604

adwk67 · 2025-04-04T07:15:36Z

Description

fixes: #580

Jenkins tests 🟢
Openshift tests 🟢 (logging test passed when run again in isolation):-

--- FAIL: kuttl (1785.90s)
    --- FAIL: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/ldap_airflow-latest-2.10.4_ldap-authentication-server-verification-tls_openshift-true_executor-kubernetes (300.02s)
        --- PASS: kuttl/harness/opa_airflow-2.10.4_opa-latest-1.0.1_openshift-true (333.59s)
        --- PASS: kuttl/harness/orphaned-resources_airflow-latest-2.10.4_openshift-true (177.55s)
        --- PASS: kuttl/harness/smoke_airflow-2.10.4_openshift-true_executor-kubernetes (172.15s)
        --- PASS: kuttl/harness/external-access_airflow-2.10.4_openshift-true (191.09s)
        --- PASS: kuttl/harness/cluster-operation_airflow-latest-2.10.4_openshift-true (289.47s)
        --- PASS: kuttl/harness/oidc_airflow-2.10.4_openshift-true (169.49s)
        --- PASS: kuttl/harness/resources_airflow-latest-2.10.4_openshift-true (169.28s)
        --- FAIL: kuttl/harness/logging_airflow-2.10.4_openshift-true_executor-kubernetes (836.07s)
        --- PASS: kuttl/harness/mount-dags-configmap_airflow-latest-2.10.4_openshift-true_executor-kubernetes (168.22s)
        --- PASS: kuttl/harness/overrides_airflow-latest-2.10.4_openshift-true (191.15s)
        --- PASS: kuttl/harness/mount-dags-gitsync_airflow-latest-2.10.4_openshift-true_executor-kubernetes (311.44s)
FAIL

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

Author

Give feedback

Changes are OpenShift compatible
CRD changes approved
CRD documentation for all fields, following the style guide.
Helm chart can be installed and deployed operator works
Integration tests passed (for non trivial changes)
Changes need to be "offline" compatible
Options

Reviewer

Give feedback

Code contains useful comments
Code contains useful logging statements
(Integration-)Test cases added
Documentation added or updated. Follows the style guide.
Changelog updated
Cargo.toml only contains references to git tags (not specific commits or branches)
Options

Acceptance

Give feedback

Feature Tracker has been updated
Proper release label has been added
Roadmap has been updated
Options

… class

maltesander

First batch, tests work fine, will do some more manual testing.

tests/templates/kuttl/external-access/50-access-airflow.txt.j2

rust/operator-binary/src/crd/mod.rs

Co-authored-by: Malte Sander <[email protected]>

maltesander

Tested and works. Just some more nitpicking / clarification.

rust/operator-binary/src/airflow_controller.rs

rust/operator-binary/src/crd/utils.rs

docs/modules/airflow/pages/usage-guide/listenerclass.adoc

rust/operator-binary/src/crd/mod.rs

Co-authored-by: Malte Sander <[email protected]>

nightkr

(Not a complete review, but a few notes..)

nightkr · 2025-04-25T11:13:04Z

rust/operator-binary/src/airflow_controller.rs

+        // all listeners will use ephemeral volumes as they can/should
+        // be removed when the pods are *terminated* (ephemeral PVCs will
+        // survive re-starts)
+        pb.add_listener_volume_by_listener_class(
+            LISTENER_VOLUME_NAME,
+            &listener_class.to_string(),
+            &recommended_labels,
+        )
+        .context(AddVolumeSnafu)?;


Clients don't care about which airflow webserver they connect to, right? In that case, we should use a group listener instead (https://docs.stackable.tech/home/stable/listener-operator/volume/#_listeners_stackable_techlistener_name), so we only end up with one load balancer that knows how to route everywhere.

If I've understood this correctly, the operator would then be responsible for creating a listener-by-name (like we do for the kafka bootstrap listener) if the webserver role (or one of its rolegroups) declares a listener class as external-stable or external-unstable, and then this group listener is passed as the name to ListenerReference::ListenerClass for the ListenerOperatorVolumeSourceBuilder for the pvcs? ~~And this listener doesn't care if webserver rolegroups specify different classes (e.g. one as external-stable and another as external-unstable)?~~ I'll do this per role-group, as we have done for Kafka.

~~Hmmm, but for Kafka we create a bootstrap-listener per role-group.~~

nightkr · 2025-04-25T11:14:12Z

rust/operator-binary/src/airflow_controller.rs

+        // all listeners will use ephemeral volumes as they can/should
+        // be removed when the pods are *terminated* (ephemeral PVCs will
+        // survive re-starts)


We should use persistent volumes for web UIs IMO, since upstream load balancers will depend on probably end up hard-coding the target addresses.

deploy/helm/airflow-operator/crds/crds.yaml

nightkr · 2025-04-28T09:24:11Z

rust/operator-binary/src/crd/mod.rs

 #[derive(Clone, Debug, Default, Display, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
 #[serde(rename_all = "PascalCase")]
-pub enum CurrentlySupportedListenerClasses {
+pub enum SupportedListenerClasses {
    #[default]
    #[serde(rename = "cluster-internal")]
+    #[strum(serialize = "cluster-internal")]
    ClusterInternal,

    #[serde(rename = "external-unstable")]
+    #[strum(serialize = "external-unstable")]
    ExternalUnstable,

    #[serde(rename = "external-stable")]
+    #[strum(serialize = "external-stable")]
    ExternalStable,
 }

-impl CurrentlySupportedListenerClasses {
-    pub fn k8s_service_type(&self) -> String {
+impl Atomic for SupportedListenerClasses {}
+
+impl SupportedListenerClasses {
+    pub fn discoverable(&self) -> bool {
        match self {
-            CurrentlySupportedListenerClasses::ClusterInternal => "ClusterIP".to_string(),
-            CurrentlySupportedListenerClasses::ExternalUnstable => "NodePort".to_string(),
-            CurrentlySupportedListenerClasses::ExternalStable => "LoadBalancer".to_string(),
+            SupportedListenerClasses::ClusterInternal => false,
+            SupportedListenerClasses::ExternalUnstable => true,
+            SupportedListenerClasses::ExternalStable => true,
        }
    }
 }


Now that we're using listener-operator, the listener class name should just be an opaque string.

Users are free to define their own custom listenerclasses, or even to redefine our preset ones (though the latter isn't really recommended since it could lead to upgrade strife..).

Looking at the implementation for kafka-operator, a PVC is created for the bootstrap listener even when it is not defined by the user (defaulting to cluster-internal). I guess there is no easy way around that, other than looking up the listener class and checking its type?

kafka-operator behaves correctly there; even when running cluster-internally we want the Listener -> Service to be the ingress point that we expose.

See dcc21f9. As we discussed offline, I've used a consistent address for the Webserver UI whatever the listener class.

…stener classes

adwk67 · 2025-04-28T12:27:18Z

Re-ran tests locally: 🟢

adwk67 added 15 commits April 3, 2025 18:26

mount listener volumes for relevant roles, type depending on listener…

5033d79

… class

merge main

3bbe747

add podrefs to write webserver endpoint to configmap

7e8456a

moved listener-class to webserver role

7d67a6b

Merge branch 'main' into feat/integrate-listener-operator

f4c6fd7

move config map creation out of role-loop

6c2e2b3

Merge branch 'main' into feat/integrate-listener-operator

56dc940

merge conflicts

f250059

Merge branch 'main' into feat/integrate-listener-operator

48ae7e3

regenerate nix

b6b969e

assign listener-class to all roles and consider all ports

5e7b41b

fixed podref name for workers

8706a3e

added integration test

07efc9f

changelog

87a1295

added docs

e646700

adwk67 marked this pull request as ready for review April 10, 2025 13:12

adwk67 moved this to Development: Waiting for Review in Stackable Engineering Apr 10, 2025

adwk67 added this to Stackable Engineering Apr 10, 2025

adwk67 self-assigned this Apr 10, 2025

Merge branch 'main' into feat/integrate-listener-operator

77bcb82

maltesander self-requested a review April 16, 2025 07:53

maltesander moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Apr 16, 2025

maltesander requested changes Apr 16, 2025

View reviewed changes

adwk67 and others added 7 commits April 16, 2025 15:25

Update rust/operator-binary/src/crd/mod.rs

64175f3

Co-authored-by: Malte Sander <[email protected]>

Update rust/operator-binary/src/crd/mod.rs

7837af3

Co-authored-by: Malte Sander <[email protected]>

Update rust/operator-binary/src/crd/mod.rs

6b359d1

Co-authored-by: Malte Sander <[email protected]>

Update rust/operator-binary/src/crd/mod.rs

ebe8079

Co-authored-by: Malte Sander <[email protected]>

corrected callout comments and regenerate charts

acbca3e

Merge branch 'main' into feat/integrate-listener-operator

8270343

use ephemeral listeners classes; fix test missing dimension

8ae3303

allow config map endpoints to be removed if cluster is stopped

f4ecd43

maltesander requested changes Apr 22, 2025

View reviewed changes

adwk67 added 6 commits April 22, 2025 11:01

review feedback

5c7022c

check for port lower-bound

1003da2

Merge branch 'main' into feat/integrate-listener-operator

8f0e539

reworked to only offer listener for webserver role

dc803f4

reworked docs

a4ec801

merge main

8beab3e

maltesander reviewed Apr 24, 2025

View reviewed changes

docs/modules/airflow/pages/usage-guide/listenerclass.adoc Outdated Show resolved Hide resolved

rust/operator-binary/src/crd/mod.rs Outdated Show resolved Hide resolved

adwk67 and others added 3 commits April 24, 2025 15:42

Update docs/modules/airflow/pages/usage-guide/listenerclass.adoc

063bded

Co-authored-by: Malte Sander <[email protected]>

Update rust/operator-binary/src/crd/mod.rs

25ce160

Co-authored-by: Malte Sander <[email protected]>

added service check to test

0095ad9

nightkr reviewed Apr 25, 2025

View reviewed changes

adwk67 added 4 commits April 25, 2025 14:39

correctly flatten WebserverConfig

7da421c

use pvcs for externally reachable endpoints

7ac6077

use group listener for webservers

0f650d3

Merge branch 'main' into feat/integrate-listener-operator

7dcff05

nightkr reviewed Apr 28, 2025

View reviewed changes

replace enum with string; use consistent webserver address for all li…

dcc21f9

…stener classes

adwk67 requested review from maltesander and nightkr April 28, 2025 12:44

removed unused test account

3d2f3e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Airflow Listener integration #604

feat: Airflow Listener integration #604

adwk67 commented Apr 4, 2025 •

edited by maltesander

Loading

Author

Reviewer

Acceptance

maltesander left a comment

maltesander left a comment

nightkr left a comment

nightkr Apr 25, 2025

adwk67 Apr 25, 2025 •

edited

Loading

adwk67 Apr 25, 2025 •

edited

Loading

adwk67 Apr 28, 2025

nightkr Apr 25, 2025

adwk67 Apr 25, 2025

nightkr Apr 28, 2025

nightkr Apr 28, 2025

adwk67 Apr 28, 2025

nightkr Apr 28, 2025

adwk67 Apr 28, 2025

adwk67 commented Apr 28, 2025

feat: Airflow Listener integration #604

Are you sure you want to change the base?

feat: Airflow Listener integration #604

Conversation

adwk67 commented Apr 4, 2025 • edited by maltesander Loading

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

maltesander left a comment

Choose a reason for hiding this comment

maltesander left a comment

Choose a reason for hiding this comment

nightkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adwk67 Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

adwk67 Apr 25, 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

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

adwk67 commented Apr 28, 2025

adwk67 commented Apr 4, 2025 •

edited by maltesander

Loading

adwk67 Apr 25, 2025 •

edited

Loading

adwk67 Apr 25, 2025 •

edited

Loading