Zookeeper backup controller #413

ibumarskov · 2021-11-23T12:34:35Z

Change log description

Implementation of automatic data backup.

Purpose of the change

Implement #408

What the code does

Adds a dedicated controller responsible for automatic backups of zookeeper data. Current
implementation provides periodic copying of transaction logs and snapshots (on-disk representation of the data) to
dedicated persistence volume with specified storage class within kubernetes cluster.

How to verify it

Create CR for zookeper backup controller
Exmaple CR of zookeeper backup:

apiVersion: "zookeeper.pravega.io/v1beta1"
kind: "ZookeeperCluster"
metadata:
  name: "example-backup"
spec:
    zookeeperCluster: "zookeeper-cluster"
    schedule: "0 0 */1 * *"
    backupsToKeep: "7"
    dataStorageClass: "backup-class"
    image:
      repository: "pravega/zkbackup"
      tag: "0.1"

Additional required actions:

Build backup docker image and upload it to hub.docker with image name pravega/zkbackup:0.1

Signed-off-by: Ilya Bumarskov <[email protected]>

codecov · 2021-11-23T12:53:33Z

Codecov Report

Base: 85.05% // Head: 85.23% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (847e788) compared to base (c29d475).
Patch coverage: 80.50% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
+ Coverage   85.05%   85.23%   +0.18%     
==========================================
  Files          12       14       +2     
  Lines        1606     1924     +318     
==========================================
+ Hits         1366     1640     +274     
- Misses        155      184      +29     
- Partials       85      100      +15

Impacted Files	Coverage Δ
controllers/zookeeperbackup_controller.go	`74.69% <74.69%> (ø)`
api/v1beta1/zookeeperbackup_types.go	`100.00% <100.00%> (ø)`
api/v1beta1/zz_generated.deepcopy.go	`98.23% <100.00%> (+0.31%)`	⬆️
controllers/zookeepercluster_controller.go	`69.07% <0.00%> (+3.61%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jkhalack · 2021-11-23T13:47:43Z

deploy/role.yaml

+      - zookeeper.pravega.io
+    resources:
+      - '*'
+      - zookeeperbackups


Would not zookeeperbackups be already included in the first line ('*')?

jkhalack · 2021-11-24T15:17:28Z

Build backup docker image and upload it to hub.docker with image name pravega/zkbackup:0.1

It might be easier to enhance the operator docker image with the backup script (e.g., copy the script here https://github.com/pravega/zookeeper-operator/blob/master/Dockerfile#L34) and use it in the CronJob with a different entrypoint. This way we won't need to maintain an extra docker image or pull it to a cluster, as it would already be present in the system.

ibumarskov · 2021-11-24T15:31:38Z

Build backup docker image and upload it to hub.docker with image name pravega/zkbackup:0.1

It might be easier to enhance the operator docker image with the backup script (e.g., copy the script here https://github.com/pravega/zookeeper-operator/blob/master/Dockerfile#L34) and use it in the CronJob with a different entrypoint. This way we won't need to maintain an extra docker image or pull it to a cluster, as it would already be present in the system.

Good point,
I'll provide new patch with fixes and unit tests soon.

lunarfs · 2022-04-07T10:33:00Z

Hi @ibumarskov where did this land? I am currently evaluating my options for having automated backup of our cluster.. any plans of getting this pr to master

Signed-off-by: Ilya Bumarskov <[email protected]>

…r-operator into zk-backup-controller

…into pravega-master

ibumarskov · 2022-04-12T13:14:07Z

Hi @ibumarskov where did this land? I am currently evaluating my options for having automated backup of our cluster.. any plans of getting this pr to master

Hi, currently I have not enough time for this task. You can finish the task if you want.

Signed-off-by: ibumarskov <[email protected]>

Signed-off-by: Ilya Bumarskov <[email protected]>

…into pravega-master

anishakj · 2022-05-17T10:06:32Z

@ibumarskov would you like to update this PR

lunarfs · 2022-06-22T13:30:23Z

please add
cat config/crd/bases/zookeeper.pravega.io_zookeeperbackups.yaml >> charts/zookeeper-operator/templates/zookeeper.pravega.io_zookeeperclusters_crd.yaml
to the Makefile line 119
https://github.com/ibumarskov/zookeeper-operator/blob/1e7c76cab67192161adabfbc10e3c63ed146f8f9/Makefile#L118
to have the CRD contain the backup definition

lunarfs · 2022-06-22T14:01:04Z

please add
cat config/crd/bases/zookeeper.pravega.io_zookeeperbackups.yaml >> charts/zookeeper-operator/templates/zookeeper.pravega.io_zookeeperclusters_crd.yaml
to the Makefile line 119
https://github.com/ibumarskov/zookeeper-operator/blob/1e7c76cab67192161adabfbc10e3c63ed146f8f9/Makefile#L118
to have the CRD contain the backup definition

with that change i managed to get a backup scheduled after running make generate to get the new CRD.
Will get back on the status later
the yml i used for scheduling looks like this..
apiVersion: "zookeeper.pravega.io/v1beta1" kind: "ZookeeperBackup" metadata: name: "example-backup" namespace: mynamespace spec: zookeeperCluster: "zookeeper-cluster" schedule: "*/5 * * * *" backupsToKeep: "7" dataStorageClass: "backup-class" image: repository: "<MYREPO>/zookeeper-operator" tag: "0.2.13-47-localbuild-2"

README.md

api/v1beta1/zookeeperbackup_types.go

jkhalack · 2022-06-23T01:07:32Z

api/v1beta1/zookeeperbackup_types.go

+type ZookeeperBackupStatus struct {
+	// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
+	// Important: Run "make" to regenerate code after modifying this file
+}


Empty struct

build_backup/backup.sh

jkhalack · 2022-06-23T01:23:34Z

controllers/zookeeperbackup_controller.go

+
+func (r *ZookeeperBackupReconciler) Reconcile(_ context.Context, request reconcile.Request) (reconcile.Result, error) {
+	r.Log = logBk.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)
+	r.Log.Info("Reconciling ZookeeperBackup")


If this message is needed, it would be nice to log the name and the namespace of the CR, otherwise there is no value in the log message.

Actually name (CR) and namespace should be added to each log entry. ZookeeperCluster reconcile function has the same logging. Example:
{"level":"info","ts":1649845790.4947126,"logger":"controller_zookeepercluster","msg":"Reconciling ZookeeperCluster","Request.Namespace":"tf","Request.Name":"tf-zookeeper"}

jkhalack · 2022-06-23T01:24:06Z

controllers/zookeeperbackup_controller.go

+	}
+	changed := zookeeperBackup.WithDefaults()
+	if changed {
+		r.Log.Info("Setting default settings for zookeeper-backup")


Please add the name and the namespace of the CR.

All logs are provided with name/namespaces:
r.Log = logBk.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)

jkhalack · 2022-06-23T01:26:26Z

controllers/zookeeperbackup_controller.go

+	zkCluster := zookeeperBackup.Spec.ZookeeperCluster
+	err = r.Client.Get(context.TODO(), types.NamespacedName{Name: zkCluster, Namespace: zookeeperBackup.Namespace}, foundZookeeperCluster)
+	if err != nil && errors.IsNotFound(err) {
+		r.Log.Error(err, fmt.Sprintf("Zookeeper cluster '%s' not found", zkCluster))


Please, log ZookeeperBackup name and the namespace.

controllers/zookeeperbackup_controller.go

- Add backup crd to charts (makefile) - Add validation of BACKUPS_TO_KEEP variable - Improve logging Signed-off-by: ibumarskov <[email protected]>

ibumarskov · 2022-06-23T09:58:54Z

@lunarfs, @jkhalack thanks for yours comments, I have fixed all issues.
@jkhalack regarding Status of backup controller. It would be nice to show the status in backup controller, but this requires additional work and if you don't mind, it can be done as part of another pull request.

lunarfs · 2022-06-23T13:33:42Z

so a bit of feedback, I scheduled a backup yesterday, but i named the cluster wrong, the backup scheduled niceley, but i found it hard to find status on the backup, wat would have been nice was a

kubectl   get zookeeperbackup 
NAME             AGE.      STATUS
example-backup   22h.   Failed, Cluster not found

or something similar, and maybe when it compleets

kubectl   get zookeeperbackup 
NAME             AGE.      STATUS
example-backup   22h.   Backup compleeted 11.22 AM

lunarfs · 2022-06-24T11:13:41Z

so a bit of feedback, I scheduled a backup yesterday, but i named the cluster wrong, the backup scheduled niceley, but i found it hard to find status on the backup, wat would have been nice was a
kubectl   get zookeeperbackup 
NAME             AGE.      STATUS
example-backup   22h.   Failed, Cluster not found
or something similar, and maybe when it compleets
kubectl   get zookeeperbackup 
NAME             AGE.      STATUS
example-backup   22h.   Backup compleeted 11.22 AM

also I am not sure that i get it right.. but i would expect this to work

kubectl get ZookeeperBackupList
Error from server (NotFound): Unable to list "zookeeper.pravega.io/v1beta1, Resource=zookeeperbackuplists": the server could not find the requested resource (get zookeeperbackuplists.zookeeper.pravega.io)

syusuf-r7 · 2022-08-18T14:19:08Z

Hi @ibumarskov I'm also interested in this. Are there plans to get this merged into master soon?

ibumarskov · 2022-08-19T09:49:38Z

@anishakj ZookeeperClusterList returns same error:
kubectl get ZookeeperClusterList Error from server (NotFound): Unable to list "zookeeper.pravega.io/v1beta1, Resource=zookeeperclusterlists": the server could not find the requested resource (get zookeeperclusterlists.zookeeper.pravega.io)
Can you please merge this PR and continue to work on improvements within other PRs ?

Ilya Bumarskov added 5 commits November 23, 2021 16:52

Implement automatic backup for zookeper db

c73c7af

Signed-off-by: Ilya Bumarskov <[email protected]>

Restore default parameters for zookeeper cluster

5a30850

Signed-off-by: Ilya Bumarskov <[email protected]>

Set default image for backup

1f7f22e

Signed-off-by: Ilya Bumarskov <[email protected]>

Fix license

64d0b89

Signed-off-by: Ilya Bumarskov <[email protected]>

Fix formatting

b5a4c53

Signed-off-by: Ilya Bumarskov <[email protected]>

ibumarskov force-pushed the zk-backup-controller branch from 79d8c03 to 64d0b89 Compare November 23, 2021 12:52

jkhalack reviewed Nov 23, 2021

View reviewed changes

Merge branch 'master' into zk-backup-controller

49fdc6a

Ilya Bumarskov added 4 commits April 8, 2022 10:36

Fix license

9daf6d0

Signed-off-by: Ilya Bumarskov <[email protected]>

Merge branch 'zk-backup-controller' of github.com:ibumarskov/zookeepe…

5c6aaf4

…r-operator into zk-backup-controller

Merge branch 'master' of https://github.com/pravega/zookeeper-operator …

39937aa

…into pravega-master

Merge branch 'pravega-master' into zk-backup-controller

b5c4148

Align zookeeperbackup with operator-sdk 1.13

246658f

Signed-off-by: ibumarskov <[email protected]>

ibumarskov force-pushed the zk-backup-controller branch from 6b1471c to d98262d Compare April 13, 2022 06:12

Fix zookeeperbackup

a53eebc

Signed-off-by: Ilya Bumarskov <[email protected]>

ibumarskov force-pushed the zk-backup-controller branch from d98262d to a53eebc Compare April 13, 2022 07:22

Merge branch 'master' of https://github.com/pravega/zookeeper-operator …

a62cbf1

…into pravega-master

ibumarskov force-pushed the zk-backup-controller branch 4 times, most recently from 532e03b to 06853d8 Compare April 13, 2022 07:56

Merge branch 'pravega-master' into zk-backup-controller

c9583f4

ibumarskov force-pushed the zk-backup-controller branch from 06853d8 to c9583f4 Compare April 13, 2022 08:05

Merge branch 'master' into zk-backup-controller

62ed73a

Merge branch 'master' into zk-backup-controller

1e7c76c

ibumarskov force-pushed the zk-backup-controller branch from ed8c666 to 1e7c76c Compare June 22, 2022 11:55