Add zbchaos commands for enabling tests to simulate multi-region dataloss and recover #230

deepthidevaki · 2022-11-14T15:57:17Z

This PR adds commands to support testing multi-region dataloss and recovery. We have added commands for

zbchaos prepare Prepare the cluster by adding init container to the statefulset and adding a configMap. With the help of init container and the ConfigMap, we can control when a broker that suffered from dataloss is restarted.
The configMap contains flags block_{nodeId} = true|false corresponding to each nodeId. This is available to InitContainer in the mounted volume via a file corresponding to each flag /etc/config/block_{nodeId}. InitContainer is blocked if the flag is set to true. When the configMap is updated, this is reflected in the container eventually. There might be a delay, but eventually /etc/config/block_{nodeId} will have the updated value and the InitContainer can break out of the loop.
zbchaos dataloss delete Delete the broker and its data. Sets the flag in configMap to true to block the startup in InitContainer.
zbchaos dataloss recover Restarts a broker and wait until it has recovered the data. Resets the flag in configMap so that the initContainer can exit and the broker container can start. Also wait until the pods are ready, which is necessary to ensure that the broker have recovered all data.

prepare is added as a generic command, not part of dataloss, because this can be used to apply other patches (eg:- apply patch for enabling network permissions). We have to run this command only once per cluster, and repeat the tests without re-running prepare.

To test the dataloss and recovery, we want to setup a cluster with 4 brokers, and replication factor 4. Node 0 and 1 belongs to region 1 and Node 2 and 3 belongs to region 2. Assuming there is such a cluster in the given namespace, we can simulate the data loss and recovery by running the following commands:

zbchaos prepare --namespace NAMESPACE // Need tor run only once in the namespace
zbchaos dataloss delete --namespace NAMESPACE --nodeId 0
zbchaos dataloss delete --namespace NAMESPACE --nodeId 1
zbchaos recover --namespace NAMESAPCE --nodeId 0 // Wait until one node is fully recovered before recovering the second one
zbchaos recover --namespace NAMESPACE --nodeId 1

The above commands simulates full data loss of region 1 and moving the failed pods from region 1 to region 2. You can then repeat steps 2-5 to simulate moving those pods back to region 1. Or run steps 2-5 with nodes 2 and 3 to simulate dataloss of region 2.

PS:- It works for clusters in our benchmark deployed via helm. It is not fully tested for SaaS yet.

PPS:- This PR contains only the supporting commands. The test is not automated yet.

ChrisKujawa · 2022-11-15T06:43:52Z

Hey @deepthidevaki had no deep look into your PR yet, but I feel zbchaos prepare is too general. Right now we run patches related to the experiment or action within the action. Like disconnecting we run the patch for the network. If the patch was already applied this is no issue since it is idempotent.

This allows us to just run one experiment/action and if we take a look at the code the related setup code is also close to the action. Furthermore, when playing around with the tool it is not something you might miss during experimenting like you would if you had a separate command for it.

I would argue that we can do here the same and move your config magic also to the data loss action.

deepthidevaki · 2022-11-15T07:35:45Z

Thanks @Zelldon for the comment.
I thought it make sense to setup the cluster once and run all kinds of experiment on it after that. I don't have a strong opinion on it. As you said applying the patch twice is not a problem. I will move prepare as a sub command in dataloss.

For dataloss simulation, once a broker is deleted with dataloss, the broker should not be automatically restarted. To help control when a broker will be restarted, we add a init container which waits in an infinite loop until a flag is set in the config map.

The status of the Pid is Running even before it is ready. So checking the status is not enough.

ChrisKujawa · 2022-11-15T07:59:06Z

Thanks @deepthidevaki 🙇 🙏

ChrisKujawa

Thanks @deepthidevaki great addition! Really interesting solution with the init container and configmap.

I have some remarks please have a look below, before we merge it.

I feel it is a bit too specialized for the two data center setup, I guess would be great if we could make it either configurable how many brokers we have or depend on the topology (meaning the configmap and 8 entries). Furthermore, I guess it would be also nice if we could use it for experiments like: follower of partition X experience a dataloss or leader of partition Y experience a dataloss. I think for that we would need some more parameters as we have in the disconnect/connect.

I understand if you don't want to do it right now, but maybe you could then create a follow-up issue :)

go-chaos/cmd/dataloss_sim.go

go-chaos/internal/dataloss_sim_helper.go

ChrisKujawa · 2022-11-16T08:05:08Z

go-chaos/internal/dataloss_sim_helper.go

+	}
+
+	// Adds init container patch
+	patch := []byte(`{


🔧 was thinking whether we could extract this into an own function since it is quite long

ChrisKujawa · 2022-11-16T08:08:06Z

go-chaos/internal/dataloss_sim_helper.go

+				"block_6": "false",
+				"block_7": "false",
+				"block_8": "false",
+			},


💭 🔧 Might make sense to parameterize this or do it based on the topology? 🤔

I had thought about it. But I don't want to do it now. Now our tests are at max with 6 nodes, so here upto 8 nodeId would be enough. When we need to test on larger cluster we can then modify it to make it more flexible based on the topology.

go-chaos/internal/pods.go

go-chaos/internal/volume.go

ChrisKujawa · 2022-11-16T08:14:38Z

go-chaos/internal/volume.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func (c K8Client) DeletePvcOfBroker(podName string) error {


❌ Would be great to write a test for it, at least for the getVolumneForPodName

See related tests https://github.com/zeebe-io/zeebe-chaos/blob/main/go-chaos/internal/pods_test.go

Will add 👍

deepthidevaki · 2022-11-16T10:50:31Z

I feel it is a bit too specialized for the two data center setup

I don't think so. The commands are generic enough - delete data of one node/recover one node. So you could use the commands to define experiment on a cluster with any number of brokers (except the hardcoded configmap which limits the clustersize to 9) and delete data of any broker.

ChrisKujawa · 2022-11-16T10:54:04Z

I think you're right. 👍

deepthidevaki · 2022-11-16T12:05:19Z

@Zelldon I have applied your comments. I have also updated the command to pause reconciliation before applying the init container patch.

@npepinpe Would you like to take a look at it as well? For me it is ok if one of you approve it.

npepinpe · 2022-11-16T12:23:36Z

Then since Chris already did a thorough review, I'll leave you two to it 😄

ChrisKujawa

Thanks 👍 🎖️

deepthidevaki · 2022-11-16T13:46:17Z

Thanks @Zelldon for the review 💯

deepthidevaki requested a review from npepinpe November 14, 2022 15:57

deepthidevaki requested a review from ChrisKujawa as a code owner November 14, 2022 15:57

deepthidevaki mentioned this pull request Nov 14, 2022

Increase test coverage for recovery from dataloss camunda/camunda#9960

Closed

deepthidevaki force-pushed the dd-dataloss-sim branch from 3cb2943 to 5e2b2bf Compare November 15, 2022 07:42

deepthidevaki added 9 commits November 15, 2022 08:45

refactor: rename file

c9b0844

feat: add commands for simulating dataloss

dff11d8

fix: wait until pod is READY

f21a575

The status of the Pid is Running even before it is ready. So checking the status is not enough.

refactor: improve log

b2dbd30

fix: add missing format string

5964a69

fix: add license

ce74fe9

refactor: move prepare command to dataloss sub command

c8494ec

refactor: fix merge conflict

19398d8

deepthidevaki force-pushed the dd-dataloss-sim branch from 5e2b2bf to 19398d8 Compare November 15, 2022 07:47

ChrisKujawa reviewed Nov 16, 2022

View reviewed changes

deepthidevaki added 5 commits November 16, 2022 12:17

refactor: rename function

9f98693

refactor: log while waiting in initcontainer

f666dd9

refactor: simplify condition

7e52867

test: add test for get volume

bc91ece

fix: pause reconcilliation before adding init container

aa1d2c0

npepinpe removed their request for review November 16, 2022 12:23

ChrisKujawa approved these changes Nov 16, 2022

View reviewed changes

deepthidevaki merged commit 2a1cfb2 into main Nov 16, 2022

deepthidevaki deleted the dd-dataloss-sim branch November 16, 2022 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zbchaos commands for enabling tests to simulate multi-region dataloss and recover #230

Add zbchaos commands for enabling tests to simulate multi-region dataloss and recover #230

deepthidevaki commented Nov 14, 2022

ChrisKujawa commented Nov 15, 2022

deepthidevaki commented Nov 15, 2022

ChrisKujawa commented Nov 15, 2022

ChrisKujawa left a comment

ChrisKujawa Nov 16, 2022

ChrisKujawa Nov 16, 2022

deepthidevaki Nov 16, 2022

ChrisKujawa Nov 16, 2022

deepthidevaki Nov 16, 2022

deepthidevaki commented Nov 16, 2022

ChrisKujawa commented Nov 16, 2022

deepthidevaki commented Nov 16, 2022

npepinpe commented Nov 16, 2022

ChrisKujawa left a comment

deepthidevaki commented Nov 16, 2022

Add zbchaos commands for enabling tests to simulate multi-region dataloss and recover #230

Add zbchaos commands for enabling tests to simulate multi-region dataloss and recover #230

Conversation

deepthidevaki commented Nov 14, 2022

ChrisKujawa commented Nov 15, 2022

deepthidevaki commented Nov 15, 2022

ChrisKujawa commented Nov 15, 2022

ChrisKujawa left a comment

Choose a reason for hiding this comment

ChrisKujawa Nov 16, 2022

Choose a reason for hiding this comment

ChrisKujawa Nov 16, 2022

Choose a reason for hiding this comment

deepthidevaki Nov 16, 2022

Choose a reason for hiding this comment

ChrisKujawa Nov 16, 2022

Choose a reason for hiding this comment

deepthidevaki Nov 16, 2022

Choose a reason for hiding this comment

deepthidevaki commented Nov 16, 2022

ChrisKujawa commented Nov 16, 2022

deepthidevaki commented Nov 16, 2022

npepinpe commented Nov 16, 2022

ChrisKujawa left a comment

Choose a reason for hiding this comment

deepthidevaki commented Nov 16, 2022