ZooKeeper to KRaft migration #9480

ppatierno · 2023-12-21T10:11:27Z

Type of change

Enhancement / new feature

Description

This PR fixes #9433 , #9447 and #9448 about implementing the ZooKeeper to KRaft migration process.

The current status is about starting from a ZooKeeper-based cluster, creating a node pool with controllers, applying the strimzi.io/kraft: migration annotation on the Kafka CR and allowing the operator FSM to start the migration up to the "dual-write" (so creating controllers, rolling brokers, start migration). Then the user can apply the strimzi.io/kraft: enabled annotation in order to finilize the process by having the full KRaft-based cluster,

The PR won't address documentation. There will be a different PR for it. See #9633
~~This PR doesn't cover migration rollback and ZooKeeper removal as well which have different issues and will have different PRs for them.~~
This PR covers migration rollback ~~but taking into account this Kafka issue https://issues.apache.org/jira/browse/KAFKA-16101 and based on deleting the /controller znode manually as last step.~~ using rollback annotation value from the KRaftPostMigration state, the disabled from the KRaftDualWriting.
This PR also covers the ZooKeeper related resources removal when the migration ends.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md

scholzj · 2023-12-21T23:46:29Z

@ppatierno Should this be a draft?

ppatierno · 2023-12-22T08:39:21Z

@ppatierno Should this be a draft?

I can set as "draft" if you want but this PR is meant to be reviewed because it "closes" the migration implementation while waiting for implementing the rest (i.e. rollback) with a different PR (on top of this). Of course, this cannot be merged because of the Kafka 3.7 missing (until February). But this way should avoid overwhelming contributors, interested to review, with a bigger review at the end.

scholzj

Would it make sense to use some separate packages for the migration related parts? It makes it easier to delete things later.

api/src/main/java/io/strimzi/api/kafka/model/status/KafkaStatus.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

...perator/src/main/java/io/strimzi/operator/cluster/operator/resource/KRaftMigrationState.java

docker-images/kafka-based/kafka/scripts/kafka_readiness_kraft.sh

fvaleri

Left some comments, but it is working fine for me.

I also tried to kill the CO while moving from KRaftDualWriting to KRaftPostMigration, and the new instance was able to recover and complete the migration.

This PR doesn't cover migration rollback and ZooKeeper removal as well which have different issues and will have different PRs for them.

Are we describing these issues somewhere?

api/src/main/java/io/strimzi/api/kafka/model/status/KafkaStatus.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

ppatierno · 2024-02-29T12:36:41Z

/azp run regression

azure-pipelines · 2024-02-29T12:36:53Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno · 2024-02-29T17:01:15Z

/azp run kraft-regression

azure-pipelines · 2024-02-29T17:01:28Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno · 2024-02-29T17:04:26Z

/azp run upgrade

azure-pipelines · 2024-02-29T17:04:35Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-02-29T20:21:11Z

/azp run feature-gates-regression

azure-pipelines · 2024-02-29T20:21:22Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Paolo Patierno <ppatierno@live.com> Moved KafkaMetadataStateManager into the resource package Signed-off-by: Paolo Patierno <ppatierno@live.com> Added KafkaAgent endpoint to retrive ZooKeeper migration state metric Updated KafkaAgent client to retrieve ZooKeeper migration state Added logic to KafkaReconciler to check when migration ends Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed spotbugs issue Signed-off-by: Paolo Patierno <ppatierno@live.com> Moved KafkaMetadataConfigurationState in its own file Exposed metadata configuration state evaluation in corresponding KafkaMetadataConfigurationState instance methods Using loop on controllers to get the ZooKeeper migration metrics Other cleaning and refactor changes as per JS comments Signed-off-by: Paolo Patierno <ppatierno@live.com> Removed unused Random class Signed-off-by: Paolo Patierno <ppatierno@live.com> Moved the KafkaMetadataStateManager creation to the ReonciliationState because needed in the assembly operator as well Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed not exposing control plane port 9090 on KRaft broker-only nodes Fixed still exposing replication port 9091 on KRaft controllers during migration Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed duplicate KRaft configuration section when mixed-node Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed tests Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed bug about keeping replication listener on controllers until post-migration Signed-off-by: Paolo Patierno <ppatierno@live.com> Some refactoring (also based on FV comments) Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed validation for nodepools and features already in pre-migration Signed-off-by: Paolo Patierno <ppatierno@live.com> Addition on the configuration migration table Removed not used method Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed checkstyle issue Signed-off-by: Paolo Patierno <ppatierno@live.com> Running spec checker validation only when migration happens to KRaft not before Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed test Signed-off-by: Paolo Patierno <ppatierno@live.com> Updated liveness, readiness and run scripts to take process.roles into account Signed-off-by: Paolo Patierno <ppatierno@live.com> Added warning conditions when strimzi.io/kraft annotation is not used in the right way Signed-off-by: Paolo Patierno <ppatierno@live.com> Refactored the KafkaAgent and client in relation to the KRaft migration endpoint Signed-off-by: Paolo Patierno <ppatierno@live.com> Added versions validation before starting a KRaft migration Signed-off-by: Paolo Patierno <ppatierno@live.com> Removed the useKRaft flag related to the feature gate flags Making reconciliation failing fast when useKRaft feature flag is not enabled Updated related tests Signed-off-by: Paolo Patierno <ppatierno@live.com> Addressing JS comments Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed FV feedback Signed-off-by: Paolo Patierno <ppatierno@live.com> Added rollback migration to the state machine Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed spotbugs check Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed check on ZooKeeper removal only in full KRaft mode Signed-off-by: Paolo Patierno <ppatierno@live.com> Refactoring the migration path making KRaftDualWriting transient and KRaftPostMigration stable Refactored rollback to work from KRaftPostMigration Signed-off-by: Paolo Patierno <ppatierno@live.com> Removed leftover commented code Signed-off-by: Paolo Patierno <ppatierno@live.com> Moved KafkaMetadataState to the api module Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed KRaft test Signed-off-by: Paolo Patierno <ppatierno@live.com> Added ZooKeeper removal at the end of the migration Signed-off-by: Paolo Patierno <ppatierno@live.com> [ST] ZK to KRaft migration - migration process (#9580) Signed-off-by: Lukas Kral <lukywill16@gmail.com> [ST] ZK to KRaft migration - rollback process (#9589) Signed-off-by: Lukas Kral <lukywill16@gmail.com> Added migration related tests for configuration builder Signed-off-by: Paolo Patierno <ppatierno@live.com> [ST] Add AZP for migration and few fixups to the migration tests (#9615) Signed-off-by: Lukas Kral <lukywill16@gmail.com> Added unit tests for State machine of `KafkaMetadataStateManager` class (#9591) Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com> Fixed feedback Signed-off-by: Paolo Patierno <ppatierno@live.com> Added migration related tests for KafkaCluster Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed comments by JS Fixed issue with wrong authorizer during migration Signed-off-by: Paolo Patierno <ppatierno@live.com> [ST] Add ACLs to migration STs (#9644) Signed-off-by: Lukas Kral <lukywill16@gmail.com> Refactoring by passing metadata config state across the cluster creator Signed-off-by: Paolo Patierno <ppatierno@live.com> Refactored /controller znode deletion process Factored out a KRaftMigrationUtils class Signed-off-by: Paolo Patierno <ppatierno@live.com> Moved KafkaMetadataStateManager to assembly package Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed MigrationST new label selector signature Signed-off-by: Paolo Patierno <ppatierno@live.com> Made Kafka agent client local to the metadata state manager Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed a wrongly reported good migration state as invalid Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed duplicated comment Signed-off-by: Paolo Patierno <ppatierno@live.com> Added tests on Kafka Agent and related client about migration Signed-off-by: Paolo Patierno <ppatierno@live.com> Add Mock tests for `KafkaMetadataStateManager` class (#9624) Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com> Fixed OAuth with KRaft test Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed batch of comments from JS Signed-off-by: Paolo Patierno <ppatierno@live.com> Refactoring versions validation to run a KRaft migration Added unit tests on the version validation on KRaft migration Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed migration tests with correct version change instance Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed version change in the state manager test Signed-off-by: Paolo Patierno <ppatierno@live.com> Removing check on migration status from the state manager to utils class Refactored migration state manager tests Signed-off-by: Paolo Patierno <ppatierno@live.com> Moved KafkaAgent client creation for migration state to KafkaReconciler Added class for testing migration through KafkaReconciler Signed-off-by: Paolo Patierno <ppatierno@live.com> Refactored method to configure ZooKeeper, migration enabled and KRaft configuration on nodes Signed-off-by: Paolo Patierno <ppatierno@live.com> [ST] Migration - tests for migration with CO deletion during process, full path with migration and rollback (#9700) Signed-off-by: Paolo Patierno <ppatierno@live.com> Signed-off-by: Lukas Kral <lukywill16@gmail.com> Co-authored-by: Paolo Patierno <ppatierno@live.com> Added unit tests for `ZookeeperEraser` class (#9710) Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com> Added test on the broker configuration ZooKeeper migration flag Signed-off-by: Paolo Patierno <ppatierno@live.com> Added ZooKeeper migration flag as forbidden in node configuration Signed-off-by: Paolo Patierno <ppatierno@live.com> Added KafkaAssemblyOperator migration-related tests (#9720) Signed-off-by: Paolo Patierno <ppatierno@live.com> Removed logging related leftovers Signed-off-by: Paolo Patierno <ppatierno@live.com> Added unit tests for `ZookeeperReconciler` class (#9733) Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com> Fixed usage of MockKube3 in migration tests (#9744) Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed comments by JS Signed-off-by: Paolo Patierno <ppatierno@live.com> Refactoring around logging at right level Signed-off-by: Paolo Patierno <ppatierno@live.com> Fixed last JS comments Updated CHANGELOG Signed-off-by: Paolo Patierno <ppatierno@live.com> [ST] Bump resources for controller Pods in MigrationST and use AZP profile for migration tests (#9753) Signed-off-by: Lukas Kral <lukywill16@gmail.com>

Signed-off-by: Paolo Patierno <ppatierno@live.com>

fvaleri

LGTM. Great work. Thanks.

ppatierno · 2024-03-01T15:45:58Z

/azp run regression

azure-pipelines · 2024-03-01T15:46:13Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-03-01T20:18:07Z

/azp run kraft-regression

azure-pipelines · 2024-03-01T20:18:18Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-03-01T23:55:49Z

/azp run feature-gates-regression

azure-pipelines · 2024-03-01T23:56:01Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-03-01T23:56:08Z

/azp run upgrade

azure-pipelines · 2024-03-01T23:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

tombentley

Thanks Paolo.

api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaStatus.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KRaftUtils.java

tombentley · 2024-03-02T06:32:01Z

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaMetadataConfigurationState.java

+    /**
+     * @return if the Kafka metadata configuration state is from ZooKeeper-based up to KRaft migration (and dual-write) going on
+     */
+    public boolean isZooKeeperToMigration() {


I found these predictes testing for a range to be really confusing when I was reading the code. I didn't guess it was testing a range. I wonder if it would be clearer to just have a isBeforeOrEqual(KMCS other) and isAfterOrEqual(KMCS other)?

Well it would be more complicated than that, because it's a range ... so it should be something like isAfterOrEqualeZooKeeperAndBeforeOrEqualeMigration which looks worse imho. I think that Javadoc is there for helping on its meaning.

...perator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KRaftMigrationUtils.java

...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaMetadataStateManager.java

tombentley · 2024-03-02T06:53:29Z

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ZooKeeperEraser.java

+ * Class used for deleting the ZooKeeper ensemble when the ZooKeeper to KRaft migration is completed
+ * and the Kafka cluster is full KRaft, not using ZooKeeper anymore for storing metadata
+ */
+public class ZooKeeperEraser {


Erase doesn't really mean the same thing as delete or destroy. If I "erase a data base' that suggests the data is gone but the server/infra remains. I think destroy would be clearer here. Delete is OK, but even then you really need to add 'resources' in there to make it unambiguous

How would you call it? ZooKeeperDestroyer ? Anything better?

tombentley · 2024-03-02T06:56:14Z

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ZooKeeperEraser.java

+    public Future<Void> reconcile()    {
+        LOGGER.infoCr(reconciliation, "Deleting all the ZooKeeper related resources");
+        return jmxSecret()
+                .compose(i -> networkPolicy())
+                .compose(i -> serviceAccount())
+                .compose(i -> service())
+                .compose(i -> headlessService())
+                .compose(i -> certificateSecret())
+                .compose(i -> loggingAndMetricsConfigMap())
+                .compose(i -> podDisruptionBudget())
+                .compose(i -> podSet())


It mystifies me why none of these methods have the word delete in their names, but somehow PVCs are special.

Any why call this method reconcile()? It's not like you're overriding a reconcile method here. Why not call it deleteZooKeeperResources() or something?

I left reconcile to leave the overall pattern of reconciliation when it's called from the KafakAssemblyOperator.
Regarding the methods' names, I agree. I am going to call them all deleteXXXX.

tombentley · 2024-03-02T06:56:48Z

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ZooKeeperEraser.java

+     * The main reconciliation method which triggers the whole reconciliation pipeline. This is the method which is
+     * expected to be called from the outside to trigger the reconciliation.


"This is the method which is expected to be called from the outside" can't we infer that from the method visibility?

I just copied from what we have in the ZooKeeperReconciler :-P

...perator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ZooKeeperReconciler.java

ppatierno · 2024-03-02T07:50:28Z

@tombentley this PR has been here for weeks/months and now it's finally ready to be merged. All pipelines are green (except one well known issue which Lukas will fix later). I am going to merge it and evaluate your suggestions for a different PR. Sorry but it's taking too long.

ppatierno · 2024-03-04T11:30:32Z

@tombentley I opened #9770 to address your feedback. Can you review it please? Thanks!

ppatierno added the do-not-merge label Dec 21, 2023

ppatierno added this to the 0.40.0 milestone Dec 21, 2023

ppatierno mentioned this pull request Dec 21, 2023

ZooKeeper to KRaft migration #9324

Closed

5 tasks

ppatierno force-pushed the zk-kraft-migration branch from 44a5906 to d361220 Compare December 21, 2023 14:54

ppatierno marked this pull request as draft December 22, 2023 08:44

ppatierno requested review from scholzj and fvaleri December 22, 2023 08:55

scholzj reviewed Dec 22, 2023

View reviewed changes

ppatierno force-pushed the zk-kraft-migration branch 2 times, most recently from 605c3b2 to 5927e51 Compare January 3, 2024 14:36

fvaleri reviewed Jan 3, 2024

View reviewed changes

ppatierno force-pushed the zk-kraft-migration branch 7 times, most recently from f4c35b1 to d7eed02 Compare January 12, 2024 10:31

ppatierno force-pushed the zk-kraft-migration branch 3 times, most recently from 9969872 to e590474 Compare January 18, 2024 14:38

see-quick reviewed Jan 18, 2024

View reviewed changes

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java Show resolved Hide resolved

ppatierno force-pushed the zk-kraft-migration branch 2 times, most recently from 7c6805d to 3e134f3 Compare January 22, 2024 10:21

ppatierno force-pushed the zk-kraft-migration branch from 75d1298 to a118d91 Compare January 29, 2024 07:39

This was linked to issues Jan 29, 2024

[KRaft] Developing the ZooKeeper to KRaft migration rollback process #9447

Closed

[KRaft] Developing logic to remove the ZooKeeper ensemble when migrated #9448

Closed

ppatierno force-pushed the zk-kraft-migration branch from a118d91 to d5af439 Compare January 29, 2024 13:41

im-konge approved these changes Feb 29, 2024

View reviewed changes

ppatierno force-pushed the zk-kraft-migration branch 2 times, most recently from 386da52 to c0eaee9 Compare February 29, 2024 14:52

see-quick approved these changes Feb 29, 2024

View reviewed changes

ppatierno added 2 commits March 1, 2024 14:42

Saved metadata state on /tmp file (#9762)

58ce66c

Signed-off-by: Paolo Patierno <ppatierno@live.com>

ppatierno force-pushed the zk-kraft-migration branch from 0928265 to 58ce66c Compare March 1, 2024 13:43

fvaleri approved these changes Mar 1, 2024

View reviewed changes

tombentley reviewed Mar 2, 2024

View reviewed changes

ppatierno merged commit e1bcc24 into main Mar 2, 2024
41 of 43 checks passed

ppatierno deleted the zk-kraft-migration branch March 2, 2024 09:59

ppatierno mentioned this pull request Mar 4, 2024

More feedback on migration #9770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZooKeeper to KRaft migration #9480

ZooKeeper to KRaft migration #9480

ppatierno commented Dec 21, 2023 •

edited

Loading

scholzj commented Dec 21, 2023

ppatierno commented Dec 22, 2023

scholzj left a comment

fvaleri left a comment

ppatierno commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

ppatierno commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

ppatierno commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

scholzj commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

fvaleri left a comment

ppatierno commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

scholzj commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

scholzj commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

scholzj commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

tombentley left a comment

tombentley Mar 2, 2024

ppatierno Mar 4, 2024

tombentley Mar 2, 2024

ppatierno Mar 4, 2024

tombentley Mar 2, 2024

ppatierno Mar 4, 2024

tombentley Mar 2, 2024

ppatierno Mar 4, 2024

ppatierno commented Mar 2, 2024

ppatierno commented Mar 4, 2024

		* The main reconciliation method which triggers the whole reconciliation pipeline. This is the method which is
		* expected to be called from the outside to trigger the reconciliation.

ZooKeeper to KRaft migration #9480

ZooKeeper to KRaft migration #9480

Conversation

ppatierno commented Dec 21, 2023 • edited Loading

Type of change

Description

Checklist

scholzj commented Dec 21, 2023

ppatierno commented Dec 22, 2023

scholzj left a comment

Choose a reason for hiding this comment

fvaleri left a comment

Choose a reason for hiding this comment

ppatierno commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

ppatierno commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

ppatierno commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

scholzj commented Feb 29, 2024

azure-pipelines bot commented Feb 29, 2024

fvaleri left a comment

Choose a reason for hiding this comment

ppatierno commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

scholzj commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

scholzj commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

scholzj commented Mar 1, 2024

azure-pipelines bot commented Mar 1, 2024

tombentley left a 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

Choose a reason for hiding this comment

ppatierno commented Mar 2, 2024

ppatierno commented Mar 4, 2024

ppatierno commented Dec 21, 2023 •

edited

Loading