[system test] make Recovery tests run only on KRaft mode only #10637

see-quick · 2024-09-24T14:33:52Z

Type of change

Enhancement / new feature
Refactoring

Description

This PR changes our recovery tests so they can run with KRaft mode.

Checklist

Write tests
Make sure all tests pass

scholzj · 2024-09-24T14:35:41Z

Why would you want to run them in ZooKeeper mode only?

see-quick · 2024-09-25T07:26:40Z

Why would you want to run them in ZooKeeper mode only?

Well, some of the test cases do not work with KRaft here (e.g., testTopicNotAvailable, testTopicAvailable and more...). I don't have any strong opinion on this but for now, I think having these tests run only at least ZK mode is okay I think. Maybe @henryZrncik or @im-konge knows more about this.

scholzj · 2024-09-25T07:29:52Z

Why would you want to run them in ZooKeeper mode only?

Well, some of the test cases do not work with KRaft here (e.g., testTopicNotAvailable, testTopicAvailable and more...). I don't have any strong opinion on this but for now, I think having these tests run only at least ZK mode is okay I think. Maybe @henryZrncik or @im-konge knows more about this.

So, should it be fixed? Extended? Deleted? ZooKeeper will be gone soon and any ZooKeeper only tests will be deleted with it.

see-quick · 2024-09-25T14:29:34Z

Why would you want to run them in ZooKeeper mode only?

Well, some of the test cases do not work with KRaft here (e.g., testTopicNotAvailable, testTopicAvailable and more...). I don't have any strong opinion on this but for now, I think having these tests run only at least ZK mode is okay I think. Maybe @henryZrncik or @im-konge knows more about this.

So, should it be fixed? Extended? Deleted? ZooKeeper will be gone soon and any ZooKeeper only tests will be deleted with it.

I mean if those tests could run also on KRaft maybe we should update them...I think @henryZrncik and @fvaleri were trying to somehow fix it but I am not sure if that's possible (it was something related to UTO).

scholzj · 2024-09-25T16:39:51Z

Why would you want to run them in ZooKeeper mode only?

Well, some of the test cases do not work with KRaft here (e.g., testTopicNotAvailable, testTopicAvailable and more...). I don't have any strong opinion on this but for now, I think having these tests run only at least ZK mode is okay I think. Maybe @henryZrncik or @im-konge knows more about this.

So, should it be fixed? Extended? Deleted? ZooKeeper will be gone soon and any ZooKeeper only tests will be deleted with it.

I mean if those tests could run also on KRaft maybe we should update them...I think @henryZrncik and @fvaleri were trying to somehow fix it but I am not sure if that's possible (it was something related to UTO).

Right, but we need to understand what exactly the problem is. UTO is now used everywhere. So not sure why would that make a difference Zoo versus KRaft.

henryZrncik

Thanks for PR!

IMHO Changes in the RecoveryST would need a few more steps to run correctly.

Regarding NamespaceDeletionRecovery as this tests represent steps done in documentation it would be nice to keep these tests (referencing to removing zookeeper related tests) it would be probably nice to find out what is the cause of problem in kraft after you resolved problem with incorrect zookeeper configuration.

henryZrncik · 2024-09-30T11:33:57Z

systemtest/src/test/java/io/strimzi/systemtest/operators/RecoveryST.java

@@ -71,7 +66,6 @@ void testRecoveryFromKafkaStrimziPodSetDeletion() {
    }

    @IsolatedTest("We need for each test case its own Cluster Operator")
-    @KRaftNotSupported("Zookeeper is not supported by KRaft mode and is used in this test class")


regarding allowing tests in kraft:

tests with services do not make much sense in kraft as these services are no longer present.

test of pod sets would make sense but it would still fail as currently we target specifically zookeeper so this needs adjustment to target rather controller by selector. e.g. to target strimzi pod set for controller my-cluster-7-c-9aa820e3 instead of zookeeper it expects something like my-cluster-7-zookeeper.

henryZrncik · 2024-09-30T11:37:36Z

systemtest/src/test/java/io/strimzi/systemtest/operators/NamespaceDeletionRecoveryST.java

-        resourceManager.createResourceWithWait(
-            NodePoolsConverter.convertNodePoolsIfNeeded(
-                KafkaNodePoolTemplates.brokerPoolPersistentStorage(testStorage.getNamespaceName(), testStorage.getBrokerPoolName(), testStorage.getClusterName(), 3)
-                    .editSpec()
-                        .withNewPersistentClaimStorage()
-                            .withSize("1Gi")
-                            .withStorageClass(storageClassName)
-                        .endPersistentClaimStorage()
-                    .endSpec()
-                    .build(),
-                KafkaNodePoolTemplates.controllerPoolPersistentStorage(testStorage.getNamespaceName(), testStorage.getControllerPoolName(), testStorage.getClusterName(), 3)
-                    .editSpec()
-                        .withNewPersistentClaimStorage()
-                            .withSize("1Gi")
-                            .withStorageClass(storageClassName)
-                        .endPersistentClaimStorage()
-                    .endSpec()
-                    .build()
-            )
-        );
-        resourceManager.createResourceWithWait(KafkaTemplates.kafkaPersistent(testStorage.getNamespaceName(), testStorage.getClusterName(), 3, 3)


this test testTopicNotAvailable actually fails with kraft on a step of recreating Kafka Cluster after namespace is deleted. All to be recreated pods get into crash-loop as they try to connect to incorrect cluster.

For example:

Exception in thread “main” java.lang.RuntimeException: Invalid cluster.id in: /var/lib/kafka/data/kafka-log1/meta.properties. Expected 28c5tB6tSlCjEHep6l3Jww, but read TRrzEInZRgCs_R9iNz2Gkw at org.apache.kafka.metadata.properties.MetaPropertiesEnsemble.verify(MetaPropertiesEnsemble.java:509)

This feels like a bug to me @scholzj @fvaleri ?

@see-quick finally I was able to have a look at this.

deploy the Kafka cluster without Topic Operator - otherwise topics will be deleted

This is not needed anymore with the new Topic Operator, so it can be removed from the code.

To make this test work on my local Minikube I had to update the StorageClass.VolumeBindingMode from WaitForFirstConsumer to Immediate. Is there any Minikube configuration you are doing here? Is this documented somewhere?

I was able to reproduce the issue mentioned by @henryZrncik:

Exception in thread "main" java.lang.RuntimeException: Invalid cluster.id in: /var/lib/kafka/data/kafka-log0/meta.properties. Expected kQHv733NQIew9aw9uCXnDA, but read 2DLef4_8TqOVdBjzCORB1Q

I also think this is a bug, so I raised #10722. The attached fix should work with this test. Hope it helps.

This is not a bug, it means that this was not set when recovering the Kafka CR.

Signed-off-by: see-quick <[email protected]>

see-quick · 2024-10-09T10:23:11Z

Thanks for PR!

IMHO Changes in the RecoveryST would need a few more steps to run correctly.

Regarding NamespaceDeletionRecovery as this tests represent steps done in documentation it would be nice to keep these tests (referencing to removing zookeeper related tests) it would be probably nice to find out what is the cause of problem in kraft after you resolved problem with incorrect zookeeper configuration.

Thanks for investigating this problem. So let's do the following:

Remove ZK-only tests, which can't be run as KRAft I think we have 2 of them (i.e., testRecoveryFromZookeeperServiceDeletion and testRecoveryFromZookeeperHeadlessServiceDeletion).

And in the case of testTopicNotAvailable, I am unsure if that's a possible bug in KRaft? 🤔 I will double-check this one.

So in summary I think making those tests KRaft-only compatible would be better than just ZK (because of the mentioned issue that ZK will be removed soon).

Signed-off-by: see-quick <[email protected]>

see-quick added needs review System tests labels Sep 24, 2024

see-quick added this to the 0.44.0 milestone Sep 24, 2024

see-quick requested a review from a team September 24, 2024 14:33

see-quick self-assigned this Sep 24, 2024

see-quick force-pushed the recovery-zk-mode branch from 010b8d8 to 6d2db69 Compare September 25, 2024 07:27

henryZrncik reviewed Sep 30, 2024

View reviewed changes

[system test] make Recovery tests run only on ZK mode only

089052a

Signed-off-by: see-quick <[email protected]>

make tests KRaft compatible

d9b2a03

Signed-off-by: see-quick <[email protected]>

see-quick force-pushed the recovery-zk-mode branch from 6d2db69 to d9b2a03 Compare October 11, 2024 10:49

see-quick changed the title ~~[system test] make Recovery tests run only on ZK mode only~~ [system test] make Recovery tests run only on KRaft mode only Oct 11, 2024

refactor a bit

d1aa08d

Signed-off-by: see-quick <[email protected]>

fvaleri mentioned this pull request Oct 16, 2024

[DOC]: Invalid cluster.id error when initializing a new KRaft cluster with existing volumes #10722

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[system test] make Recovery tests run only on KRaft mode only #10637

[system test] make Recovery tests run only on KRaft mode only #10637

see-quick commented Sep 24, 2024 •

edited

Loading

scholzj commented Sep 24, 2024

see-quick commented Sep 25, 2024

scholzj commented Sep 25, 2024

see-quick commented Sep 25, 2024

scholzj commented Sep 25, 2024 •

edited

Loading

henryZrncik left a comment

henryZrncik Sep 30, 2024

henryZrncik Sep 30, 2024

see-quick Oct 11, 2024

fvaleri Oct 16, 2024

scholzj Oct 16, 2024

see-quick commented Oct 9, 2024

[system test] make Recovery tests run only on KRaft mode only #10637

Are you sure you want to change the base?

[system test] make Recovery tests run only on KRaft mode only #10637

Conversation

see-quick commented Sep 24, 2024 • edited Loading

Type of change

Description

Checklist

scholzj commented Sep 24, 2024

see-quick commented Sep 25, 2024

scholzj commented Sep 25, 2024

see-quick commented Sep 25, 2024

scholzj commented Sep 25, 2024 • edited Loading

henryZrncik left a comment

Choose a reason for hiding this comment

henryZrncik Sep 30, 2024

Choose a reason for hiding this comment

henryZrncik Sep 30, 2024

Choose a reason for hiding this comment

see-quick Oct 11, 2024

Choose a reason for hiding this comment

fvaleri Oct 16, 2024

Choose a reason for hiding this comment

scholzj Oct 16, 2024

Choose a reason for hiding this comment

see-quick commented Oct 9, 2024

see-quick commented Sep 24, 2024 •

edited

Loading

scholzj commented Sep 25, 2024 •

edited

Loading