Issue 63: Added support for ephemeral storage #215

Prabhaker24 · 2020-07-08T11:42:31Z

Signed-off-by: prabhaker24 [email protected]

Change log description

This pr is to enable zookeeper cluster to be able to use ephemeral storage, At any point, only one storage can be there either ephemeral or persistent and if none is specified zookeeper cluster will use persistent storage.

Purpose of the change

fixes #63

What the code does

This PR allows users to create zookeepers with ephemeral storage instead of PVs.

To enable ephemeral storage add this to the manifest,

spec:
    storageType: ephemeral

How to verify it

Create a ZK cluster with storageType as Ephemeral, in this case, Ephemeral storage should get created
Create a ZK cluster with storageType as persistence, in this case, persistence storage should get created
Create a ZK cluster with no storageType, in this case, Persistence storage should get created.
Try upgrading the operator keeping the ZK cluster same operator should be able to reflect the correct persistence configuration post-upgrade.
Note:- In case of ephemeral storage, the cluster may not be able to come back up if more than quorum number of nodes are restarted simultaneously.
I have verified all the above 4 scenarios

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

codecov-commenter · 2020-07-08T14:24:42Z

Codecov Report

Merging #215 into master will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   83.37%   83.60%   +0.23%     
==========================================
  Files          11       11              
  Lines        1203     1220      +17     
==========================================
+ Hits         1003     1020      +17     
  Misses        137      137              
  Partials       63       63

Impacted Files	Coverage Δ
...g/apis/zookeeper/v1beta1/zookeepercluster_types.go	`98.30% <100.00%> (+0.08%)`	⬆️
...kg/apis/zookeeper/v1beta1/zz_generated.deepcopy.go	`100.00% <100.00%> (ø)`
...er/zookeepercluster/zookeepercluster_controller.go	`60.73% <100.00%> (ø)`
pkg/zk/generators.go	`98.31% <100.00%> (+0.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 410c7e8...1db7f35. Read the comment docs.

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

SrishT · 2020-07-10T05:25:34Z

charts/zookeeper/templates/zookeeper.yaml

@@ -43,10 +43,18 @@ spec:
    syncLimit: {{ .Values.config.syncLimit }}
    quorumListenOnAllIPs: {{ .Values.config.quorumListenOnAllIPs }}
  {{- end }}
+  {{- if .Values.ehemeral.enabled }}


this should be ephemeral
Also this check should be removed since the value of ephemeral.enabled needs to be set in all cases

SrishT · 2020-07-10T05:29:30Z

charts/zookeeper/templates/zookeeper.yaml

+  {{- if .Values.ehemeral.enabled }}
+  ephemeral:
+    enabled: {{ .Values.ephemeral.enabled }}
+    emptydirvolumesource:


add the check before this.
add the following if block before setting value for emptydirvolumesource

ephemeral: enabled: {{ .Values.ephemeral.enabled }} {{- if .Values.ephemeral.emptydirvolumesource }} emptydirvolumesource: medium: {{ .Values.ephemeral.emptydirvolumesource.medium }} sizeLimit: {{ .Values.ephemeral.emptydirvolumesource.sizeLimit }} {{- end }}

Also the check in front of persistence can be changed to be created only when ephemeral.enabled is set to false

SrishT · 2020-07-10T05:36:29Z

charts/zookeeper/values.yaml

@@ -26,6 +26,12 @@ config: {}
  # syncLimit: 2
  # quorumListenOnAllIPs: false

+ephemeral:
+  enabled: false
+  #emptydirvolumesource:


don't comment emptydirvolumesource
Instead set it to

emptydirvolumesource: {} #medium: ""/Memory #sizeLimit: 15Gi

If user wishes to provide a value he can remove {} and uncomment the options

SrishT · 2020-07-10T05:37:40Z

test/e2e/resources/rbac-operator.yaml

@@ -95,7 +89,7 @@ spec:
              fieldPath: metadata.name
        - name: OPERATOR_NAME
          value: zookeeper-operator
-        image: pravega/zookeeper-operator:latest
+        image: testzkop/zookeeper-operator-testimages:v0.2.8-rc0-4-dirty


We shouldn't be using test images here

I have changed it.

anishakj · 2020-07-10T05:52:03Z

pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go

@@ -67,6 +67,10 @@ type ZookeeperClusterSpec struct {
 	// PersistentVolumeClaimSpec and VolumeReclaimPolicy can be specified in here.
 	Persistence *Persistence `json:"persistence,omitempty"`

+	//Ephemeral is the configuration which helps create ephemeral storage
+	//At anypoint only one of Persistence or Ephemeral should be present in the manifest
+	Ephemeral *Ephemeral `json:"ephemeral,omitempty"`


Can we move this under storage, we can provide multiple storage options under strorage like ephemeral and persistent storage

I have changed the structure and added storage struct and moved persistence and ephemeral in it

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

…into Issue-63-enabling-ephemeral-storage

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

SrishT · 2020-07-16T11:09:23Z

charts/zookeeper/templates/zookeeper.yaml

+        sizeLimit: {{ .Values.ephemeral.emptydirvolumesource.sizeLimit }}
+       {{- end }}
+    {{- end }}
+    {{- if eq .Values.storageType.type "persistence" }}


This should be part of {{- else }} and not as part of a separate if block. Only then would this be treated as the default even when no value is provided for storageType.type

SrishT · 2020-07-16T11:14:13Z

charts/zookeeper/values.yaml

@@ -25,13 +25,24 @@ config: {}
  # tickTime: 2000
  # syncLimit: 2
  # quorumListenOnAllIPs: false
+storageType:


rename this strorage and make persistence and ephemeral as part of this block

storage: type: persistence persistence: storageClassName: standard reclaimPolicy: Delete volumeSize: 20Gi ephemeral: emptydirvolumesource: {} medium: Memory sizeLimit: 15Gi

SrishT · 2020-07-16T11:15:29Z

charts/zookeeper/values.yaml

+    volumeSize: 20Gi
+
+ephemeral:
+    enabled: true


please remove the enabled field from here. Whether ephemeral will be enabled/not will be determined by the value of storage.type field

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

HoustonPutman

Overall I love the new structure of the storage section! Thanks so much for taking care of this @Prabhaker24

HoustonPutman · 2020-07-22T15:13:59Z

charts/zookeeper/values.yaml

+    reclaimPolicy: Delete
+    volumeSize: 20Gi
+
+emptydirvolumesource: {}


Shouldn't this be ephemeral.emptydirvolumesource?

yes, you are correct I have changed it.

HoustonPutman · 2020-07-22T15:14:29Z

charts/zookeeper/README.md

 | `persistence.reclaimPolicy` | Reclaim policy for persistent volumes | `Delete` |
 | `persistence.storageClassName` | Storage class for persistent volumes | `standard` |
 | `persistence.volumeSize` | Size of the volume requested for persistent volumes | `20Gi` |
+| `ephemeral.enabled` | whether ephemeral storage is enabled or not | `false` |


This line is no longer needed, correct?

I have removed it

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

Prabhaker24 · 2020-07-23T04:50:10Z

Overall I love the new structure of the storage section! Thanks so much for taking care of this @Prabhaker24

Thanks @HoustonPutman

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

SrishT · 2020-07-23T10:32:22Z

charts/zookeeper/README.md

 | `persistence.reclaimPolicy` | Reclaim policy for persistent volumes | `Delete` |
 | `persistence.storageClassName` | Storage class for persistent volumes | `standard` |
 | `persistence.volumeSize` | Size of the volume requested for persistent volumes | `20Gi` |
+| `ephemeral.emptydirvolumesource.medium` |  What type of storage medium should back the directory. | `""` |
+| `ephemeral.emptydirvolumesource.sizeLimit` | Total amount of local storage required for the EmptyDir volume. | `nil` |


nil is not a valid default value in helm. Please leave it empty or give an empty string.

SrishT · 2020-07-23T10:35:22Z

charts/zookeeper/values.yaml

 persistence:
-  storageClassName: standard
-  ## specifying reclaim policy for PersistentVolumes
-  ## accepted values - Delete / Retain
-  reclaimPolicy: Delete
-  volumeSize: 20Gi
+    storageClassName: standard
+    # specifying reclaim policy for PersistentVolumes
+    # accepted values - Delete / Retain
+    reclaimPolicy: Delete
+    volumeSize: 20Gi
+
+ephemeral:


Please add one more level of indentation so that the blocks persistence and ephemeral come within the storage block

SrishT · 2020-07-23T10:37:01Z

charts/zookeeper/values.yaml

+
+ephemeral:
+  emptydirvolumesource: {}
+    #medium: ""/Memory


instead of mentioning Memory as an accepted value for medium as /Memory, please add a comment in the previous line which lists the accepted values, and keep just the default value here

anishakj · 2020-07-23T11:40:31Z

test/e2e/zookeepercluster_test.go

@@ -62,6 +62,7 @@ func testZookeeperCluster(t *testing.T) {
 		"testUpgradeCluster":        testUpgradeCluster,
 		"testCreateRecreateCluster": testCreateRecreateCluster,
 		"testScaleCluster":          testScaleCluster,
+		"testephemeralstorage":      testephemeralstorage,


change to testEphemeralStorage

pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go

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

SrishT

LGTM

anishakj

Could you please change the file zookeeper_v1beta1_zookeepercluster_cr.yaml to reflect spec changes

anishakj · 2020-07-24T09:38:41Z

README.md

+spec:
+  replicas: 3
+  storage:        
+    storagetype: ephemeral


Change to storageType

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

Prabhaker24 · 2020-07-24T10:04:51Z

Could you please change the file zookeeper_v1beta1_zookeepercluster_cr.yaml to reflect spec changes

Thanks for reminding this, I have additionally changed the tag to 0.2.8 as well.

README.md

pbelgundi · 2020-07-24T10:33:17Z

pkg/controller/zookeepercluster/zookeepercluster_controller.go

@@ -610,7 +610,7 @@ func (r *ReconcileZookeeperCluster) yamlConfigMap(instance *zookeeperv1beta1.Zoo
 }

 func (r *ReconcileZookeeperCluster) reconcileFinalizers(instance *zookeeperv1beta1.ZookeeperCluster) (err error) {
-	if instance.Spec.Persistence.VolumeReclaimPolicy != zookeeperv1beta1.VolumeReclaimPolicyDelete {
+	if instance.Spec.Storage.Persistence != nil && instance.Spec.Storage.Persistence.VolumeReclaimPolicy != zookeeperv1beta1.VolumeReclaimPolicyDelete {


why the check for " instance.Spec.Storage.Persistence != nil"

Because in case if the storage is of type Ephemeral, "instance.Spec.Storage.Persistence" will be nil and in that case, this statement will panic

pkg/zk/generators.go

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

anishakj

LGTM

pbelgundi · 2020-08-07T06:36:27Z

README.md

+NAME      REPLICAS   READY REPLICAS   VERSION   DESIRED VERSION   INTERNAL ENDPOINT    EXTERNAL ENDPOINT   AGE
+example   3          3                 0.2.7     0.2.7             10.100.200.18:2181   N/A                 94s
+```
+>Note: User should only provide either persistence or ephemeral in the spec, if none of the values is specified default is persistence


This should be "persistent" and not "persistence" storage.

I have rephrased this statement.

pbelgundi · 2020-08-07T06:38:38Z

README.md

+```
+>Note: User should only provide either persistence or ephemeral in the spec, if none of the values is specified default is persistence
+
+>Note: We don't guarantee Data Recovery in case of Ephemeral Storage, so the users should keep in mind that in the case of zookeeper pod restarts user might lose the data.


This is not about data recovery but about cluster recovery. In case of ephemeral storage, the cluster may not be able to come back up if >quorum number of nodes are restarted simultaneously and that is exactly what we should state here.
Did we test if restarts on < quorum number of nodes works with ephemeral storage. In theory it should work, but we should test it and clarify accordingly in the doc.

Yes, @pbelgundi you are right I have changed the documentation and I have tested that in case we are restarting less than quorum number of nodes with ephemeral storage we are able to get ensemble.

pbelgundi · 2020-08-07T06:41:39Z

deploy/crds/zookeeper_v1beta1_zookeepercluster_cr.yaml

  persistence:
-    reclaimPolicy: Delete
+    reclaimPolicy: Retain


Why are we changing this? This has implications for Pravega and we need it to be Delete for Pravega to work on reinstall without reinstalling Zookeeper.

I have created two sample cr for both pravega and ecs and mentioned in the cr folder

pbelgundi · 2020-08-07T06:42:53Z

pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go

+		s.Ephemeral.EmptyDirVolumeSource = v1.EmptyDirVolumeSource{}
+		changed = true
+	}
+	if !strings.EqualFold(s.StorageType, "ephemeral") && s.Persistence != nil && s.Persistence.withDefaults() {


Please replace this "if" with "else" for the earlier "if"

I have restructured this part of the code for readability.

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

…into Issue-63-enabling-ephemeral-storage

pbelgundi · 2020-08-14T04:06:30Z

README.md

+```
+>Note: User should only provide value for either the field persistence or ephemeral in the spec if none of the values is specified default is persistence
+
+>Note: In case of ephemeral storage, the cluster may not be able to come back up if more than quorum number of nodes are restarted simultaneously.


Please also mention data loss on node restart.

pbelgundi

LGTM, with just 1 minor comment.

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

prabhaker24 added 4 commits July 8, 2020 16:58

added support for ephemeral storage

2f022a7

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

changed emepheral ete

8e02a92

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

fixed test

ba3e241

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

fixed format

17f5dff

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

prabhaker24 added 2 commits July 9, 2020 16:03

doc changes and ut added

e549b2d

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

fixed doc

dada168

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

Prabhaker24 requested a review from anishakj July 9, 2020 10:41

Prabhaker24 marked this pull request as ready for review July 9, 2020 10:41

Prabhaker24 requested review from pbelgundi and spiegela July 9, 2020 10:42

changed charts

5f20b7e

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

SrishT requested changes Jul 10, 2020

View reviewed changes

anishakj requested changes Jul 10, 2020

View reviewed changes

prabhaker24 added 6 commits July 15, 2020 20:14

addressed review comments

143a52b

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

fixed ete

5b747c7

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

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

cb7c588

…into Issue-63-enabling-ephemeral-storage

changed readme file

fefa364

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

addressed comments

df5ca92

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

fixed charts

e663fc7

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

SrishT requested changes Jul 16, 2020

View reviewed changes

prabhaker24 added 2 commits July 17, 2020 09:35

addressed comments

023271f

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

changed default storage to persistence

4967437

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

HoustonPutman reviewed Jul 22, 2020

View reviewed changes

HoustonPutman mentioned this pull request Jul 22, 2020

Support setting persistent volume reclaim policy apache/solr-operator#103

Closed

changed doc

5560fa5

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

changed in zookeeper.yaml

ebe8cca

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

SrishT requested changes Jul 23, 2020

View reviewed changes

anishakj reviewed Jul 23, 2020

View reviewed changes

SrishT requested changes Jul 23, 2020

View reviewed changes

pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go Outdated Show resolved Hide resolved

addressed comments

0b5d7c9

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

SrishT approved these changes Jul 24, 2020

View reviewed changes

anishakj reviewed Jul 24, 2020

View reviewed changes

prabhaker24 added 2 commits July 24, 2020 15:27

addressed comments

6c5e449

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

pulled changes from master

b369dd8

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

pbelgundi reviewed Jul 24, 2020

View reviewed changes

README.md Show resolved Hide resolved

pbelgundi reviewed Jul 24, 2020

View reviewed changes

pbelgundi requested changes Jul 24, 2020

View reviewed changes

pkg/zk/generators.go Outdated Show resolved Hide resolved

changed structure for ephemeral storage

11cf150

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

Prabhaker24 changed the title ~~added support for ephemeral storage~~ Issue 63: Added support for ephemeral storage Jul 28, 2020

HoustonPutman mentioned this pull request Jul 31, 2020

Issue 63: Enabling ephemeral storage for zookeeper. #64

Closed

prabhaker24 added 2 commits August 3, 2020 10:46

doc added

942e972

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

fixed readme

2d44c1e

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

anishakj approved these changes Aug 7, 2020

View reviewed changes

pbelgundi requested changes Aug 7, 2020

View reviewed changes

prabhaker24 added 2 commits August 13, 2020 23:48

addressed comments

6b4259f

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

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

0cad266

…into Issue-63-enabling-ephemeral-storage

pbelgundi reviewed Aug 14, 2020

View reviewed changes

pbelgundi approved these changes Aug 14, 2020

View reviewed changes

addressed comment

1db7f35

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

anishakj merged commit d88ce7c into master Aug 14, 2020

Prabhaker24 mentioned this pull request Sep 10, 2020

support mutli images #230

Merged

HoustonPutman mentioned this pull request Jul 14, 2021

Add ephemeral option for zk storage. apache/solr-operator#284

Merged

Issue 63: Added support for ephemeral storage #215

Issue 63: Added support for ephemeral storage #215

Conversation

Prabhaker24 commented Jul 8, 2020 • edited Loading

Change log description

Purpose of the change

What the code does

How to verify it

codecov-commenter commented Jul 8, 2020 • edited Loading

Codecov Report

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

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

HoustonPutman 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

Prabhaker24 commented Jul 23, 2020

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

SrishT left a comment

Choose a reason for hiding this comment

anishakj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prabhaker24 commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anishakj 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

Choose a reason for hiding this comment

pbelgundi left a comment

Choose a reason for hiding this comment

Prabhaker24 commented Jul 8, 2020 •

edited

Loading

codecov-commenter commented Jul 8, 2020 •

edited

Loading