Migrate kafka-operator to use listener-operator for exposing brokers #443

nightkr · 2022-08-04T14:41:40Z

Description

Fixes #714.

Blockers:

ClusterResources::delete_orphaned_resources also deletes objects that are not owned directly by it operator-rs#861

Followup issues/regressions:

Stop NodePort services from flapping when a replica comes on- or offline listener-operator#220
Allow listenerclasses to specify whether to prefer IP or Hostname addresses listener-operator#139

Review Checklist

Code contains useful comments
CRD change approved (or not applicable)
(Integration-)Test cases added (or not applicable)
Documentation added (or not applicable)
Changelog updated (or not applicable)
Cargo.toml only contains references to git tags (not specific commits or branches)
Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

This is far from final (for example, we'd need to expose a configurable LoadBalancerClass, for example), but I wanted to see whether lb-op would be a good fit for this operator first.

nightkr · 2022-08-24T12:32:24Z

Blocked on stackabletech/listener-operator#1

maltesander · 2022-08-30T10:16:01Z

Tested and works!

…erator into spike/lb-operator

lfrancke · 2022-10-24T20:05:41Z

I've moved this back into "Ready for Development" because the Listener operator is now merged. AFAIU this should now be ready to be worked on again?

siegfriedweber · 2022-10-26T11:40:43Z

I've moved this back into "Ready for Development" because the Listener operator is now merged. AFAIU this should now be ready to be worked on again?

This pull request is updated as requested by stackabletech/listener-operator#5. When stackabletech/operator-rs#496 is merged and operator-rs and the listener-operator are released then this pull request can be made ready for review.

nightkr · 2023-09-12T14:55:49Z

Tests currently fail due to stackabletech/listener-operator#110.

nightkr · 2023-09-20T00:42:26Z

The tests now pass!

sbernauer · 2023-09-20T12:27:09Z

@nightkr do you plan to merge this or add the WIP functionality of secret-op beforehand, so that the kafka broker certificates will include the listener addresses?
It'd be awesome if we get it right in the first run :)

nightkr · 2023-09-20T13:33:36Z

Preferably not, since we're still working out a few API kinks there (truststore passwords), and I'd rather keep this PR focused. ^^

sbernauer

I started review but was sidetracked again. Submitting my comments early, as you are back :)

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

docs/modules/kafka/examples/getting_started/kafka.yaml

CHANGELOG.md

rust/crd/src/security.rs

rust/operator-binary/src/discovery.rs

rust/operator-binary/src/kafka_controller.rs

Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>

…erator into spike/lb-operator

sbernauer

Just to check: stackablectl stacklet list currently returns:

│ kafka     ┆ simple-kafka ┆ default   ┆ broker-default-bootstrap-kafka    172.18.0.2:32639 ┆ Available, Reconciling, Running │
│           ┆              ┆           ┆ broker-default-bootstrap-metrics  172.18.0.2:30178 ┆                                 │

stackablectl uses the app.kubernetes.io/ labels to find all Listeners for a given stacklet. Only the bootstrap Listener has these labels set, is this intentional? I think this is due to the fact that pvcs inherit the labels of the Pod, but ephemeral volumes not

➜  kafka-operator git:(spike/lb-operator) ✗ k get listener                                                                                                           kind-kind
NAME                                            AGE
simple-kafka-broker-default-0-listener-broker   6m51s
simple-kafka-broker-default-1-listener-broker   6m49s
simple-kafka-broker-default-2-listener-broker   6m51s
simple-kafka-broker-default-bootstrap           10m
➜  kafka-operator git:(spike/lb-operator) ✗ k get listener -l app.kubernetes.io/name=kafka                                                                           kind-kind
NAME                                    AGE
simple-kafka-broker-default-bootstrap   10m

When I set the recommended labels on the ephermal volumes they get propagated to the Listener and all Listeners show up

│ kafka     ┆ simple-kafka ┆ default   ┆ broker-default-0-listener-broker-kafka    172.18.0.2:32579 ┆ Available, Reconciling, Running │
│           ┆              ┆           ┆ broker-default-0-listener-broker-metrics  172.18.0.2:30432 ┆                                 │
│           ┆              ┆           ┆ broker-default-1-listener-broker-kafka    172.18.0.2:31088 ┆                                 │
│           ┆              ┆           ┆ broker-default-1-listener-broker-metrics  172.18.0.2:30439 ┆                                 │
│           ┆              ┆           ┆ broker-default-2-listener-broker-kafka    172.18.0.2:31803 ┆                                 │
│           ┆              ┆           ┆ broker-default-2-listener-broker-metrics  172.18.0.2:31150 ┆                                 │
│           ┆              ┆           ┆ broker-default-bootstrap-kafka            172.18.0.2:32639 ┆                                 │
│           ┆              ┆           ┆ broker-default-bootstrap-metrics          172.18.0.2:30178 ┆                                 │

I think I would be in favor of adding the recommended labels to all listener volumes, WDYT?
This is the same thing as we did in hdfs-op in stackabletech/hdfs-operator#534, I would be in favor of copying the implementation from there

rust/operator-binary/src/kafka_controller.rs

nightkr · 2024-10-02T10:12:41Z

Clients should only configure the bootstrap address(es), the Kafka client is then responsible for resolving that to the relevant replicas.

sbernauer · 2024-10-02T10:19:46Z

Agreed. I'm a bit torn between "we should label everything" and "I don't want everything to show in stacklet list".
I'm happy to merge as-is, but than please add a comment above Labels::new() why we choose to not label the broker Listener objects.

sbernauer · 2024-10-02T10:21:31Z

Just for the record: I think there is value in having the metrics endpoint for every individual broker, but as most customers will probably use Prometheus ServiceMonitor this is rather low prio

sbernauer · 2024-10-02T10:28:21Z

Somewhat related: Is it intentional that the simple-kafka-broker-default-bootstrap Listener contains the ingressAddress three times in the status?
Do we want to de-duplicate that list in listener-op?

apiVersion: listeners.stackable.tech/v1alpha1
kind: Listener
metadata:
  name: simple-kafka-broker-default-bootstrap
  namespace: default
# ...
spec:
  className: external-stable
  extraPodSelectorLabels: {}
  ports:
  - name: metrics
    port: 9606
    protocol: TCP
  - name: kafka
    port: 9092
    protocol: TCP
  publishNotReadyAddresses: null
status:
  ingressAddresses:
  - address: 172.18.0.2
    addressType: IP
    ports:
      kafka: 32639
      metrics: 30178
  - address: 172.18.0.2
    addressType: IP
    ports:
      kafka: 32639
      metrics: 30178
  - address: 172.18.0.2
    addressType: IP
    ports:
      kafka: 32639
      metrics: 30178
  nodePorts:
    kafka: 32639
    metrics: 30178
  serviceName: simple-kafka-broker-default-bootstrap

nightkr · 2024-10-02T13:31:37Z

Somewhat related: Is it intentional that the simple-kafka-broker-default-bootstrap Listener contains the ingressAddress three times in the status?

That smells like a minor bug that I would suggest filing against list-op.

sbernauer

This are the two changes needed to label the Listener objects correctly. I would prefer to label the Listeners correctly.
This causes them to show up in stacklet list, if this is undesired we could filter them out there

sbernauer · 2024-10-02T13:34:00Z

rust/operator-binary/src/kafka_controller.rs

+    pvcs.push(
+        ListenerOperatorVolumeSourceBuilder::new(
+            &ListenerReference::ListenerName(kafka.bootstrap_service_name(rolegroup_ref)),
+            &Labels::new(),


Suggested change

&Labels::new(),

&Labels::role_selector(kafka, APP_NAME, &rolegroup_ref.role)

.context(LabelBuildSnafu)?,

7aa5919 applies it to the Listener itself.

7dae34d for the PVC

Had to revert it slightly to avoid upgrade issues: 2913081

sbernauer · 2024-10-02T13:34:17Z

rust/operator-binary/src/kafka_controller.rs

+        .add_listener_volume_by_listener_class(
+            LISTENER_BROKER_VOLUME_NAME,
+            &merged_config.broker_listener_class,
+            &Labels::new(),


Suggested change

&Labels::new(),

&Labels::role_group_selector(

kafka,

APP_NAME,

&rolegroup_ref.role,

&rolegroup_ref.role_group,

)

.context(LabelBuildSnafu)?,

sbernauer · 2024-10-02T13:47:54Z

That smells like a minor bug that I would suggest filing against list-op.

I was not sure if this was a bug. Created stackabletech/listener-operator#229

It broke upgrading

nightkr · 2024-10-04T10:08:02Z

Tests passed for 2913081: https://testing.stackable.tech/job/kafka-operator-it-custom/8/, https://testing.stackable.tech/job/kafka-operator-it-custom/9/

Migrate kafka-operator to use lb-operator for exposing brokers

6c9c18f

This is far from final (for example, we'd need to expose a configurable LoadBalancerClass, for example), but I wanted to see whether lb-op would be a good fit for this operator first.

nightkr force-pushed the spike/lb-operator branch from 9c751ca to 6c9c18f Compare August 4, 2022 14:43

sbernauer assigned nightkr Aug 11, 2022

nightkr mentioned this pull request Aug 24, 2022

[Merged by Bors] - Initial listener-operator implementation stackabletech/listener-operator#1

Closed

regenerated charts

ae8ed3a

nightkr added 2 commits September 8, 2022 16:28

Update for lb-operator -> listener-operator rebrand

f57097b

Merge branch 'spike/lb-operator' of github.com:stackabletech/kafka-op…

d963287

…erator into spike/lb-operator

razvan mentioned this pull request Sep 27, 2022

Handle stale information/clean up stale resources #378

Closed

6 tasks

Merge branch 'main' into spike/lb-operator

1f5a4e5

siegfriedweber force-pushed the spike/lb-operator branch from 7e287d8 to 1f5a4e5 Compare October 18, 2022 14:53

lfrancke mentioned this pull request Oct 24, 2022

Listener Operator first release stackabletech/issues#281

Open

siegfriedweber added 4 commits October 25, 2022 11:19

Merge branch 'main' into spike/lb-operator

9c4b0b3

Use ListenerOperatorVolumeSourceBuilder

3a72373

Use shortcut for creating the listener volume

f35b23b

Allow git source for operator-rs temporarily

0b70333

sbernauer and others added 5 commits March 1, 2023 17:03

Merge remote-tracking branch 'origin/main' into spike/lb-operator

1d5749e

Rename node_cmd -> node_address_cmd

e9d768a

Remove operator-rs entry in deny.toml

006b122

Merge remote-tracking branch 'origin/main' into spike/lb-operator

6d84c59

Merge branch 'main' into spike/lb-operator

df70f88

nightkr marked this pull request as ready for review September 20, 2023 00:42

Merge branch 'main' into spike/lb-operator

6d47407

sbernauer self-requested a review September 18, 2024 07:08

sbernauer requested changes Sep 20, 2024

View reviewed changes

nightkr and others added 8 commits October 1, 2024 16:21

Update docs/modules/kafka/examples/getting_started/kafka.yaml

194d98d

Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>

Rename TLS volume variables for consistency

b01c22f

Rename volumes/paths too, correspondingly

220bc4c

Merge branch 'spike/lb-operator' of github.com:stackabletech/kafka-op…

2da65c6

…erator into spike/lb-operator

Remove stale FIXME

df56f73

Add RBAC permission to delete listeners (orphan clenaup)

66db99c

Make service_ports return ListenerPorts instead

362dc80

Recommend using the same ListenerClass across rolegroups

fa9b8e2

nightkr requested a review from sbernauer October 1, 2024 16:01

sbernauer requested changes Oct 2, 2024

View reviewed changes

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

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

sbernauer mentioned this pull request Oct 2, 2024

chore(kafka): Remove kubectl stackabletech/docker-images#884

Open

sbernauer requested changes Oct 2, 2024

View reviewed changes

sbernauer mentioned this pull request Oct 2, 2024

Duplicate ingressAddresses in Listener status when using NodePort ListenerClass stackabletech/listener-operator#229

Open

nightkr added 5 commits October 2, 2024 16:01

Apply recommended labels to broker listeners

88dde6b

Apply the correct rolegroup to bootstrap listeners, too

7aa5919

Apply the correct rolegroup label to the rolegroup configmaps

534d88e

Apply recommended labels to the bootstrap listener PVC too

7dae34d

Merge branch 'main' into spike/lb-operator

853d5f1

nightkr requested a review from sbernauer October 2, 2024 14:34

nightkr added 2 commits October 3, 2024 15:34

Remove version from persistent PVCs

2913081

It broke upgrading

Add note about upgrading to changelog

11a4814

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate kafka-operator to use listener-operator for exposing brokers #443

Migrate kafka-operator to use listener-operator for exposing brokers #443

nightkr commented Aug 4, 2022 •

edited

Loading

nightkr commented Aug 24, 2022

maltesander commented Aug 30, 2022

lfrancke commented Oct 24, 2022

siegfriedweber commented Oct 26, 2022

nightkr commented Sep 12, 2023

nightkr commented Sep 20, 2023

sbernauer commented Sep 20, 2023 •

edited

Loading

nightkr commented Sep 20, 2023

sbernauer left a comment

sbernauer left a comment •

edited

Loading

nightkr commented Oct 2, 2024

sbernauer commented Oct 2, 2024

sbernauer commented Oct 2, 2024

sbernauer commented Oct 2, 2024

nightkr commented Oct 2, 2024

sbernauer left a comment

sbernauer Oct 2, 2024

nightkr Oct 2, 2024

nightkr Oct 2, 2024

nightkr Oct 3, 2024

sbernauer Oct 2, 2024

nightkr Oct 2, 2024

sbernauer commented Oct 2, 2024

nightkr commented Oct 4, 2024

	&Labels::new(),
	&Labels::role_selector(kafka, APP_NAME, &rolegroup_ref.role)
	.context(LabelBuildSnafu)?,

-            &Labels::new(),
+            &Labels::role_group_selector(
+                kafka,
+                APP_NAME,
+                &rolegroup_ref.role,
+                &rolegroup_ref.role_group,
+            )
+            .context(LabelBuildSnafu)?,

Migrate kafka-operator to use listener-operator for exposing brokers #443

Are you sure you want to change the base?

Migrate kafka-operator to use listener-operator for exposing brokers #443

Conversation

nightkr commented Aug 4, 2022 • edited Loading

Description

Review Checklist

nightkr commented Aug 24, 2022

maltesander commented Aug 30, 2022

lfrancke commented Oct 24, 2022

siegfriedweber commented Oct 26, 2022

nightkr commented Sep 12, 2023

nightkr commented Sep 20, 2023

sbernauer commented Sep 20, 2023 • edited Loading

nightkr commented Sep 20, 2023

sbernauer left a comment

Choose a reason for hiding this comment

sbernauer left a comment • edited Loading

Choose a reason for hiding this comment

nightkr commented Oct 2, 2024

sbernauer commented Oct 2, 2024

sbernauer commented Oct 2, 2024

sbernauer commented Oct 2, 2024

nightkr commented Oct 2, 2024

sbernauer left a comment

Choose a reason for hiding this comment

sbernauer Oct 2, 2024

Choose a reason for hiding this comment

nightkr Oct 2, 2024

Choose a reason for hiding this comment

nightkr Oct 2, 2024

Choose a reason for hiding this comment

nightkr Oct 3, 2024

Choose a reason for hiding this comment

sbernauer Oct 2, 2024

Choose a reason for hiding this comment

nightkr Oct 2, 2024

Choose a reason for hiding this comment

sbernauer commented Oct 2, 2024

nightkr commented Oct 4, 2024

nightkr commented Aug 4, 2022 •

edited

Loading

sbernauer commented Sep 20, 2023 •

edited

Loading

sbernauer left a comment •

edited

Loading