Issue 63: Enabling ephemeral storage for zookeeper. #64

HoustonPutman · 2019-05-02T21:19:48Z

Changelog Description

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

To enable ephemeral storage, omit

spec:
    persistence

This change breaks backwards compatibility, but it does adhere to the documentation of the Zookeeper CRD:

zookeeper-operator/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go

Lines 68 to 71 in 68336c7

    
           // PersistentVolumeClaimSpec is the spec to describe PVC for the container 
        
           // This field is optional. If no PVC spec, stateful containers will use 
        
           // emptyDir as volume. 
        
           PersistentVolumeClaimSpec *v1.PersistentVolumeClaimSpec `json:"persistence,omitempty"`

Purpose of the change

Fix #63

How to verify

Start the operator, create a Zk cluster without persistence.

Signed-off-by: Houston Putman hputman1@bloomberg.net

codecov-io · 2019-05-03T18:41:41Z

Codecov Report

Merging #64 into master will decrease coverage by 0.61%.
The diff coverage is 70.45%.

@@           Coverage Diff           @@
##           master   #64      +/-   ##
=======================================
- Coverage   52.61%   52%   -0.62%     
=======================================
  Files           5     5              
  Lines         785   800      +15     
=======================================
+ Hits          413   416       +3     
- Misses        345   355      +10     
- Partials       27    29       +2

Impacted Files	Coverage Δ
...kg/apis/zookeeper/v1beta1/zz_generated.deepcopy.go	`0% <0%> (ø)`	⬆️
...g/apis/zookeeper/v1beta1/zookeepercluster_types.go	`90% <100%> (+0.08%)`	⬆️
pkg/zk/generators.go	`82.43% <72.72%> (-3.35%)`	⬇️

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 7d42ddf...2c7f01d. Read the comment docs.

codecov-io · 2019-05-03T18:41:42Z

Codecov Report

Merging #64 into master will decrease coverage by 0.61%.
The diff coverage is 70.45%.

@@           Coverage Diff           @@
##           master   #64      +/-   ##
=======================================
- Coverage   52.61%   52%   -0.62%     
=======================================
  Files           5     5              
  Lines         785   800      +15     
=======================================
+ Hits          413   416       +3     
- Misses        345   355      +10     
- Partials       27    29       +2

Impacted Files	Coverage Δ
...kg/apis/zookeeper/v1beta1/zz_generated.deepcopy.go	`0% <0%> (ø)`	⬆️
...g/apis/zookeeper/v1beta1/zookeepercluster_types.go	`90% <100%> (+0.08%)`	⬆️
pkg/zk/generators.go	`82.43% <72.72%> (-3.35%)`	⬇️

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 7d42ddf...2c7f01d. Read the comment docs.

Signed-off-by: Houston Putman <hputman1@bloomberg.net>

HoustonPutman · 2019-05-14T16:58:38Z

Rebased to comply with new Persistence option.

Now ephemeral storage is enabled if Persistence is not provided.

Signed-off-by: Houston Putman <hputman1@bloomberg.net>

adrianmo

Thanks for the contribution @HoustonPutman. Let me share my thoughts on this.

On one hand, I'm a bit concerned about altering the default persistence settings as it can inadvertently impact users who might be thinking that their data is still persisted, but it's not.

On the other hand, I think this change makes sense, adheres to the documentation, and we are still on time to make breaking changes as the project is in alpha state.

From my side, I'm more inclined to making this change. I'll review the code if there's a consensus on accepting the change.

@Tristan1900 @spiegela what do you think?

Tristan1900 · 2019-05-30T20:39:28Z

I agree with you @adrianmo, this is a good change since it just implements what the doc promises. I would vote to make the change.

HoustonPutman · 2019-05-31T15:14:07Z

I should also mention, that I updated the format checking script to output the diffs of the expected vs. actual formatting. Making it much easier to debug, since the formatting is tested against multiple go versions.

HoustonPutman · 2019-06-26T19:23:56Z

Have y'all had a chance to look at the implementation?

prune998 · 2019-09-09T17:55:56Z

Any issue on that ? Can it be merged ?

spiegela · 2019-10-10T22:21:45Z

pkg/controller/zookeepercluster/zookeepercluster_controller.go

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

 func (r *ReconcileZookeeperCluster) reconcileFinalizers(instance *zookeeperv1beta1.ZookeeperCluster) (err error) {


Looks like we need coverage of reconcileFinalizers, but it isn't currently there, so I'll file that in a separate issue.

spiegela

Sorry for the delay getting back around to this. These changes all look good to me. Thanks!

HoustonPutman · 2019-10-31T16:43:19Z

Thanks for approving @spiegela! Can we get this merged?

spiegela · 2019-11-04T14:25:19Z

@pbelgundi can you also provide a review for this?

priyavj08 · 2020-01-22T05:43:07Z

when can we expect this to be merged?

thanks

HoustonPutman · 2020-03-25T18:36:38Z

Is there anything blocking this getting approved/merged @pbelgundi?

anishakj

LGTM

pbelgundi

Before any change is merged, it is necessary to have it tested it for the following usecases, manually if automated testing is not feasible:

Deployment & Deletion ( same & separate NS)
DR (delete 1, multiple, all ZK pods, delete ZK STS and it should be able to come back up functioning correctly…)
Upgrades/Rollback
Cluster Scale Up/Down.

pbelgundi · 2020-05-08T04:17:27Z

pkg/zk/generators.go

+			Spec: persistence.PersistentVolumeClaimSpec,
+		})
+	} else {
+		extraVolumes = append(extraVolumes, v1.Volume{


Our zookeeperStartup.sh script relies on persistent volumes to be able to handle server restarts which will be broken when using ephemeral volumes.

pbelgundi

Defaulting to ephemeral storage breaks our DR Strategy. While I like the idea of using ephemeral storage with Zookeeper, at this point it can't be made a default option.

spiegela · 2020-06-19T14:40:23Z

Defaulting to ephemeral storage breaks our DR Strategy. While I like the idea of using ephemeral storage with Zookeeper, at this point it can't be made a default option.

@pbelgundi If we make this a non-default option, is everything else in this PR kosher? If so, perhaps we should create a parallel PR with the changes we need to get this unblocked.

pbelgundi · 2020-06-23T05:07:22Z

Defaulting to ephemeral storage breaks our DR Strategy. While I like the idea of using ephemeral storage with Zookeeper, at this point it can't be made a default option.

@pbelgundi If we make this a non-default option, is everything else in this PR kosher? If so, perhaps we should create a parallel PR with the changes we need to get this unblocked.

Yes, I'm good as long as existing defaults don't change.

HoustonPutman · 2020-07-31T15:28:44Z

Better implementation available at #215

HoustonPutman force-pushed the issue-63-ephem-storage branch 2 times, most recently from df7f4f9 to 5a25d41 Compare May 3, 2019 16:00

Enabling ephemeral storage for zookeeper.

518a4b9

Signed-off-by: Houston Putman <hputman1@bloomberg.net>

HoustonPutman force-pushed the issue-63-ephem-storage branch from 2c7f01d to b8201e9 Compare May 14, 2019 16:56

Fixed fmt issues. check_format.shj now shows diff of fmt errors.

bda3a6b

Signed-off-by: Houston Putman <hputman1@bloomberg.net>

HoustonPutman force-pushed the issue-63-ephem-storage branch from b8201e9 to bda3a6b Compare May 14, 2019 17:22

adrianmo requested review from adrianmo, spiegela and Tristan1900 May 15, 2019 13:27

adrianmo reviewed May 15, 2019

View reviewed changes

spiegela reviewed Oct 10, 2019

View reviewed changes

spiegela approved these changes Oct 10, 2019

View reviewed changes

HoustonPutman mentioned this pull request Oct 31, 2019

Solr operator going in CrashLoopBackOff state apache/solr-operator#41

Closed

fpj requested a review from pbelgundi November 14, 2019 17:40

HoustonPutman mentioned this pull request Jan 7, 2020

Fixing Rest config NPE & ZK Persistence Reconciling/Defaulting & Backup/Restore Volume mount apache/solr-operator#65

Merged

pbelgundi requested review from anishakj and removed request for pbelgundi May 4, 2020 09:31

anishakj approved these changes May 7, 2020

View reviewed changes

pbelgundi reviewed May 8, 2020

View reviewed changes

pbelgundi requested changes May 8, 2020

View reviewed changes

pbelgundi assigned Prabhaker24 Jun 23, 2020

HoustonPutman closed this Jul 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 63: Enabling ephemeral storage for zookeeper. #64

Issue 63: Enabling ephemeral storage for zookeeper. #64

HoustonPutman commented May 2, 2019 •

edited

Loading

codecov-io commented May 3, 2019

codecov-io commented May 3, 2019

HoustonPutman commented May 14, 2019

adrianmo left a comment

Tristan1900 commented May 30, 2019

HoustonPutman commented May 31, 2019

HoustonPutman commented Jun 26, 2019

prune998 commented Sep 9, 2019

spiegela Oct 10, 2019

spiegela left a comment

HoustonPutman commented Oct 31, 2019

spiegela commented Nov 4, 2019

priyavj08 commented Jan 22, 2020

HoustonPutman commented Mar 25, 2020

anishakj left a comment

pbelgundi left a comment

pbelgundi May 8, 2020

pbelgundi left a comment

spiegela commented Jun 19, 2020

pbelgundi commented Jun 23, 2020

HoustonPutman commented Jul 31, 2020

	// PersistentVolumeClaimSpec is the spec to describe PVC for the container
	// This field is optional. If no PVC spec, stateful containers will use
	// emptyDir as volume.
	PersistentVolumeClaimSpec *v1.PersistentVolumeClaimSpec `json:"persistence,omitempty"`

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

		func (r ReconcileZookeeperCluster) reconcileFinalizers(instance zookeeperv1beta1.ZookeeperCluster) (err error) {

Issue 63: Enabling ephemeral storage for zookeeper. #64

Issue 63: Enabling ephemeral storage for zookeeper. #64

Conversation

HoustonPutman commented May 2, 2019 • edited Loading

Changelog Description

Purpose of the change

How to verify

codecov-io commented May 3, 2019

Codecov Report

codecov-io commented May 3, 2019

Codecov Report

HoustonPutman commented May 14, 2019

adrianmo left a comment

Choose a reason for hiding this comment

Tristan1900 commented May 30, 2019

HoustonPutman commented May 31, 2019

HoustonPutman commented Jun 26, 2019

prune998 commented Sep 9, 2019

spiegela Oct 10, 2019

Choose a reason for hiding this comment

spiegela left a comment

Choose a reason for hiding this comment

HoustonPutman commented Oct 31, 2019

spiegela commented Nov 4, 2019

priyavj08 commented Jan 22, 2020

HoustonPutman commented Mar 25, 2020

anishakj left a comment

Choose a reason for hiding this comment

pbelgundi left a comment

Choose a reason for hiding this comment

pbelgundi May 8, 2020

Choose a reason for hiding this comment

pbelgundi left a comment

Choose a reason for hiding this comment

spiegela commented Jun 19, 2020

pbelgundi commented Jun 23, 2020

HoustonPutman commented Jul 31, 2020

HoustonPutman commented May 2, 2019 •

edited

Loading